[Intel-gfx] [PATCH v2 3/4] drm/i915/psr: Control PSR interrupts via debugfs

[Pandiyan, Dhinakaran]

On Wed, 2018-03-28 at 13:28 +0300, Ville Syrjälä wrote:
> On Tue, Mar 27, 2018 at 06:33:11PM +0000, Pandiyan, Dhinakaran wrote:
> > On Tue, 2018-03-27 at 13:24 +0300, Ville Syrjälä wrote:
> > > On Mon, Mar 26, 2018 at 06:16:22PM -0700, Dhinakaran Pandiyan wrote:
> > > > Interrupts other than the one for AUX errors are required only for debug,
> > > > so unmask them via debugfs when the user requests debug.
> > > > 
> > > > User can make such a request with
> > > > echo 1 > <DEBUG_FS>/dri/0/i915_edp_psr_debug
> > > > 
> > > > v2: Unroll loops (Ville)
> > > >     Avoid resetting error mask bits.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 36 +++++++++++++++++++++++-
> > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > >  drivers/gpu/drm/i915/i915_irq.c     | 55 +++++++++++--------------------------
> > > >  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
> > > >  drivers/gpu/drm/i915/intel_psr.c    | 54 ++++++++++++++++++++++++++++++++++++
> > > >  5 files changed, 108 insertions(+), 40 deletions(-)
> > > > 
<snip>
> > > >  
> > > > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable)
> > > > +{
> > > > +	u32 mask;
> > > > +
> > > > +	/* No PSR interrupts on VLV/CHV */
> > > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > +		return;
> > > > +
> > > > +	mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
> > > > +	       EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 8)
> > > > +		mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
> > > > +			EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
> > > > +			EDP_PSR_POST_EXIT(TRANSCODER_B) |
> > > > +			EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
> > > > +			EDP_PSR_POST_EXIT(TRANSCODER_C) |
> > > > +			EDP_PSR_PRE_ENTRY(TRANSCODER_C);
> > > > +
> > > > +	if (enable) {
> > > > +		WRITE_ONCE(dev_priv->psr.debug, true);
> > > > +		I915_WRITE(EDP_PSR_IMR, I915_READ(EDP_PSR_IMR) & ~mask);
> > > 
> > > Why RMW?
> > > 
> > Avoids updating this function when new PSR error bits are added in
> > i915_irq.c
> 
> The usual rule is "avoid RMW unles there is a really compelling reason
> for it". There is none in this case AFAICS.
> 
> > 
> > Would you prefer 
> > 
> > mask |= EDP_PSR_ERROR(TRANCODER_A) | ...  here? 
> > 
> > I think this has started to look ugly already. The loop was concise IMO.
> > 
> > The other option is to
> > 
> > #define HSW_PSR_INTR_DBG_MASK = 0x7
> > #define BDW_PSR_INTR_DBG_MASK = 0x07070707 
> 
> Not sure what these have to do with the RMW.
> 
They don't, that was an alternative to the series of OR's we have to do
to derive the mask.

0x07070707 is the mask of all the relevant bits (error + debug). The
above definitions could be used as 

if (INTEL_GEN(dev_priv) > 8)
	mask = BDW_PSR_INTR_DBG_MASK;
else
	mask = HSW_PSR_INTR_DBG_MASK


if (enable)
	I915_WRITE(EDP_PSR_IMR, ~mask)

Anyway, I am not sure how you want this to be written at this point. Can
you suggest in code what you like to see here?

> > 
> > 
> > 
> > > > +	} else {
> > > > +		I915_WRITE(EDP_PSR_IMR, I915_READ(EDP_PSR_IMR) | mask);
> > > > +		WRITE_ONCE(dev_priv->psr.debug, false);
> > > > +	}
> > > > +}
> > > > +
> > > > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> > > > +{
> > > > +	u32 transcoders = BIT(TRANSCODER_EDP);
> > > > +	enum transcoder cpu_transcoder;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 8)
> > > > +		transcoders |= BIT(TRANSCODER_A) |
> > > > +			       BIT(TRANSCODER_B) |
> > > > +			       BIT(TRANSCODER_C);
> > > > +
> > > > +	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > > > +		/* FIXME: Exit PSR when this happens. */
> > > 
> > > Should this maybe say "retrain the link manually when this happens"?
> > > 
> > 
> > Yeah, we should do both in fact. Makes sense to exit PSR, link train
> > manually and keep it disabled.
> 
> BTW is there a hardware mechnism to inject this failure? Would be nice
> for testing it.
> 
Haven't seen anything like that in bspec.

> If there is no way, maybe we could provide garbage settings for the AUX
> stuff and that would cause the failure?
> 
Probably, are you aware of a way that aux failures can be triggered in
general? We could add it as an IGT test to make sure we do get
interrupts and handle it correctly.

> > 
> > 
> > 
> > > > +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > > +			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > > > +				      transcoder_name(cpu_transcoder));
> > > > +
> > > > +		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder))
> > > > +			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> > > > +				      transcoder_name(cpu_transcoder));
> > > > +
> > > > +		if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder))
> > > > +			DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > > +				      transcoder_name(cpu_transcoder));
> > > > +	}
> > > > +}
> > > > +
> > > >  static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > >  {
> > > >  	uint8_t psr_caps = 0;
> > > > -- 
> > > > 2.14.1
> > > 
>