Re: [RFC 3/5] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/03/2014 05:29 PM, Daniel Vetter wrote:
> On Mon, Nov 3, 2014 at 6:20 PM, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>> I did not like your idea, well I did not think it is feasible - as in easily
>> doable, of stealing the DMA addresses since the SG tables between view don't
>> have a 1:1 relationship in number of chunk/pages.
>
> Ok, so sg table coalescing is getting in the way. On the source sg
> table stored in obj->pages we can fix this by using one of the
> per-page sg table walkers - they'll take care of all the annoying
> details. See how we walk the sg tables in the pte insert fucntions for
> examples.

Obviously I am doing the same elsewhere...

> On the new target sg table I'd simply not bother with merging and
> allocate a full sg table with sg_alloc_table. Due to the rotation
> there won't be a lot of contiguous stuff anyway.

For things like mirrored 2d/3d still it could save space. And it kind of 
feels suboptimal to special case and add low level code when there is a 
nice helper functions which handles it all.

> That leaves the ugly problem of resorting the table without going
> nuts. Either allocate a temporary array (allocated with drm_malloc_ab
> since kmalloc won't be enough for this) of dma_addr_t and use your
> approach. Or just walk the sg_tables row-vise on the rotate layout and
> only fill in the relevant holes for an O(rows*num_pages) complexity. I
> don't expect anyone to actually notice this little inefficient tbh ;-)
>
> In short, I don't see a problem ;-)

I just find it a bit ugly and hackish to poke in sg internals only to 
avoid having get_pages/put_pages (Or call them differently if the name 
bothers you the most. get_page_view/put_page_view perhaps?), even if the 
lifetime of that temporary sg_table is limited to VMA creation (as it in 
fact was in my patch).

So I suppose while you see a vtable as a "serious overkill", I saw it as 
a way to avoid messing around in sg internals and reuse existing code. 
Or in other words:

if (ggtt_view->type == NORMAL)
   pages = get_normal();
else if(ggtt_view->type == X1)
   pages = get_x1();
else if(ggtt_view->type == X2)
    pages = get_x2();

etc.. rather than simply:

if (ggtt_view->get_page_view)
   pages = ggtt_view->get_page_view)
else
    pages = obj->pages;

And be done with it. But of course my chances or merging this are slim 
unless I satisfy your taste so please let me know if your stance here is 
still "unshaken".

Also on the question of getting rid of one of VMA id and ggtt_view type, 
how exactly did you envisage that? Simply to imply that the only users 
of multiple VMAs are the GGTT views?

Regards,

Tvrtko








_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx