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

[Ben Widawsky <ben@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@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@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.
> -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@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center