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@xxxxxxxxx> > > > > > > > > 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@xxxxxxxxx> > > > Cc: Harsh Chheda <harsh.j.chheda@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
