[PATCH 1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[PATCH 1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[José Roberto de Souza]

For now PSR2 is still disabled by default for all platforms but is
our intention to let debugfs to enable it for debug and tests
proporses, so intel_psr2_enabled() that is also used by debugfs to
decide if PSR2 is going to be enabled needs to take in consideration
the debug field.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4c4dd1c310ce..15a2121aa64f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -71,8 +71,11 @@ static bool psr_global_enabled(u32 debug)
 static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
 			       const struct intel_crtc_state *crtc_state)
 {
+	const u32 debug_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
+
 	/* Disable PSR2 by default for all platforms */
-	if (i915_modparams.enable_psr == -1)
+	if (i915_modparams.enable_psr == -1 &&
+	    debug_mode != I915_PSR_DEBUG_ENABLE)
 		return false;
 
 	/* Cannot enable DSC and PSR2 simultaneously */
-- 
2.19.2

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

[PATCH 2/5] drm/i915: Refactor PSR status debugfs

[José Roberto de Souza]

The old debugfs fields was not following a naming partern and it was
a bit confusing.

So it went from:
~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
Sink_Support: yes
PSR mode: PSR1
Enabled: yes
Busy frontbuffer bits: 0x000
Main link in standby mode: no
HW Enabled & Active bit: yes
Source PSR status: 0x24050006 [SRDONACK]

To:
~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
Sink support: yes [0x00000003]
Status: PSR1 enabled
Source PSR ctl: enabled [0x81f00e26]
Source PSR status: SRDENT [0x40040006]
Busy frontbuffer bits: 0x00000000

The 'Main link in standby mode' was removed as it is not useful but
if needed by someone the information is still in the register value
of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled was
squashed into Status, some renames and reorders and we have this
cleaner version. This will also make easy to parse debugfs for IGT
tests.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++--------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 38dcee1ca062..86303ba02666 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
 static void
 psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 {
-	u32 val, psr_status;
+	u32 val, status_val;
+	const char *status = "unknown";
 
 	if (dev_priv->psr.psr2_enabled) {
 		static const char * const live_status[] = {
@@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 			"BUF_ON",
 			"TG_ON"
 		};
-		psr_status = I915_READ(EDP_PSR2_STATUS);
-		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >>
-			EDP_PSR2_STATUS_STATE_SHIFT;
-		if (val < ARRAY_SIZE(live_status)) {
-			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
-				   psr_status, live_status[val]);
-			return;
-		}
+		val = I915_READ(EDP_PSR2_STATUS);
+		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
+			      EDP_PSR2_STATUS_STATE_SHIFT;
+		if (status_val < ARRAY_SIZE(live_status))
+			status = live_status[status_val];
 	} else {
 		static const char * const live_status[] = {
 			"IDLE",
@@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 			"SRDOFFACK",
 			"SRDENT_ON",
 		};
-		psr_status = I915_READ(EDP_PSR_STATUS);
-		val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >>
-			EDP_PSR_STATUS_STATE_SHIFT;
-		if (val < ARRAY_SIZE(live_status)) {
-			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
-				   psr_status, live_status[val]);
-			return;
-		}
+		val = I915_READ(EDP_PSR_STATUS);
+		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
+			      EDP_PSR_STATUS_STATE_SHIFT;
+		if (status_val < ARRAY_SIZE(live_status))
+			status = live_status[status_val];
 	}
 
-	seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status, "unknown");
+	seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val);
 }
 
 static int i915_edp_psr_status(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	u32 psrperf = 0;
-	bool enabled = false;
-	bool sink_support;
+	struct i915_psr *psr = &dev_priv->psr;
+	const char *status;
+	bool enabled;
+	u32 val;
 
 	if (!HAS_PSR(dev_priv))
 		return -ENODEV;
 
-	sink_support = dev_priv->psr.sink_support;
-	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
-	if (!sink_support)
+	seq_printf(m, "Sink support: %s", yesno(psr->sink_support));
+	if (!psr->sink_support) {
+		seq_puts(m, "\n");
 		return 0;
+	}
 
 	intel_runtime_pm_get(dev_priv);
+	mutex_lock(&psr->lock);
 
-	mutex_lock(&dev_priv->psr.lock);
-	seq_printf(m, "PSR mode: %s\n",
-		   dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1");
-	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
-	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
-		   dev_priv->psr.busy_frontbuffer_bits);
+	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
 
-	if (dev_priv->psr.psr2_enabled)
-		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
+	if (psr->enabled)
+		status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1 enabled";
 	else
-		enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
+		status = "disabled";
+	seq_printf(m, "Status: %s\n", status);
 
-	seq_printf(m, "Main link in standby mode: %s\n",
-		   yesno(dev_priv->psr.link_standby));
+	if (!psr->enabled)
+		goto unlock;
 
-	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
+	if (psr->psr2_enabled) {
+		val = I915_READ(EDP_PSR2_CTL);
+		enabled = val & EDP_PSR2_ENABLE;
+	} else {
+		val = I915_READ(EDP_PSR_CTL);
+		enabled = val & EDP_PSR_ENABLE;
+	}
+	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
+		   enableddisabled(enabled), val);
+	psr_source_status(dev_priv, m);
+	seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
+		   psr->busy_frontbuffer_bits);
 
 	/*
 	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
 	 */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
-			EDP_PSR_PERF_CNT_MASK;
-
-		seq_printf(m, "Performance_Counter: %u\n", psrperf);
+		val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK;
+		seq_printf(m, "Performance counter: %u\n", val);
 	}
 
-	psr_source_status(dev_priv, m);
-	mutex_unlock(&dev_priv->psr.lock);
-
-	if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) {
+	if (psr->debug & I915_PSR_DEBUG_IRQ) {
 		seq_printf(m, "Last attempted entry at: %lld\n",
-			   dev_priv->psr.last_entry_attempt);
-		seq_printf(m, "Last exit at: %lld\n",
-			   dev_priv->psr.last_exit);
+			   psr->last_entry_attempt);
+		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
 	}
 
+unlock:
+	mutex_unlock(&psr->lock);
 	intel_runtime_pm_put(dev_priv);
+
 	return 0;
 }
 
-- 
2.19.2

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

Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs

[Dhinakaran Pandiyan]

On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> The old debugfs fields was not following a naming partern and it was
> a bit confusing.
> 
> So it went from:
> ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> Sink_Support: yes
> PSR mode: PSR1
> Enabled: yes
> Busy frontbuffer bits: 0x000
> Main link in standby mode: no
> HW Enabled & Active bit: yes
> Source PSR status: 0x24050006 [SRDONACK]
> 
> To:
> ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> Sink support: yes [0x00000003]
> Status: PSR1 enabled
> Source PSR ctl: enabled [0x81f00e26]
> Source PSR status: SRDENT [0x40040006]
> Busy frontbuffer bits: 0x00000000
> 
> The 'Main link in standby mode' was removed as it is not useful but
> if needed by someone the information is still in the register value
> of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled was
> squashed into Status, some renames and reorders and we have this
> cleaner version. This will also make easy to parse debugfs for IGT
> tests.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++----------
> ----
>  1 file changed, 49 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 38dcee1ca062..86303ba02666 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
>  static void
>  psr_source_status(struct drm_i915_private *dev_priv, struct seq_file
> *m)
>  {
> -	u32 val, psr_status;
> +	u32 val, status_val;
> +	const char *status = "unknown";
>  
>  	if (dev_priv->psr.psr2_enabled) {
>  		static const char * const live_status[] = {
> @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private
> *dev_priv, struct seq_file *m)
>  			"BUF_ON",
>  			"TG_ON"
>  		};
> -		psr_status = I915_READ(EDP_PSR2_STATUS);
> -		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >>
> -			EDP_PSR2_STATUS_STATE_SHIFT;
> -		if (val < ARRAY_SIZE(live_status)) {
> -			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> -				   psr_status, live_status[val]);
> -			return;
> -		}
> +		val = I915_READ(EDP_PSR2_STATUS);
> +		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> +			      EDP_PSR2_STATUS_STATE_SHIFT;
> +		if (status_val < ARRAY_SIZE(live_status))
> +			status = live_status[status_val];
>  	} else {
>  		static const char * const live_status[] = {
>  			"IDLE",
> @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private
> *dev_priv, struct seq_file *m)
>  			"SRDOFFACK",
>  			"SRDENT_ON",
>  		};
> -		psr_status = I915_READ(EDP_PSR_STATUS);
> -		val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >>
> -			EDP_PSR_STATUS_STATE_SHIFT;
> -		if (val < ARRAY_SIZE(live_status)) {
> -			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> -				   psr_status, live_status[val]);
> -			return;
> -		}
> +		val = I915_READ(EDP_PSR_STATUS);
> +		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> +			      EDP_PSR_STATUS_STATE_SHIFT;
> +		if (status_val < ARRAY_SIZE(live_status))
> +			status = live_status[status_val];
>  	}
>  
> -	seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status,
> "unknown");
> +	seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val);
>  }
>  
>  static int i915_edp_psr_status(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	u32 psrperf = 0;
> -	bool enabled = false;
> -	bool sink_support;
> +	struct i915_psr *psr = &dev_priv->psr;
> +	const char *status;
> +	bool enabled;
> +	u32 val;
>  
>  	if (!HAS_PSR(dev_priv))
>  		return -ENODEV;
>  
> -	sink_support = dev_priv->psr.sink_support;
> -	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
> -	if (!sink_support)
> +	seq_printf(m, "Sink support: %s", yesno(psr->sink_support));
> +	if (!psr->sink_support) {
> +		seq_puts(m, "\n");
>  		return 0;
> +	}
>  
>  	intel_runtime_pm_get(dev_priv);
> +	mutex_lock(&psr->lock);
>  
> -	mutex_lock(&dev_priv->psr.lock);
> -	seq_printf(m, "PSR mode: %s\n",
> -		   dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1");
> -	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
> -	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> -		   dev_priv->psr.busy_frontbuffer_bits);
> +	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
This can be moved closer to where "Sink support" is printed. Also,  the
extra zeroes that get printed due to "%08x" look odd, please consider
changing it to "%02x".

>  
> -	if (dev_priv->psr.psr2_enabled)
> -		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> +	if (psr->enabled)
> +		status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1
> enabled";
>  	else
> -		enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> +		status = "disabled";
> +	seq_printf(m, "Status: %s\n", status);
Can we just use the ctl field and get rid of this? This status and and
the one below can be confusing to those not familiar with the code
base.

We should be able to parse all the information we need from two lines
Source PSR ctl:		[PSR1,PSR2] enabled <reg value>
Source PSR status:	<state> <reg value>



>  
> -	seq_printf(m, "Main link in standby mode: %s\n",
> -		   yesno(dev_priv->psr.link_standby));
> +	if (!psr->enabled)
> +		goto unlock;
>  
> -	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
> +	if (psr->psr2_enabled) {
> +		val = I915_READ(EDP_PSR2_CTL);
> +		enabled = val & EDP_PSR2_ENABLE;
> +	} else {
> +		val = I915_READ(EDP_PSR_CTL);
> +		enabled = val & EDP_PSR_ENABLE;
> +	}
> +	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> +		   enableddisabled(enabled), val);
> +	psr_source_status(dev_priv, m);
> +	seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> +		   psr->busy_frontbuffer_bits);
>  
>  	/*
>  	 * SKL+ Perf counter is reset to 0 everytime DC state is
> entered
>  	 */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> -			EDP_PSR_PERF_CNT_MASK;
> -
> -		seq_printf(m, "Performance_Counter: %u\n", psrperf);
> +		val = I915_READ(EDP_PSR_PERF_CNT) &
> EDP_PSR_PERF_CNT_MASK;
> +		seq_printf(m, "Performance counter: %u\n", val);
>  	}
>  
> -	psr_source_status(dev_priv, m);
> -	mutex_unlock(&dev_priv->psr.lock);
> -
> -	if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) {
> +	if (psr->debug & I915_PSR_DEBUG_IRQ) {
>  		seq_printf(m, "Last attempted entry at: %lld\n",
> -			   dev_priv->psr.last_entry_attempt);
> -		seq_printf(m, "Last exit at: %lld\n",
> -			   dev_priv->psr.last_exit);
> +			   psr->last_entry_attempt);
> +		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
>  	}
>  
> +unlock:
> +	mutex_unlock(&psr->lock);
>  	intel_runtime_pm_put(dev_priv);
> +
>  	return 0;
>  }
>  

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

Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 8097 bytes --]

On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > The old debugfs fields was not following a naming partern and it
> > was
> > a bit confusing.
> > 
> > So it went from:
> > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > Sink_Support: yes
> > PSR mode: PSR1
> > Enabled: yes
> > Busy frontbuffer bits: 0x000
> > Main link in standby mode: no
> > HW Enabled & Active bit: yes
> > Source PSR status: 0x24050006 [SRDONACK]
> > 
> > To:
> > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > Sink support: yes [0x00000003]
> > Status: PSR1 enabled
> > Source PSR ctl: enabled [0x81f00e26]
> > Source PSR status: SRDENT [0x40040006]
> > Busy frontbuffer bits: 0x00000000
> > 
> > The 'Main link in standby mode' was removed as it is not useful but
> > if needed by someone the information is still in the register value
> > of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled
> > was
> > squashed into Status, some renames and reorders and we have this
> > cleaner version. This will also make easy to parse debugfs for IGT
> > tests.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++----------
> > ----
> >  1 file changed, 49 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 38dcee1ca062..86303ba02666 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2665,7 +2665,8 @@ DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
> >  static void
> >  psr_source_status(struct drm_i915_private *dev_priv, struct
> > seq_file
> > *m)
> >  {
> > -	u32 val, psr_status;
> > +	u32 val, status_val;
> > +	const char *status = "unknown";
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> >  		static const char * const live_status[] = {
> > @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private
> > *dev_priv, struct seq_file *m)
> >  			"BUF_ON",
> >  			"TG_ON"
> >  		};
> > -		psr_status = I915_READ(EDP_PSR2_STATUS);
> > -		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >>
> > -			EDP_PSR2_STATUS_STATE_SHIFT;
> > -		if (val < ARRAY_SIZE(live_status)) {
> > -			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > -				   psr_status, live_status[val]);
> > -			return;
> > -		}
> > +		val = I915_READ(EDP_PSR2_STATUS);
> > +		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> > +			      EDP_PSR2_STATUS_STATE_SHIFT;
> > +		if (status_val < ARRAY_SIZE(live_status))
> > +			status = live_status[status_val];
> >  	} else {
> >  		static const char * const live_status[] = {
> >  			"IDLE",
> > @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private
> > *dev_priv, struct seq_file *m)
> >  			"SRDOFFACK",
> >  			"SRDENT_ON",
> >  		};
> > -		psr_status = I915_READ(EDP_PSR_STATUS);
> > -		val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >>
> > -			EDP_PSR_STATUS_STATE_SHIFT;
> > -		if (val < ARRAY_SIZE(live_status)) {
> > -			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > -				   psr_status, live_status[val]);
> > -			return;
> > -		}
> > +		val = I915_READ(EDP_PSR_STATUS);
> > +		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> > +			      EDP_PSR_STATUS_STATE_SHIFT;
> > +		if (status_val < ARRAY_SIZE(live_status))
> > +			status = live_status[status_val];
> >  	}
> >  
> > -	seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status,
> > "unknown");
> > +	seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val);
> >  }
> >  
> >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > -	u32 psrperf = 0;
> > -	bool enabled = false;
> > -	bool sink_support;
> > +	struct i915_psr *psr = &dev_priv->psr;
> > +	const char *status;
> > +	bool enabled;
> > +	u32 val;
> >  
> >  	if (!HAS_PSR(dev_priv))
> >  		return -ENODEV;
> >  
> > -	sink_support = dev_priv->psr.sink_support;
> > -	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
> > -	if (!sink_support)
> > +	seq_printf(m, "Sink support: %s", yesno(psr->sink_support));
> > +	if (!psr->sink_support) {
> > +		seq_puts(m, "\n");
> >  		return 0;
> > +	}
> >  
> >  	intel_runtime_pm_get(dev_priv);
> > +	mutex_lock(&psr->lock);
> >  
> > -	mutex_lock(&dev_priv->psr.lock);
> > -	seq_printf(m, "PSR mode: %s\n",
> > -		   dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1");
> > -	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
> > -	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > -		   dev_priv->psr.busy_frontbuffer_bits);
> > +	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
> This can be moved closer to where "Sink support" is printed.
> Also,  the
> extra zeroes that get printed due to "%08x" look odd, please consider
> changing it to "%02x".

Done

> 
> >  
> > -	if (dev_priv->psr.psr2_enabled)
> > -		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > +	if (psr->enabled)
> > +		status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1
> > enabled";
> >  	else
> > -		enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > +		status = "disabled";
> > +	seq_printf(m, "Status: %s\n", status);
> Can we just use the ctl field and get rid of this? This status and
> and
> the one below can be confusing to those not familiar with the code
> base.
> 
> We should be able to parse all the information we need from two lines
> Source PSR ctl:		[PSR1,PSR2] enabled <reg value>
> Source PSR status:	<state> <reg value>

I'm fine in having just one but when there is frontbuffer modifications
Status is kept as enabled and Source PSR ctl and Source PSR status are
set to idle states, we know that but those not familiar with the code
could think it is disabled but it usualy happen so fast that I guess it
is not a problem.

And when PSR is disabled we print something else other than Sink
support?

> 
> 
> 
> >  
> > -	seq_printf(m, "Main link in standby mode: %s\n",
> > -		   yesno(dev_priv->psr.link_standby));
> > +	if (!psr->enabled)
> > +		goto unlock;
> >  
> > -	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
> > +	if (psr->psr2_enabled) {
> > +		val = I915_READ(EDP_PSR2_CTL);
> > +		enabled = val & EDP_PSR2_ENABLE;
> > +	} else {
> > +		val = I915_READ(EDP_PSR_CTL);
> > +		enabled = val & EDP_PSR_ENABLE;
> > +	}
> > +	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > +		   enableddisabled(enabled), val);
> > +	psr_source_status(dev_priv, m);
> > +	seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> > +		   psr->busy_frontbuffer_bits);
> >  
> >  	/*
> >  	 * SKL+ Perf counter is reset to 0 everytime DC state is
> > entered
> >  	 */
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > -		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> > -			EDP_PSR_PERF_CNT_MASK;
> > -
> > -		seq_printf(m, "Performance_Counter: %u\n", psrperf);
> > +		val = I915_READ(EDP_PSR_PERF_CNT) &
> > EDP_PSR_PERF_CNT_MASK;
> > +		seq_printf(m, "Performance counter: %u\n", val);
> >  	}
> >  
> > -	psr_source_status(dev_priv, m);
> > -	mutex_unlock(&dev_priv->psr.lock);
> > -
> > -	if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) {
> > +	if (psr->debug & I915_PSR_DEBUG_IRQ) {
> >  		seq_printf(m, "Last attempted entry at: %lld\n",
> > -			   dev_priv->psr.last_entry_attempt);
> > -		seq_printf(m, "Last exit at: %lld\n",
> > -			   dev_priv->psr.last_exit);
> > +			   psr->last_entry_attempt);
> > +		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> >  	}
> >  
> > +unlock:
> > +	mutex_unlock(&psr->lock);
> >  	intel_runtime_pm_put(dev_priv);
> > +
> >  	return 0;
> >  }
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs

[Dhinakaran Pandiyan]

On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote:
> On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > > The old debugfs fields was not following a naming partern and it
> > > was
> > > a bit confusing.
> > > 
> > > So it went from:
> > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > Sink_Support: yes
> > > PSR mode: PSR1
> > > Enabled: yes
> > > Busy frontbuffer bits: 0x000
> > > Main link in standby mode: no
> > > HW Enabled & Active bit: yes
> > > Source PSR status: 0x24050006 [SRDONACK]
> > > 
> > > To:
> > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > Sink support: yes [0x00000003]
> > > Status: PSR1 enabled
> > > Source PSR ctl: enabled [0x81f00e26]
> > > Source PSR status: SRDENT [0x40040006]
> > > Busy frontbuffer bits: 0x00000000
> > > 
> > > The 'Main link in standby mode' was removed as it is not useful
> > > but
> > > if needed by someone the information is still in the register
> > > value
> > > of 'Source PSR ctl' inside of the brackets, PSR mode and Enabled
> > > was
> > > squashed into Status, some renames and reorders and we have this
> > > cleaner version. This will also make easy to parse debugfs for
> > > IGT
> > > tests.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++------
> > > ----
> > > ----
> > >  1 file changed, 49 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 38dcee1ca062..86303ba02666 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2665,7 +2665,8 @@
> > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
> > >  static void
> > >  psr_source_status(struct drm_i915_private *dev_priv, struct
> > > seq_file
> > > *m)
> > >  {
> > > -	u32 val, psr_status;
> > > +	u32 val, status_val;
> > > +	const char *status = "unknown";
> > >  
> > >  	if (dev_priv->psr.psr2_enabled) {
> > >  		static const char * const live_status[] = {
> > > @@ -2681,14 +2682,11 @@ psr_source_status(struct drm_i915_private
> > > *dev_priv, struct seq_file *m)
> > >  			"BUF_ON",
> > >  			"TG_ON"
> > >  		};
> > > -		psr_status = I915_READ(EDP_PSR2_STATUS);
> > > -		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK) >>
> > > -			EDP_PSR2_STATUS_STATE_SHIFT;
> > > -		if (val < ARRAY_SIZE(live_status)) {
> > > -			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > > -				   psr_status, live_status[val]);
> > > -			return;
> > > -		}
> > > +		val = I915_READ(EDP_PSR2_STATUS);
> > > +		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> > > +			      EDP_PSR2_STATUS_STATE_SHIFT;
> > > +		if (status_val < ARRAY_SIZE(live_status))
> > > +			status = live_status[status_val];
> > >  	} else {
> > >  		static const char * const live_status[] = {
> > >  			"IDLE",
> > > @@ -2700,74 +2698,78 @@ psr_source_status(struct drm_i915_private
> > > *dev_priv, struct seq_file *m)
> > >  			"SRDOFFACK",
> > >  			"SRDENT_ON",
> > >  		};
> > > -		psr_status = I915_READ(EDP_PSR_STATUS);
> > > -		val = (psr_status & EDP_PSR_STATUS_STATE_MASK) >>
> > > -			EDP_PSR_STATUS_STATE_SHIFT;
> > > -		if (val < ARRAY_SIZE(live_status)) {
> > > -			seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > > -				   psr_status, live_status[val]);
> > > -			return;
> > > -		}
> > > +		val = I915_READ(EDP_PSR_STATUS);
> > > +		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> > > +			      EDP_PSR_STATUS_STATE_SHIFT;
> > > +		if (status_val < ARRAY_SIZE(live_status))
> > > +			status = live_status[status_val];
> > >  	}
> > >  
> > > -	seq_printf(m, "Source PSR status: 0x%x [%s]\n", psr_status,
> > > "unknown");
> > > +	seq_printf(m, "Source PSR status: %s [0x%08x]\n", status, val);
> > >  }
> > >  
> > >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  {
> > >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > > -	u32 psrperf = 0;
> > > -	bool enabled = false;
> > > -	bool sink_support;
> > > +	struct i915_psr *psr = &dev_priv->psr;
> > > +	const char *status;
> > > +	bool enabled;
> > > +	u32 val;
> > >  
> > >  	if (!HAS_PSR(dev_priv))
> > >  		return -ENODEV;
> > >  
> > > -	sink_support = dev_priv->psr.sink_support;
> > > -	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
> > > -	if (!sink_support)
> > > +	seq_printf(m, "Sink support: %s", yesno(psr->sink_support));
> > > +	if (!psr->sink_support) {
> > > +		seq_puts(m, "\n");
> > >  		return 0;
> > > +	}
> > >  
> > >  	intel_runtime_pm_get(dev_priv);
> > > +	mutex_lock(&psr->lock);
> > >  
> > > -	mutex_lock(&dev_priv->psr.lock);
> > > -	seq_printf(m, "PSR mode: %s\n",
> > > -		   dev_priv->psr.psr2_enabled ? "PSR2" : "PSR1");
> > > -	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
> > > -	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > -		   dev_priv->psr.busy_frontbuffer_bits);
> > > +	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
> > 
> > This can be moved closer to where "Sink support" is printed.
> > Also,  the
> > extra zeroes that get printed due to "%08x" look odd, please
> > consider
> > changing it to "%02x".
> 
> Done
> 
> > 
> > >  
> > > -	if (dev_priv->psr.psr2_enabled)
> > > -		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > > +	if (psr->enabled)
> > > +		status = psr->psr2_enabled ? "PSR2 enabled" : "PSR1
> > > enabled";
> > >  	else
> > > -		enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > > +		status = "disabled";
> > > +	seq_printf(m, "Status: %s\n", status);
> > 
> > Can we just use the ctl field and get rid of this? This status and
> > and
> > the one below can be confusing to those not familiar with the code
> > base.
> > 
> > We should be able to parse all the information we need from two
> > lines
> > Source PSR ctl:		[PSR1,PSR2] enabled <reg value>
> > Source PSR status:	<state> <reg value>
> 
> I'm fine in having just one but when there is frontbuffer
> modifications
> Status is kept as enabled and Source PSR ctl and Source PSR status
> are
> set to idle states, 

That is a good point -  psr_invalidate() would disable the feature in 
hardware but the driver state is enabled. I guess, you could change
"Status" to "PSR mode"/"Feature state". 
...
PSR mode:		{PSR1 enabled, PSR2 enabled, disabled}
Source PSR ctl:		{enabled, disabled} ...
Source PSR status:	...
...

Is this better?

Also, it might be worth printing the "PSR mode:" line even when there
is no sink_support. It's an useful piece of debug information and
doesn't access the hardware.


> we know that but those not familiar with the code
> could think it is disabled but it usualy happen so fast that I guess
> it
> is not a problem.
> 
> And when PSR is disabled we print something else other than Sink
> support?
> 
> > 
> > 
> > 
> > >  
> > > -	seq_printf(m, "Main link in standby mode: %s\n",
> > > -		   yesno(dev_priv->psr.link_standby));
> > > +	if (!psr->enabled)
> > > +		goto unlock;
> > >  
> > > -	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
> > > +	if (psr->psr2_enabled) {
> > > +		val = I915_READ(EDP_PSR2_CTL);
> > > +		enabled = val & EDP_PSR2_ENABLE;
> > > +	} else {
> > > +		val = I915_READ(EDP_PSR_CTL);
> > > +		enabled = val & EDP_PSR_ENABLE;
> > > +	}
> > > +	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > > +		   enableddisabled(enabled), val);
> > > +	psr_source_status(dev_priv, m);
> > > +	seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> > > +		   psr->busy_frontbuffer_bits);
> > >  
> > >  	/*
> > >  	 * SKL+ Perf counter is reset to 0 everytime DC state is
> > > entered
> > >  	 */
> > >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > -		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> > > -			EDP_PSR_PERF_CNT_MASK;
> > > -
> > > -		seq_printf(m, "Performance_Counter: %u\n", psrperf);
> > > +		val = I915_READ(EDP_PSR_PERF_CNT) &
> > > EDP_PSR_PERF_CNT_MASK;
> > > +		seq_printf(m, "Performance counter: %u\n", val);
> > >  	}
> > >  
> > > -	psr_source_status(dev_priv, m);
> > > -	mutex_unlock(&dev_priv->psr.lock);
> > > -
> > > -	if (READ_ONCE(dev_priv->psr.debug) & I915_PSR_DEBUG_IRQ) {
> > > +	if (psr->debug & I915_PSR_DEBUG_IRQ) {
> > >  		seq_printf(m, "Last attempted entry at: %lld\n",
> > > -			   dev_priv->psr.last_entry_attempt);
> > > -		seq_printf(m, "Last exit at: %lld\n",
> > > -			   dev_priv->psr.last_exit);
> > > +			   psr->last_entry_attempt);
> > > +		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> > >  	}
> > >  
> > > +unlock:
> > > +	mutex_unlock(&psr->lock);
> > >  	intel_runtime_pm_put(dev_priv);
> > > +
> > >  	return 0;
> > >  }
> > >  

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

Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 10160 bytes --]

On Tue, 2018-12-11 at 10:32 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote:
> > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote:
> > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > > > The old debugfs fields was not following a naming partern and
> > > > it
> > > > was
> > > > a bit confusing.
> > > > 
> > > > So it went from:
> > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > > Sink_Support: yes
> > > > PSR mode: PSR1
> > > > Enabled: yes
> > > > Busy frontbuffer bits: 0x000
> > > > Main link in standby mode: no
> > > > HW Enabled & Active bit: yes
> > > > Source PSR status: 0x24050006 [SRDONACK]
> > > > 
> > > > To:
> > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > > Sink support: yes [0x00000003]
> > > > Status: PSR1 enabled
> > > > Source PSR ctl: enabled [0x81f00e26]
> > > > Source PSR status: SRDENT [0x40040006]
> > > > Busy frontbuffer bits: 0x00000000
> > > > 
> > > > The 'Main link in standby mode' was removed as it is not useful
> > > > but
> > > > if needed by someone the information is still in the register
> > > > value
> > > > of 'Source PSR ctl' inside of the brackets, PSR mode and
> > > > Enabled
> > > > was
> > > > squashed into Status, some renames and reorders and we have
> > > > this
> > > > cleaner version. This will also make easy to parse debugfs for
> > > > IGT
> > > > tests.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Suggested-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++------
> > > > ----
> > > > ----
> > > >  1 file changed, 49 insertions(+), 47 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 38dcee1ca062..86303ba02666 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2665,7 +2665,8 @@
> > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
> > > >  static void
> > > >  psr_source_status(struct drm_i915_private *dev_priv, struct
> > > > seq_file
> > > > *m)
> > > >  {
> > > > -	u32 val, psr_status;
> > > > +	u32 val, status_val;
> > > > +	const char *status = "unknown";
> > > >  
> > > >  	if (dev_priv->psr.psr2_enabled) {
> > > >  		static const char * const live_status[] = {
> > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct
> > > > drm_i915_private
> > > > *dev_priv, struct seq_file *m)
> > > >  			"BUF_ON",
> > > >  			"TG_ON"
> > > >  		};
> > > > -		psr_status = I915_READ(EDP_PSR2_STATUS);
> > > > -		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK)
> > > > >>
> > > > -			EDP_PSR2_STATUS_STATE_SHIFT;
> > > > -		if (val < ARRAY_SIZE(live_status)) {
> > > > -			seq_printf(m, "Source PSR status: 0x%x
> > > > [%s]\n",
> > > > -				   psr_status,
> > > > live_status[val]);
> > > > -			return;
> > > > -		}
> > > > +		val = I915_READ(EDP_PSR2_STATUS);
> > > > +		status_val = (val & EDP_PSR2_STATUS_STATE_MASK)
> > > > >>
> > > > +			      EDP_PSR2_STATUS_STATE_SHIFT;
> > > > +		if (status_val < ARRAY_SIZE(live_status))
> > > > +			status = live_status[status_val];
> > > >  	} else {
> > > >  		static const char * const live_status[] = {
> > > >  			"IDLE",
> > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct
> > > > drm_i915_private
> > > > *dev_priv, struct seq_file *m)
> > > >  			"SRDOFFACK",
> > > >  			"SRDENT_ON",
> > > >  		};
> > > > -		psr_status = I915_READ(EDP_PSR_STATUS);
> > > > -		val = (psr_status & EDP_PSR_STATUS_STATE_MASK)
> > > > >>
> > > > -			EDP_PSR_STATUS_STATE_SHIFT;
> > > > -		if (val < ARRAY_SIZE(live_status)) {
> > > > -			seq_printf(m, "Source PSR status: 0x%x
> > > > [%s]\n",
> > > > -				   psr_status,
> > > > live_status[val]);
> > > > -			return;
> > > > -		}
> > > > +		val = I915_READ(EDP_PSR_STATUS);
> > > > +		status_val = (val & EDP_PSR_STATUS_STATE_MASK)
> > > > >>
> > > > +			      EDP_PSR_STATUS_STATE_SHIFT;
> > > > +		if (status_val < ARRAY_SIZE(live_status))
> > > > +			status = live_status[status_val];
> > > >  	}
> > > >  
> > > > -	seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > > > psr_status,
> > > > "unknown");
> > > > +	seq_printf(m, "Source PSR status: %s [0x%08x]\n",
> > > > status, val);
> > > >  }
> > > >  
> > > >  static int i915_edp_psr_status(struct seq_file *m, void *data)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > > >private);
> > > > -	u32 psrperf = 0;
> > > > -	bool enabled = false;
> > > > -	bool sink_support;
> > > > +	struct i915_psr *psr = &dev_priv->psr;
> > > > +	const char *status;
> > > > +	bool enabled;
> > > > +	u32 val;
> > > >  
> > > >  	if (!HAS_PSR(dev_priv))
> > > >  		return -ENODEV;
> > > >  
> > > > -	sink_support = dev_priv->psr.sink_support;
> > > > -	seq_printf(m, "Sink_Support: %s\n",
> > > > yesno(sink_support));
> > > > -	if (!sink_support)
> > > > +	seq_printf(m, "Sink support: %s", yesno(psr-
> > > > >sink_support));
> > > > +	if (!psr->sink_support) {
> > > > +		seq_puts(m, "\n");
> > > >  		return 0;
> > > > +	}
> > > >  
> > > >  	intel_runtime_pm_get(dev_priv);
> > > > +	mutex_lock(&psr->lock);
> > > >  
> > > > -	mutex_lock(&dev_priv->psr.lock);
> > > > -	seq_printf(m, "PSR mode: %s\n",
> > > > -		   dev_priv->psr.psr2_enabled ? "PSR2" :
> > > > "PSR1");
> > > > -	seq_printf(m, "Enabled: %s\n", yesno(dev_priv-
> > > > >psr.enabled));
> > > > -	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > > -		   dev_priv->psr.busy_frontbuffer_bits);
> > > > +	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
> > > 
> > > This can be moved closer to where "Sink support" is printed.
> > > Also,  the
> > > extra zeroes that get printed due to "%08x" look odd, please
> > > consider
> > > changing it to "%02x".
> > 
> > Done
> > 
> > > >  
> > > > -	if (dev_priv->psr.psr2_enabled)
> > > > -		enabled = I915_READ(EDP_PSR2_CTL) &
> > > > EDP_PSR2_ENABLE;
> > > > +	if (psr->enabled)
> > > > +		status = psr->psr2_enabled ? "PSR2 enabled" :
> > > > "PSR1
> > > > enabled";
> > > >  	else
> > > > -		enabled = I915_READ(EDP_PSR_CTL) &
> > > > EDP_PSR_ENABLE;
> > > > +		status = "disabled";
> > > > +	seq_printf(m, "Status: %s\n", status);
> > > 
> > > Can we just use the ctl field and get rid of this? This status
> > > and
> > > and
> > > the one below can be confusing to those not familiar with the
> > > code
> > > base.
> > > 
> > > We should be able to parse all the information we need from two
> > > lines
> > > Source PSR ctl:		[PSR1,PSR2] enabled <reg value>
> > > Source PSR status:	<state> <reg value>
> > 
> > I'm fine in having just one but when there is frontbuffer
> > modifications
> > Status is kept as enabled and Source PSR ctl and Source PSR status
> > are
> > set to idle states, 
> 
> That is a good point -  psr_invalidate() would disable the feature
> in 
> hardware but the driver state is enabled. I guess, you could change
> "Status" to "PSR mode"/"Feature state". 
> ...
> PSR mode:		{PSR1 enabled, PSR2 enabled, disabled}
> Source PSR ctl:		{enabled, disabled} ...
> Source PSR status:	...
> ...
> 
> Is this better?
> 

Yeah looks good to me.

> Also, it might be worth printing the "PSR mode:" line even when there
> is no sink_support. It's an useful piece of debug information and
> doesn't access the hardware.

Okay

Resending serie with fixes soon, thanks for the review.

> 
> 
> > we know that but those not familiar with the code
> > could think it is disabled but it usualy happen so fast that I
> > guess
> > it
> > is not a problem.
> > 
> > And when PSR is disabled we print something else other than Sink
> > support?
> > 
> > > 
> > > 
> > > >  
> > > > -	seq_printf(m, "Main link in standby mode: %s\n",
> > > > -		   yesno(dev_priv->psr.link_standby));
> > > > +	if (!psr->enabled)
> > > > +		goto unlock;
> > > >  
> > > > -	seq_printf(m, "HW Enabled & Active bit: %s\n",
> > > > yesno(enabled));
> > > > +	if (psr->psr2_enabled) {
> > > > +		val = I915_READ(EDP_PSR2_CTL);
> > > > +		enabled = val & EDP_PSR2_ENABLE;
> > > > +	} else {
> > > > +		val = I915_READ(EDP_PSR_CTL);
> > > > +		enabled = val & EDP_PSR_ENABLE;
> > > > +	}
> > > > +	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > > > +		   enableddisabled(enabled), val);
> > > > +	psr_source_status(dev_priv, m);
> > > > +	seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> > > > +		   psr->busy_frontbuffer_bits);
> > > >  
> > > >  	/*
> > > >  	 * SKL+ Perf counter is reset to 0 everytime DC state
> > > > is
> > > > entered
> > > >  	 */
> > > >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > > -		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> > > > -			EDP_PSR_PERF_CNT_MASK;
> > > > -
> > > > -		seq_printf(m, "Performance_Counter: %u\n",
> > > > psrperf);
> > > > +		val = I915_READ(EDP_PSR_PERF_CNT) &
> > > > EDP_PSR_PERF_CNT_MASK;
> > > > +		seq_printf(m, "Performance counter: %u\n",
> > > > val);
> > > >  	}
> > > >  
> > > > -	psr_source_status(dev_priv, m);
> > > > -	mutex_unlock(&dev_priv->psr.lock);
> > > > -
> > > > -	if (READ_ONCE(dev_priv->psr.debug) &
> > > > I915_PSR_DEBUG_IRQ) {
> > > > +	if (psr->debug & I915_PSR_DEBUG_IRQ) {
> > > >  		seq_printf(m, "Last attempted entry at:
> > > > %lld\n",
> > > > -			   dev_priv->psr.last_entry_attempt);
> > > > -		seq_printf(m, "Last exit at: %lld\n",
> > > > -			   dev_priv->psr.last_exit);
> > > > +			   psr->last_entry_attempt);
> > > > +		seq_printf(m, "Last exit at: %lld\n", psr-
> > > > >last_exit);
> > > >  	}
> > > >  
> > > > +unlock:
> > > > +	mutex_unlock(&psr->lock);
> > > >  	intel_runtime_pm_put(dev_priv);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 11222 bytes --]

On Tue, 2018-12-11 at 18:54 +0000, Souza, Jose wrote:
> On Tue, 2018-12-11 at 10:32 -0800, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote:
> > > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote:
> > > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > > > > The old debugfs fields was not following a naming partern and
> > > > > it
> > > > > was
> > > > > a bit confusing.
> > > > > 
> > > > > So it went from:
> > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > > > Sink_Support: yes
> > > > > PSR mode: PSR1
> > > > > Enabled: yes
> > > > > Busy frontbuffer bits: 0x000
> > > > > Main link in standby mode: no
> > > > > HW Enabled & Active bit: yes
> > > > > Source PSR status: 0x24050006 [SRDONACK]
> > > > > 
> > > > > To:
> > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > > > Sink support: yes [0x00000003]
> > > > > Status: PSR1 enabled
> > > > > Source PSR ctl: enabled [0x81f00e26]
> > > > > Source PSR status: SRDENT [0x40040006]
> > > > > Busy frontbuffer bits: 0x00000000
> > > > > 
> > > > > The 'Main link in standby mode' was removed as it is not
> > > > > useful
> > > > > but
> > > > > if needed by someone the information is still in the register
> > > > > value
> > > > > of 'Source PSR ctl' inside of the brackets, PSR mode and
> > > > > Enabled
> > > > > was
> > > > > squashed into Status, some renames and reorders and we have
> > > > > this
> > > > > cleaner version. This will also make easy to parse debugfs
> > > > > for
> > > > > IGT
> > > > > tests.
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Suggested-by: Dhinakaran Pandiyan <
> > > > > dhinakaran.pandiyan@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++--
> > > > > ----
> > > > > ----
> > > > > ----
> > > > >  1 file changed, 49 insertions(+), 47 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > index 38dcee1ca062..86303ba02666 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > @@ -2665,7 +2665,8 @@
> > > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
> > > > >  static void
> > > > >  psr_source_status(struct drm_i915_private *dev_priv, struct
> > > > > seq_file
> > > > > *m)
> > > > >  {
> > > > > -	u32 val, psr_status;
> > > > > +	u32 val, status_val;
> > > > > +	const char *status = "unknown";
> > > > >  
> > > > >  	if (dev_priv->psr.psr2_enabled) {
> > > > >  		static const char * const live_status[] = {
> > > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct
> > > > > drm_i915_private
> > > > > *dev_priv, struct seq_file *m)
> > > > >  			"BUF_ON",
> > > > >  			"TG_ON"
> > > > >  		};
> > > > > -		psr_status = I915_READ(EDP_PSR2_STATUS);
> > > > > -		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK)
> > > > > -			EDP_PSR2_STATUS_STATE_SHIFT;
> > > > > -		if (val < ARRAY_SIZE(live_status)) {
> > > > > -			seq_printf(m, "Source PSR status: 0x%x
> > > > > [%s]\n",
> > > > > -				   psr_status,
> > > > > live_status[val]);
> > > > > -			return;
> > > > > -		}
> > > > > +		val = I915_READ(EDP_PSR2_STATUS);
> > > > > +		status_val = (val & EDP_PSR2_STATUS_STATE_MASK)
> > > > > +			      EDP_PSR2_STATUS_STATE_SHIFT;
> > > > > +		if (status_val < ARRAY_SIZE(live_status))
> > > > > +			status = live_status[status_val];
> > > > >  	} else {
> > > > >  		static const char * const live_status[] = {
> > > > >  			"IDLE",
> > > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct
> > > > > drm_i915_private
> > > > > *dev_priv, struct seq_file *m)
> > > > >  			"SRDOFFACK",
> > > > >  			"SRDENT_ON",
> > > > >  		};
> > > > > -		psr_status = I915_READ(EDP_PSR_STATUS);
> > > > > -		val = (psr_status & EDP_PSR_STATUS_STATE_MASK)
> > > > > -			EDP_PSR_STATUS_STATE_SHIFT;
> > > > > -		if (val < ARRAY_SIZE(live_status)) {
> > > > > -			seq_printf(m, "Source PSR status: 0x%x
> > > > > [%s]\n",
> > > > > -				   psr_status,
> > > > > live_status[val]);
> > > > > -			return;
> > > > > -		}
> > > > > +		val = I915_READ(EDP_PSR_STATUS);
> > > > > +		status_val = (val & EDP_PSR_STATUS_STATE_MASK)
> > > > > +			      EDP_PSR_STATUS_STATE_SHIFT;
> > > > > +		if (status_val < ARRAY_SIZE(live_status))
> > > > > +			status = live_status[status_val];
> > > > >  	}
> > > > >  
> > > > > -	seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > > > > psr_status,
> > > > > "unknown");
> > > > > +	seq_printf(m, "Source PSR status: %s [0x%08x]\n",
> > > > > status, val);
> > > > >  }
> > > > >  
> > > > >  static int i915_edp_psr_status(struct seq_file *m, void
> > > > > *data)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > > > > private);
> > > > > -	u32 psrperf = 0;
> > > > > -	bool enabled = false;
> > > > > -	bool sink_support;
> > > > > +	struct i915_psr *psr = &dev_priv->psr;
> > > > > +	const char *status;
> > > > > +	bool enabled;
> > > > > +	u32 val;
> > > > >  
> > > > >  	if (!HAS_PSR(dev_priv))
> > > > >  		return -ENODEV;
> > > > >  
> > > > > -	sink_support = dev_priv->psr.sink_support;
> > > > > -	seq_printf(m, "Sink_Support: %s\n",
> > > > > yesno(sink_support));
> > > > > -	if (!sink_support)
> > > > > +	seq_printf(m, "Sink support: %s", yesno(psr-
> > > > > > sink_support));
> > > > > +	if (!psr->sink_support) {
> > > > > +		seq_puts(m, "\n");
> > > > >  		return 0;
> > > > > +	}
> > > > >  
> > > > >  	intel_runtime_pm_get(dev_priv);
> > > > > +	mutex_lock(&psr->lock);
> > > > >  
> > > > > -	mutex_lock(&dev_priv->psr.lock);
> > > > > -	seq_printf(m, "PSR mode: %s\n",
> > > > > -		   dev_priv->psr.psr2_enabled ? "PSR2" :
> > > > > "PSR1");
> > > > > -	seq_printf(m, "Enabled: %s\n", yesno(dev_priv-
> > > > > > psr.enabled));
> > > > > -	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > > > -		   dev_priv->psr.busy_frontbuffer_bits);
> > > > > +	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
> > > > 
> > > > This can be moved closer to where "Sink support" is printed.
> > > > Also,  the
> > > > extra zeroes that get printed due to "%08x" look odd, please
> > > > consider
> > > > changing it to "%02x".
> > > 
> > > Done
> > > 
> > > > >  
> > > > > -	if (dev_priv->psr.psr2_enabled)
> > > > > -		enabled = I915_READ(EDP_PSR2_CTL) &
> > > > > EDP_PSR2_ENABLE;
> > > > > +	if (psr->enabled)
> > > > > +		status = psr->psr2_enabled ? "PSR2 enabled" :
> > > > > "PSR1
> > > > > enabled";
> > > > >  	else
> > > > > -		enabled = I915_READ(EDP_PSR_CTL) &
> > > > > EDP_PSR_ENABLE;
> > > > > +		status = "disabled";
> > > > > +	seq_printf(m, "Status: %s\n", status);
> > > > 
> > > > Can we just use the ctl field and get rid of this? This status
> > > > and
> > > > and
> > > > the one below can be confusing to those not familiar with the
> > > > code
> > > > base.
> > > > 
> > > > We should be able to parse all the information we need from two
> > > > lines
> > > > Source PSR ctl:		[PSR1,PSR2] enabled <reg value>
> > > > Source PSR status:	<state> <reg value>
> > > 
> > > I'm fine in having just one but when there is frontbuffer
> > > modifications
> > > Status is kept as enabled and Source PSR ctl and Source PSR
> > > status
> > > are
> > > set to idle states, 
> > 
> > That is a good point -  psr_invalidate() would disable the feature
> > in 
> > hardware but the driver state is enabled. I guess, you could change
> > "Status" to "PSR mode"/"Feature state". 
> > ...
> > PSR mode:		{PSR1 enabled, PSR2 enabled, disabled}
> > Source PSR ctl:		{enabled, disabled} ...
> > Source PSR status:	...
> > ...
> > 
> > Is this better?
> > 
> 
> Yeah looks good to me.
> 
> > Also, it might be worth printing the "PSR mode:" line even when
> > there
> > is no sink_support. It's an useful piece of debug information and
> > doesn't access the hardware.

About this one, to read psr->enable and psr->psr2_enabled we take PSR
mutex and it is only initialized if sink support PSR.
If that information is useful when doing IGT changes I will send it in
a separate patch, sounds good?

> > 
> Okay
> 
> Resending serie with fixes soon, thanks for the review.
> 
> > 
> > > we know that but those not familiar with the code
> > > could think it is disabled but it usualy happen so fast that I
> > > guess
> > > it
> > > is not a problem.
> > > 
> > > And when PSR is disabled we print something else other than Sink
> > > support?
> > > 
> > > > 
> > > > >  
> > > > > -	seq_printf(m, "Main link in standby mode: %s\n",
> > > > > -		   yesno(dev_priv->psr.link_standby));
> > > > > +	if (!psr->enabled)
> > > > > +		goto unlock;
> > > > >  
> > > > > -	seq_printf(m, "HW Enabled & Active bit: %s\n",
> > > > > yesno(enabled));
> > > > > +	if (psr->psr2_enabled) {
> > > > > +		val = I915_READ(EDP_PSR2_CTL);
> > > > > +		enabled = val & EDP_PSR2_ENABLE;
> > > > > +	} else {
> > > > > +		val = I915_READ(EDP_PSR_CTL);
> > > > > +		enabled = val & EDP_PSR_ENABLE;
> > > > > +	}
> > > > > +	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > > > > +		   enableddisabled(enabled), val);
> > > > > +	psr_source_status(dev_priv, m);
> > > > > +	seq_printf(m, "Busy frontbuffer bits: 0x%08x\n",
> > > > > +		   psr->busy_frontbuffer_bits);
> > > > >  
> > > > >  	/*
> > > > >  	 * SKL+ Perf counter is reset to 0 everytime DC state
> > > > > is
> > > > > entered
> > > > >  	 */
> > > > >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > > > -		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> > > > > -			EDP_PSR_PERF_CNT_MASK;
> > > > > -
> > > > > -		seq_printf(m, "Performance_Counter: %u\n",
> > > > > psrperf);
> > > > > +		val = I915_READ(EDP_PSR_PERF_CNT) &
> > > > > EDP_PSR_PERF_CNT_MASK;
> > > > > +		seq_printf(m, "Performance counter: %u\n",
> > > > > val);
> > > > >  	}
> > > > >  
> > > > > -	psr_source_status(dev_priv, m);
> > > > > -	mutex_unlock(&dev_priv->psr.lock);
> > > > > -
> > > > > -	if (READ_ONCE(dev_priv->psr.debug) &
> > > > > I915_PSR_DEBUG_IRQ) {
> > > > > +	if (psr->debug & I915_PSR_DEBUG_IRQ) {
> > > > >  		seq_printf(m, "Last attempted entry at:
> > > > > %lld\n",
> > > > > -			   dev_priv->psr.last_entry_attempt);
> > > > > -		seq_printf(m, "Last exit at: %lld\n",
> > > > > -			   dev_priv->psr.last_exit);
> > > > > +			   psr->last_entry_attempt);
> > > > > +		seq_printf(m, "Last exit at: %lld\n", psr-
> > > > > > last_exit);
> > > > >  	}
> > > > >  
> > > > > +unlock:
> > > > > +	mutex_unlock(&psr->lock);
> > > > >  	intel_runtime_pm_put(dev_priv);
> > > > > +
> > > > >  	return 0;
> > > > >  }
> > > > >  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 2/5] drm/i915: Refactor PSR status debugfs

[Dhinakaran Pandiyan]

On Wed, 2019-01-02 at 09:09 -0800, Souza, Jose wrote:
> On Tue, 2018-12-11 at 18:54 +0000, Souza, Jose wrote:
> > On Tue, 2018-12-11 at 10:32 -0800, Dhinakaran Pandiyan wrote:
> > > On Tue, 2018-12-11 at 04:44 -0800, Souza, Jose wrote:
> > > > On Mon, 2018-12-10 at 22:51 -0800, Dhinakaran Pandiyan wrote:
> > > > > On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > The old debugfs fields was not following a naming partern
> > > > > > and
> > > > > > it
> > > > > > was
> > > > > > a bit confusing.
> > > > > > 
> > > > > > So it went from:
> > > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > > > > Sink_Support: yes
> > > > > > PSR mode: PSR1
> > > > > > Enabled: yes
> > > > > > Busy frontbuffer bits: 0x000
> > > > > > Main link in standby mode: no
> > > > > > HW Enabled & Active bit: yes
> > > > > > Source PSR status: 0x24050006 [SRDONACK]
> > > > > > 
> > > > > > To:
> > > > > > ~$ sudo more /sys/kernel/debug/dri/0/i915_edp_psr_status
> > > > > > Sink support: yes [0x00000003]
> > > > > > Status: PSR1 enabled
> > > > > > Source PSR ctl: enabled [0x81f00e26]
> > > > > > Source PSR status: SRDENT [0x40040006]
> > > > > > Busy frontbuffer bits: 0x00000000
> > > > > > 
> > > > > > The 'Main link in standby mode' was removed as it is not
> > > > > > useful
> > > > > > but
> > > > > > if needed by someone the information is still in the
> > > > > > register
> > > > > > value
> > > > > > of 'Source PSR ctl' inside of the brackets, PSR mode and
> > > > > > Enabled
> > > > > > was
> > > > > > squashed into Status, some renames and reorders and we have
> > > > > > this
> > > > > > cleaner version. This will also make easy to parse debugfs
> > > > > > for
> > > > > > IGT
> > > > > > tests.
> > > > > > 
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > Suggested-by: Dhinakaran Pandiyan <
> > > > > > dhinakaran.pandiyan@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_debugfs.c | 96 +++++++++++++++--
> > > > > > ----
> > > > > > ----
> > > > > > ----
> > > > > >  1 file changed, 49 insertions(+), 47 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > index 38dcee1ca062..86303ba02666 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > @@ -2665,7 +2665,8 @@
> > > > > > DEFINE_SHOW_ATTRIBUTE(i915_psr_sink_status);
> > > > > >  static void
> > > > > >  psr_source_status(struct drm_i915_private *dev_priv,
> > > > > > struct
> > > > > > seq_file
> > > > > > *m)
> > > > > >  {
> > > > > > -	u32 val, psr_status;
> > > > > > +	u32 val, status_val;
> > > > > > +	const char *status = "unknown";
> > > > > >  
> > > > > >  	if (dev_priv->psr.psr2_enabled) {
> > > > > >  		static const char * const live_status[] = {
> > > > > > @@ -2681,14 +2682,11 @@ psr_source_status(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv, struct seq_file *m)
> > > > > >  			"BUF_ON",
> > > > > >  			"TG_ON"
> > > > > >  		};
> > > > > > -		psr_status = I915_READ(EDP_PSR2_STATUS);
> > > > > > -		val = (psr_status & EDP_PSR2_STATUS_STATE_MASK)
> > > > > > -			EDP_PSR2_STATUS_STATE_SHIFT;
> > > > > > -		if (val < ARRAY_SIZE(live_status)) {
> > > > > > -			seq_printf(m, "Source PSR status: 0x%x
> > > > > > [%s]\n",
> > > > > > -				   psr_status,
> > > > > > live_status[val]);
> > > > > > -			return;
> > > > > > -		}
> > > > > > +		val = I915_READ(EDP_PSR2_STATUS);
> > > > > > +		status_val = (val & EDP_PSR2_STATUS_STATE_MASK)
> > > > > > +			      EDP_PSR2_STATUS_STATE_SHIFT;
> > > > > > +		if (status_val < ARRAY_SIZE(live_status))
> > > > > > +			status = live_status[status_val];
> > > > > >  	} else {
> > > > > >  		static const char * const live_status[] = {
> > > > > >  			"IDLE",
> > > > > > @@ -2700,74 +2698,78 @@ psr_source_status(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv, struct seq_file *m)
> > > > > >  			"SRDOFFACK",
> > > > > >  			"SRDENT_ON",
> > > > > >  		};
> > > > > > -		psr_status = I915_READ(EDP_PSR_STATUS);
> > > > > > -		val = (psr_status & EDP_PSR_STATUS_STATE_MASK)
> > > > > > -			EDP_PSR_STATUS_STATE_SHIFT;
> > > > > > -		if (val < ARRAY_SIZE(live_status)) {
> > > > > > -			seq_printf(m, "Source PSR status: 0x%x
> > > > > > [%s]\n",
> > > > > > -				   psr_status,
> > > > > > live_status[val]);
> > > > > > -			return;
> > > > > > -		}
> > > > > > +		val = I915_READ(EDP_PSR_STATUS);
> > > > > > +		status_val = (val & EDP_PSR_STATUS_STATE_MASK)
> > > > > > +			      EDP_PSR_STATUS_STATE_SHIFT;
> > > > > > +		if (status_val < ARRAY_SIZE(live_status))
> > > > > > +			status = live_status[status_val];
> > > > > >  	}
> > > > > >  
> > > > > > -	seq_printf(m, "Source PSR status: 0x%x [%s]\n",
> > > > > > psr_status,
> > > > > > "unknown");
> > > > > > +	seq_printf(m, "Source PSR status: %s [0x%08x]\n",
> > > > > > status, val);
> > > > > >  }
> > > > > >  
> > > > > >  static int i915_edp_psr_status(struct seq_file *m, void
> > > > > > *data)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = node_to_i915(m-
> > > > > > > private);
> > > > > > 
> > > > > > -	u32 psrperf = 0;
> > > > > > -	bool enabled = false;
> > > > > > -	bool sink_support;
> > > > > > +	struct i915_psr *psr = &dev_priv->psr;
> > > > > > +	const char *status;
> > > > > > +	bool enabled;
> > > > > > +	u32 val;
> > > > > >  
> > > > > >  	if (!HAS_PSR(dev_priv))
> > > > > >  		return -ENODEV;
> > > > > >  
> > > > > > -	sink_support = dev_priv->psr.sink_support;
> > > > > > -	seq_printf(m, "Sink_Support: %s\n",
> > > > > > yesno(sink_support));
> > > > > > -	if (!sink_support)
> > > > > > +	seq_printf(m, "Sink support: %s", yesno(psr-
> > > > > > > sink_support));
> > > > > > 
> > > > > > +	if (!psr->sink_support) {
> > > > > > +		seq_puts(m, "\n");
> > > > > >  		return 0;
> > > > > > +	}
> > > > > >  
> > > > > >  	intel_runtime_pm_get(dev_priv);
> > > > > > +	mutex_lock(&psr->lock);
> > > > > >  
> > > > > > -	mutex_lock(&dev_priv->psr.lock);
> > > > > > -	seq_printf(m, "PSR mode: %s\n",
> > > > > > -		   dev_priv->psr.psr2_enabled ? "PSR2" :
> > > > > > "PSR1");
> > > > > > -	seq_printf(m, "Enabled: %s\n", yesno(dev_priv-
> > > > > > > psr.enabled));
> > > > > > 
> > > > > > -	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > > > > -		   dev_priv->psr.busy_frontbuffer_bits);
> > > > > > +	seq_printf(m, " [0x%08x]\n", psr->dp->psr_dpcd[0]);
> > > > > 
> > > > > This can be moved closer to where "Sink support" is printed.
> > > > > Also,  the
> > > > > extra zeroes that get printed due to "%08x" look odd, please
> > > > > consider
> > > > > changing it to "%02x".
> > > > 
> > > > Done
> > > > 
> > > > > >  
> > > > > > -	if (dev_priv->psr.psr2_enabled)
> > > > > > -		enabled = I915_READ(EDP_PSR2_CTL) &
> > > > > > EDP_PSR2_ENABLE;
> > > > > > +	if (psr->enabled)
> > > > > > +		status = psr->psr2_enabled ? "PSR2 enabled" :
> > > > > > "PSR1
> > > > > > enabled";
> > > > > >  	else
> > > > > > -		enabled = I915_READ(EDP_PSR_CTL) &
> > > > > > EDP_PSR_ENABLE;
> > > > > > +		status = "disabled";
> > > > > > +	seq_printf(m, "Status: %s\n", status);
> > > > > 
> > > > > Can we just use the ctl field and get rid of this? This
> > > > > status
> > > > > and
> > > > > and
> > > > > the one below can be confusing to those not familiar with the
> > > > > code
> > > > > base.
> > > > > 
> > > > > We should be able to parse all the information we need from
> > > > > two
> > > > > lines
> > > > > Source PSR ctl:		[PSR1,PSR2] enabled <reg value>
> > > > > Source PSR status:	<state> <reg value>
> > > > 
> > > > I'm fine in having just one but when there is frontbuffer
> > > > modifications
> > > > Status is kept as enabled and Source PSR ctl and Source PSR
> > > > status
> > > > are
> > > > set to idle states, 
> > > 
> > > That is a good point -  psr_invalidate() would disable the
> > > feature
> > > in 
> > > hardware but the driver state is enabled. I guess, you could
> > > change
> > > "Status" to "PSR mode"/"Feature state". 
> > > ...
> > > PSR mode:		{PSR1 enabled, PSR2 enabled, disabled}
> > > Source PSR ctl:		{enabled, disabled} ...
> > > Source PSR status:	...
> > > ...
> > > 
> > > Is this better?
> > > 
> > 
> > Yeah looks good to me.
> > 
> > > Also, it might be worth printing the "PSR mode:" line even when
> > > there
> > > is no sink_support. It's an useful piece of debug information and
> > > doesn't access the hardware.
> 
> About this one, to read psr->enable and psr->psr2_enabled we take PSR
> mutex and it is only initialized if sink support PSR.
> If that information is useful when doing IGT changes I will send it
> in
> a separate patch, sounds good?

