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

["Nagaraju, Vathsala" <vathsala.nagaraju@xxxxxxxxx>]

On 5/23/2018 1:28 AM, Dhinakaran Pandiyan wrote:
> On Tue, 2018-05-22 at 14:27 +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.
> >
> > v2: rebase
> > v3: DK
> >      Rename psr2_live_status to psr_source_status
> >
> > 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 | 66 +++++++++++++++++++++++--
> > ------------
> >   drivers/gpu/drm/i915/i915_reg.h     |  1 +
> >   2 files changed, 43 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5251544..e4a2f15 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2596,25 +2596,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_source_status(u32 val, bool is_psr2_enabled)
> Please change this to psr_source_status(drm_i915_private *dev_priv)
to print in format , source psr status %x [%s] , where %x = complete psr 
source register value(0x6f940) , %s = psr_status_bits [31 :28/29].
if we want handle everything as part of psr_source_status() , then we 
need to return register value in some pointer.
if not then we read the  reg and then pass it to psr_source_status which 
returns live status string.
> > +{
> > +	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"
> > +		};
> With that, you can		
> 		live_status = I915_READ(EDP_PSR2_STATUS);
> > +		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",
> > +		};
> 		live_status = I915_READ(EDP_PSR_STATUS);
> > +		val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> > +			EDP_PSR_STATUS_STATE_SHIFT;
> > +		if (val < ARRAY_SIZE(live_status))
> > +			return live_status[val];
> > +	}
> >   
> >   	return "unknown";
> >   }
> > @@ -2647,6 +2664,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;
> > @@ -2714,12 +2732,12 @@ 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);
> Please move this inside psr_source_status(), I don't see a point in
> checking for psr2_enabled here and again in psr_source_status()
>
>
>
> > +	seq_printf(m, "SOURCE_PSR_STATUS: %x[%s]\n",
> It's easier on the eyes if these strings are consistent, there is no
> benefit in writing only this string differently.
>
> Sink status is printed as "Sink PSR status: 0x%x [%s]\n". Please do the
> same by writing this as "Source PSR status: 0x%x [%s]\n"
>   
> > +		     psr_status,
> > +		     psr_source_status(psr_status, dev_priv-
> > > psr.psr2_enabled));
> >   
> >   	if (dev_priv->psr.enabled) {
> >   		struct drm_dp_aux *aux = &dev_priv->psr.enabled-
> > > aux;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 513b4a4..3c42021 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4069,6 +4069,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
> You ignored my review from last time, please move this where the mask
> 'EDP_PSR_STATUS_STATE_MASK' is defined. The rest of the file maintains
> this consistently and there is no reason to change that.

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