[Intel-gfx] [PATCH v4] drm/i915: Enable edp psr error interrupts on hsw

[Dhinakaran Pandiyan]

On Tue, 2018-04-17 at 20:41 +0300, Ville Syrjälä wrote:
> On Mon, Apr 16, 2018 at 05:43:54PM -0700, Paulo Zanoni wrote:
> > Em Qui, 2018-04-05 às 15:00 -0700, Dhinakaran Pandiyan escreveu:
> > > From: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > 
> > > The definitions for the error register should be valid on bdw/skl
> > > too,
> > > but there we haven't even enabled DE_MISC handling yet.
> > > 
> > > Somewhat confusing the the moved register offset on bdw is only for
> > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > > on bdw.
> > > 
> > > v2: Fixes from Ville.
> > > 
> > > v3: From DK
> > >  * Rebased on drm-tip
> > >  * Removed BDW IIR bit definition, looks like an unintentional change
> > > that
> > > should be in the following patch.
> > > 
> > > v4: From DK
> > >  * Don't mask REG_WRITE.
> > > 
> > > References: bspec/11974 [SRD Interrupt Bit Definition DevHSW]
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Reviewed-by: Jose Roberto de Souza <jose.souza at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 34
> > > ++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h |  8 ++++++++
> > >  2 files changed, 42 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index 27aee25429b7..c2d3f30778ee 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2391,6 +2391,26 @@ static void ilk_display_irq_handler(struct
> > > drm_i915_private *dev_priv,
> > >  		ironlake_rps_change_irq_handler(dev_priv);
> > >  }
> > >  
> > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > +	u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > > +
> > > +	if (edp_psr_iir & EDP_PSR_ERROR)
> > > +		DRM_DEBUG_KMS("PSR error\n");
> > > +
> > > +	if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > > +		DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
> > > +		I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > 
> > Why are we masking it here? During these 2 vblanks it's possible that
> > something will happen (e.g., frontbuffer writing, cursor moving, page
> > flipping).
> 
> The masking was here to avoid seeing a big flood of entry interrupts.
> 
> > Are we guaranteed to get a POST_EXIT interrupt even if we
> > give up entering PSR before it is actually entered?
> 
> No. The exit interrupt only happens if you actually reached PSR.
> 
> > 
> > 
> > > +	}
> > > +
> > > +	if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > > +		DRM_DEBUG_KMS("PSR exit completed\n");
> > > +		I915_WRITE(EDP_PSR_IMR, 0);
> > > +	}
> > > +
> > > +	I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > > +}
> > > +
> > >  static void ivb_display_irq_handler(struct drm_i915_private
> > > *dev_priv,
> > >  				    u32 de_iir)
> > >  {
> > > @@ -2403,6 +2423,9 @@ static void ivb_display_irq_handler(struct
> > > drm_i915_private *dev_priv,
> > >  	if (de_iir & DE_ERR_INT_IVB)
> > >  		ivb_err_int_handler(dev_priv);
> > >  
> > > +	if (de_iir & DE_EDP_PSR_INT_HSW)
> > > +		hsw_edp_psr_irq_handler(dev_priv);
> > > +
> > >  	if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > >  		dp_aux_irq_handler(dev_priv);
> > >  
> > > @@ -3260,6 +3283,11 @@ static void ironlake_irq_reset(struct
> > > drm_device *dev)
> > >  	if (IS_GEN7(dev_priv))
> > >  		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> > >  
> > > +	if (IS_HASWELL(dev_priv)) {
> > > +		I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > > +		I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > > +	}
> > 
> > We need another IIR write we do for the other platforms. This is not
> > cargo cult (as mentioned in previous review emails), this is required
> > since our hardware is able to store more than one IIR interrupt. Please
> > do it like we do for the other interrupts.
> 

My reply and the review were specifically about the POSTING_READ(). I
agree on the second IIR write, but that was not what the original review
comment was about.

I'm still skeptical about inserting the POSTING_READ() between write's
There are at commit messages and emails that indicate the
POSTING_READS() are indeed cargo culted. Let me find more data about
this and get back.

> I don't remember if the PSR IIR is double buffered or not. I would
> assumeme it is. It is super easy to verify though if you want to be
> sure:
> 1. unmask the interrupt
> 2. generate at least two interrupts
> 3. mask the interrupt
> 4. clear IIR
> 5. read IIR
> 6. clear IIR
> 7. read IIR
> 
> Step 5 should still show the IIR bit set if the IIR is double buffered.
> And step 7 it should show it to be clear.
> 

I'll give it a try, thanks!