Re: [PATCH 10/12] drm/i915/debugfs: Print sink PSR state and debug info

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2018-03-22 at 16:31 -0700, Rodrigo Vivi wrote:
> On Thu, Mar 22, 2018 at 02:48:46PM -0700, José Roberto de Souza
> wrote:
> 
> please add some justification on why this is useful....

Okay something like this should be fine?

IGT tests could be improved with sink status, knowing for sure that
hardware have activate PSR before get the CRC.
This is also userful to check if hardware is really doing PSR2
selective update with the y-coordinate.


> 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 54
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 16f9977995df..0a0642c61cd0 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2603,6 +2603,44 @@ static const char *psr2_live_status(u32 val)
> >  	return "unknown";
> >  }
> >  
> > +static const char *psr_sink_self_refresh_status(u8 val)
> > +{
> > +	static const char * const sink_status[] = {
> > +		"inactive",
> > +		"transitioning to active",
> > +		"active",
> > +		"active receiving selective update",
> > +		"transitioning to inactive",
> > +		"reserved",
> > +		"reserved",
> > +		"sink internal error"
> > +	};
> > +
> > +	val &= DP_PSR_SINK_STATE_MASK;
> > +	if (val < ARRAY_SIZE(sink_status))
> > +		return sink_status[val];
> > +
> > +	return "unknown";
> > +}
> > +
> > +static void psr_sink_last_received_psr_sdp_sprintf(struct seq_file
> > *m, u32 val)
> > +{
> > +	if (val & DP_PSR_STATE_BIT)
> > +		seq_puts(m, "\tPSR active\n");
> 
> I'm a bit confused here why we are printing status here again if we
> are adding the
> sink_status char * array with some status definition up there.
> 
> Any simplification possible?

Huum yeah, DP_PSR_STATE_BIT and DP_SU_VALID changes will cause the
status of the sink to change, so I this 2 can be removed.

> 
> > +	if (val & DP_UPDATE_RFB_BIT)
> > +		seq_puts(m, "\tUpdate RFB\n");
> > +	if (val & DP_CRC_VALID_BIT)
> > +		seq_puts(m, "\tCRC valid\n");
> > +	if (val & DP_SU_VALID)
> > +		seq_puts(m, "\tSU valid\n");
> > +	if (val & DP_FIRST_SCAN_LINE_SU_REGION)
> > +		seq_puts(m, "\tFirst scan line of the SU
> > region\n");
> > +	if (val & DP_LAST_SCAN_LINE_SU_REGION)
> > +		seq_puts(m, "\tLast scan line of the SU
> > region\n");
> > +	if (val & DP_Y_COORDINATE_VALID)
> > +		seq_puts(m, "\tY-Coordinate valid\n");
> > +}
> > +
> >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > >private);
> > @@ -2684,6 +2722,22 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> >  			   psr2, psr2_live_status(psr2));
> >  	}
> > +
> > +	if (dev_priv->psr.enabled) {
> > +		struct drm_dp_aux *aux = &dev_priv->psr.enabled-
> > >aux;
> > +		u8 val;
> > +
> > +		if (drm_dp_dpcd_readb(aux, DP_PSR_STATUS, &val) ==
> > 1)
> > +			seq_printf(m, "Sink self refresh status:
> > 0x%x [%s]\n",
> > +				   val,
> > psr_sink_self_refresh_status(val));
> > +
> > +		if (drm_dp_dpcd_readb(aux,
> > DP_LAST_RECEIVED_PSR_SDP, &val)
> > +		    == 1) {
> > +			seq_printf(m, "Sink last received PSR SDP:
> > 0x%x\n",
> > +				   val);
> > +			psr_sink_last_received_psr_sdp_sprintf(m,
> > val);
> > +		}
> > +	}
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  
> >  	intel_runtime_pm_put(dev_priv);
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx