[Intel-gfx] [RFC PATCH 00/11] i915 HW context support

[Daniel Vetter]

On Tue, Feb 14, 2012 at 10:09:07PM +0100, Ben Widawsky wrote:
> These patches are a heavily revised version of the patches I wrote over
> a year ago. These patches have passed basic tests on SNB, and IVB, and
> older versions worked on ILK.  In theory, context support should work
> all the way back to Gen4, but I haven't tested it. Also since I suspect
> ILK may be unstable, so the code has it disabled for now.
> 
> HW contexts provide a way for the GPU to save an restore certain state
> in between batchbuffer boundaries. Typically, GPU clients must re-emit
> the entire state every time they run because the client does not know
> what has been destroyed since the last time. With these patches the
> driver will emit special instructions to do this on behalf of the client
> if it has registered a context, and included that with the batchbuffer.
> 
> [... From Ken Graunke ]
> This is needed to properly implement Transform Feedback in the presence
> of Geometry Shaders, since software wouldn't be able to track the SVBI0
> register to restore it to its previous state.
> 
> The IOCTLs defined here may also be used with ppgtt (real ppgtt) but the
> interface may be insufficient for that. The contexts do provide a
> 
> I've tested this with an experiemental mesa from Ken Graunke, as well as
> some basic i-g-t tests.
> 
> i-g-t:
> git://people.freedesktop.org/~bwidawsk/intel-gpu-tools context_support
> 
> libdrm:
> git://people.freedesktop.org/~bwidawsk/drm context_support
> 
> kernel:
> git://people.freedesktop.org/~bwidawsk/drm-intel context_support
> 
> I'm getting really tired of looking at these, please help me polish them
> up and make them more ready for consumption. I did consider moving the
> power context allocation to this set of APIs, but it didn't seem worth
> it to me. I can go either way.
> 
> Ben Widawsky (11):
>   drm/i915: MI_ARB_ON_OFF
>   drm/i915: track tlb invalidate GFX_MODE state
>   drm/i915: PIPE_CONTROL_TLB_INVALIDATE
>   drm/i915/context: CXT_SIZE register offsets added
>   drm/i915/context: Preliminary context support
>   drm/i915/context: ringbuffer context switch code
>   drm/i915/context: implementation details
>   drm/i915/context: extend contexts to execbuffer2
>   drm/i915/context: add params
>   drm/i915/context: track contexts per device
>   drm/i915: show contexts in debugfs
> 
>  drivers/gpu/drm/i915/Makefile              |    1 +
>  drivers/gpu/drm/i915/i915_context.c        |  562 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_debugfs.c        |    6 +
>  drivers/gpu/drm/i915/i915_dma.c            |   10 +
>  drivers/gpu/drm/i915/i915_drv.h            |   47 +++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   26 ++-
>  drivers/gpu/drm/i915/i915_reg.h            |   17 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  122 ++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |   11 +-
>  include/drm/i915_drm.h                     |   18 +
>  10 files changed, 815 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_context.c

Eric summed up most of my concerns when I've quickly read through these,
a few are left though. First are bikesheddy in nature, but they hindered
me a bit in digging into the last one, so here we go.

- The patch splitting is a bit strange for me to follow. For new features
  I prefere patches split up into:
  * prep work on existing code, if necessary
  * implement the feature
  * applying any ugly workarounds (if they warrant special attention)
  * enable/use the new feature/wire up the new interfaces
  I don't expect you to rework the entire series, but I think some more
  descriptive commit message would help a lot here (e.g. "implementation
  details" is imo a bit too generic for a patch headline).

- You add a new ringbuffer function pointer but don't actually use it to
  abstract away any generation/chip specific things. Imo such indirection
  should only be added when it's actually used - we then stand a chance to
  get it somewhat right and useful. Furthermore I don't have a high notion
  of the ringbuffer interface, so better not shove more stuff on top of
  it.

Now the real thing is that I expected some decent reference-counting on
the context backing storage bo to get the lifetimes right, but haven't
found it. And indeed you have a comment in the context free function to
the effect that if the switch away form a context fails things will blow
up.

I think when you switch the low-level context handling and switching code
over to just deal with the storage bo you can properly reference count
that and just keep onto that if the ring still uses it, but the userspace
handle gets freed. It should work pretty much exactly like framebuffer
objects and pageflipping (safe for that pageflips run on a different
asynchronous queue than render stuff, so they're a bit more funky and you
can't use the active list to help you in the object tracking).

I think it would even make sense to make the default context just a bo
associated with the render ring and ditch the context struct around it -
but I couldn't follow this idea through the convoluted patch series
structure.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48