Re: [Intel-xe] [CI 1/3] drm/xe: proper setting of irq enabled flag

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 22, 2023 at 03:54:09PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 22, 2023 at 08:11:05AM -0400, Rodrigo Vivi wrote:
> > On Fri, Sep 22, 2023 at 11:18:37AM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 21, 2023 at 05:07:58PM -0400, Rodrigo Vivi wrote:
> > > > From: Dani Liberman <dliberman@habana.ai>
> > > >
> > > > IRQ enabled flag should be set only after request irq succeeds.
> > > >
> > > > Reviewed-by: Ohad Sharabi <osharabi@habana.ai>
> > > > Signed-off-by: Dani Liberman <dliberman@habana.ai>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_irq.c | 8 +++-----
> > > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> > > > index ccb934f8fa34..f98aa1f06c8f 100644
> > > > --- a/drivers/gpu/drm/xe/xe_irq.c
> > > > +++ b/drivers/gpu/drm/xe/xe_irq.c
> > > > @@ -590,16 +590,14 @@ int xe_irq_install(struct xe_device *xe)
> > > >  		return -EINVAL;
> > > >  	}
> > > >
> > > > -	xe->irq.enabled = true;
> > > > -
> > > >  	xe_irq_reset(xe);
> > > >
> > > >  	err = request_irq(irq, irq_handler,
> > > >  			  IRQF_SHARED, DRIVER_NAME, xe);
> > > > -	if (err < 0) {
> > > > -		xe->irq.enabled = false;
> > > > +	if (err < 0)
> > > >  		return err;
> > > > -	}
> > > > +
> > > > +	xe->irq.enabled = true;
> > >
> > > Why does this even exist?
> > 
> > I had tried to kill this, but then I noticed it was needed
> > for i915-display.
> > 
> > @drivers/gpu/drm/xe/display/ext/i915_irq.c:
> > 
> > bool intel_irqs_enabled(struct xe_device *xe)
> > {
> >  	/*
> >          * XXX: i915 has a racy handling of the irq.enabled, since it doesn't
> >          * lock its transitions. Because of that, the irq.enabled sometimes
> >          * is not read with the irq.lock in place.
> >          * However, the most critical cases like vblank and page flips are
> >          * properly using the locks.
> >          * We cannot take the lock in here or run any kind of assert because
> >          * of i915 inconsistency.
> >          * But at this point the xe irq is better protected against races,
> >          * although the full solution would be protecting the i915 side.
> >          */
> 
> None of that really makes sense to me. It's a single boolean so locking
> is pointless.
> 
> Hmm, I guess we are using it for runtime pm to make sure we don't
> try to access the device when it's asleep. But that should only
> really happen if we would be using a shared legacy interrupt.

hmmm.. should we change those cases to use pm_runtime_suspended kind
of check then?

> 
> And the other user is the power well pipe IER/IMR initialization stuff,
> where I suppose we are trying to not unmask/enable the per-pipe
> interrupts if interrupts in general are supposed to be off. Though
> the higher level interrupts should still be masked off anyway
> so not sure that is actually required except to maintain 100% state
> in all irq registers.

hmm... good point. should we then ensure we are only checking that
for legacy?

> 
> Everything else just looks like asserts to make sure we haven't
> screwed the init/resume/etc. order too badly.

yeap, I honestly couldn't make sense on why in many of them, but
it looks the case.

But with all of this in mind, the comment here indeed is not good.

But anyway, it is not related to this series. While we don't change
or remove the need for that check, this series looks correct to me,
so I just went ahead and pushed it.

Any modification on the irq.enabled should be a follow-up.

> 
> > 	return xe->irq.enabled;
> > }
> > 
> > 
> > >
> > > >
> > > >  	xe_irq_postinstall(xe);
> > > >
> > > > --
> > > > 2.41.0
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel