On 11/03/2014 05:29 PM, Daniel Vetter wrote:
On Mon, Nov 3, 2014 at 6:20 PM, Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
