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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

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.

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

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

-- 
Ville Syrjälä
Intel