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

From: vathsala nagaraju <vathsala.nagaraju@intel.com>
To: dhinakaran.pandiyan@intel.com, "Souza, Jose" <jose.souza@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Subject: Re: [PATCH] drm/i915/psr : Add psr1 live status
Date: Fri, 27 Apr 2018 11:28:15 +0530	[thread overview]
Message-ID: <5AE2BBF7.7010702@intel.com> (raw)
In-Reply-To: <1524617806.24461.62.camel@dk-H97M-D3H>

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

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