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

[Matthew Auld <matthew.auld@intel.com>]

On 26/04/2021 16:22, Tvrtko Ursulin wrote:
> 
> On 26/04/2021 11:18, 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()
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/gen8_ppgtt.c |  5 ++++-
>>   drivers/gpu/drm/i915/gt/intel_gtt.c  | 21 +++++++++++++++++++--
>>   drivers/gpu/drm/i915/gt/intel_gtt.h  |  1 +
>>   3 files changed, 24 insertions(+), 3 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(&ppgtt->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..bbe5b09e59ec 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
>> @@ -7,10 +7,23 @@
>>   #include <linux/fault-inject.h>
>> +#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 dma objects have the same reservation class */
> 
> Class or actual object? And could the comment say why this is important?

It's the dma-resv object. The 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. I can try to improve the comment.

> 
> Regards,
> 
> Tvrtko
> 
>> +    if (!IS_ERR(obj))
>> +        obj->base.resv = &vm->resv;
>> +    return obj;
>> +}
>> +
>>   struct drm_i915_gem_object *alloc_pt_dma(struct i915_address_space 
>> *vm, int sz)
>>   {
>>       struct drm_i915_gem_object *obj;
>> @@ -27,9 +40,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 +54,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);
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel