drm/i915/psr : Add psr1 live status
diff mbox

Message ID 1526979431-716-1-git-send-email-vathsala.nagaraju@intel.com
State New
Headers show

Commit Message

vathsala nagaraju May 22, 2018, 8:57 a.m. UTC
From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

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@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 66 +++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_reg.h     |  1 +
 2 files changed, 43 insertions(+), 24 deletions(-)

Comments

Dhinakaran Pandiyan May 22, 2018, 7:58 p.m. UTC | #1
On Tue, 2018-05-22 at 14:27 +0530, vathsala nagaraju wrote:
> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> 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@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  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)

> +{
> +	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.
vathsala nagaraju May 23, 2018, 5:37 a.m. UTC | #2
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@intel.com>
>>
>> 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@intel.com>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   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.
>
>
>
Dhinakaran Pandiyan May 23, 2018, 5:41 p.m. UTC | #3
On Wed, 2018-05-23 at 11:07 +0530, Nagaraju, Vathsala wrote:
> 
> 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@intel.com>
> > > 
> > > 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@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > 
> > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > > ---
> > >   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.

Pass struct seq_file *m to psr_source_status() and print the status
there.

Patch
diff mbox

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)
+{
+	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";
 }
@@ -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_PSR_STATUS);
+	seq_printf(m, "SOURCE_PSR_STATUS: %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
 
 #define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base + 0x44)
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff