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

Pandiyan, Dhinakaran dhinakaran.pandiyan at intel.com
Wed Apr 4 23:58:18 UTC 2018





On Wed, 2018-04-04 at 00:54 +0000, Souza, Jose wrote:
> On Tue, 2018-04-03 at 14:24 -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
> > 
> > There are no locks to serialize PSR debug enabling from
> > irq_postinstall() and debugfs for simplicity. As irq_postinstall() is
> > called only during module initialization/resume and IGT subtests
> > aren't expected to modify PSR debug at those times, we should be
> > safe.
> > 
> > v2: Unroll loops (Ville)
> >     Avoid resetting error mask bits.
> > 
> > v3: Unmask interrupts in postinstall() if debug was still enabled.
> >     Avoid RMW (Ville)
> > 
> > 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     | 57 +++++++++++--------------
> > ----------
> >  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c    | 60
> > +++++++++++++++++++++++++++++++++++++
> >  5 files changed, 116 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 1dba2c451255..28f91df5b401 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2690,6 +2690,39 @@ static int i915_edp_psr_status(struct seq_file
> > *m, void *data)
> >  	return 0;
> >  }
> >  
> > +static int
> > +i915_edp_psr_debug_set(void *data, u64 val)
> > +{
> > +	struct drm_i915_private *dev_priv = data;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return -ENODEV;
> > +
> > +	DRM_DEBUG_KMS("PSR debug %s\n", enableddisabled(val));
> > +
> > +	intel_runtime_pm_get(dev_priv);
> > +	intel_psr_debug_control(dev_priv, !!val);
> > +	intel_runtime_pm_put(dev_priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +i915_edp_psr_debug_get(void *data, u64 *val)
> > +{
> > +	struct drm_i915_private *dev_priv = data;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return -ENODEV;
> > +
> > +	*val = READ_ONCE(dev_priv->psr.debug);
> > +	return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
> > +			i915_edp_psr_debug_get,
> > i915_edp_psr_debug_set,
> > +			"%llu\n");
> > +
> >  static int i915_sink_crc(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > >private);
> > @@ -4812,7 +4845,8 @@ static const struct i915_debugfs_files {
> >  	{"i915_guc_log_relay", &i915_guc_log_relay_fops},
> >  	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
> >  	{"i915_ipc_status", &i915_ipc_status_fops},
> > -	{"i915_drrs_ctl", &i915_drrs_ctl_fops}
> > +	{"i915_drrs_ctl", &i915_drrs_ctl_fops},
> > +	{"i915_edp_psr_debug", &i915_edp_psr_debug_fops}
> 
> 
> same as bellow, why not i915_edp_psr_int_debug?
> 
> >  };
> >  
> >  int i915_debugfs_register(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 5373b171bb96..b97ed0cd4ca9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -609,6 +609,7 @@ struct i915_psr {
> >  	bool has_hw_tracking;
> >  	bool psr2_enabled;
> >  	u8 sink_sync_latency;
> > +	bool debug;
> 
> maybe change to a name that gives more information about the use of
> this flag? like int_debug?
> 

Enabling interrupts is one part of enabling PSR debug, that is how I
think of it. We'll probably use the same control to enable other debug
mechanisms as well. At the least to update timestamps from psr_activate
and psr_exit for VLV/CHV.


> >  
> >  	void (*enable_source)(struct intel_dp *,
> >  			      const struct intel_crtc_state *);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 8a894adf2ca1..714570955196 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2391,40 +2391,6 @@ 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);
> > -	u32 edp_psr_imr = I915_READ(EDP_PSR_IMR);
> > -	u32 mask = BIT(TRANSCODER_EDP);
> > -	enum transcoder cpu_transcoder;
> > -
> > -	if (INTEL_GEN(dev_priv) >= 8)
> > -		mask |= BIT(TRANSCODER_A) |
> > -			BIT(TRANSCODER_B) |
> > -			BIT(TRANSCODER_C);
> > -
> > -	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > mask) {
> > -		if (edp_psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > -			DRM_DEBUG_KMS("Transcoder %s PSR error\n",
> > -				      transcoder_name(cpu_transcoder
> > ));
> > -
> > -		if (edp_psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder))
> > {
> > -			DRM_DEBUG_KMS("Transcoder %s PSR prepare
> > entry in 2 vblanks\n",
> > -				      transcoder_name(cpu_transcoder
> > ));
> > -			edp_psr_imr |=
> > EDP_PSR_PRE_ENTRY(cpu_transcoder);
> > -		}
> > -
> > -		if (edp_psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder))
> > {
> > -			DRM_DEBUG_KMS("Transcoder %s PSR exit
> > completed\n",
> > -				      transcoder_name(cpu_transcoder
> > ));
> > -			edp_psr_imr &=
> > ~EDP_PSR_PRE_ENTRY(cpu_transcoder);
> > -		}
> > -	}
> > -
> > -	I915_WRITE(EDP_PSR_IMR, edp_psr_imr);
> > -	I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > -}
> > -
> >  static void ivb_display_irq_handler(struct drm_i915_private
> > *dev_priv,
> >  				    u32 de_iir)
> >  {
> > @@ -2437,8 +2403,12 @@ 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_EDP_PSR_INT_HSW) {
> > +		u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > +		intel_psr_irq_handler(dev_priv, psr_iir);
> > +		I915_WRITE(EDP_PSR_IIR, psr_iir);
> > +	}
> >  
> >  	if (de_iir & DE_AUX_CHANNEL_A_IVB)
> >  		dp_aux_irq_handler(dev_priv);
> > @@ -2580,7 +2550,10 @@ gen8_de_irq_handler(struct drm_i915_private
> > *dev_priv, u32 master_ctl)
> >  			}
> >  
> >  			if (iir & GEN8_DE_EDP_PSR) {
> > -				hsw_edp_psr_irq_handler(dev_priv);
> > +				u32 psr_iir =
> > I915_READ(EDP_PSR_IIR);
> > +
> > +				intel_psr_irq_handler(dev_priv,
> > psr_iir);
> > +				I915_WRITE(EDP_PSR_IIR, psr_iir);
> >  				found = true;
> >  			}
> >  
> > @@ -3729,7 +3702,8 @@ static int ironlake_irq_postinstall(struct
> > drm_device *dev)
> >  
> >  	if (IS_HASWELL(dev_priv)) {
> >  		gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > -		I915_WRITE(EDP_PSR_IMR, 0);
> > +		I915_WRITE(EDP_PSR_IMR,
> > ~EDP_PSR_ERROR(TRANSCODER_EDP));
> > +		intel_psr_debug_control(dev_priv, dev_priv-
> > >psr.debug);
> >  		display_mask |= DE_EDP_PSR_INT_HSW;
> >  	}
> >  
> > @@ -3845,6 +3819,7 @@ static void gen8_de_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >  	u32 de_port_enables;
> >  	u32 de_misc_masked = GEN8_DE_MISC_GSE | GEN8_DE_EDP_PSR;
> >  	enum pipe pipe;
> > +	u32 psr_masked;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9) {
> >  		de_pipe_masked |= GEN9_DE_PIPE_IRQ_FAULT_ERRORS;
> > @@ -3869,7 +3844,11 @@ static void gen8_de_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >  		de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
> >  
> >  	gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > -	I915_WRITE(EDP_PSR_IMR, 0);
> > +	psr_masked = EDP_PSR_ERROR(TRANSCODER_EDP) |
> > +		     EDP_PSR_ERROR(TRANSCODER_A) |
> > EDP_PSR_ERROR(TRANSCODER_B) |
> > +		     EDP_PSR_ERROR(TRANSCODER_C);
> > +	I915_WRITE(EDP_PSR_IMR, ~psr_masked);
> 
> intel_psr_debug_control() will already write to EDP_PSR_IMR with the
> right value, why write twice?
> 

Good point. The benefit is it keeps whatever happens in psr_debug_control()
separate from what irq_postinstall should always do. Doesn't outweigh the
benefit of avoiding an additional MMIO write, I think. I will remove the IMR
write from here.


> > +	intel_psr_debug_control(dev_priv, dev_priv->psr.debug);
> >  
> >  	for_each_pipe(dev_priv, pipe) {
> >  		dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d1452fd2a58d..53cb76b920e6 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1891,6 +1891,8 @@ void intel_psr_single_frame_update(struct
> > drm_i915_private *dev_priv,
> >  				   unsigned frontbuffer_bits);
> >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> >  			      struct intel_crtc_state *crtc_state);
> > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool
> > enable);
> > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > psr_iir);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2d53f7398a6d..56ff2d7691a1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -93,6 +93,66 @@ static void psr_aux_io_power_put(struct intel_dp
> > *intel_dp)
> >  	intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> >  }
> >  
> > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool
> > enable)
> > +{
> > +	u32 debug_mask, error_mask;
> > +
> > +	/* No PSR interrupts on VLV/CHV */
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > +		return;
> > +
> > +	error_mask = EDP_PSR_ERROR(TRANSCODER_EDP);
> > +	debug_mask = error_mask | EDP_PSR_POST_EXIT(TRANSCODER_EDP)
> > |
> > +		     EDP_PSR_PRE_ENTRY(TRANSCODER_EDP);
> > +
> > +	if (INTEL_GEN(dev_priv) >= 8) {
> > +		error_mask |= EDP_PSR_ERROR(TRANSCODER_A) |
> > +			      EDP_PSR_ERROR(TRANSCODER_B) |
> > +			      EDP_PSR_ERROR(TRANSCODER_C);
> > +
> > +		debug_mask |= error_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, ~debug_mask);
> > +	} else {
> > +		I915_WRITE(EDP_PSR_IMR, ~error_mask);
> > +		WRITE_ONCE(dev_priv->psr.debug, false);
> > +	}
> 
> 
> nit pick but why not something like?
> 
> u32 debug_mask, mask;
> 
> mask = EDP_PSR_ERROR(TRANSCODER_EDP);
> debug_mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP)
> ...
> 
> if (enable)
>     mask |= debug_mask;
> 
> I915_WRITE(EDP_PSR_IMR, ~mask)
> WRITE_ONCE(dev_priv->psr.debug, enable);
> 
> 

Don't see much advantage in writing it one way or the other. Having said
that I have no issues in making the change.

> > +}
> > +
> > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > psr_iir)
> 
> two minnor things here:
> - why you moved the function here? it adds more noise to the diff.

Wanted the PSR irq_handler to be close to the rest of PSR code. We'll
eventually want to call other functions in this file to handle errors.

> - the rename from hsw_edp_psr_irq_handler to intel_psr_irq_handler
> should have being done in 'drm/i915: Enable edp psr error interrupts on
> bdw+'
> 

I wanted to make only minimal rebase changes for the previous two
patches since I did not write them originally.

> > +{
> > +	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 and link train manually when this
> > happens. */
> > +		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_coord_required(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t psr_caps = 0;



More information about the Intel-gfx mailing list