Re: [PATCH v4] drm/i915: Enable edp psr error interrupts on hsw

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

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@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@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Jose Roberto de Souza <jose.souza@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.

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx