[Intel-gfx] [PATCH 06/11] drm/i915/context: ringbuffer context switch code

[Ben Widawsky]

On Wed, 15 Feb 2012 12:11:58 -0800
Eric Anholt <eric at anholt.net> wrote:

> On Tue, 14 Feb 2012 22:09:13 +0100, Ben Widawsky <ben at bwidawsk.net> wrote:
> > This is the HW dependent context switch code.
> > 
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |    3 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |  117 +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |    6 ++-
> >  3 files changed, 125 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 34e6f4f..4175929 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -965,6 +965,9 @@ struct drm_i915_gem_context {
> >  	bool is_initialized;
> >  };
> >  
> > +#define I915_CONTEXT_NORMAL_SWITCH	(1 << 0)
> > +#define I915_CONTEXT_SAVE_ONLY		(1 << 1)
> > +#define I915_CONTEXT_FORCED_SWITCH	(1 << 2)
> >  #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
> >  
> >  #define IS_I830(dev)		((dev)->pci_device == 0x3577)
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index e71e7fc..dcdc80e 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -942,6 +942,122 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
> >  	return 0;
> >  }
> >  
> > +static int do_ring_switch(struct intel_ring_buffer *ring,
> > +			  struct drm_i915_gem_context *new_context,
> > +			  u32 hw_flags)
> 
> Can we call this function do_mi_set_context()?  It doesn't look like it
> has to do with switching rings.

Sure.

> 
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	int ret = 0;
> > +
> > +	if (!new_context->is_initialized) {
> > +		ret = ring->flush(ring, 0, 0);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = intel_ring_begin(ring, 2);
> > +		if (ret)
> > +			return ret;
> > +
> > +		intel_ring_emit(ring, MI_NOOP | (1 << 22) | new_context->id);
> > +		intel_ring_emit(ring, MI_NOOP);
> > +		intel_ring_advance(ring);
> > +	}
> 
> Not sure what this block is doing, nor have the docs enlightened me.
> Comment?

This incantation came from a document which I can no longer find. I'll
try to remove it and see if anything breaks. It's likely this was from
an old ILK doc if you're curious enough to look (mine doesn't appear old
enough)

> 
> > +	if (IS_GEN6(dev) && new_context->is_initialized &&
> > +	    ring->itlb_before_ctx_switch) {
> > +		/* w/a: If Flush TLB Invalidation Mode is enabled, driver must
> > +		 * do a TLB invalidation prior to MI_SET_CONTEXT
> > +		 */
> > +		gen6_render_ring_flush(ring, 0, 0);
> > +	}
> > +
> > +	ret = intel_ring_begin(ring, 6);
> > +	if (ret)
> > +		return ret;
> > +
> > +	intel_ring_emit(ring, MI_NOOP);
> > +
> > +	switch (INTEL_INFO(dev)->gen) {
> > +	case 5:
> > +		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
> > +		break;
> > +	case 6:
> > +		intel_ring_emit(ring, MI_NOOP);
> > +		break;
> > +	case 7:
> > +		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > +		break;
> > +	case 4:
> > +	default:
> > +		BUG();
> > +	}
> 
> I can't see what this MI_ARB_ON_OFF is about.  We don't use run lists,
> so preemption can't occur, right?  And if it's needed on gen7, why isn't
> it needed on the previous chipsets, where the command apparently exists
> as well?

Just following the docs to the letter on the ARB_ON_OFF thing. We may
need the TLB flush on gen7 as well as gen6. I choose the BSPEC
workaround list as the ultimate guide. As best I can tell, the logic is
correct according to that.

> 
> Also, MI_SUSPEND_FLUSH?  (also exists on all chispets) It "Blocks MMIO
> sync flush or any flushes related to VT-d while enabled."  We don't use
> sync flushes, and presumably if we do VT-d related flushes, we still
> want them to work, right?  Why do hardware render contexts change that?

I can't find this one either anymore. This I very clearly recall as a
required workaround for ILK. Since I'm not exposing this currently for
less than GEN6 this is a don't care. My current ILK docs are not the
same as the ones I used when I wrote this. If you look at
ironlake_enable_rc6() you can also see this workaround used.

> 
> > +	intel_ring_emit(ring, MI_SET_CONTEXT);
> > +	intel_ring_emit(ring, new_context->obj->gtt_offset |
> > +			MI_MM_SPACE_GTT |
> > +			MI_SAVE_EXT_STATE_EN |
> > +			MI_RESTORE_EXT_STATE_EN |
> > +			hw_flags);
> 
> 
> > +static struct drm_i915_gem_context *
> > +render_ring_context_switch(struct intel_ring_buffer *ring,
> > +			   struct drm_i915_gem_context *new_context,
> > +			   u32 flags)
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	bool force = (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
> > +	struct drm_i915_gem_context *last = NULL;
> > +	uint32_t hw_flags = 0;
> > +
> > +	/* last_context is only protected by struct_mutex */
> > +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > +
> > +	BUG_ON(new_context->obj == NULL || new_context->obj->gtt_offset == 0);
> > +
> > +	if (!force && (ring->last_context == new_context))
> > +		return new_context;
> > +
> > +	if (flags & I915_CONTEXT_SAVE_ONLY)
> > +		hw_flags = MI_RESTORE_INHIBIT;
> > +
> > +	if (do_ring_switch(ring, new_context, hw_flags))
> > +		return NULL;
> > +
> > +	last = ring->last_context;
> > +	ring->last_context = new_context;
> > +
> > +	/* The first context switch with default context is special */
> > +	if (last == NULL && new_context->is_default)
> > +		return new_context;
> 
> I'm concerned that by returning new_context here, we're about to
> release_context() and unpin the context we just switched to, in
> i915_switch_context().  I think we don't want to ever return new_context
> here, and the caller wants to not do the request stuff in the case that
> From == NULL.

The only case where last is ever null is when setting the default
context. I can add an else BUG() to this if it makes you more
comfortable. I used to have a comment about how this is an ugly hack
where a NULL return means error, and anything else means success. I
should probably add that comment back, not sure what happened to it. But
I only care about this when initializing the default context.