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 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.
         */
	return xe->irq.enabled;
}


>
> >
> >  	xe_irq_postinstall(xe);
> >
> > --
> > 2.41.0
>
> --
> Ville Syrjälä
> Intel