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

[Ben Widawsky]

On Wed, 15 Feb 2012 23:12:20 +0100
Daniel Vetter <daniel at ffwll.ch> wrote:

> 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).

I will try to rework the series for better ordering. Since Eric also
stated his distaste for the ordering. I believe the patches are correct
as is, just the ordering needs some tweaks, so shouldn't be too much
work.

> 
> - 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.

I'll sum up our private conversations on this matter... You dislike
intel_ringbuffer.c and are therefore quite partial. I really don't care
much one way or the other. Context switching should work for media ring
iirc, and I do get to take a shortcut to determine if the pointer is
NULL. I don't want to argue whose opinion deserves more merit, so I'll
just change it and move on (and now my shot, it would have been easier
if you responded to the patch that added the function pointer instead of
the intro email).
> 
> 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.

The reference counting (aside from creation type stuff) should all be
encapsulated in i915_switch_context. There it should add the relevant bo
to the active list as needed, and it should be freed by the existing
infrastructure. The free case is a bit special. I'll look into a proper
fix, but currently the only way it can fail is if the ring buffer is
full, I think - so a few retries should handle all but the absurd.

> 
> 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.

Could you try to explain your first point a bit differently, or maybe we
can discuss it when I see you next week? I don't really understand what
you're asking for.

As for the default context being just a bo associated with the ring, I
really don't see much difference here, if you want it badly, we can do
it, but I think it's nice to keep it mostly not a special case, save for
the context id being special.

> 
> Cheers, Daniel

Thanks for the reviewing.