[Intel-gfx] [PATCH] drm/i915/guc: make forwarding of VBlanks conditional

[Ville Syrjälä]

On Fri, Jun 17, 2016 at 04:14:48PM +0200, Daniel Vetter wrote:
> On Fri, Jun 17, 2016 at 04:51:34PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 17, 2016 at 02:42:39PM +0100, Dave Gordon wrote:
> > > Apparently we shouldn't forward VBlanks to the GuC unconditionally,
> > > as it may trigger a race condition in the GuC's internal dispatcher.
> > > 
> > > From an internal message from Harsh Chheda <harsh.j.chheda at intel.com>
> > > >
> > > > If the context has switched out the [GuC] scheduler should know so
> > > > that it can  put the context in wait state and schedule something
> > > > else.  If the context which is waiting on Vblank is still on the CS
> > > > then [the GuC] scheduler does not need to know. If in this case wait
> > > > on Vblank is sent to Guc while the context is still on CS there is
> > > > an edge condition when the Vblank may get satisfied right after
> > > > sending interrupt to guc and context may complete. Scheduler will
> > > > then incorrectly mark the context to be in wait state.
> > > 
> > > So we need to change this register setting to GFX_FORWARD_VBLANK_COND.
> > 
> > No one has ever managed to explain why the guc needs to get vblanks
> > in the first place. The original commit message adding this stuff is
> > not helpful at all.
> 
> It's used for the guc's magic slpc so that it can try to hit flips at
> vrefresh.

Is that a documented fact, or just an assumption? There's nothing
in the commit message, there's no comment in the code, and I don't
see anything in the spec either.

> I maintain that this design is bonghits, and we should forward

I assume you mean "should not".

> _any_ display interrupts to guc. Instead the kernel should tell guc when
> it's doing a sloppy job and missing deadlines. Because the kernel actually
> knows, and doesn't have to infer what might be going on by watching
> vblanks and flip_complete interrupts.
> 
> Also, with atomic we won't be using cs flips any more, who knows whether
> that guc magic even still works with that ...
> 
> So I'd vote to just stop telling guc this stuff and be done with it.

I would have to agree.

> -Daniel
> 
> > 
> > > 
> > > Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> > > Cc: Harsh Chheda <harsh.j.chheda at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > > index 8fe96a2..af9f51e 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > > @@ -106,7 +106,7 @@ static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
> > >  	u32 tmp;
> > >  
> > >  	/* tell all command streamers to forward interrupts and vblank to GuC */
> > > -	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_ALWAYS);
> > > +	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_COND);
> > >  	irqs |= _MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING);
> > >  	for_each_engine(engine, dev_priv)
> > >  		I915_WRITE(RING_MODE_GEN7(engine), irqs);
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC