subject:"\[Intel\-gfx\] \[PATCH v2 4\/7\] drm\/i915\/gtt\/dgfx\: place the PD in LMEM"

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

2021-04-27 Thread Tvrtko Ursulin




On 27/04/2021 09:54, Matthew Auld wrote:

It's a requirement that for dgfx we place all the paging structures in
device local-memory.

v2: use i915_coherent_map_type()
v3: improve the shared dma-resv object comment

Signed-off-by: Matthew Auld 
Cc: Tvrtko Ursulin 
---
  drivers/gpu/drm/i915/gt/gen8_ppgtt.c |  5 -
  drivers/gpu/drm/i915/gt/intel_gtt.c  | 30 +---
  drivers/gpu/drm/i915/gt/intel_gtt.h  |  1 +
  3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
index f83496836f0f..11fb5df45a0f 100644
--- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
@@ -712,7 +712,10 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt)
 */
ppgtt->vm.has_read_only = !IS_GEN_RANGE(gt->i915, 11, 12);
  
-	ppgtt->vm.alloc_pt_dma = alloc_pt_dma;

+   if (HAS_LMEM(gt->i915))
+   ppgtt->vm.alloc_pt_dma = alloc_pt_lmem;
+   else
+   ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
  
  	err = gen8_init_scratch(>vm);

if (err)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c 
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index d386b89e2758..061c39d2ad51 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -7,10 +7,26 @@
  
  #include 
  
+#include "gem/i915_gem_lmem.h"

  #include "i915_trace.h"
  #include "intel_gt.h"
  #include "intel_gtt.h"
  
+struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space *vm, int sz)

+{
+   struct drm_i915_gem_object *obj;
+
+   obj = i915_gem_object_create_lmem(vm->i915, sz, 0);
+   /*
+* Ensure all paging structures for this vm share the same dma-resv
+* object underneath, with the idea that one object_lock() will lock
+* them all at once.


Okay but I am still missing the part about why is this beneficial and 
not a downside. I suppose it is not a concept added by this patch so not 
fair to ask for explanation here anyway.


Reviewed-by: Tvrtko Ursulin 

Regards,

Tvrtko


+*/
+   if (!IS_ERR(obj))
+   obj->base.resv = >resv;
+   return obj;
+}
+
  struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm, int 
sz)
  {
struct drm_i915_gem_object *obj;
@@ -19,7 +35,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct 
i915_address_space *vm, int sz)
i915_gem_shrink_all(vm->i915);
  
  	obj = i915_gem_object_create_internal(vm->i915, sz);

-   /* ensure all dma objects have the same reservation class */
+   /*
+* Ensure all paging structures for this vm share the same dma-resv
+* object underneath, with the idea that one object_lock() will lock
+* them all at once.
+*/
if (!IS_ERR(obj))
obj->base.resv = >resv;
return obj;
@@ -27,9 +47,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct 
i915_address_space *vm, int sz)
  
  int map_pt_dma(struct i915_address_space *vm, struct drm_i915_gem_object *obj)

  {
+   enum i915_map_type type;
void *vaddr;
  
-	vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);

+   type = i915_coherent_map_type(vm->i915, obj, true);
+   vaddr = i915_gem_object_pin_map_unlocked(obj, type);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);
  
@@ -39,9 +61,11 @@ int map_pt_dma(struct i915_address_space *vm, struct drm_i915_gem_object *obj)
  
  int map_pt_dma_locked(struct i915_address_space *vm, struct drm_i915_gem_object *obj)

  {
+   enum i915_map_type type;
void *vaddr;
  
-	vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);

+   type = i915_coherent_map_type(vm->i915, obj, true);
+   vaddr = i915_gem_object_pin_map(obj, type);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);
  
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h

index 40e486704558..44ce27c51631 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -527,6 +527,7 @@ int setup_scratch_page(struct i915_address_space *vm);
  void free_scratch(struct i915_address_space *vm);
  
  struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm, int sz);

+struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space *vm, int 
sz);
  struct i915_page_table *alloc_pt(struct i915_address_space *vm);
  struct i915_page_directory *alloc_pd(struct i915_address_space *vm);
  struct i915_page_directory *__alloc_pd(int npde);


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

2021-04-27 Thread Matthew Auld


On 27/04/2021 14:34, Tang, CQ wrote:




-Original Message-
From: Intel-gfx  On Behalf Of
Matthew Auld
Sent: Tuesday, April 27, 2021 1:54 AM
To: intel-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

It's a requirement that for dgfx we place all the paging structures in device
local-memory.

v2: use i915_coherent_map_type()
v3: improve the shared dma-resv object comment

Signed-off-by: Matthew Auld 
Cc: Tvrtko Ursulin 
---
  drivers/gpu/drm/i915/gt/gen8_ppgtt.c |  5 -
drivers/gpu/drm/i915/gt/intel_gtt.c  | 30 +---
drivers/gpu/drm/i915/gt/intel_gtt.h  |  1 +
  3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
index f83496836f0f..11fb5df45a0f 100644
--- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
@@ -712,7 +712,10 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt
*gt)
 */
ppgtt->vm.has_read_only = !IS_GEN_RANGE(gt->i915, 11, 12);

-   ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
+   if (HAS_LMEM(gt->i915))
+   ppgtt->vm.alloc_pt_dma = alloc_pt_lmem;


Here we might want to allocate lmem from the 'gt' in the argument,  however, 
below inside alloc_pt_lmem(), we always allocate lmem to tile0.
Is this desired?


Yeah, AFAIK that is all handled in some later patches which have not yet 
made there way upstream. For DG1 they don't really do anything 
interesting, but yes we need them for Xe HP at some point.




--CQ


+   else
+   ppgtt->vm.alloc_pt_dma = alloc_pt_dma;

err = gen8_init_scratch(>vm);
if (err)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index d386b89e2758..061c39d2ad51 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -7,10 +7,26 @@

  #include 

+#include "gem/i915_gem_lmem.h"
  #include "i915_trace.h"
  #include "intel_gt.h"
  #include "intel_gtt.h"

+struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space
+*vm, int sz) {
+   struct drm_i915_gem_object *obj;
+
+   obj = i915_gem_object_create_lmem(vm->i915, sz, 0);
+   /*
+* Ensure all paging structures for this vm share the same dma-resv
+* object underneath, with the idea that one object_lock() will lock
+* them all at once.
+*/
+   if (!IS_ERR(obj))
+   obj->base.resv = >resv;
+   return obj;
+}
+
  struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm,
int sz)  {
struct drm_i915_gem_object *obj;
@@ -19,7 +35,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct
i915_address_space *vm, int sz)
i915_gem_shrink_all(vm->i915);

obj = i915_gem_object_create_internal(vm->i915, sz);
-   /* ensure all dma objects have the same reservation class */
+   /*
+* Ensure all paging structures for this vm share the same dma-resv
+* object underneath, with the idea that one object_lock() will lock
+* them all at once.
+*/
if (!IS_ERR(obj))
obj->base.resv = >resv;
return obj;
@@ -27,9 +47,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct
i915_address_space *vm, int sz)

  int map_pt_dma(struct i915_address_space *vm, struct
drm_i915_gem_object *obj)  {
+   enum i915_map_type type;
void *vaddr;

-   vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+   type = i915_coherent_map_type(vm->i915, obj, true);
+   vaddr = i915_gem_object_pin_map_unlocked(obj, type);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);

@@ -39,9 +61,11 @@ int map_pt_dma(struct i915_address_space *vm,
struct drm_i915_gem_object *obj)

  int map_pt_dma_locked(struct i915_address_space *vm, struct
drm_i915_gem_object *obj)  {
+   enum i915_map_type type;
void *vaddr;

-   vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
+   type = i915_coherent_map_type(vm->i915, obj, true);
+   vaddr = i915_gem_object_pin_map(obj, type);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);

diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 40e486704558..44ce27c51631 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -527,6 +527,7 @@ int setup_scratch_page(struct i915_address_space
*vm);  void free_scratch(struct i915_address_space *vm);

  struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm,
int sz);
+struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space
+*vm, int sz);
  struct i915_page_table *alloc_pt(struct i915_address_space *vm);  struct
i915_page_directory *alloc_pd(stru

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

2021-04-27 Thread Tang, CQ




> -Original Message-
> From: Intel-gfx  On Behalf Of
> Matthew Auld
> Sent: Tuesday, April 27, 2021 1:54 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM
> 
> It's a requirement that for dgfx we place all the paging structures in device
> local-memory.
> 
> v2: use i915_coherent_map_type()
> v3: improve the shared dma-resv object comment
> 
> Signed-off-by: Matthew Auld 
> Cc: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c |  5 -
> drivers/gpu/drm/i915/gt/intel_gtt.c  | 30 +---
> drivers/gpu/drm/i915/gt/intel_gtt.h  |  1 +
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index f83496836f0f..11fb5df45a0f 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -712,7 +712,10 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt
> *gt)
>*/
>   ppgtt->vm.has_read_only = !IS_GEN_RANGE(gt->i915, 11, 12);
> 
> - ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
> + if (HAS_LMEM(gt->i915))
> + ppgtt->vm.alloc_pt_dma = alloc_pt_lmem;

Here we might want to allocate lmem from the 'gt' in the argument,  however, 
below inside alloc_pt_lmem(), we always allocate lmem to tile0.
Is this desired?

--CQ

> + else
> + ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
> 
>   err = gen8_init_scratch(>vm);
>   if (err)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> b/drivers/gpu/drm/i915/gt/intel_gtt.c
> index d386b89e2758..061c39d2ad51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> @@ -7,10 +7,26 @@
> 
>  #include 
> 
> +#include "gem/i915_gem_lmem.h"
>  #include "i915_trace.h"
>  #include "intel_gt.h"
>  #include "intel_gtt.h"
> 
> +struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space
> +*vm, int sz) {
> + struct drm_i915_gem_object *obj;
> +
> + obj = i915_gem_object_create_lmem(vm->i915, sz, 0);
> + /*
> +  * Ensure all paging structures for this vm share the same dma-resv
> +  * object underneath, with the idea that one object_lock() will lock
> +  * them all at once.
> +  */
> + if (!IS_ERR(obj))
> + obj->base.resv = >resv;
> + return obj;
> +}
> +
>  struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm,
> int sz)  {
>   struct drm_i915_gem_object *obj;
> @@ -19,7 +35,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct
> i915_address_space *vm, int sz)
>   i915_gem_shrink_all(vm->i915);
> 
>   obj = i915_gem_object_create_internal(vm->i915, sz);
> - /* ensure all dma objects have the same reservation class */
> + /*
> +  * Ensure all paging structures for this vm share the same dma-resv
> +  * object underneath, with the idea that one object_lock() will lock
> +  * them all at once.
> +  */
>   if (!IS_ERR(obj))
>   obj->base.resv = >resv;
>   return obj;
> @@ -27,9 +47,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct
> i915_address_space *vm, int sz)
> 
>  int map_pt_dma(struct i915_address_space *vm, struct
> drm_i915_gem_object *obj)  {
> + enum i915_map_type type;
>   void *vaddr;
> 
> - vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> + type = i915_coherent_map_type(vm->i915, obj, true);
> + vaddr = i915_gem_object_pin_map_unlocked(obj, type);
>   if (IS_ERR(vaddr))
>   return PTR_ERR(vaddr);
> 
> @@ -39,9 +61,11 @@ int map_pt_dma(struct i915_address_space *vm,
> struct drm_i915_gem_object *obj)
> 
>  int map_pt_dma_locked(struct i915_address_space *vm, struct
> drm_i915_gem_object *obj)  {
> + enum i915_map_type type;
>   void *vaddr;
> 
> - vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + type = i915_coherent_map_type(vm->i915, obj, true);
> + vaddr = i915_gem_object_pin_map(obj, type);
>   if (IS_ERR(vaddr))
>   return PTR_ERR(vaddr);
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index 40e486704558..44ce27c51631 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -527,6 +527,7 @@ int setup_scratch_page(struct i915_address_space
> *vm);  void free_scratch(struct i915_address_space *vm);
> 
>  struct drm_i915_gem_object *alloc_pt_dma

[Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

2021-04-27 Thread Matthew Auld

It's a requirement that for dgfx we place all the paging structures in
device local-memory.

v2: use i915_coherent_map_type()
v3: improve the shared dma-resv object comment

Signed-off-by: Matthew Auld 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/gt/gen8_ppgtt.c |  5 -
 drivers/gpu/drm/i915/gt/intel_gtt.c  | 30 +---
 drivers/gpu/drm/i915/gt/intel_gtt.h  |  1 +
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
index f83496836f0f..11fb5df45a0f 100644
--- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
@@ -712,7 +712,10 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt)
 */
ppgtt->vm.has_read_only = !IS_GEN_RANGE(gt->i915, 11, 12);
 
-   ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
+   if (HAS_LMEM(gt->i915))
+   ppgtt->vm.alloc_pt_dma = alloc_pt_lmem;
+   else
+   ppgtt->vm.alloc_pt_dma = alloc_pt_dma;
 
err = gen8_init_scratch(>vm);
if (err)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c 
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index d386b89e2758..061c39d2ad51 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -7,10 +7,26 @@
 
 #include 
 
+#include "gem/i915_gem_lmem.h"
 #include "i915_trace.h"
 #include "intel_gt.h"
 #include "intel_gtt.h"
 
+struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space *vm, int 
sz)
+{
+   struct drm_i915_gem_object *obj;
+
+   obj = i915_gem_object_create_lmem(vm->i915, sz, 0);
+   /*
+* Ensure all paging structures for this vm share the same dma-resv
+* object underneath, with the idea that one object_lock() will lock
+* them all at once.
+*/
+   if (!IS_ERR(obj))
+   obj->base.resv = >resv;
+   return obj;
+}
+
 struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm, int sz)
 {
struct drm_i915_gem_object *obj;
@@ -19,7 +35,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct 
i915_address_space *vm, int sz)
i915_gem_shrink_all(vm->i915);
 
obj = i915_gem_object_create_internal(vm->i915, sz);
-   /* ensure all dma objects have the same reservation class */
+   /*
+* Ensure all paging structures for this vm share the same dma-resv
+* object underneath, with the idea that one object_lock() will lock
+* them all at once.
+*/
if (!IS_ERR(obj))
obj->base.resv = >resv;
return obj;
@@ -27,9 +47,11 @@ struct drm_i915_gem_object *alloc_pt_dma(struct 
i915_address_space *vm, int sz)
 
 int map_pt_dma(struct i915_address_space *vm, struct drm_i915_gem_object *obj)
 {
+   enum i915_map_type type;
void *vaddr;
 
-   vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+   type = i915_coherent_map_type(vm->i915, obj, true);
+   vaddr = i915_gem_object_pin_map_unlocked(obj, type);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);
 
@@ -39,9 +61,11 @@ int map_pt_dma(struct i915_address_space *vm, struct 
drm_i915_gem_object *obj)
 
 int map_pt_dma_locked(struct i915_address_space *vm, struct 
drm_i915_gem_object *obj)
 {
+   enum i915_map_type type;
void *vaddr;
 
-   vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
+   type = i915_coherent_map_type(vm->i915, obj, true);
+   vaddr = i915_gem_object_pin_map(obj, type);
if (IS_ERR(vaddr))
return PTR_ERR(vaddr);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h 
b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 40e486704558..44ce27c51631 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -527,6 +527,7 @@ int setup_scratch_page(struct i915_address_space *vm);
 void free_scratch(struct i915_address_space *vm);
 
 struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space *vm, int 
sz);
+struct drm_i915_gem_object *alloc_pt_lmem(struct i915_address_space *vm, int 
sz);
 struct i915_page_table *alloc_pt(struct i915_address_space *vm);
 struct i915_page_directory *alloc_pd(struct i915_address_space *vm);
 struct i915_page_directory *__alloc_pd(int npde);
-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

Re: [Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

[Intel-gfx] [PATCH v2 4/7] drm/i915/gtt/dgfx: place the PD in LMEM

4 matches

Site Navigation

Mail list logo

Footer information