[Intel-gfx] Discuss GVT context hacks in i915

[Wang, Zhi A]

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?

-Daniel
>
> Thanks,
> Zhi.