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@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. 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
