[PATCH] drm: modify pages_to_sg prime helper to create optimized SG table

[Daniel Vetter]

On Thu, Jan 31, 2013 at 2:26 PM, Rahul Sharma <r.sh.open at gmail.com> wrote:
>> The other thing is that only doing sg entry coalescing for
>> prime/dma_buf will lead to a QA problem, since all the normal sg
>> tables still have a 1:1 relationship. So imo the right way to move
>> forward with this is to convert the various sg table constructors in
>> i915/ttm/radeon/nouveau/... over to coalesce pages. This allows us to
>> fix any fallout step-by-step.
>>
>> Then, once each driver is ready for this, we can merge your patch.
>
> Looks fine to me.

I've looked some more at the drivers, and it seems like ttm uses the
sg tables only for dma_buf imported objects. Also, all drivers already
use drm_prime_sg_to_page_addr_arrays, which does the right thing. So
ttm drivers should indeed be all fine (I've stopped chaising
callchains before hitting ttm_tt_populate, where the magic happens).

So that seems to only leave i915, which has a few backing storage
walkers in i915_gem_execbuffer.c and the sg constructor
i915_gem_object_get_pages_gtt in i915_gem.c. That one is a bit a pain
since it doesn't start out with a simple struct page * array. But I
think we could just coalesce sg entries anyway without reaping any
size benefits for the allocate sg table, to ensure that i915 doesn't
break accidentally when facing such an sg table.

I'm rather busy the next few days, but should be able to look into
patches later on.

>>>> For the patch itself I think there's now a generic pages_to_sg helper
>>>> in the dma core which does exactly what you want it to do. I can dig
>>>> it out if you can't find it.
>>>>
>>>
>>> Sorry, I din't get this part. Please elaborate a bit.
>>
>> See sg_alloc_table_from_pages in lib/scatterlist.c introduced with:
>>
>> commit efc42bc98058a36d761b16a114823db1a902ed05
>> Author: Tomasz Stanislawski <t.stanislaws at samsung.com>
>> Date:   Mon Jun 18 09:25:01 2012 +0200
>>
>>     scatterlist: add sg_alloc_table_from_pages function
>>
>
> Yes, my patch is using sg_alloc_table_from_pages

Oops, I've totally missed that. My apologies for the confusion.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch