diff mbox

drm/i915/psr : Add psr1 live status

Message ID 1524216963-22326-1-git-send-email-vathsala.nagaraju@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

vathsala nagaraju April 20, 2018, 9:36 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.

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 | 68 ++++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_reg.h     |  1 +
 2 files changed, 45 insertions(+), 24 deletions(-)

Comments

Souza, Jose April 20, 2018, 5:14 p.m. UTC | #1 
On Fri, 2018-04-20 at 15:06 +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.

> 

> 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 | 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()


Other than that looks good to me.

> +	seq_printf(m, "EDP_PSR%s_STATUS: %x [%s]\n",

> +		      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
Rodrigo Vivi April 20, 2018, 5:35 p.m. UTC | #2 
On Fri, Apr 20, 2018 at 03:06:03PM +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.

Does it really work?

I mean... I heard DK complaining that any read to these
MMIO in some gen9 platforms were triggering the PSR exit
or something like that. So, is this really reliable?

Or it is one of those info that will misslead users to file
non existent bugs?

> 
> 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 | 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_PSR_STATUS);
> +	seq_printf(m, "EDP_PSR%s_STATUS: %x [%s]\n",
> +		      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
> -- 
> 1.9.1
>
vathsala nagaraju April 21, 2018, 4 a.m. UTC | #3 
-----Original Message-----
From: Vivi, Rodrigo 
Sent: Friday, April 20, 2018 11:06 PM
To: Nagaraju, Vathsala <vathsala.nagaraju@intel.com>
Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
Subject: Re: [PATCH] drm/i915/psr : Add psr1 live status

On Fri, Apr 20, 2018 at 03:06:03PM +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.

Does it really work?

I mean... I heard DK complaining that any read to these MMIO in some gen9 platforms were triggering the PSR exit or something like that. So, is this really reliable?

https://patchwork.freedesktop.org/patch/218153/ 
https://patchwork.freedesktop.org/patch/218154/
"Writes to pipe related registers will still cause HW to exit PSR."

Or it is one of those info that will misslead users to file non existent bugs?
Google used this heavily for psr2 status during video playback etc,  so far no has filed any non-existent bug using this interface.
This status is useful.
> 
> 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 | 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_PSR_STATUS);
> +	seq_printf(m, "EDP_PSR%s_STATUS: %x [%s]\n",
> +		      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
> --
> 1.9.1
>
vathsala nagaraju April 23, 2018, 8 a.m. UTC | #4 
On Saturday 21 April 2018 09:30 AM, Nagaraju, Vathsala wrote:
>
> -----Original Message-----
> From: Vivi, Rodrigo
> Sent: Friday, April 20, 2018 11:06 PM
> To: Nagaraju, Vathsala <vathsala.nagaraju@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
> Subject: Re: [PATCH] drm/i915/psr : Add psr1 live status
>
> On Fri, Apr 20, 2018 at 03:06:03PM +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.
> Does it really work?
>
> I mean... I heard DK complaining that any read to these MMIO in some gen9 platforms were triggering the PSR exit or something like that. So, is this really reliable?
https://patchwork.freedesktop.org/patch/218153/ 
https://patchwork.freedesktop.org/patch/218154/ "Writes to pipe related 
registers will still cause HW to exit PSR."

Reading of PSR_STATUS is not causing  the exit. Confirmed by reading pipe_event before to read and after read of psr_status reg.

Test case: pause the youtube video full screen.

Currenlty for psr1 , PSR_mask (bit 16 , mask display reg write ) is unset , there is continous psr_exit.

Or it is one of those info that will misslead users to file non existent bugs?

  Google used this heavily for psr2 status during video playback etc,  so far no has filed any non-existent bug using this interface.
This status is useful.


if you think it will confuse the user to file more bugs, we can add exit reason from PSR_EVENT register.


>> 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 | 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_PSR_STATUS);
>> +	seq_printf(m, "EDP_PSR%s_STATUS: %x [%s]\n",
>> +		      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
>> --
>> 1.9.1
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dhinakaran Pandiyan April 25, 2018, 12:56 a.m. UTC | #5 
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@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.
> > 
> > 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 | 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()

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)
{

}


> 
> 
> 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
vathsala nagaraju April 27, 2018, 5:58 a.m. UTC | #6 
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@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.
>>>
>>> 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 | 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
diff mbox

Patch

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_PSR_STATUS);
+	seq_printf(m, "EDP_PSR%s_STATUS: %x [%s]\n",
+		      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