[Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context reset and switch with Execlists

[Daniel, Thomas]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, August 11, 2014 3:30 PM
> To: Daniel, Thomas
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context
> reset and switch with Execlists
> 
> On Thu, Jul 24, 2014 at 05:04:19PM +0100, Thomas Daniel wrote:
> > From: Oscar Mateo <oscar.mateo at intel.com>
> >
> > These two functions make no sense in an Logical Ring Context &
> > Execlists world.
> >
> > v2: We got rid of lrc_enabled and centralized everything in the
> > sanitized i915.enbale_execlists instead.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index fbe7278..288f5de 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device
> *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int i;
> >
> > +	if (i915.enable_execlists)
> > +		return;
> 
> This will conflict badly with Alistair's patch at a functional level. I'm pretty sure
> that we want _some_ form of reset for the context state, since the hw didn't
> just magically load the previously running context. So NACK on this hunk.
OK I'll wait to see the final version of Alistair's patch and decide what to do
about this hunk.

> 
> > +
> >  	/* Prevent the hardware from restoring the last context (which
> hung) on
> >  	 * the next switch */
> >  	for (i = 0; i < I915_NUM_RINGS; i++) { @@ -514,6 +517,9 @@ int
> > i915_gem_context_enable(struct drm_i915_private *dev_priv)
> >  		ppgtt->enable(ppgtt);
> >  	}
> >
> > +	if (i915.enable_execlists)
> > +		return 0;
> 
> Again this conflicts with Alistair's patch. Furthermore it looks redudant since
> you no-op out i915_switch_context separately.
I don't think this is a conflict.  Doesn't Alistair's change here just involve
writing PDPs for full PPGTT?  We don't want to do that in lrc mode.

> 
> > +
> >  	/* FIXME: We should make this work, even in reset */
> >  	if (i915_reset_in_progress(&dev_priv->gpu_error))
> >  		return 0;
> > @@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs
> > *ring,  {
> >  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >
> > +	if (i915.enable_execlists)
> > +		return 0;
> 
> I've hoped we don't need this with the legacy ringbuffer cmdsubmission fuly
> split out. If there are other paths (resume, gpu reset) where this comes into
> play then I guess we need to look at where the best place is to make this call.
> So until this comes with a bit a better justification I'll punt on this for now.
> -Daniel
Yes, the command submission lrc path doesn't call this but other codepaths
do.  If we keep the check in context_enable() the only remaining call I see is
in i915_gpu_idle().  I don't mind if the check is done there but perhaps a
WARN_ON should then be added into switch_context() because we don't
want to be putting illegal MI_SET_CONTEXT commands into the ring.

Thomas.
>
> > +
> >  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> >
> >  	if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake
> > context */
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch