Re: [PATCH] drm/i915/psr : Add psr1 live status

[vathsala nagaraju <vathsala.nagaraju@xxxxxxxxx>]

On Wednesday 25 April 2018 06:26 AM, Dhinakaran Pandiyan wrote:
> On Fri, 2018-04-20 at 17:14 +0000, Souza, Jose wrote:
> > On Fri, 2018-04-20 at 15:06 +0530, vathsala nagaraju wrote:
> > > From: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx>
> > >
> > > Prints live state of psr1.Extending the existing
> > > PSR2 live state function to cover psr1.
> > >
> > > Tested on KBL with psr2 and psr1 panel.
> > >
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > >
> > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/i915_debugfs.c | 68 ++++++++++++++++++++++++---
> > > ----------
> > >   drivers/gpu/drm/i915/i915_reg.h     |  1 +
> > >   2 files changed, 45 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index e0274f4..3056f04 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2580,25 +2580,42 @@ static int i915_guc_log_relay_release(struct
> > > inode *inode, struct file *file)
> > >   	.release = i915_guc_log_relay_release,
> > >   };
> > >   
> > > -static const char *psr2_live_status(u32 val)
> > > -{
> > > -	static const char * const live_status[] = {
> > > -		"IDLE",
> > > -		"CAPTURE",
> > > -		"CAPTURE_FS",
> > > -		"SLEEP",
> > > -		"BUFON_FW",
> > > -		"ML_UP",
> > > -		"SU_STANDBY",
> > > -		"FAST_SLEEP",
> > > -		"DEEP_SLEEP",
> > > -		"BUF_ON",
> > > -		"TG_ON"
> > > -	};
> > > -
> > > -	val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> > > EDP_PSR2_STATUS_STATE_SHIFT;
> > > -	if (val < ARRAY_SIZE(live_status))
> > > -		return live_status[val];
> > > +static const char *psr_live_status(bool is_psr2_enabled, u32 val)
> > > +{
> > > +	if (is_psr2_enabled) {
> > > +		static const char * const live_status[] = {
> > > +			"IDLE",
> > > +			"CAPTURE",
> > > +			"CAPTURE_FS",
> > > +			"SLEEP",
> > > +			"BUFON_FW",
> > > +			"ML_UP",
> > > +			"SU_STANDBY",
> > > +			"FAST_SLEEP",
> > > +			"DEEP_SLEEP",
> > > +			"BUF_ON",
> > > +			"TG_ON"
> > > +		};
> > > +		val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> > > +			EDP_PSR2_STATUS_STATE_SHIFT;
> > > +		if (val < ARRAY_SIZE(live_status))
> > > +			return live_status[val];
> > > +	} else {
> > > +		static const char * const live_status[] = {
> > > +			"IDLE",
> > > +			"SRDONACK",
> > > +			"SRDENT",
> > > +			"BUFOFF",
> > > +			"BUFON",
> > > +			"AUXACK",
> > > +			"SRDOFFACK",
> > > +			"SRDENT_ON",
> > > +		};
> > > +		val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> > > +			EDP_PSR_STATUS_STATE_SHIFT;
> > > +		if (val < ARRAY_SIZE(live_status))
> > > +			return live_status[val];
> > > +	}
> > >   
> > >   	return "unknown";
> > >   }
> > > @@ -2611,6 +2628,7 @@ static int i915_edp_psr_status(struct seq_file
> > > *m, void *data)
> > >   	enum pipe pipe;
> > >   	bool enabled = false;
> > >   	bool sink_support;
> > > +	u32 psr_status;
> > >   
> > >   	if (!HAS_PSR(dev_priv))
> > >   		return -ENODEV;
> > > @@ -2678,12 +2696,14 @@ static int i915_edp_psr_status(struct
> > > seq_file *m, void *data)
> > >   
> > >   		seq_printf(m, "Performance_Counter: %u\n", psrperf);
> > >   	}
> > > -	if (dev_priv->psr.psr2_enabled) {
> > > -		u32 psr2 = I915_READ(EDP_PSR2_STATUS);
> > >   
> > > -		seq_printf(m, "EDP_PSR2_STATUS: %x [%s]\n",
> > > -			   psr2, psr2_live_status(psr2));
> > > -	}
> > > +	psr_status = (dev_priv->psr.psr2_enabled) ?
> > > I915_READ(EDP_PSR2_STATUS) :
> > > +						    I915_READ(EDP_PS
> > > R_STATUS);
> > Maybe move the read of the PSR or PSR2 status to inside of
> > psr_live_status()
We are printing psr_status  and it's live status[ additional debug 
information] ,
reading the psr_status here and only getting live status from 
psr_live_status function.

I am thinking we could reduce some clutter by changing both the status
functions to have this signature.


static void psr_source_status(dev_priv, m)
{

}

static void psr_sink_status(dev_priv, m)
{

}


Sure , we can change. Will send the v2 version.

> > Other than that looks good to me.
> >
> > > +	seq_printf(m, "EDP_PSR%s_STATUS: %x [%s]\n",
> 			^source_status or whatever the correct parallel to sink status that
> Jose is using.
>
>
> > > +		      dev_priv->psr.psr2_enabled ? "2" : "1",
> > > +		      psr_status,
> > > +		      psr_live_status(dev_priv->psr.psr2_enabled,
> > > psr_status));
> > > +
> > >   	mutex_unlock(&dev_priv->psr.lock);
> > >   
> > >   	intel_runtime_pm_put(dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index fb10602..c9598b4 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4058,6 +4058,7 @@ enum {
> > >   #define   EDP_PSR_STATUS_SENDING_TP2_TP3	(1<<8)
> > >   #define   EDP_PSR_STATUS_SENDING_TP1		(1<<4)
> > >   #define   EDP_PSR_STATUS_IDLE_MASK		0xf
> > > +#define   EDP_PSR_STATUS_STATE_SHIFT		29
> > >   
> > >   #define EDP_PSR_PERF_CNT		_MMIO(dev_priv-
> > > > psr_mmio_base + 0x44)
> > >   #define   EDP_PSR_PERF_CNT_MASK		0xffffff

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