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

[Daniel Vetter]

On Tue, Nov 04, 2014 at 11:15:16AM +0000, Tvrtko Ursulin wrote:
> 
> On 11/03/2014 05:29 PM, Daniel Vetter wrote:
> >On Mon, Nov 3, 2014 at 6:20 PM, Tvrtko Ursulin
> ><tvrtko.ursulin at 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.

If space would be a concern we wouldn't use sg tables. They have a lot of
fluff. Also afaik there's not sg interface to create an sg table from a
list of dma_addr_t, which is what we want.

> >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).

sg tables are just data structures for drivers to map dma memory ranges.
As a driver your supposed to noodle around in them. So maybe I don't
understand your concern?

Wrt naming, get/put imply reference counting, which this doesn't do.
create_page_view/free_page_view I think are better names. Not sure about
the page_view name though.

> 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".

Well imo vfuncs are a pain, so we generally add them only when there's
more than 2 implementations. Atm we have one. So if the other two show up
then I'm ok, but before that it just makes chasing callchains more
annoying.

> 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?

Yeah. We don't have anything else, and I don't expect anything else
really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch