[Intel-gfx] Discuss GVT context hacks in i915

[Wang, Zhi A]

Pretty clear. Thanks for the ideas! They really inspire me. :)

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] 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 at lists.freedesktop.org
Subject: Re: [Intel-gfx] 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