Yeah, that's fine.

-DK

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

[PATCH 3/5] drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2

[José Roberto de Souza]

PSR2 only trigger interruptions for AUX error, so let's not print
useless information in debugsfs.
Also adding a comment to intel_psr_irq_handler() about that.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 drivers/gpu/drm/i915/intel_psr.c    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 86303ba02666..505d93b31eb6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2760,7 +2760,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		seq_printf(m, "Performance counter: %u\n", val);
 	}
 
-	if (psr->debug & I915_PSR_DEBUG_IRQ) {
+	if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) {
 		seq_printf(m, "Last attempted entry at: %lld\n",
 			   psr->last_entry_attempt);
 		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 15a2121aa64f..85349a57689c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -203,6 +203,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 			mask |= EDP_PSR_ERROR(shift);
 		}
 
+		/* PSR2 don't trigger PRE_ENTRY and POST_EXIT interruptions */
 		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
 			dev_priv->psr.last_entry_attempt = time_ns;
 			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
-- 
2.19.2

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

Re: [PATCH 3/5] drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2

[Dhinakaran Pandiyan]

On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> PSR2 only trigger interruptions for AUX error, so let's not print
> useless information in debugsfs.
> Also adding a comment to intel_psr_irq_handler() about that.

The code still allows the enabling of interrupt debugging for PSR2. Fix
that to reject debugfs writes?


> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 86303ba02666..505d93b31eb6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2760,7 +2760,7 @@ static int i915_edp_psr_status(struct seq_file
> *m, void *data)
>  		seq_printf(m, "Performance counter: %u\n", val);
>  	}
>  
> -	if (psr->debug & I915_PSR_DEBUG_IRQ) {
> +	if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) {
>  		seq_printf(m, "Last attempted entry at: %lld\n",
>  			   psr->last_entry_attempt);
>  		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 15a2121aa64f..85349a57689c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -203,6 +203,7 @@ void intel_psr_irq_handler(struct
> drm_i915_private *dev_priv, u32 psr_iir)
>  			mask |= EDP_PSR_ERROR(shift);
>  		}
>  
> +		/* PSR2 don't trigger PRE_ENTRY and POST_EXIT
> interruptions */
>  		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
>  			dev_priv->psr.last_entry_attempt = time_ns;
>  			DRM_DEBUG_KMS("[transcoder %s] PSR entry
> attempt in 2 vblanks\n",

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

Re: [PATCH 3/5] drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 2107 bytes --]

On Mon, 2018-12-10 at 23:03 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > PSR2 only trigger interruptions for AUX error, so let's not print
> > useless information in debugsfs.
> > Also adding a comment to intel_psr_irq_handler() about that.
> 
> The code still allows the enabling of interrupt debugging for PSR2.
> Fix
> that to reject debugfs writes?

Done

> 
> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> >  drivers/gpu/drm/i915/intel_psr.c    | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 86303ba02666..505d93b31eb6 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2760,7 +2760,7 @@ static int i915_edp_psr_status(struct
> > seq_file
> > *m, void *data)
> >  		seq_printf(m, "Performance counter: %u\n", val);
> >  	}
> >  
> > -	if (psr->debug & I915_PSR_DEBUG_IRQ) {
> > +	if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) {
> >  		seq_printf(m, "Last attempted entry at: %lld\n",
> >  			   psr->last_entry_attempt);
> >  		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 15a2121aa64f..85349a57689c 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -203,6 +203,7 @@ void intel_psr_irq_handler(struct
> > drm_i915_private *dev_priv, u32 psr_iir)
> >  			mask |= EDP_PSR_ERROR(shift);
> >  		}
> >  
> > +		/* PSR2 don't trigger PRE_ENTRY and POST_EXIT
> > interruptions */
> >  		if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
> >  			dev_priv->psr.last_entry_attempt = time_ns;
> >  			DRM_DEBUG_KMS("[transcoder %s] PSR entry
> > attempt in 2 vblanks\n",

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 4/5] drm/i915: Add PSR2 selective update status registers and bits definitions

[José Roberto de Souza]

This register contains how many blocks was sent in the past selective
updates.
Those registers are not kept set all the times but pulling it after flip
can show that the expected values are set for the current frame and the
previous ones too.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0a7d60509ca7..7d634f34ca7d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4248,6 +4248,12 @@ enum {
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
+#define EDP_PSR2_SU_STATUS					_MMIO(0x6f914)
+#define EDP_PSR2_SU_STATUS2					_MMIO(0x6F918)
+#define EDP_PSR2_SU_STATUS3					_MMIO(0x6F91C)
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i)	((i) * 10)
+#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i)	(0x3FF << ((i) * 10))
+
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
 #define PCH_ADPA                _MMIO(0xe1100)
-- 
2.19.2

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

Re: [PATCH 4/5] drm/i915: Add PSR2 selective update status registers and bits definitions

[Dhinakaran Pandiyan]

On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> This register contains how many blocks was sent in the past selective
> updates.
> Those registers are not kept set all the times but pulling it after
> flip
> can show that the expected values are set for the current frame and
> the
> previous ones too.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 0a7d60509ca7..7d634f34ca7d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4248,6 +4248,12 @@ enum {
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> +#define EDP_PSR2_SU_STATUS					_MMIO(0
> x6f914)
> +#define EDP_PSR2_SU_STATUS2					_MMIO(0
> x6F918)
> +#define EDP_PSR2_SU_STATUS3					_MMIO(0
> x6F91C)
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i)	((i) *
> 10)
> +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i)	(0x3FF
> << ((i) * 10))
How about moving the MMIO selection logic to the macros? 

#define PSR2_SU_HISTORY 8
#define _PSR2_SU_STATUS_0 0x6f914
#define _PSR2_SU_STATUS_1 0x6f918
#define _PSR2_SU_STATUS(dword) _MMIO(_PICK_EVEN((dword),\
_PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
#define PSR2_SU_SHIFT(frame) ((frame) % 3) * 10
#define PSR2_SU_MASK(frame)  (0x3ff << PSR2_SU_SHIFT(frame))
#define PSR2_SU_BLOCKS(frame) _PSR2_SU_STATUS((frame) / 3)


> +
>  /* VGA port control */
>  #define ADPA			_MMIO(0x61100)
>  #define PCH_ADPA                _MMIO(0xe1100)

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

Re: [PATCH 4/5] drm/i915: Add PSR2 selective update status registers and bits definitions

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 2030 bytes --]

On Mon, 2018-12-10 at 23:51 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > This register contains how many blocks was sent in the past
> > selective
> > updates.
> > Those registers are not kept set all the times but pulling it after
> > flip
> > can show that the expected values are set for the current frame and
> > the
> > previous ones too.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 0a7d60509ca7..7d634f34ca7d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4248,6 +4248,12 @@ enum {
> >  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> >  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> >  
> > +#define EDP_PSR2_SU_STATUS					_MMIO(0
> > x6f914)
> > +#define EDP_PSR2_SU_STATUS2					
> > _MMIO(0
> > x6F918)
> > +#define EDP_PSR2_SU_STATUS3					
> > _MMIO(0
> > x6F91C)
> > +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i)	
> > ((i) *
> > 10)
> > +#define  EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i)	(0x3FF
> > << ((i) * 10))
> How about moving the MMIO selection logic to the macros? 
> 
> #define PSR2_SU_HISTORY 8
> #define _PSR2_SU_STATUS_0 0x6f914
> #define _PSR2_SU_STATUS_1 0x6f918
> #define _PSR2_SU_STATUS(dword) _MMIO(_PICK_EVEN((dword),\
> _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> #define PSR2_SU_SHIFT(frame) ((frame) % 3) * 10
> #define PSR2_SU_MASK(frame)  (0x3ff << PSR2_SU_SHIFT(frame))
> #define PSR2_SU_BLOCKS(frame) _PSR2_SU_STATUS((frame) / 3)

Looks better

> 
> 
> > +
> >  /* VGA port control */
> >  #define ADPA			_MMIO(0x61100)
> >  #define PCH_ADPA                _MMIO(0xe1100)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 5/5] drm/i915/debugfs: Print PSR selective update status register values

[José Roberto de Souza]

The value of this registers will be used to test if PSR2 is doing
selective update and if the number of blocks match with the expected.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 42 ++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 505d93b31eb6..754b33194e09 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2760,10 +2760,44 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		seq_printf(m, "Performance counter: %u\n", val);
 	}
 
-	if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) {
-		seq_printf(m, "Last attempted entry at: %lld\n",
-			   psr->last_entry_attempt);
-		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
+	if (!psr->psr2_enabled) {
+		if (psr->debug & I915_PSR_DEBUG_IRQ) {
+			seq_printf(m, "Last attempted entry at: %lld\n",
+				   psr->last_entry_attempt);
+			seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
+		}
+	} else {
+		u8 i;
+
+		val = I915_READ(EDP_PSR2_SU_STATUS);
+		seq_printf(m, "PSR2 SU status: 0x%08x\n", val);
+		for (i = 0; val && i < 3; i++) {
+			u32 num;
+
+			num = val & EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
+			num = num >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
+			seq_printf(m, "\tSU num blocks in frame N-%u: %u\n", i, num);
+		}
+
+		val = I915_READ(EDP_PSR2_SU_STATUS2);
+		seq_printf(m, "PSR2 SU status2: 0x%08x\n", val);
+		for (i = 0; val && i < 3; i++) {
+			u32 num;
+
+			num = val & EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
+			num = num >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
+			seq_printf(m, "\tSU num blocks in frame N-%u: %u\n", i + 3, num);
+		}
+
+		val = I915_READ(EDP_PSR2_SU_STATUS3);
+		seq_printf(m, "PSR2 SU status3: 0x%08x\n", val);
+		for (i = 0; val && i < 2; i++) {
+			u32 num;
+
+			num = val & EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
+			num = num >> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
+			seq_printf(m, "\tSU num blocks in frame N-%u: %u\n", i + 6, num);
+		}
 	}
 
 unlock:
-- 
2.19.2

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

Re: [PATCH 5/5] drm/i915/debugfs: Print PSR selective update status register values

[Dhinakaran Pandiyan]

On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> The value of this registers will be used to test if PSR2 is doing
> selective update and if the number of blocks match with the expected.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 42 ++++++++++++++++++++++++++-
> --
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 505d93b31eb6..754b33194e09 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2760,10 +2760,44 @@ static int i915_edp_psr_status(struct
> seq_file *m, void *data)
>  		seq_printf(m, "Performance counter: %u\n", val);
>  	}
>  
> -	if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) {
> -		seq_printf(m, "Last attempted entry at: %lld\n",
> -			   psr->last_entry_attempt);
> -		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> +	if (!psr->psr2_enabled) {
> +		if (psr->debug & I915_PSR_DEBUG_IRQ) {
> +			seq_printf(m, "Last attempted entry at:
> %lld\n",
> +				   psr->last_entry_attempt);
> +			seq_printf(m, "Last exit at: %lld\n", psr-
> >last_exit);
> +		}
> +	} else {
> +		u8 i;
> +
> +		val = I915_READ(EDP_PSR2_SU_STATUS);
> +		seq_printf(m, "PSR2 SU status: 0x%08x\n", val);
> +		for (i = 0; val && i < 3; i++) {
> +			u32 num;
> +
> +			num = val &
> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
> +			num = num >>
> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
> +			seq_printf(m, "\tSU num blocks in frame N-%u:
> %u\n", i, num);
> +		}
> +
> +		val = I915_READ(EDP_PSR2_SU_STATUS2);
> +		seq_printf(m, "PSR2 SU status2: 0x%08x\n", val);
> +		for (i = 0; val && i < 3; i++) {
> +			u32 num;
> +
> +			num = val &
> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
> +			num = num >>
> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
> +			seq_printf(m, "\tSU num blocks in frame N-%u:
> %u\n", i + 3, num);
> +		}
> +
> +		val = I915_READ(EDP_PSR2_SU_STATUS3);
> +		seq_printf(m, "PSR2 SU status3: 0x%08x\n", val);
> +		for (i = 0; val && i < 2; i++) {
> +			u32 num;
> +
> +			num = val &
> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
> +			num = num >>
> EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
> +			seq_printf(m, "\tSU num blocks in frame N-%u:
> %u\n", i + 6, num);
nitpick: Have you considered reducing the text that's getting printed
here? I guess we might not need to increase the read buffer size in IGT
if we do some thing like this. 

Frame	SU blocks
0	f
1	o
2	o
....

I'll leave it to you if you want to change, but I do prefer making this
less verbose.

> +		}
>  	}
>  
>  unlock:

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

Re: [PATCH 5/5] drm/i915/debugfs: Print PSR selective update status register values

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 3548 bytes --]

On Tue, 2018-12-11 at 14:20 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > The value of this registers will be used to test if PSR2 is doing
> > selective update and if the number of blocks match with the
> > expected.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 42
> > ++++++++++++++++++++++++++-
> > --
> >  1 file changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 505d93b31eb6..754b33194e09 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2760,10 +2760,44 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  		seq_printf(m, "Performance counter: %u\n", val);
> >  	}
> >  
> > -	if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) {
> > -		seq_printf(m, "Last attempted entry at: %lld\n",
> > -			   psr->last_entry_attempt);
> > -		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> > +	if (!psr->psr2_enabled) {
> > +		if (psr->debug & I915_PSR_DEBUG_IRQ) {
> > +			seq_printf(m, "Last attempted entry at:
> > %lld\n",
> > +				   psr->last_entry_attempt);
> > +			seq_printf(m, "Last exit at: %lld\n", psr-
> > > last_exit);
> > +		}
> > +	} else {
> > +		u8 i;
> > +
> > +		val = I915_READ(EDP_PSR2_SU_STATUS);
> > +		seq_printf(m, "PSR2 SU status: 0x%08x\n", val);
> > +		for (i = 0; val && i < 3; i++) {
> > +			u32 num;
> > +
> > +			num = val &
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
> > +			num = num >>
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
> > +			seq_printf(m, "\tSU num blocks in frame N-%u:
> > %u\n", i, num);
> > +		}
> > +
> > +		val = I915_READ(EDP_PSR2_SU_STATUS2);
> > +		seq_printf(m, "PSR2 SU status2: 0x%08x\n", val);
> > +		for (i = 0; val && i < 3; i++) {
> > +			u32 num;
> > +
> > +			num = val &
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
> > +			num = num >>
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
> > +			seq_printf(m, "\tSU num blocks in frame N-%u:
> > %u\n", i + 3, num);
> > +		}
> > +
> > +		val = I915_READ(EDP_PSR2_SU_STATUS3);
> > +		seq_printf(m, "PSR2 SU status3: 0x%08x\n", val);
> > +		for (i = 0; val && i < 2; i++) {
> > +			u32 num;
> > +
> > +			num = val &
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_MASK(i);
> > +			num = num >>
> > EDP_PSR2_SU_STATUS_NUM_SU_BLOCKS_IN_FRAME_SHIFT(i);
> > +			seq_printf(m, "\tSU num blocks in frame N-%u:
> > %u\n", i + 6, num);
> nitpick: Have you considered reducing the text that's getting printed
> here? I guess we might not need to increase the read buffer size in
> IGT
> if we do some thing like this. 
> 
> Frame	SU blocks
> 0	f
> 1	o
> 2	o
> ....
> 
> I'll leave it to you if you want to change, but I do prefer making
> this
> less verbose.

Down to this:

Sink support: yes [0x03]
PSR mode: PSR2 enabled
Source PSR ctl: enabled [0xc2000216]
Source PSR status: CAPTURE [0x14080030]
Busy frontbuffer bits: 0x00000000
Frame:  PSR2 SU blocks:
0       0
1       0
2       0
3       0
4       0
5       0
6       0
7       0

I will just check if the SU blocks are easy to parse from IGT tests.

> 
> > +		}
> >  	}
> >  
> >  unlock:

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
URL   : https://patchwork.freedesktop.org/series/53510/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7d15d6359a35 drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
e8491ed887e0 drm/i915: Refactor PSR status debugfs
-:22: WARNING:BAD_SIGN_OFF: Use a single space after To:
#22: 
To:

-:22: ERROR:BAD_SIGN_OFF: Unrecognized email address: ''
#22: 
To:

total: 1 errors, 1 warnings, 0 checks, 144 lines checked
338a52915431 drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2
a0f017269bbc drm/i915: Add PSR2 selective update status registers and bits definitions
02e29e5d17d8 drm/i915/debugfs: Print PSR selective update status register values

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

✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
URL   : https://patchwork.freedesktop.org/series/53510/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5263 -> Patchwork_11014
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_11014 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11014, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/53510/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11014:

### IGT changes ###

#### Warnings ####

  * igt@kms_psr@cursor_plane_move:
    - fi-kbl-r:           PASS -> SKIP +3
    - fi-whl-u:           PASS -> SKIP +3
    - {fi-icl-u3}:        PASS -> SKIP +3

  * igt@kms_psr@primary_mmap_gtt:
    - fi-kbl-7560u:       PASS -> SKIP +3

  * igt@kms_psr@primary_page_flip:
    - fi-skl-6600u:       PASS -> SKIP +3

  * igt@kms_psr@sprite_plane_onoff:
    - fi-skl-6700hq:      PASS -> SKIP +3

  
Known issues
------------

  Here are the changes found in Patchwork_11014 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_create@basic-files:
    - fi-bsw-n3050:       PASS -> FAIL [fdo#108656]

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       NOTRUN -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_coherency:
    - fi-gdg-551:         PASS -> DMESG-FAIL [fdo#107164]

  * igt@i915_selftest@live_contexts:
    - fi-cfl-8109u:       PASS -> DMESG-FAIL [fdo#108698]

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-skl-6770hq:      DMESG-WARN [fdo#105541] -> PASS
    - {fi-kbl-7567u}:     DMESG-WARN [fdo#105602] / [fdo#108529] -> PASS +1

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u3}:        FAIL [fdo#103167] -> PASS

  * igt@pm_rpm@module-reload:
    - {fi-kbl-7567u}:     DMESG-WARN [fdo#108529] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         FAIL [fdo#104008] -> PASS

  
#### Warnings ####

  * igt@i915_selftest@live_contexts:
    - {fi-icl-u3}:        INCOMPLETE [fdo#108315] -> DMESG-FAIL [fdo#108569]

  * igt@kms_chamelium@common-hpd-after-suspend:
    - {fi-kbl-7567u}:     DMESG-FAIL [fdo#105079] -> DMESG-WARN [fdo#108473]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107164]: https://bugs.freedesktop.org/show_bug.cgi?id=107164
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108315]: https://bugs.freedesktop.org/show_bug.cgi?id=108315
  [fdo#108473]: https://bugs.freedesktop.org/show_bug.cgi?id=108473
  [fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108656]: https://bugs.freedesktop.org/show_bug.cgi?id=108656
  [fdo#108698]: https://bugs.freedesktop.org/show_bug.cgi?id=108698


Participating hosts (50 -> 45)
------------------------------

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


Build changes
-------------

    * Linux: CI_DRM_5263 -> Patchwork_11014

  CI_DRM_5263: 823664600f1dc6b351612283cd13836b0d768251 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4740: dd8de0efa64e50bc06c2882a0028d98ad870e752 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11014: 02e29e5d17d8c10b93c527a923d3234cb70ace5e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

02e29e5d17d8 drm/i915/debugfs: Print PSR selective update status register values
a0f017269bbc drm/i915: Add PSR2 selective update status registers and bits definitions
338a52915431 drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2
e8491ed887e0 drm/i915: Refactor PSR status debugfs
7d15d6359a35 drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11014/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
URL   : https://patchwork.freedesktop.org/series/53510/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5263_full -> Patchwork_11014_full
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_11014_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11014_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11014_full:

### IGT changes ###

#### Possible regressions ####

  * {igt@kms_rotation_crc@multiplane-rotation-cropping-top}:
    - shard-kbl:          PASS -> DMESG-FAIL

  
#### Warnings ####

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-msflip-blt:
    - shard-skl:          PASS -> SKIP +109

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt:
    - {shard-iclb}:       PASS -> SKIP +128

  * igt@kms_vblank@pipe-a-query-idle-hang:
    - shard-snb:          PASS -> SKIP +6

  
Known issues
------------

  Here are the changes found in Patchwork_11014_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@pi-ringfull-blt:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#103158]

  * igt@gem_exec_schedule@pi-ringfull-render:
    - shard-skl:          NOTRUN -> FAIL [fdo#103158] +1

  * igt@gem_ppgtt@blt-vs-render-ctxn:
    - shard-skl:          NOTRUN -> TIMEOUT [fdo#108039]

  * igt@gem_userptr_blits@readonly-unsync:
    - shard-skl:          NOTRUN -> TIMEOUT [fdo#108887]

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@i915_suspend@shrink:
    - shard-skl:          NOTRUN -> INCOMPLETE [fdo#106886]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - {shard-iclb}:       NOTRUN -> DMESG-WARN [fdo#107956] +1

  * igt@kms_ccs@pipe-c-crc-sprite-planes-basic:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#107725] +1

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x85-sliding:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#103232] +2

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#107724] +7

  * igt@kms_draw_crc@draw-method-rgb565-blt-untiled:
    - {shard-iclb}:       NOTRUN -> WARN [fdo#108336]

  * igt@kms_flip@dpms-vs-vblank-race:
    - shard-kbl:          PASS -> FAIL [fdo#103060]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-onoff:
    - {shard-iclb}:       PASS -> DMESG-FAIL [fdo#107724]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1
    - shard-glk:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - {shard-iclb}:       PASS -> FAIL [fdo#103167] +3

  * igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-render:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#107724] / [fdo#108336] +5

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - {shard-iclb}:       PASS -> FAIL [fdo#105683] / [fdo#108040]

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-skl:          NOTRUN -> INCOMPLETE [fdo#104108] / [fdo#105959]

  * {igt@kms_plane@pixel-format-pipe-a-planes-source-clamping}:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#106885] +1

  * igt@kms_plane@pixel-format-pipe-b-planes:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#103166]

  * {igt@kms_plane@pixel-format-pipe-c-planes-source-clamping}:
    - shard-apl:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - shard-glk:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          PASS -> FAIL [fdo#107815]

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - {shard-iclb}:       PASS -> FAIL [fdo#103166] +1

  * igt@kms_plane_scaling@pipe-b-scaler-with-pixel-format:
    - {shard-iclb}:       NOTRUN -> DMESG-WARN [fdo#107724] +3

  * igt@kms_setmode@basic:
    - shard-skl:          NOTRUN -> FAIL [fdo#99912]

  * igt@kms_sysfs_edid_timing:
    - shard-skl:          NOTRUN -> FAIL [fdo#100047]

  * igt@kms_universal_plane@universal-plane-pipe-c-functional:
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#107713]

  * igt@pm_rpm@gem-execbuf-stress-extra-wait:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#108840]

  * igt@pm_rpm@pc8-residency:
    - shard-skl:          SKIP -> INCOMPLETE [fdo#107807]

  * igt@pm_rpm@system-suspend-devices:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@hang:
    - shard-glk:          INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

  * igt@kms_atomic_transition@plane-all-modeset-transition-internal-panels:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] -> PASS +10

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-kbl:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-sliding:
    - shard-skl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-skl:          INCOMPLETE [fdo#104108] -> PASS

  * igt@kms_cursor_legacy@all-pipes-torture-move:
    - shard-hsw:          DMESG-WARN [fdo#107122] -> PASS

  * igt@kms_draw_crc@draw-method-rgb565-mmap-cpu-untiled:
    - shard-skl:          FAIL [fdo#103184] -> PASS

  * igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-xtiled:
    - shard-glk:          FAIL [fdo#103184] -> PASS

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-skl:          FAIL [fdo#103833] / [fdo#105682] -> PASS

  * igt@kms_fbcon_fbt@psr:
    - {shard-iclb}:       FAIL [fdo#107882] -> SKIP

  * igt@kms_fbcon_fbt@psr-suspend:
    - shard-skl:          FAIL [fdo#107882] -> SKIP

  * igt@kms_flip@dpms-vs-vblank-race-interruptible:
    - shard-hsw:          DMESG-WARN [fdo#102614] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-mmap-cpu:
    - {shard-iclb}:       DMESG-FAIL [fdo#107724] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
    - shard-glk:          FAIL [fdo#103167] -> PASS +3

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-wc:
    - {shard-iclb}:       DMESG-FAIL [fdo#107724] -> SKIP

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-render:
    - shard-skl:          FAIL [fdo#103167] -> SKIP

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-blt:
    - {shard-iclb}:       FAIL [fdo#103167] -> SKIP +4

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - {shard-iclb}:       FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          FAIL [fdo#107815] / [fdo#108145] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
    - shard-apl:          FAIL [fdo#103166] -> PASS +1

  * igt@kms_psr@sprite_mmap_gtt:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] -> SKIP

  * igt@kms_rotation_crc@primary-rotation-180:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +3

  * igt@perf_pmu@rc6-runtime-pm:
    - shard-skl:          FAIL [fdo#105010] -> PASS

  * igt@pm_rpm@legacy-planes-dpms:
    - shard-skl:          INCOMPLETE [fdo#105959] / [fdo#107807] -> PASS

  
#### Warnings ####

  * igt@i915_selftest@live_contexts:
    - {shard-iclb}:       INCOMPLETE [fdo#108315] -> DMESG-FAIL [fdo#108569]

  * igt@kms_cursor_crc@cursor-64x64-onscreen:
    - {shard-iclb}:       FAIL [fdo#103232] -> DMESG-WARN [fdo#107724] / [fdo#108336] +1

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103833]: https://bugs.freedesktop.org/show_bug.cgi?id=103833
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105010]: https://bugs.freedesktop.org/show_bug.cgi?id=105010
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
  [fdo#105959]: https://bugs.freedesktop.org/show_bug.cgi?id=105959
  [fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
  [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
  [fdo#107122]: https://bugs.freedesktop.org/show_bug.cgi?id=107122
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107882]: https://bugs.freedesktop.org/show_bug.cgi?id=107882
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108039]: https://bugs.freedesktop.org/show_bug.cgi?id=108039
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108315]: https://bugs.freedesktop.org/show_bug.cgi?id=108315
  [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#108887]: https://bugs.freedesktop.org/show_bug.cgi?id=108887
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5263 -> Patchwork_11014

  CI_DRM_5263: 823664600f1dc6b351612283cd13836b0d768251 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4740: dd8de0efa64e50bc06c2882a0028d98ad870e752 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11014: 02e29e5d17d8c10b93c527a923d3234cb70ace5e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11014/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Dhinakaran Pandiyan]

On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> For now PSR2 is still disabled by default for all platforms but is
> our intention to let debugfs to enable it for debug and tests
> proporses, so intel_psr2_enabled() that is also used by debugfs to
> decide if PSR2 is going to be enabled needs to take in consideration
> the debug field.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 4c4dd1c310ce..15a2121aa64f 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -71,8 +71,11 @@ static bool psr_global_enabled(u32 debug)
>  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  			       const struct intel_crtc_state
> *crtc_state)
>  {
> +	const u32 debug_mode = dev_priv->psr.debug &
> I915_PSR_DEBUG_MODE_MASK;
> +
>  	/* Disable PSR2 by default for all platforms */
> -	if (i915_modparams.enable_psr == -1)
> +	if (i915_modparams.enable_psr == -1 &&
> +	    debug_mode != I915_PSR_DEBUG_ENABLE)
>  		return false;

@@ -71,17 +71,17 @@ static bool psr_global_enabled(u32 debug)
 static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
                               const struct intel_crtc_state
*crtc_state)
 {
-       /* Disable PSR2 by default for all platforms */
-       if (i915_modparams.enable_psr == -1)
-               return false;
-
        /* Cannot enable DSC and PSR2 simultaneously */
        WARN_ON(crtc_state->dsc_params.compression_enable &&
                crtc_state->has_psr2);
 
        switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
+       case I915_PSR_DEBUG_DISABLE:
        case I915_PSR_DEBUG_FORCE_PSR1:
                return false;
+       case I915_PSR_DEBUG_DEFAULT:
+               if (i915_modparams.enable_psr <= 0)
+                       return false;
        default:
                return crtc_state->has_psr2;
        }

Does this read any better? Keeping the condition checks together and
also having a consistent priority between debugfs and module parameter
options is easier to follow IMHO.

>  
>  	/* Cannot enable DSC and PSR2 simultaneously */

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

Re: [PATCH 1/5] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 2727 bytes --]

On Mon, 2018-12-10 at 19:52 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 15:00 -0800, José Roberto de Souza wrote:
> > For now PSR2 is still disabled by default for all platforms but is
> > our intention to let debugfs to enable it for debug and tests
> > proporses, so intel_psr2_enabled() that is also used by debugfs to
> > decide if PSR2 is going to be enabled needs to take in
> > consideration
> > the debug field.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 4c4dd1c310ce..15a2121aa64f 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -71,8 +71,11 @@ static bool psr_global_enabled(u32 debug)
> >  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
> >  			       const struct intel_crtc_state
> > *crtc_state)
> >  {
> > +	const u32 debug_mode = dev_priv->psr.debug &
> > I915_PSR_DEBUG_MODE_MASK;
> > +
> >  	/* Disable PSR2 by default for all platforms */
> > -	if (i915_modparams.enable_psr == -1)
> > +	if (i915_modparams.enable_psr == -1 &&
> > +	    debug_mode != I915_PSR_DEBUG_ENABLE)
> >  		return false;
> 
> @@ -71,17 +71,17 @@ static bool psr_global_enabled(u32 debug)
>  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>                                const struct intel_crtc_state
> *crtc_state)
>  {
> -       /* Disable PSR2 by default for all platforms */
> -       if (i915_modparams.enable_psr == -1)
> -               return false;
> -
>         /* Cannot enable DSC and PSR2 simultaneously */
>         WARN_ON(crtc_state->dsc_params.compression_enable &&
>                 crtc_state->has_psr2);
>  
>         switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> +       case I915_PSR_DEBUG_DISABLE:
>         case I915_PSR_DEBUG_FORCE_PSR1:
>                 return false;
> +       case I915_PSR_DEBUG_DEFAULT:
> +               if (i915_modparams.enable_psr <= 0)
> +                       return false;
>         default:
>                 return crtc_state->has_psr2;
>         }
> 
> Does this read any better? Keeping the condition checks together and
> also having a consistent priority between debugfs and module
> parameter
> options is easier to follow IMHO.

Yes, looks better. Changing to this.
Thanks

> 
> >  
> >  	/* Cannot enable DSC and PSR2 simultaneously */

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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