Pretty clear. Thanks for the ideas! They really inspire me. :) -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter Sent: Tuesday, February 16, 2016 1:20 AM To: Wang, Zhi A Cc: Vetter, Daniel; Joonas Lahtinen; Chris Wilson; Lv, Zhiyuan; Tian, Kevin; intel-gfx@xxxxxxxxxxxxxxxxxxxxx Subject: Re: Discuss GVT context hacks in i915 On Mon, Feb 15, 2016 at 04:03:49PM +0000, Wang, Zhi A wrote: > Hi Daniel: > Thanks for shedding the light! See my comments below. :) > > -----Original Message----- > From: Vetter, Daniel > Sent: Monday, February 15, 2016 5:32 PM > To: Wang, Zhi A; Joonas Lahtinen; Chris Wilson; Lv, Zhiyuan; Tian, Kevin > Subject: Re: Discuss GVT context hacks in i915 > > Am 15/02/2016 um 10:11 schrieb Zhi Wang: > > Hi Guys: > > Previously we have sent our GVT-g device model RFC code, and thanks > > for the advices and comments! As you see GVT-g needs a special kind of > > LRC context as shadow context so it could be able to use i915 GEM > > submission subsystem, which requires some changes in i915. > > > > Probably we can discuss them one by one and settle them down before > > the next version of RFC code. :) > > > > Difference between a i915 LRC context and a GVT LRC context > > ------ > > > > * The engine context initialization of a GVT LRC context can be > > bypassed, as GVT-g will initialize it by guest LRC context. > > > > http://www.spinics.net/lists/intel-gfx/msg86546.html > > > > * The addressing mode bit in the context descriptor should be able to > > configured at runtime, as some guest is still using 32bit PPGTT > > address space. > > > > http://www.spinics.net/lists/intel-gfx/msg86544.html > > > > * As the GVT LRC context will go with the shadow PPGTT page table > > populated by GVT-g, a GVT LRC context doesn't need a i915 PPGTT > > instance, also its PDP root pointer doesn't need to be populated by i915. > > > > http://www.spinics.net/lists/intel-gfx/msg86545.html > > > > * The ring buffer size of a GVT LRC context ring buffer object should > > be configurable or hard-coded to the max size, as some guest (e.g. > > windows) has a large ring buffer and could submit a lot of commands at > > one time. > > > > Hacking i915 > > ------- > > > > The above changes needs to hack i915, in the RFC code, you can see a > > lot of codes like below: > > > > if (!gvt_context) { > > /* do something i915 needed */ > > } else { > > /* do something GVT-g needed */ > > } > > > > This kinds of code piece is GVT-g specific, another approach should be > > refining some i915 APIs or say something generic, like: > > > > if (context->need_i915_ppgtt) { > > /* allocating i915 ppgtt instance */ > > } > > > > if (context->ppgtt) { > > /* update PDP root pointers in LRC context. */ > > } > > > > if (context->need_initialization) { > > /* emit commands to initialize the engine context of LRC context */ > > } > > > > I'm wondering which kinds do you prefer? Or maybe you can give me some > > advices? :) > > Without looking at more than what's in your mail here and in the links, > this style is the midlayer mistake: We have some abstraction (lrc > context, ppgtt) and force everyone to go through the same code paths. > Which means deep down in those paths we have lots of if (special_case) > conditions, which make the code a maintainance nightmare. Yes execlist > support has already a lot of such bad examples unfortunately. > > The better design idea is to reuse the data structures and helper > functions, but have new top-level entry functions for creating e.g. a > xengt lrc context. So e.g. have a lrc init function for xengt which > takes the setup stuff as parameters. Wrt ppgtt my idea was to reuse > struct i915_hw_ppgtt for managing the shadow pagetable, with xengt using > the i915_gem_gtt.c functions to write shadow pagetable entries. That way > i915 still knows the virtual->physical mapping, which aids in e.g. crash > dump recording. Of course you're not going to bind entire vma, but > instead will use the lower-level functions that just bind pages. > > [Zhi] Thanks! Just want to make sure that you prefer that GVT-g specific > modifications should be put into a fork of top-level i915 APIs? For example, > we prepare a new function to create the GVT context, which is a fork of > simplified i915_gem_create_context(). > > For i915_hw_ppgtt and GVT-g shadow page table, we tried to think about how > to merge these two similar things into one, but have some opens: > > Most of the GTT/PPGTT page table entry routines in i915_gem_gtt.c, e.g. the > abstractions/ insert_entries() are aimed to generate the page table entry, but > GVT-g shadow page implementation also need the per-platform page table > entry bit field extraction routines. For example, extract the GFN from guest page > table, which means we have to add some new callbacks which native i915 > will not use at all. Is it OK for host i915 to add such kinds of callbacks? > > b. GVT-g shadow page table implementation should be the most complicated > part in GVT-g, maybe the first easy step should be putting the shadow page > table root pointer into i915_hw_ppgtt. E.g. GVT-g allocates a fake i915_hw_ppgtt > only use it to store root pointer and addressing mode bit? > > For ppgtt size selection I think we should have a field for that in > i915_hw_ppgtt, that the lrc setup code reads. > > For the ringbuffer size you could just create a fake ringbuffer object I > think. > > [Zhi] You mean we should add another ring buffer object allocation function? In GVT > context we reuse the i915 ring buffer submission interface like intel_ring_{begin, > advance}. If we follow this direction above, probably we should abstract the ringbuffer > object allocation functions, and put different ring buffer allocation callbacks when > initialize the intel ring buffer object? Like I said I didn't look at the code. If you already use the existing ringbuffer alloc functions just add a low-levl one that has an additional size parameter instead of the deafult. Assuming that works for you. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
