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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Apr 17, 2018 at 01:01:39PM -0700, Dhinakaran Pandiyan wrote:
> 
> 
> 
> 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@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.
> > 
> 
> 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!

I'm far behind this week and merging the series was on top of my todo list...
:( sorry for that.

I know that you guys already talked about it and know how to do the follow-up ;)

> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx