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

[PATCH v2 1/6] 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.

v2: Using the switch/case that intel_psr2_enabled() already had to
handle this(DK)

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index dce39f06b682..0ef6c5f8c298 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -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;
 	}
-- 
2.20.0

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

[PATCH v2 2/6] 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 [0x03]
PSR mode: PSR1 enabled
Source PSR ctl: enabled [0x81f00e26]
Source PSR status: IDLE [0x04010006]
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 PSR mode, some renames and reorders and we have this
cleaner version. This will also make easy to parse debugfs for IGT
tests.

v2: Printing sink PSR version with only 2 hex digits as it is a byte

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 | 95 ++++++++++++++---------------
 1 file changed, 47 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 193823048f96..1a31921598e7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2529,7 +2529,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[] = {
@@ -2545,14 +2546,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",
@@ -2564,74 +2562,75 @@ 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));
+	seq_printf(m, " [0x%02x]\n", psr->dp->psr_dpcd[0]);
+	if (!psr->sink_support)
 		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);
-
-	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, "PSR mode: %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.20.0

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

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

[Souza, Jose]

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 193823048f96..13005d5306ba 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2529,7 +2529,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[] = {
@@ -2545,14 +2546,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",
@@ -2564,74 +2562,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->dp)
+		seq_printf(m, " [0x%02x]", psr->dp->psr_dpcd[0]);
+	seq_puts(m, "\n");
+
+	if (!psr->sink_support)
 		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);
-
-	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, "PSR mode: %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;
 }
 

On Thu, 2019-01-03 at 06:21 -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 [0x03]
> PSR mode: PSR1 enabled
> Source PSR ctl: enabled [0x81f00e26]
> Source PSR status: IDLE [0x04010006]
> 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 PSR mode, some renames and reorders and we have this
> cleaner version. This will also make easy to parse debugfs for IGT
> tests.
> 
> v2: Printing sink PSR version with only 2 hex digits as it is a byte
> 
> 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 | 95 ++++++++++++++-------------
> --
>  1 file changed, 47 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 193823048f96..1a31921598e7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2529,7 +2529,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[] = {
> @@ -2545,14 +2546,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",
> @@ -2564,74 +2562,75 @@ 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));
> +	seq_printf(m, " [0x%02x]\n", psr->dp->psr_dpcd[0]);
> +	if (!psr->sink_support)
>  		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);
> -
> -	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, "PSR mode: %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;
>  }
>  

[-- 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 v2 2/6] drm/i915: Refactor PSR status debugfs

[Dhinakaran Pandiyan]

On Thu, 2019-01-03 at 06:21 -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 [0x03]
> PSR mode: PSR1 enabled
> Source PSR ctl: enabled [0x81f00e26]
> Source PSR status: IDLE [0x04010006]
> 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 PSR mode, some renames and reorders and we have this
> cleaner version. This will also make easy to parse debugfs for IGT
> tests.
> 
> v2: Printing sink PSR version with only 2 hex digits as it is a byte
> 
> 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 | 95 ++++++++++++++-------------
> --
>  1 file changed, 47 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 193823048f96..1a31921598e7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2529,7 +2529,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[] = {
> @@ -2545,14 +2546,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",
> @@ -2564,74 +2562,75 @@ 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));
> +	seq_printf(m, " [0x%02x]\n", psr->dp->psr_dpcd[0]);
> +	if (!psr->sink_support)
>  		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);
> -
> -	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, "PSR mode: %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
>  	 */
The counter is probably still useful for debugging along with
i915.enable_dc = 0, allows us to isolate PSR from DMC. That's something
to explore in the future; this patch lgtm
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 

However, I suggest get an ack Rodrigo.


>  	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 v2 2/6] drm/i915: Refactor PSR status debugfs

[Rodrigo Vivi]

On Wed, Jan 09, 2019 at 02:34:36PM -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-01-03 at 06:21 -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 [0x03]
> > PSR mode: PSR1 enabled
> > Source PSR ctl: enabled [0x81f00e26]
> > Source PSR status: IDLE [0x04010006]
> > 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 PSR mode, some renames and reorders and we have this
> > cleaner version. This will also make easy to parse debugfs for IGT
> > tests.
> > 
> > v2: Printing sink PSR version with only 2 hex digits as it is a byte
> > 
> > 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 | 95 ++++++++++++++-------------
> > --
> >  1 file changed, 47 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 193823048f96..1a31921598e7 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2529,7 +2529,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[] = {
> > @@ -2545,14 +2546,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",
> > @@ -2564,74 +2562,75 @@ 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));
> > +	seq_printf(m, " [0x%02x]\n", psr->dp->psr_dpcd[0]);
> > +	if (!psr->sink_support)
> >  		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);
> > -
> > -	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, "PSR mode: %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
> >  	 */
> The counter is probably still useful for debugging along with
> i915.enable_dc = 0, allows us to isolate PSR from DMC. That's something
> to explore in the future; this patch lgtm

yeap... we just need to be careful how to expose it to avoid
the confusion of people thinking PSR is broken only because
counter got reset...

> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> 
> 
> However, I suggest get an ack Rodrigo.

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

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

[PATCH v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[José Roberto de Souza]

intel_psr_set_debugfs_mode() don't just handle the PSR mode but it is
also handling input validation, setting the new debug value and
changing PSR IRQ masks.
Lets move the roles listed above to the caller to make the function
name and what it does accurate.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h    |  2 +-
 drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++----------------
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1a31921598e7..77b097b50fd5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val)
 {
 	struct drm_i915_private *dev_priv = data;
 	struct drm_modeset_acquire_ctx ctx;
-	int ret;
+	const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
+	int ret = 0;
 
 	if (!CAN_PSR(dev_priv))
 		return -ENODEV;
 
 	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
 
+	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
+	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
+		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
+		return -EINVAL;
+	}
+
+	if (!mode)
+		goto skip_mode;
+
 	intel_runtime_pm_get(dev_priv);
 
 	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
 
 retry:
-	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
+	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode);
 	if (ret == -EDEADLK) {
 		ret = drm_modeset_backoff(&ctx);
 		if (!ret)
@@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val)
 
 	intel_runtime_pm_put(dev_priv);
 
+skip_mode:
+	if (!ret) {
+		mutex_lock(&dev_priv->psr.lock);
+		dev_priv->psr.debug = val;
+		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
+		mutex_unlock(&dev_priv->psr.lock);
+	}
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1a11c2beb7f3..2367f07ba29e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2063,7 +2063,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 		      const struct intel_crtc_state *old_crtc_state);
 int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
 			       struct drm_modeset_acquire_ctx *ctx,
-			       u64 value);
+			       u32 mode);
 void intel_psr_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned frontbuffer_bits,
 			  enum fb_op_origin origin);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 0ef6c5f8c298..bba4f7da68b3 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -69,13 +69,14 @@ 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 struct intel_crtc_state *crtc_state,
+			       u32 debug)
 {
 	/* 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) {
+	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
 	case I915_PSR_DEBUG_DISABLE:
 	case I915_PSR_DEBUG_FORCE_PSR1:
 		return false;
@@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		goto unlock;
 	}
 
-	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
+	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state,
+							dev_priv->psr.debug);
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 	dev_priv->psr.prepared = true;
 	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
@@ -944,7 +946,7 @@ static bool switching_psr(struct drm_i915_private *dev_priv,
 
 int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
 			       struct drm_modeset_acquire_ctx *ctx,
-			       u64 val)
+			       u32 mode)
 {
 	struct drm_device *dev = &dev_priv->drm;
 	struct drm_connector_state *conn_state;
@@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
 	struct intel_dp *dp;
 	int ret;
 	bool enable;
-	u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
-
-	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
-	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
-		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
-		return -EINVAL;
-	}
 
 	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
 	if (ret)
@@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
 	if (ret)
 		return ret;
 
-	enable = psr_global_enabled(val);
+	enable = psr_global_enabled(mode);
 
 	if (!enable || switching_psr(dev_priv, crtc_state, mode))
 		intel_psr_disable_locked(dev_priv->psr.dp);
 
-	dev_priv->psr.debug = val;
 	if (crtc)
-		dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
-
-	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
+		dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
+								crtc_state,
+								mode);
 
 	if (dev_priv->psr.prepared && enable)
 		intel_psr_enable_locked(dev_priv, crtc_state);
-- 
2.20.0

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

Re: [PATCH v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[Maarten Lankhorst]

Op 03-01-2019 om 15:21 schreef José Roberto de Souza:
> intel_psr_set_debugfs_mode() don't just handle the PSR mode but it is
> also handling input validation, setting the new debug value and
> changing PSR IRQ masks.
> Lets move the roles listed above to the caller to make the function
> name and what it does accurate.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++----------------
>  3 files changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1a31921598e7..77b097b50fd5 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val)
>  {
>  	struct drm_i915_private *dev_priv = data;
>  	struct drm_modeset_acquire_ctx ctx;
> -	int ret;
> +	const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> +	int ret = 0;
>  
>  	if (!CAN_PSR(dev_priv))
>  		return -ENODEV;
>  
>  	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
>  
> +	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> +	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
> +		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
> +		return -EINVAL;
> +	}

This would only work for (psr.debug & MASK) == (val & MASK).

So you need to take the lock before you can be sure.

While at it, you probably also need the intel_runtime_pm_get() reference.. so you really don't complicate locking much.

I would honestly just grab the extra locks unnecessarily for simplicity. It's only used from debugfs after all.

> +
> +	if (!mode)
> +		goto skip_mode;
> +
>  	intel_runtime_pm_get(dev_priv);
>  
>  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>  
>  retry:
> -	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> +	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode);
>  	if (ret == -EDEADLK) {
>  		ret = drm_modeset_backoff(&ctx);
>  		if (!ret)
> @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val)
>  
>  	intel_runtime_pm_put(dev_priv);
>  
> +skip_mode:
> +	if (!ret) {
> +		mutex_lock(&dev_priv->psr.lock);
> +		dev_priv->psr.debug = val;
> +		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> +		mutex_unlock(&dev_priv->psr.lock);
> +	}
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1a11c2beb7f3..2367f07ba29e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2063,7 +2063,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  		      const struct intel_crtc_state *old_crtc_state);
>  int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  			       struct drm_modeset_acquire_ctx *ctx,
> -			       u64 value);
> +			       u32 mode);
>  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  			  unsigned frontbuffer_bits,
>  			  enum fb_op_origin origin);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 0ef6c5f8c298..bba4f7da68b3 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -69,13 +69,14 @@ 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 struct intel_crtc_state *crtc_state,
> +			       u32 debug)
>  {
>  	/* 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) {
> +	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
>  	case I915_PSR_DEBUG_DISABLE:
>  	case I915_PSR_DEBUG_FORCE_PSR1:
>  		return false;
> @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  		goto unlock;
>  	}
>  
> -	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> +	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state,
> +							dev_priv->psr.debug);
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  	dev_priv->psr.prepared = true;
>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> @@ -944,7 +946,7 @@ static bool switching_psr(struct drm_i915_private *dev_priv,
>  
>  int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  			       struct drm_modeset_acquire_ctx *ctx,
> -			       u64 val)
> +			       u32 mode)
>  {
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct drm_connector_state *conn_state;
> @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  	struct intel_dp *dp;
>  	int ret;
>  	bool enable;
> -	u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> -
> -	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> -	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
> -		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
> -		return -EINVAL;
> -	}
>  
>  	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
>  	if (ret)
> @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  	if (ret)
>  		return ret;
>  
> -	enable = psr_global_enabled(val);
> +	enable = psr_global_enabled(mode);
>  
>  	if (!enable || switching_psr(dev_priv, crtc_state, mode))
>  		intel_psr_disable_locked(dev_priv->psr.dp);
>  
> -	dev_priv->psr.debug = val;
>  	if (crtc)
> -		dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> -
> -	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> +		dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> +								crtc_state,
> +								mode);
>  
>  	if (dev_priv->psr.prepared && enable)
>  		intel_psr_enable_locked(dev_priv, crtc_state);


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

Re: [PATCH v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[Souza, Jose]

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

On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote:
> Op 03-01-2019 om 15:21 schreef José Roberto de Souza:
> > intel_psr_set_debugfs_mode() don't just handle the PSR mode but it
> > is
> > also handling input validation, setting the new debug value and
> > changing PSR IRQ masks.
> > Lets move the roles listed above to the caller to make the function
> > name and what it does accurate.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
> >  drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++---------------
> > -
> >  3 files changed, 31 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 1a31921598e7..77b097b50fd5 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >  {
> >  	struct drm_i915_private *dev_priv = data;
> >  	struct drm_modeset_acquire_ctx ctx;
> > -	int ret;
> > +	const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > +	int ret = 0;
> >  
> >  	if (!CAN_PSR(dev_priv))
> >  		return -ENODEV;
> >  
> >  	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
> >  
> > +	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> > +	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > +		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
> > +		return -EINVAL;
> > +	}
> 
> This would only work for (psr.debug & MASK) == (val & MASK).
> 
> So you need to take the lock before you can be sure.
> 
> While at it, you probably also need the intel_runtime_pm_get()
> reference.. so you really don't complicate locking much.
> 
> I would honestly just grab the extra locks unnecessarily for
> simplicity. It's only used from debugfs after all.

Thanks for the catch.

Something like this?

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 938ad2107ead..3a6ccf815ee1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val)
                return -EINVAL;
        }

-       if (!mode)
-               goto skip_mode;
-
        intel_runtime_pm_get(dev_priv);

+       mutex_lock(&dev_priv->psr.lock);
+       if (mode == (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK))
+               goto skip_mode;
+       mutex_unlock(&dev_priv->psr.lock);
+
        drm_modeset_acquire_init(&ctx,
DRM_MODESET_ACQUIRE_INTERRUPTIBLE);

 retry:
@@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val)
        drm_modeset_drop_locks(&ctx);
        drm_modeset_acquire_fini(&ctx);

-       intel_runtime_pm_put(dev_priv);
-
 skip_mode:
        if (!ret) {
                mutex_lock(&dev_priv->psr.lock);
@@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val)
                mutex_unlock(&dev_priv->psr.lock);
        }

+       intel_runtime_pm_put(dev_priv);
+
        return ret;
 }




> 
> > +
> > +	if (!mode)
> > +		goto skip_mode;
> > +
> >  	intel_runtime_pm_get(dev_priv);
> >  
> >  	drm_modeset_acquire_init(&ctx,
> > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> >  
> >  retry:
> > -	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > +	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode);
> >  	if (ret == -EDEADLK) {
> >  		ret = drm_modeset_backoff(&ctx);
> >  		if (!ret)
> > @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >  
> >  	intel_runtime_pm_put(dev_priv);
> >  
> > +skip_mode:
> > +	if (!ret) {
> > +		mutex_lock(&dev_priv->psr.lock);
> > +		dev_priv->psr.debug = val;
> > +		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > +		mutex_unlock(&dev_priv->psr.lock);
> > +	}
> > +
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 1a11c2beb7f3..2367f07ba29e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2063,7 +2063,7 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  		      const struct intel_crtc_state *old_crtc_state);
> >  int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> >  			       struct drm_modeset_acquire_ctx *ctx,
> > -			       u64 value);
> > +			       u32 mode);
> >  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> >  			  unsigned frontbuffer_bits,
> >  			  enum fb_op_origin origin);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 0ef6c5f8c298..bba4f7da68b3 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -69,13 +69,14 @@ 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 struct intel_crtc_state
> > *crtc_state,
> > +			       u32 debug)
> >  {
> >  	/* 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) {
> > +	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
> >  	case I915_PSR_DEBUG_DISABLE:
> >  	case I915_PSR_DEBUG_FORCE_PSR1:
> >  		return false;
> > @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp
> > *intel_dp,
> >  		goto unlock;
> >  	}
> >  
> > -	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > crtc_state);
> > +	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > crtc_state,
> > +							dev_priv-
> > >psr.debug);
> >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> >  	dev_priv->psr.prepared = true;
> >  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)-
> > >pipe;
> > @@ -944,7 +946,7 @@ static bool switching_psr(struct
> > drm_i915_private *dev_priv,
> >  
> >  int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> >  			       struct drm_modeset_acquire_ctx *ctx,
> > -			       u64 val)
> > +			       u32 mode)
> >  {
> >  	struct drm_device *dev = &dev_priv->drm;
> >  	struct drm_connector_state *conn_state;
> > @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >  	struct intel_dp *dp;
> >  	int ret;
> >  	bool enable;
> > -	u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > -
> > -	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
> > -	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > -		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
> > -		return -EINVAL;
> > -	}
> >  
> >  	ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> > ctx);
> >  	if (ret)
> > @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >  	if (ret)
> >  		return ret;
> >  
> > -	enable = psr_global_enabled(val);
> > +	enable = psr_global_enabled(mode);
> >  
> >  	if (!enable || switching_psr(dev_priv, crtc_state, mode))
> >  		intel_psr_disable_locked(dev_priv->psr.dp);
> >  
> > -	dev_priv->psr.debug = val;
> >  	if (crtc)
> > -		dev_priv->psr.psr2_enabled =
> > intel_psr2_enabled(dev_priv, crtc_state);
> > -
> > -	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > +		dev_priv->psr.psr2_enabled =
> > intel_psr2_enabled(dev_priv,
> > +								crtc_st
> > ate,
> > +								mode);
> >  
> >  	if (dev_priv->psr.prepared && enable)
> >  		intel_psr_enable_locked(dev_priv, crtc_state);
> 
> 

[-- 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 v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[Maarten Lankhorst]

Op 04-01-2019 om 14:28 schreef Souza, Jose:
> On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote:
>> Op 03-01-2019 om 15:21 schreef José Roberto de Souza:
>>> intel_psr_set_debugfs_mode() don't just handle the PSR mode but it
>>> is
>>> also handling input validation, setting the new debug value and
>>> changing PSR IRQ masks.
>>> Lets move the roles listed above to the caller to make the function
>>> name and what it does accurate.
>>>
>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++--
>>>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
>>>  drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++---------------
>>> -
>>>  3 files changed, 31 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 1a31921598e7..77b097b50fd5 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64 val)
>>>  {
>>>  	struct drm_i915_private *dev_priv = data;
>>>  	struct drm_modeset_acquire_ctx ctx;
>>> -	int ret;
>>> +	const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
>>> +	int ret = 0;
>>>  
>>>  	if (!CAN_PSR(dev_priv))
>>>  		return -ENODEV;
>>>  
>>>  	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
>>>  
>>> +	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
>>> +	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
>>> +		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
>>> +		return -EINVAL;
>>> +	}
>> This would only work for (psr.debug & MASK) == (val & MASK).
>>
>> So you need to take the lock before you can be sure.
>>
>> While at it, you probably also need the intel_runtime_pm_get()
>> reference.. so you really don't complicate locking much.
>>
>> I would honestly just grab the extra locks unnecessarily for
>> simplicity. It's only used from debugfs after all.
> Thanks for the catch.
>
> Something like this?
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 938ad2107ead..3a6ccf815ee1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val)
>                 return -EINVAL;
>         }
>
> -       if (!mode)
> -               goto skip_mode;
> -
>         intel_runtime_pm_get(dev_priv);
>
> +       mutex_lock(&dev_priv->psr.lock);
> +       if (mode == (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK))
> +               goto skip_mode;
> +       mutex_unlock(&dev_priv->psr.lock);
> +
>         drm_modeset_acquire_init(&ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>
>  retry:
> @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       intel_runtime_pm_put(dev_priv);
> -
>  skip_mode:
>         if (!ret) {
>                 mutex_lock(&dev_priv->psr.lock);
> @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val)
>                 mutex_unlock(&dev_priv->psr.lock);
>         }
>
> +       intel_runtime_pm_put(dev_priv);
> +
>         return ret;
>  }
>
>
>
>
>>> +
>>> +	if (!mode)
>>> +		goto skip_mode;
>>> +
>>>  	intel_runtime_pm_get(dev_priv);
>>>  
>>>  	drm_modeset_acquire_init(&ctx,
>>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>>>  
>>>  retry:
>>> -	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
>>> +	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode);
>>>  	if (ret == -EDEADLK) {
>>>  		ret = drm_modeset_backoff(&ctx);
>>>  		if (!ret)
>>> @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64 val)
>>>  
>>>  	intel_runtime_pm_put(dev_priv);
>>>  
>>> +skip_mode:
>>> +	if (!ret) {
>>> +		mutex_lock(&dev_priv->psr.lock);
>>> +		dev_priv->psr.debug = val;
>>> +		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
>>> +		mutex_unlock(&dev_priv->psr.lock);
>>> +	}
>>> +
>>>  	return ret;
>>>  }
>>>  
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index 1a11c2beb7f3..2367f07ba29e 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -2063,7 +2063,7 @@ void intel_psr_disable(struct intel_dp
>>> *intel_dp,
>>>  		      const struct intel_crtc_state *old_crtc_state);
>>>  int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>>>  			       struct drm_modeset_acquire_ctx *ctx,
>>> -			       u64 value);
>>> +			       u32 mode);
>>>  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>>>  			  unsigned frontbuffer_bits,
>>>  			  enum fb_op_origin origin);
>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>>> b/drivers/gpu/drm/i915/intel_psr.c
>>> index 0ef6c5f8c298..bba4f7da68b3 100644
>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>> @@ -69,13 +69,14 @@ 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 struct intel_crtc_state
>>> *crtc_state,
>>> +			       u32 debug)
>>>  {
>>>  	/* 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) {
>>> +	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
>>>  	case I915_PSR_DEBUG_DISABLE:
>>>  	case I915_PSR_DEBUG_FORCE_PSR1:
>>>  		return false;
>>> @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp
>>> *intel_dp,
>>>  		goto unlock;
>>>  	}
>>>  
>>> -	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
>>> crtc_state);
>>> +	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
>>> crtc_state,
>>> +							dev_priv-
>>>> psr.debug);
>>>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>>>  	dev_priv->psr.prepared = true;
>>>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)-
>>>> pipe;
>>> @@ -944,7 +946,7 @@ static bool switching_psr(struct
>>> drm_i915_private *dev_priv,
>>>  
>>>  int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>>>  			       struct drm_modeset_acquire_ctx *ctx,
>>> -			       u64 val)
>>> +			       u32 mode)
>>>  {
>>>  	struct drm_device *dev = &dev_priv->drm;
>>>  	struct drm_connector_state *conn_state;
>>> @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct
>>> drm_i915_private *dev_priv,
>>>  	struct intel_dp *dp;
>>>  	int ret;
>>>  	bool enable;
>>> -	u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
>>> -
>>> -	if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) ||
>>> -	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
>>> -		DRM_DEBUG_KMS("Invalid debug mask %llx\n", val);
>>> -		return -EINVAL;
>>> -	}
>>>  
>>>  	ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
>>> ctx);
>>>  	if (ret)
>>> @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct
>>> drm_i915_private *dev_priv,
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> -	enable = psr_global_enabled(val);
>>> +	enable = psr_global_enabled(mode);
>>>  
>>>  	if (!enable || switching_psr(dev_priv, crtc_state, mode))
>>>  		intel_psr_disable_locked(dev_priv->psr.dp);
>>>  
>>> -	dev_priv->psr.debug = val;
>>>  	if (crtc)
>>> -		dev_priv->psr.psr2_enabled =
>>> intel_psr2_enabled(dev_priv, crtc_state);
>>> -
>>> -	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
>>> +		dev_priv->psr.psr2_enabled =
>>> intel_psr2_enabled(dev_priv,
>>> +								crtc_st
>>> ate,
>>> +								mode);
>>>  
>>>  	if (dev_priv->psr.prepared && enable)
>>>  		intel_psr_enable_locked(dev_priv, crtc_state);
>>
Hm I would change the psr irq inside the lock, then jump to pm_put to finish and call that label out. Most race-free way to do so. :)

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

Re: [PATCH v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[Souza, Jose]

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

On Fri, 2019-01-04 at 15:35 +0100, Maarten Lankhorst wrote:
> Op 04-01-2019 om 14:28 schreef Souza, Jose:
> > On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote:
> > > Op 03-01-2019 om 15:21 schreef José Roberto de Souza:
> > > > intel_psr_set_debugfs_mode() don't just handle the PSR mode but
> > > > it
> > > > is
> > > > also handling input validation, setting the new debug value and
> > > > changing PSR IRQ masks.
> > > > Lets move the roles listed above to the caller to make the
> > > > function
> > > > name and what it does accurate.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++-
> > > > -
> > > >  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
> > > >  drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++-----------
> > > > ----
> > > > -
> > > >  3 files changed, 31 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 1a31921598e7..77b097b50fd5 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64
> > > > val)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = data;
> > > >  	struct drm_modeset_acquire_ctx ctx;
> > > > -	int ret;
> > > > +	const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > +	int ret = 0;
> > > >  
> > > >  	if (!CAN_PSR(dev_priv))
> > > >  		return -ENODEV;
> > > >  
> > > >  	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
> > > >  
> > > > +	if (val & ~(I915_PSR_DEBUG_IRQ |
> > > > I915_PSR_DEBUG_MODE_MASK) ||
> > > > +	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > > > +		DRM_DEBUG_KMS("Invalid debug mask %llx\n",
> > > > val);
> > > > +		return -EINVAL;
> > > > +	}
> > > This would only work for (psr.debug & MASK) == (val & MASK).
> > > 
> > > So you need to take the lock before you can be sure.
> > > 
> > > While at it, you probably also need the intel_runtime_pm_get()
> > > reference.. so you really don't complicate locking much.
> > > 
> > > I would honestly just grab the extra locks unnecessarily for
> > > simplicity. It's only used from debugfs after all.
> > Thanks for the catch.
> > 
> > Something like this?
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 938ad2107ead..3a6ccf815ee1 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >                 return -EINVAL;
> >         }
> > 
> > -       if (!mode)
> > -               goto skip_mode;
> > -
> >         intel_runtime_pm_get(dev_priv);
> > 
> > +       mutex_lock(&dev_priv->psr.lock);
> > +       if (mode == (dev_priv->psr.debug &
> > I915_PSR_DEBUG_MODE_MASK))
> > +               goto skip_mode;
> > +       mutex_unlock(&dev_priv->psr.lock);
> > +
> >         drm_modeset_acquire_init(&ctx,
> > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > 
> >  retry:
> > @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >         drm_modeset_drop_locks(&ctx);
> >         drm_modeset_acquire_fini(&ctx);
> > 
> > -       intel_runtime_pm_put(dev_priv);
> > -
> >  skip_mode:
> >         if (!ret) {
> >                 mutex_lock(&dev_priv->psr.lock);
> > @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >                 mutex_unlock(&dev_priv->psr.lock);
> >         }
> > 
> > +       intel_runtime_pm_put(dev_priv);
> > +
> >         return ret;
> >  }
> > 
> > 
> > 
> > 
> > > > +
> > > > +	if (!mode)
> > > > +		goto skip_mode;
> > > > +
> > > >  	intel_runtime_pm_get(dev_priv);
> > > >  
> > > >  	drm_modeset_acquire_init(&ctx,
> > > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > > >  
> > > >  retry:
> > > > -	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
> > > > +	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode);
> > > >  	if (ret == -EDEADLK) {
> > > >  		ret = drm_modeset_backoff(&ctx);
> > > >  		if (!ret)
> > > > @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64
> > > > val)
> > > >  
> > > >  	intel_runtime_pm_put(dev_priv);
> > > >  
> > > > +skip_mode:
> > > > +	if (!ret) {
> > > > +		mutex_lock(&dev_priv->psr.lock);
> > > > +		dev_priv->psr.debug = val;
> > > > +		intel_psr_irq_control(dev_priv, dev_priv-
> > > > >psr.debug);
> > > > +		mutex_unlock(&dev_priv->psr.lock);
> > > > +	}
> > > > +
> > > >  	return ret;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 1a11c2beb7f3..2367f07ba29e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -2063,7 +2063,7 @@ void intel_psr_disable(struct intel_dp
> > > > *intel_dp,
> > > >  		      const struct intel_crtc_state
> > > > *old_crtc_state);
> > > >  int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > *dev_priv,
> > > >  			       struct drm_modeset_acquire_ctx
> > > > *ctx,
> > > > -			       u64 value);
> > > > +			       u32 mode);
> > > >  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> > > >  			  unsigned frontbuffer_bits,
> > > >  			  enum fb_op_origin origin);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 0ef6c5f8c298..bba4f7da68b3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -69,13 +69,14 @@ 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 struct intel_crtc_state
> > > > *crtc_state,
> > > > +			       u32 debug)
> > > >  {
> > > >  	/* 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)
> > > > {
> > > > +	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
> > > >  	case I915_PSR_DEBUG_DISABLE:
> > > >  	case I915_PSR_DEBUG_FORCE_PSR1:
> > > >  		return false;
> > > > @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,
> > > >  		goto unlock;
> > > >  	}
> > > >  
> > > > -	dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv,
> > > > crtc_state);
> > > > +	dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv,
> > > > crtc_state,
> > > > +							dev_pri
> > > > v-
> > > > > psr.debug);
> > > >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> > > >  	dev_priv->psr.prepared = true;
> > > >  	dev_priv->psr.pipe = to_intel_crtc(crtc_state-
> > > > >base.crtc)-
> > > > > pipe;
> > > > @@ -944,7 +946,7 @@ static bool switching_psr(struct
> > > > drm_i915_private *dev_priv,
> > > >  
> > > >  int intel_psr_set_debugfs_mode(struct drm_i915_private
> > > > *dev_priv,
> > > >  			       struct drm_modeset_acquire_ctx
> > > > *ctx,
> > > > -			       u64 val)
> > > > +			       u32 mode)
> > > >  {
> > > >  	struct drm_device *dev = &dev_priv->drm;
> > > >  	struct drm_connector_state *conn_state;
> > > > @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > >  	struct intel_dp *dp;
> > > >  	int ret;
> > > >  	bool enable;
> > > > -	u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
> > > > -
> > > > -	if (val & ~(I915_PSR_DEBUG_IRQ |
> > > > I915_PSR_DEBUG_MODE_MASK) ||
> > > > -	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
> > > > -		DRM_DEBUG_KMS("Invalid debug mask %llx\n",
> > > > val);
> > > > -		return -EINVAL;
> > > > -	}
> > > >  
> > > >  	ret = drm_modeset_lock(&dev-
> > > > >mode_config.connection_mutex,
> > > > ctx);
> > > >  	if (ret)
> > > > @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	enable = psr_global_enabled(val);
> > > > +	enable = psr_global_enabled(mode);
> > > >  
> > > >  	if (!enable || switching_psr(dev_priv, crtc_state,
> > > > mode))
> > > >  		intel_psr_disable_locked(dev_priv->psr.dp);
> > > >  
> > > > -	dev_priv->psr.debug = val;
> > > >  	if (crtc)
> > > > -		dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv, crtc_state);
> > > > -
> > > > -	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > > +		dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv,
> > > > +								
> > > > crtc_st
> > > > ate,
> > > > +								
> > > > mode);
> > > >  
> > > >  	if (dev_priv->psr.prepared && enable)
> > > >  		intel_psr_enable_locked(dev_priv, crtc_state);
> Hm I would change the psr irq inside the lock, then jump to pm_put to
> finish and call that label out. Most race-free way to do so. :)
> 

Well doing that will keep intel_psr_set_debugfs_mode() doing more stuff
than it is supposed to but yeah it is taking the lock 3 times through
i915_edp_psr_debug_set(), I'm not so concerned about race conditions
because the use cases that we have would not cause it.


[-- 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 v2 3/6] drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode

[Maarten Lankhorst]

Op 04-01-2019 om 16:52 schreef Souza, Jose:
> On Fri, 2019-01-04 at 15:35 +0100, Maarten Lankhorst wrote:
>> Op 04-01-2019 om 14:28 schreef Souza, Jose:
>>> On Fri, 2019-01-04 at 07:53 +0100, Maarten Lankhorst wrote:
>>>> Op 03-01-2019 om 15:21 schreef José Roberto de Souza:
>>>>> intel_psr_set_debugfs_mode() don't just handle the PSR mode but
>>>>> it
>>>>> is
>>>>> also handling input validation, setting the new debug value and
>>>>> changing PSR IRQ masks.
>>>>> Lets move the roles listed above to the caller to make the
>>>>> function
>>>>> name and what it does accurate.
>>>>>
>>>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++++++-
>>>>> -
>>>>>  drivers/gpu/drm/i915/intel_drv.h    |  2 +-
>>>>>  drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++-----------
>>>>> ----
>>>>> -
>>>>>  3 files changed, 31 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index 1a31921598e7..77b097b50fd5 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -2639,19 +2639,29 @@ i915_edp_psr_debug_set(void *data, u64
>>>>> val)
>>>>>  {
>>>>>  	struct drm_i915_private *dev_priv = data;
>>>>>  	struct drm_modeset_acquire_ctx ctx;
>>>>> -	int ret;
>>>>> +	const u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
>>>>> +	int ret = 0;
>>>>>  
>>>>>  	if (!CAN_PSR(dev_priv))
>>>>>  		return -ENODEV;
>>>>>  
>>>>>  	DRM_DEBUG_KMS("Setting PSR debug to %llx\n", val);
>>>>>  
>>>>> +	if (val & ~(I915_PSR_DEBUG_IRQ |
>>>>> I915_PSR_DEBUG_MODE_MASK) ||
>>>>> +	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
>>>>> +		DRM_DEBUG_KMS("Invalid debug mask %llx\n",
>>>>> val);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>> This would only work for (psr.debug & MASK) == (val & MASK).
>>>>
>>>> So you need to take the lock before you can be sure.
>>>>
>>>> While at it, you probably also need the intel_runtime_pm_get()
>>>> reference.. so you really don't complicate locking much.
>>>>
>>>> I would honestly just grab the extra locks unnecessarily for
>>>> simplicity. It's only used from debugfs after all.
>>> Thanks for the catch.
>>>
>>> Something like this?
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 938ad2107ead..3a6ccf815ee1 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2656,11 +2656,13 @@ i915_edp_psr_debug_set(void *data, u64 val)
>>>                 return -EINVAL;
>>>         }
>>>
>>> -       if (!mode)
>>> -               goto skip_mode;
>>> -
>>>         intel_runtime_pm_get(dev_priv);
>>>
>>> +       mutex_lock(&dev_priv->psr.lock);
>>> +       if (mode == (dev_priv->psr.debug &
>>> I915_PSR_DEBUG_MODE_MASK))
>>> +               goto skip_mode;
>>> +       mutex_unlock(&dev_priv->psr.lock);
>>> +
>>>         drm_modeset_acquire_init(&ctx,
>>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>>>
>>>  retry:
>>> @@ -2674,8 +2676,6 @@ i915_edp_psr_debug_set(void *data, u64 val)
>>>         drm_modeset_drop_locks(&ctx);
>>>         drm_modeset_acquire_fini(&ctx);
>>>
>>> -       intel_runtime_pm_put(dev_priv);
>>> -
>>>  skip_mode:
>>>         if (!ret) {
>>>                 mutex_lock(&dev_priv->psr.lock);
>>> @@ -2684,6 +2684,8 @@ i915_edp_psr_debug_set(void *data, u64 val)
>>>                 mutex_unlock(&dev_priv->psr.lock);
>>>         }
>>>
>>> +       intel_runtime_pm_put(dev_priv);
>>> +
>>>         return ret;
>>>  }
>>>
>>>
>>>
>>>
>>>>> +
>>>>> +	if (!mode)
>>>>> +		goto skip_mode;
>>>>> +
>>>>>  	intel_runtime_pm_get(dev_priv);
>>>>>  
>>>>>  	drm_modeset_acquire_init(&ctx,
>>>>> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>>>>>  
>>>>>  retry:
>>>>> -	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val);
>>>>> +	ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, mode);
>>>>>  	if (ret == -EDEADLK) {
>>>>>  		ret = drm_modeset_backoff(&ctx);
>>>>>  		if (!ret)
>>>>> @@ -2663,6 +2673,14 @@ i915_edp_psr_debug_set(void *data, u64
>>>>> val)
>>>>>  
>>>>>  	intel_runtime_pm_put(dev_priv);
>>>>>  
>>>>> +skip_mode:
>>>>> +	if (!ret) {
>>>>> +		mutex_lock(&dev_priv->psr.lock);
>>>>> +		dev_priv->psr.debug = val;
>>>>> +		intel_psr_irq_control(dev_priv, dev_priv-
>>>>>> psr.debug);
>>>>> +		mutex_unlock(&dev_priv->psr.lock);
>>>>> +	}
>>>>> +
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index 1a11c2beb7f3..2367f07ba29e 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -2063,7 +2063,7 @@ void intel_psr_disable(struct intel_dp
>>>>> *intel_dp,
>>>>>  		      const struct intel_crtc_state
>>>>> *old_crtc_state);
>>>>>  int intel_psr_set_debugfs_mode(struct drm_i915_private
>>>>> *dev_priv,
>>>>>  			       struct drm_modeset_acquire_ctx
>>>>> *ctx,
>>>>> -			       u64 value);
>>>>> +			       u32 mode);
>>>>>  void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>>>>>  			  unsigned frontbuffer_bits,
>>>>>  			  enum fb_op_origin origin);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c
>>>>> b/drivers/gpu/drm/i915/intel_psr.c
>>>>> index 0ef6c5f8c298..bba4f7da68b3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>>>> @@ -69,13 +69,14 @@ 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 struct intel_crtc_state
>>>>> *crtc_state,
>>>>> +			       u32 debug)
>>>>>  {
>>>>>  	/* 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)
>>>>> {
>>>>> +	switch (debug & I915_PSR_DEBUG_MODE_MASK) {
>>>>>  	case I915_PSR_DEBUG_DISABLE:
>>>>>  	case I915_PSR_DEBUG_FORCE_PSR1:
>>>>>  		return false;
>>>>> @@ -758,7 +759,8 @@ void intel_psr_enable(struct intel_dp
>>>>> *intel_dp,
>>>>>  		goto unlock;
>>>>>  	}
>>>>>  
>>>>> -	dev_priv->psr.psr2_enabled =
>>>>> intel_psr2_enabled(dev_priv,
>>>>> crtc_state);
>>>>> +	dev_priv->psr.psr2_enabled =
>>>>> intel_psr2_enabled(dev_priv,
>>>>> crtc_state,
>>>>> +							dev_pri
>>>>> v-
>>>>>> psr.debug);
>>>>>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>>>>>  	dev_priv->psr.prepared = true;
>>>>>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state-
>>>>>> base.crtc)-
>>>>>> pipe;
>>>>> @@ -944,7 +946,7 @@ static bool switching_psr(struct
>>>>> drm_i915_private *dev_priv,
>>>>>  
>>>>>  int intel_psr_set_debugfs_mode(struct drm_i915_private
>>>>> *dev_priv,
>>>>>  			       struct drm_modeset_acquire_ctx
>>>>> *ctx,
>>>>> -			       u64 val)
>>>>> +			       u32 mode)
>>>>>  {
>>>>>  	struct drm_device *dev = &dev_priv->drm;
>>>>>  	struct drm_connector_state *conn_state;
>>>>> @@ -954,13 +956,6 @@ int intel_psr_set_debugfs_mode(struct
>>>>> drm_i915_private *dev_priv,
>>>>>  	struct intel_dp *dp;
>>>>>  	int ret;
>>>>>  	bool enable;
>>>>> -	u32 mode = val & I915_PSR_DEBUG_MODE_MASK;
>>>>> -
>>>>> -	if (val & ~(I915_PSR_DEBUG_IRQ |
>>>>> I915_PSR_DEBUG_MODE_MASK) ||
>>>>> -	    mode > I915_PSR_DEBUG_FORCE_PSR1) {
>>>>> -		DRM_DEBUG_KMS("Invalid debug mask %llx\n",
>>>>> val);
>>>>> -		return -EINVAL;
>>>>> -	}
>>>>>  
>>>>>  	ret = drm_modeset_lock(&dev-
>>>>>> mode_config.connection_mutex,
>>>>> ctx);
>>>>>  	if (ret)
>>>>> @@ -990,16 +985,15 @@ int intel_psr_set_debugfs_mode(struct
>>>>> drm_i915_private *dev_priv,
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>  
>>>>> -	enable = psr_global_enabled(val);
>>>>> +	enable = psr_global_enabled(mode);
>>>>>  
>>>>>  	if (!enable || switching_psr(dev_priv, crtc_state,
>>>>> mode))
>>>>>  		intel_psr_disable_locked(dev_priv->psr.dp);
>>>>>  
>>>>> -	dev_priv->psr.debug = val;
>>>>>  	if (crtc)
>>>>> -		dev_priv->psr.psr2_enabled =
>>>>> intel_psr2_enabled(dev_priv, crtc_state);
>>>>> -
>>>>> -	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
>>>>> +		dev_priv->psr.psr2_enabled =
>>>>> intel_psr2_enabled(dev_priv,
>>>>> +								
>>>>> crtc_st
>>>>> ate,
>>>>> +								
>>>>> mode);
>>>>>  
>>>>>  	if (dev_priv->psr.prepared && enable)
>>>>>  		intel_psr_enable_locked(dev_priv, crtc_state);
>> Hm I would change the psr irq inside the lock, then jump to pm_put to
>> finish and call that label out. Most race-free way to do so. :)
>>
> Well doing that will keep intel_psr_set_debugfs_mode() doing more stuff
> than it is supposed to but yeah it is taking the lock 3 times through
> i915_edp_psr_debug_set(), I'm not so concerned about race conditions
> because the use cases that we have would not cause it.
>
Meh we should make fastboot default, then we can test a real path..

Create a commit with crtc_state->mode_changed to true..

has_drrs and has_psr(2) can be updated dynamically based on actual values.

the encoder->update_pipe() callback can en/disable psr/drrs as needed.

~Maarten

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

[PATCH v2 4/6] 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 debugfs.
Also adding a comment to intel_psr_irq_handler() about that.

v2: Warning and not letting user set PSR_DEBUG_IRQ when PSR2 is
enabled(Dhinakaran)

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 | 6 +++++-
 drivers/gpu/drm/i915/intel_psr.c    | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 77b097b50fd5..5ebf0e647ac7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2621,7 +2621,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);
@@ -2676,6 +2676,10 @@ i915_edp_psr_debug_set(void *data, u64 val)
 skip_mode:
 	if (!ret) {
 		mutex_lock(&dev_priv->psr.lock);
+		if (dev_priv->psr.psr2_enabled && (val & I915_PSR_DEBUG_IRQ)) {
+			val &= ~I915_PSR_DEBUG_IRQ;
+			DRM_WARN("PSR debug IRQ cannot be enabled with PSR2");
+		}
 		dev_priv->psr.debug = val;
 		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
 		mutex_unlock(&dev_priv->psr.lock);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index bba4f7da68b3..a875546880fa 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -201,6 +201,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.20.0

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

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

[Dhinakaran Pandiyan]

On Thu, 2019-01-03 at 06:21 -0800, José Roberto de Souza wrote:
> PSR2 only trigger interruptions for AUX error, so let's not print
> useless information in debugfs.
> Also adding a comment to intel_psr_irq_handler() about that.
> 
Is it worth keeping this stuff for PSR1 if PSR2 is not supported, did
not work well enough for PSR1 IGTs either. In any case, are these
interrupts present on ICL?


> v2: Warning and not letting user set PSR_DEBUG_IRQ when PSR2 is
> enabled(Dhinakaran)
> 
> 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 | 6 +++++-
>  drivers/gpu/drm/i915/intel_psr.c    | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 77b097b50fd5..5ebf0e647ac7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2621,7 +2621,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) {
Is there a case where PSR2 and IRQ debug are enabled now that you
disallow setting of this value?


>  		seq_printf(m, "Last attempted entry at: %lld\n",
>  			   psr->last_entry_attempt);
>  		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> @@ -2676,6 +2676,10 @@ i915_edp_psr_debug_set(void *data, u64 val)
>  skip_mode:
>  	if (!ret) {
>  		mutex_lock(&dev_priv->psr.lock);
> +		if (dev_priv->psr.psr2_enabled && (val &
> I915_PSR_DEBUG_IRQ)) {
> +			val &= ~I915_PSR_DEBUG_IRQ;
> +			DRM_WARN("PSR debug IRQ cannot be enabled with
> PSR2");

WARN is inconsistent with DEBUG level logging that this function
already uses.

> +		}
>  		dev_priv->psr.debug = val;
>  		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
We are accessing hardware outside of rpm_get()/rpm_put() here.


>  		mutex_unlock(&dev_priv->psr.lock);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index bba4f7da68b3..a875546880fa 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -201,6 +201,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 v2 4/6] 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: 3669 bytes --]

On Wed, 2019-01-09 at 17:24 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-01-03 at 06:21 -0800, José Roberto de Souza wrote:
> > PSR2 only trigger interruptions for AUX error, so let's not print
> > useless information in debugfs.
> > Also adding a comment to intel_psr_irq_handler() about that.
> > 
> Is it worth keeping this stuff for PSR1 if PSR2 is not supported, did
> not work well enough for PSR1 IGTs either. In any case, are these
> interrupts present on ICL?

The PSR1 interrupts are present for ICL.
I guess I only used this once to debug PSR1 do you have used it, it was
useful? If not we should remove it as you suggested.

But anyway as Maarten commented in the previous patch, he suggested us
to follow the approach to test the real path when changing PSR state
from debugfs after enable fastboot. So I will hold this patch and the
previous one.


> 
> 
> > v2: Warning and not letting user set PSR_DEBUG_IRQ when PSR2 is
> > enabled(Dhinakaran)
> > 
> > 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 | 6 +++++-
> >  drivers/gpu/drm/i915/intel_psr.c    | 1 +
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 77b097b50fd5..5ebf0e647ac7 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2621,7 +2621,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) {
> Is there a case where PSR2 and IRQ debug are enabled now that you
> disallow setting of this value?
> 
> 
> >  		seq_printf(m, "Last attempted entry at: %lld\n",
> >  			   psr->last_entry_attempt);
> >  		seq_printf(m, "Last exit at: %lld\n", psr->last_exit);
> > @@ -2676,6 +2676,10 @@ i915_edp_psr_debug_set(void *data, u64 val)
> >  skip_mode:
> >  	if (!ret) {
> >  		mutex_lock(&dev_priv->psr.lock);
> > +		if (dev_priv->psr.psr2_enabled && (val &
> > I915_PSR_DEBUG_IRQ)) {
> > +			val &= ~I915_PSR_DEBUG_IRQ;
> > +			DRM_WARN("PSR debug IRQ cannot be enabled with
> > PSR2");
> 
> WARN is inconsistent with DEBUG level logging that this function
> already uses.

I guess in this case a warn is necessary otherwise if user din't had
increase the drm debug level and tried to enable PSR IRQ interruptions
while PSR2 is actived, this user would not get any error or warning.

> 
> > +		}
> >  		dev_priv->psr.debug = val;
> >  		intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> We are accessing hardware outside of rpm_get()/rpm_put() here.

nice catch.
Thanks

> 
> 
> >  		mutex_unlock(&dev_priv->psr.lock);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index bba4f7da68b3..a875546880fa 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -201,6 +201,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 v2 5/6] 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.

v2: Improved macros(Dhinakaran)

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 44958d994bfa..f9712d05314b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4272,6 +4272,15 @@ enum {
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
+#define _PSR2_SU_STATUS_0		0x6F914
+#define _PSR2_SU_STATUS_1		0x6F918
+#define _PSR2_SU_STATUS_2		0x6F91C
+#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
+#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
+#define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
+#define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
+#define PSR2_SU_STATUS_FRAMES		8
+
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
 #define PCH_ADPA                _MMIO(0xe1100)
-- 
2.20.0

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

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

[Dhinakaran Pandiyan]

On Thu, 2019-01-03 at 06:21 -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 
I suppose you mean 'polling'.

> flip
> can show that the expected values are set for the current frame and
> the
> previous ones too.
The values correspond to the last 8 frames actually.

> 
> v2: Improved macros(Dhinakaran)
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> 
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 44958d994bfa..f9712d05314b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4272,6 +4272,15 @@ enum {
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> +#define _PSR2_SU_STATUS_0		0x6F914
> +#define _PSR2_SU_STATUS_1		0x6F918
> +#define _PSR2_SU_STATUS_2		0x6F91C
> +#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
> ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> +#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
> ) / 3))
> +#define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
> +#define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
> PSR2_SU_STATUS_SHIFT(frame))
> +#define PSR2_SU_STATUS_FRAMES		8
> +
>  /* 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 v2 5/6] drm/i915: Add PSR2 selective update status registers and bits definitions

[Souza, Jose]

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

On Wed, 2019-01-09 at 18:18 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-01-03 at 06:21 -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 
> I suppose you mean 'polling'.

Exacly that, thanks for catching this up.

> 
> > flip
> > can show that the expected values are set for the current frame and
> > the
> > previous ones too.
> The values correspond to the last 8 frames actually.

changed

> 
> > v2: Improved macros(Dhinakaran)
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


Thanks

> 
> > 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 | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 44958d994bfa..f9712d05314b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4272,6 +4272,15 @@ enum {
> >  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> >  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> >  
> > +#define _PSR2_SU_STATUS_0		0x6F914
> > +#define _PSR2_SU_STATUS_1		0x6F918
> > +#define _PSR2_SU_STATUS_2		0x6F91C
> > +#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
> > ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > +#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
> > ) / 3))
> > +#define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
> > +#define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
> > PSR2_SU_STATUS_SHIFT(frame))
> > +#define PSR2_SU_STATUS_FRAMES		8
> > +
> >  /* 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 v2 6/6] 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.

v2:
- Using new macros
- Changed the string output

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 | 32 +++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5ebf0e647ac7..4e92078bc65d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2621,10 +2621,34 @@ 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 frame;
+
+		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
+
+		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame++) {
+			u32 su_blocks;
+
+			/*
+			 * Avoid register reads as each register contains more
+			 * than one frame value
+			 */
+			if ((frame % 3) == 0)
+				val = I915_READ(PSR2_SU_STATUS(frame));
+
+			su_blocks = val & PSR2_SU_STATUS_MASK(frame);
+			su_blocks = su_blocks >> PSR2_SU_STATUS_SHIFT(frame);
+			/* Only printing frames with SU blocks */
+			if (!su_blocks)
+				continue;
+			seq_printf(m, "%d\t%d\n", frame, su_blocks);
+		}
 	}
 
 unlock:
-- 
2.20.0

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

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

[Dhinakaran Pandiyan]

On Thu, 2019-01-03 at 06:21 -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.
> 
> v2:
> - Using new macros
> - Changed the string output
> 
> 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 | 32 +++++++++++++++++++++++++
> ----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5ebf0e647ac7..4e92078bc65d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2621,10 +2621,34 @@ 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 frame;
> +
> +		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
> +
> +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame++)
> {
> +			u32 su_blocks;
> +
> +			/*
> +			 * Avoid register reads as each register
> contains more
> +			 * than one frame value
> +			 */
I don't think the comment is needed, but if you want to retain it I
suggest explicitly saying each register contains data for three frames?

Not sure if it matters, how about completing all the three register
reads before printing so that we reduce the chances of crossing a frame
boundary between register reads?

> +			if ((frame % 3) == 0)
> +				val = I915_READ(PSR2_SU_STATUS(frame));
> +
> +			su_blocks = val & PSR2_SU_STATUS_MASK(frame);
> +			su_blocks = su_blocks >>
> PSR2_SU_STATUS_SHIFT(frame);
> +			/* Only printing frames with SU blocks */
> +			if (!su_blocks)
> +				continue;
Why not print zero if that's the number of blocks updated in a frame?

> +			seq_printf(m, "%d\t%d\n", frame, su_blocks);
> +		}
>  	}
>  
>  unlock:

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

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

[Souza, Jose]

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

On Wed, 2019-01-09 at 18:11 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-01-03 at 06:21 -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.
> > 
> > v2:
> > - Using new macros
> > - Changed the string output
> > 
> > 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 | 32 +++++++++++++++++++++++++
> > ----
> >  1 file changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5ebf0e647ac7..4e92078bc65d 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2621,10 +2621,34 @@ 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 frame;
> > +
> > +		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
> > +
> > +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame++)
> > {
> > +			u32 su_blocks;
> > +
> > +			/*
> > +			 * Avoid register reads as each register
> > contains more
> > +			 * than one frame value
> > +			 */
> I don't think the comment is needed, but if you want to retain it I
> suggest explicitly saying each register contains data for three
> frames?

The last one have only 2 frames.

> 
> Not sure if it matters, how about completing all the three register
> reads before printing so that we reduce the chances of crossing a
> frame
> boundary between register reads?

Hum sounds good, I will do that.

> 
> > +			if ((frame % 3) == 0)
> > +				val = I915_READ(PSR2_SU_STATUS(frame));
> > +
> > +			su_blocks = val & PSR2_SU_STATUS_MASK(frame);
> > +			su_blocks = su_blocks >>
> > PSR2_SU_STATUS_SHIFT(frame);
> > +			/* Only printing frames with SU blocks */
> > +			if (!su_blocks)
> > +				continue;
> Why not print zero if that's the number of blocks updated in a frame?

I think 8 frames with value zero are not worth to print everytime and
IGT will only care about the frame 0 but I don't see any problem in
printing it.


> 
> > +			seq_printf(m, "%d\t%d\n", frame, su_blocks);
> > +		}
> >  	}
> >  
> >  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 [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Patchwork]

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
218d4fcb7953 drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
10edb2d95dc6 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, 142 lines checked
4ad5fd0a930e drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode
3a8341314414 drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2
907aac6f75be drm/i915: Add PSR2 selective update status registers and bits definitions
-:33: WARNING:LONG_LINE: line over 100 characters
#33: FILE: drivers/gpu/drm/i915/i915_reg.h:4278:
+#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))

total: 0 errors, 1 warnings, 0 checks, 15 lines checked
9914c810a78e 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: failure for series starting with [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
URL   : https://patchwork.freedesktop.org/series/54692/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5361 -> Patchwork_11181
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_11181 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11181, 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/54692/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@debugfs_test@read_all_entries:
    - fi-hsw-peppy:       PASS -> INCOMPLETE
    - fi-kbl-7500u:       PASS -> INCOMPLETE
    - fi-cfl-8109u:       PASS -> INCOMPLETE
    - fi-hsw-4770r:       PASS -> INCOMPLETE
    - fi-kbl-guc:         PASS -> INCOMPLETE
    - fi-kbl-x1275:       PASS -> INCOMPLETE
    - fi-bdw-5557u:       PASS -> INCOMPLETE
    - fi-kbl-7567u:       PASS -> INCOMPLETE
    - fi-kbl-8809g:       PASS -> INCOMPLETE
    - fi-cfl-8700k:       PASS -> INCOMPLETE
    - fi-hsw-4770:        PASS -> INCOMPLETE
    - fi-cfl-guc:         PASS -> INCOMPLETE

  
#### Warnings ####

  * igt@kms_psr@cursor_plane_move:
    - fi-kbl-r:           PASS -> SKIP +3
    - fi-whl-u:           PASS -> SKIP +3
    - fi-icl-u2:          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

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@runner@aborted}:
    - fi-bdw-gvtdvm:      NOTRUN -> FAIL
    - fi-cfl-8109u:       NOTRUN -> FAIL
    - fi-hsw-peppy:       NOTRUN -> FAIL
    - fi-glk-dsi:         NOTRUN -> FAIL
    - fi-hsw-4770:        NOTRUN -> FAIL
    - fi-kbl-7500u:       NOTRUN -> FAIL
    - fi-bxt-j4205:       NOTRUN -> FAIL
    - fi-bxt-dsi:         NOTRUN -> FAIL
    - fi-cfl-guc:         NOTRUN -> FAIL
    - fi-glk-j4005:       NOTRUN -> FAIL
    - fi-kbl-7567u:       NOTRUN -> FAIL
    - fi-kbl-x1275:       NOTRUN -> FAIL
    - fi-cfl-8700k:       NOTRUN -> FAIL
    - fi-hsw-4770r:       NOTRUN -> FAIL
    - fi-kbl-8809g:       NOTRUN -> FAIL
    - fi-apl-guc:         NOTRUN -> FAIL
    - fi-bdw-5557u:       NOTRUN -> FAIL
    - fi-kbl-guc:         NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@read_all_entries:
    - fi-skl-iommu:       PASS -> INCOMPLETE [fdo#108901]
    - fi-glk-dsi:         PASS -> INCOMPLETE [fdo#103359] / [k.org#198133]
    - fi-bdw-gvtdvm:      PASS -> INCOMPLETE [fdo#105600]
    - fi-bxt-j4205:       PASS -> INCOMPLETE [fdo#103927]
    - fi-skl-6770hq:      PASS -> INCOMPLETE [fdo#108901]
    - fi-skl-6260u:       PASS -> INCOMPLETE [fdo#108901]
    - fi-skl-gvtdvm:      PASS -> INCOMPLETE [fdo#105600] / [fdo#108901]
    - fi-skl-guc:         PASS -> INCOMPLETE [fdo#108901]
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927]
    - fi-glk-j4005:       PASS -> INCOMPLETE [fdo#103359] / [k.org#198133]
    - fi-bxt-dsi:         PASS -> INCOMPLETE [fdo#103927]
    - fi-skl-6700k2:      PASS -> INCOMPLETE [fdo#108901]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

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

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

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

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105600]: https://bugs.freedesktop.org/show_bug.cgi?id=105600
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108901]: https://bugs.freedesktop.org/show_bug.cgi?id=108901
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (52 -> 46)
------------------------------

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


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

    * Linux: CI_DRM_5361 -> Patchwork_11181

  CI_DRM_5361: 076417527f6abadb519f6d5e7cec575973888607 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4756: 75081c6bfb9998bd7cbf35a7ac0578c683fe55a8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11181: 9914c810a78e6ffc2e3ef2d9d775f23d6e4fc99b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

9914c810a78e drm/i915/debugfs: Print PSR selective update status register values
907aac6f75be drm/i915: Add PSR2 selective update status registers and bits definitions
3a8341314414 drm/i915/psr: Do not print last attempted entry or exit in PSR debugfs while in PSR2
4ad5fd0a930e drm/i915/psr: Make intel_psr_set_debugfs_mode() only handle PSR mode
10edb2d95dc6 drm/i915: Refactor PSR status debugfs
218d4fcb7953 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_11181/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks (rev2)
URL   : https://patchwork.freedesktop.org/series/54692/
State : failure

== Summary ==

Applying: drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
Applying: drm/i915: Refactor PSR status debugfs
error: corrupt patch at line 10
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0002 drm/i915: Refactor PSR status debugfs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✗ Fi.CI.BAT: failure for series starting with [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks (rev3)
URL   : https://patchwork.freedesktop.org/series/54692/
State : failure

== Summary ==

Applying: drm/i915/psr: Allow PSR2 to be enabled when debugfs asks
Applying: drm/i915: Refactor PSR status debugfs
error: corrupt patch at line 10
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0002 drm/i915: Refactor PSR status debugfs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

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

[Dhinakaran Pandiyan]

On Thu, 2019-01-03 at 06:21 -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.
> 
> v2: Using the switch/case that intel_psr2_enabled() already had to
> handle this(DK)

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> 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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index dce39f06b682..0ef6c5f8c298 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -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;
>  	}

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