Re: [PATCH] drm/i915/guc: make forwarding of VBlanks conditional

[Daniel Vetter <daniel@ffwll.ch>]

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@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. I maintain that this design is bonghits, and we should forward
_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.
-Daniel

> 
> > 
> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> > Cc: Harsh Chheda <harsh.j.chheda@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx