[Intel-gfx] [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+

[Paulo Zanoni]

2014-03-19 14:50 GMT-03:00 Ben Widawsky <ben at bwidawsk.net>:
> On Wed, Mar 19, 2014 at 09:28:32AM +0100, Daniel Vetter wrote:
>> On Tue, Mar 18, 2014 at 11:20:09AM -0700, Ben Widawsky wrote:
>> > On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote:
>> > > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > >
>> > > Instead of trying to clear it again. It should already be masked and
>> > > disabled and zeroed at preinstall/uninstall.
>> > >
>> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
>> > >  1 file changed, 15 insertions(+), 17 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > > index 6d4daf2..4d0a8b1 100644
>> > > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > > @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>> > >   I915_WRITE(type##IIR, 0xffffffff); \
>> > >  } while (0)
>> > >
>> > > +/*
>> > > + * We should clear IMR at preinstall/uninstall, and just check at postinstall.
>> > > + */
>> > > +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
>> > > + u32 val = I915_READ(reg); \
>> > > + if (val) \
>> > > +         DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
>> > > +                   (reg), val); \
>> > > +} while (0)
>> > > +
>> > >  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
>> > > + GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
>> > >   I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
>> > >   I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
>> > >  } while (0)
>> > >
>> > >  #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
>> > > + GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
>> > >   I915_WRITE(type##IMR, (imr_val)); \
>> > >   I915_WRITE(type##IER, (ier_val)); \
>> > >  } while (0)
>> >
>> > Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ
>> > which gives an error that we can do nothing about other than clear it
>> > anyway.
>> >
>> > I'd be in favor of dropping this patch.
>>
>> The point of the assert is to make sure that the new IIR clearing logic
>> with blocking everything+clearing in the preinstall hook actually does
>> what it's supposed to do.
>>
>> Since the point of this exercise is to reuse this code for runtime
>> suspend/resume where races are much easier to hit I think this is a good
>> self-check of the code.

Just an additional comment: the current code just writes to IIR once
in case it's not zero, and this is not a guarantee that it will be
cleared. So if we decided to drop this patch, I would suggest to fix
the IIR-clearing code.

>> -Daniel
>>
>
> Okay, I am feeling somewhat pressured to stick a reviewed-by on this
> since Daniel likes it.
>
> Change the macro to WARN instead of DRM_ERROR, and, clear the IIR if
> it's non-zero. With that change, it's:
> Reviewed-by-with-reservations: Ben Widawsky <ben at bwidawsk.net>
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni