[Intel-gfx] [PATCH] drm/i915: Trim partial view sg lists

[Tvrtko Ursulin]

On 26/09/2018 09:15, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-26 09:03:53)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Partial views are small but there can be many of them, and since the sg
>> list space for them is allocated pessimistically, we can save some slab by
>> trimming the unused tail entries.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>> On the other hand with the diminishing importance of mmap ggtt do we
>> expect to see partial views in use these days?
> 
> We have over a decade of hw that depends on them, and mesa is still not
> entirely rid of them (and there's always i915_dri.so). So partials are
> very much sticking around for a long time to come.
> 
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h     | 2 ++
>>   drivers/gpu/drm/i915/i915_gem.c     | 2 +-
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++
>>   3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 8624b4bdc242..0fe22263aa9b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2323,6 +2323,8 @@ static inline struct scatterlist *__sg_next(struct scatterlist *sg)
>>               (((__iter).curr += PAGE_SIZE) >= (__iter).max) ?           \
>>               (__iter) = __sgt_iter(__sg_next((__iter).sgp), false), 0 : 0)
>>   
>> +bool i915_sg_trim(struct sg_table *orig_st);
>> +
>>   static inline unsigned int i915_sg_page_sizes(struct scatterlist *sg)
>>   {
>>          unsigned int page_sizes;
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index db9688d14912..7156ddef178b 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2491,7 +2491,7 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
>>          mutex_unlock(&obj->mm.lock);
>>   }
>>   
>> -static bool i915_sg_trim(struct sg_table *orig_st)
>> +bool i915_sg_trim(struct sg_table *orig_st)
>>   {
>>          struct sg_table new_st;
>>          struct scatterlist *sg, *new_sg;
> 
> You can spare me having to move a few lines later if you move this to
> i915_scatterlist_utils.c?

To be honest I am feeling optimistic today and said to myself I'd try to 
add a proper trim to lib/scatterlist.c. It may take a better part of the 
year to merge it :) but the dumb trim is starting to annoy me.
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index f6c7ab413081..1ec721c20581 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3835,6 +3835,8 @@ intel_partial_pages(const struct i915_ggtt_view *view,
>>                  count -= len >> PAGE_SHIFT;
>>                  if (count == 0) {
>>                          sg_mark_end(sg);
>> +                       i915_sg_trim(st); /* Drop any unused tail entries. */
>> +
> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

With the move to i915_scatterlist_utils.c and can keep it then?

What do you need the new file for - the error state stuff?

Regards,

Tvrtko