[Intel-gfx] [PATCH] drm/i915/display: Add a debugfs entry for fifo underruns

[Intel-gfx] [PATCH] drm/i915/display: Add a debugfs entry for fifo underruns

[Swati Sharma]

From: Mohammed Khajapasha <mohammed.khajapasha@intel.com>

Add a debugfs entry i915_fifo_underruns to indicate the count of
fifo underruns for each pipe.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
---
 .../drm/i915/display/intel_display_debugfs.c  | 28 ++++++++++++++++++
 .../drm/i915/display/intel_display_types.h    |  2 ++
 .../drm/i915/display/intel_fifo_underrun.c    | 29 +++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 9e2fb8626c96..d64b4675766c 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -9,6 +9,7 @@
 #include <drm/drm_fourcc.h>
 
 #include "i915_debugfs.h"
+#include "intel_crtc.h"
 #include "i915_irq.h"
 #include "i915_reg.h"
 #include "intel_de.h"
@@ -1057,6 +1058,32 @@ static int i915_lpsp_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_fifo_underruns(struct seq_file *m, void *unused)
+{
+	struct drm_i915_private *dev_priv = node_to_i915(m->private);
+	struct intel_crtc *crtc;
+	enum pipe pipe;
+	unsigned long flags;
+	u32 cpu_fifo_underrun_count[I915_MAX_PIPES];
+	u32 pch_fifo_underrun_count[I915_MAX_PIPES];
+
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+	for_each_pipe(dev_priv, pipe) {
+		crtc = intel_crtc_for_pipe(dev_priv, pipe);
+		cpu_fifo_underrun_count[pipe] = crtc->cpu_fifo_underrun_count;
+		pch_fifo_underrun_count[pipe] = crtc->pch_fifo_underrun_count;
+	}
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+
+	seq_printf(m, "\t%-10s \t%10s\n", "cpu fifounderruns", "pch fifounderruns");
+	for_each_pipe(dev_priv, pipe)
+		seq_printf(m, "pipe:%c %10u %20u\n", pipe_name(pipe),
+			   cpu_fifo_underrun_count[pipe],
+			   pch_fifo_underrun_count[pipe]);
+
+	return 0;
+}
+
 static int i915_dp_mst_info(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -1586,6 +1613,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = {
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
 	{"i915_ddb_info", i915_ddb_info, 0},
 	{"i915_lpsp_status", i915_lpsp_status, 0},
+	{"i915_fifo_underruns", i915_fifo_underruns, 0},
 };
 
 static const struct {
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 9ccae7a46020..b0468ac70059 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1438,6 +1438,8 @@ struct intel_crtc {
 
 #ifdef CONFIG_DEBUG_FS
 	struct intel_pipe_crc pipe_crc;
+	u32 cpu_fifo_underrun_count;
+	u32 pch_fifo_underrun_count;
 #endif
 };
 
diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
index d636d21fa9ce..7131dd400ac3 100644
--- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
@@ -88,6 +88,17 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
 	return true;
 }
 
+static void intel_fifo_underrun_inc_count(struct intel_crtc *crtc,
+					  bool is_cpu_fifo)
+{
+#ifdef CONFIG_DEBUG_FS
+	if (is_cpu_fifo)
+		crtc->cpu_fifo_underrun_count++;
+	else
+		crtc->pch_fifo_underrun_count++;
+#endif
+}
+
 static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -103,6 +114,7 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
 	intel_de_write(dev_priv, reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
 	intel_de_posting_read(dev_priv, reg);
 
+	intel_fifo_underrun_inc_count(crtc, true);
 	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
 	drm_err(&dev_priv->drm, "pipe %c underrun\n", pipe_name(crtc->pipe));
 }
@@ -156,6 +168,7 @@ static void ivb_check_fifo_underruns(struct intel_crtc *crtc)
 	intel_de_write(dev_priv, GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
 	intel_de_posting_read(dev_priv, GEN7_ERR_INT);
 
+	intel_fifo_underrun_inc_count(crtc, true);
 	trace_intel_cpu_fifo_underrun(dev_priv, pipe);
 	drm_err(&dev_priv->drm, "fifo underrun on pipe %c\n", pipe_name(pipe));
 }
@@ -244,6 +257,7 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
 		       SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
 	intel_de_posting_read(dev_priv, SERR_INT);
 
+	intel_fifo_underrun_inc_count(crtc, false);
 	trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);
 	drm_err(&dev_priv->drm, "pch fifo underrun on pch transcoder %c\n",
 		pipe_name(pch_transcoder));
@@ -286,6 +300,11 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 
 	old = !crtc->cpu_fifo_underrun_disabled;
 	crtc->cpu_fifo_underrun_disabled = !enable;
+#ifdef CONFIG_DEBUG_FS
+	/* don't reset count in fifo underrun irq path */
+	if (!in_irq() && !enable)
+		crtc->cpu_fifo_underrun_count = 0;
+#endif
 
 	if (HAS_GMCH(dev_priv))
 		i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
@@ -365,6 +384,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
 
 	old = !crtc->pch_fifo_underrun_disabled;
 	crtc->pch_fifo_underrun_disabled = !enable;
+#ifdef CONFIG_DEBUG_FS
+	/* don't reset count in fifo underrun irq path */
+	if (!in_irq() && !enable)
+		crtc->pch_fifo_underrun_count = 0;
+#endif
 
 	if (HAS_PCH_IBX(dev_priv))
 		ibx_set_fifo_underrun_reporting(&dev_priv->drm,
@@ -434,6 +458,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 			drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
 	}
 
+	intel_fifo_underrun_inc_count(crtc, true);
 	intel_fbc_handle_fifo_underrun_irq(dev_priv);
 }
 
@@ -449,6 +474,10 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum pipe pch_transcoder)
 {
+	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pch_transcoder);
+
+	intel_fifo_underrun_inc_count(crtc, false);
+
 	if (intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder,
 						  false)) {
 		trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);
-- 
2.25.1

Re: [Intel-gfx] [PATCH] drm/i915/display: Add a debugfs entry for fifo underruns

[Andi Shyti]

Hi Swati,

[...]

> +static void intel_fifo_underrun_inc_count(struct intel_crtc *crtc,
> +					  bool is_cpu_fifo)

I'm not a big fan of the true/false parameters in functions. I
actually hate them because it's never clear from the caller what
the true/false means.

Isn't it clear to just have some wrappers

#define intel_fifo_underrun_inc_cpu_count(...)
#define intel_fifo_underrun_inc_cph_count(...)

or similar?

> +{
> +#ifdef CONFIG_DEBUG_FS
> +	if (is_cpu_fifo)
> +		crtc->cpu_fifo_underrun_count++;
> +	else
> +		crtc->pch_fifo_underrun_count++;
> +#endif
> +}
> +
>  static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -103,6 +114,7 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>  	intel_de_write(dev_priv, reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
>  	intel_de_posting_read(dev_priv, reg);
>  
> +	intel_fifo_underrun_inc_count(crtc, true);
>  	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
>  	drm_err(&dev_priv->drm, "pipe %c underrun\n", pipe_name(crtc->pipe));
>  }
> @@ -156,6 +168,7 @@ static void ivb_check_fifo_underruns(struct intel_crtc *crtc)
>  	intel_de_write(dev_priv, GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
>  	intel_de_posting_read(dev_priv, GEN7_ERR_INT);
>  
> +	intel_fifo_underrun_inc_count(crtc, true);
>  	trace_intel_cpu_fifo_underrun(dev_priv, pipe);
>  	drm_err(&dev_priv->drm, "fifo underrun on pipe %c\n", pipe_name(pipe));
>  }
> @@ -244,6 +257,7 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
>  		       SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
>  	intel_de_posting_read(dev_priv, SERR_INT);
>  
> +	intel_fifo_underrun_inc_count(crtc, false);
>  	trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);
>  	drm_err(&dev_priv->drm, "pch fifo underrun on pch transcoder %c\n",
>  		pipe_name(pch_transcoder));
> @@ -286,6 +300,11 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>  
>  	old = !crtc->cpu_fifo_underrun_disabled;
>  	crtc->cpu_fifo_underrun_disabled = !enable;
> +#ifdef CONFIG_DEBUG_FS
> +	/* don't reset count in fifo underrun irq path */
> +	if (!in_irq() && !enable)
> +		crtc->cpu_fifo_underrun_count = 0;
> +#endif
>  
>  	if (HAS_GMCH(dev_priv))
>  		i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
> @@ -365,6 +384,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
>  
>  	old = !crtc->pch_fifo_underrun_disabled;
>  	crtc->pch_fifo_underrun_disabled = !enable;
> +#ifdef CONFIG_DEBUG_FS
> +	/* don't reset count in fifo underrun irq path */
> +	if (!in_irq() && !enable)
> +		crtc->pch_fifo_underrun_count = 0;
> +#endif

All these ifdefs are a bit ugly. Can we put all these stuff
inside the debugfs.c file that is compiled only if DEBUG_FS is
configured?

Andi

>  
>  	if (HAS_PCH_IBX(dev_priv))
>  		ibx_set_fifo_underrun_reporting(&dev_priv->drm,
> @@ -434,6 +458,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  			drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
>  	}
>  
> +	intel_fifo_underrun_inc_count(crtc, true);
>  	intel_fbc_handle_fifo_underrun_irq(dev_priv);
>  }

Re: [Intel-gfx] [PATCH] drm/i915/display: Add a debugfs entry for fifo underruns

[Jani Nikula]

On Wed, 08 Feb 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote:
> Hi Swati,
>
> [...]
>
>> +static void intel_fifo_underrun_inc_count(struct intel_crtc *crtc,
>> +					  bool is_cpu_fifo)
>
> I'm not a big fan of the true/false parameters in functions. I
> actually hate them because it's never clear from the caller what
> the true/false means.
>
> Isn't it clear to just have some wrappers
>
> #define intel_fifo_underrun_inc_cpu_count(...)
> #define intel_fifo_underrun_inc_cph_count(...)
>
> or similar?
>
>> +{
>> +#ifdef CONFIG_DEBUG_FS
>> +	if (is_cpu_fifo)
>> +		crtc->cpu_fifo_underrun_count++;
>> +	else
>> +		crtc->pch_fifo_underrun_count++;
>> +#endif
>> +}
>> +
>>  static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> @@ -103,6 +114,7 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>>  	intel_de_write(dev_priv, reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
>>  	intel_de_posting_read(dev_priv, reg);
>>  
>> +	intel_fifo_underrun_inc_count(crtc, true);
>>  	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
>>  	drm_err(&dev_priv->drm, "pipe %c underrun\n", pipe_name(crtc->pipe));
>>  }
>> @@ -156,6 +168,7 @@ static void ivb_check_fifo_underruns(struct intel_crtc *crtc)
>>  	intel_de_write(dev_priv, GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
>>  	intel_de_posting_read(dev_priv, GEN7_ERR_INT);
>>  
>> +	intel_fifo_underrun_inc_count(crtc, true);
>>  	trace_intel_cpu_fifo_underrun(dev_priv, pipe);
>>  	drm_err(&dev_priv->drm, "fifo underrun on pipe %c\n", pipe_name(pipe));
>>  }
>> @@ -244,6 +257,7 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
>>  		       SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
>>  	intel_de_posting_read(dev_priv, SERR_INT);
>>  
>> +	intel_fifo_underrun_inc_count(crtc, false);
>>  	trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);
>>  	drm_err(&dev_priv->drm, "pch fifo underrun on pch transcoder %c\n",
>>  		pipe_name(pch_transcoder));
>> @@ -286,6 +300,11 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>>  
>>  	old = !crtc->cpu_fifo_underrun_disabled;
>>  	crtc->cpu_fifo_underrun_disabled = !enable;
>> +#ifdef CONFIG_DEBUG_FS
>> +	/* don't reset count in fifo underrun irq path */
>> +	if (!in_irq() && !enable)
>> +		crtc->cpu_fifo_underrun_count = 0;
>> +#endif
>>  
>>  	if (HAS_GMCH(dev_priv))
>>  		i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
>> @@ -365,6 +384,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
>>  
>>  	old = !crtc->pch_fifo_underrun_disabled;
>>  	crtc->pch_fifo_underrun_disabled = !enable;
>> +#ifdef CONFIG_DEBUG_FS
>> +	/* don't reset count in fifo underrun irq path */
>> +	if (!in_irq() && !enable)
>> +		crtc->pch_fifo_underrun_count = 0;
>> +#endif
>
> All these ifdefs are a bit ugly. Can we put all these stuff
> inside the debugfs.c file that is compiled only if DEBUG_FS is
> configured?

The opposite, the debugfs should be added in this file instead. :)

See my reply.

BR,
Jani.




>
> Andi
>
>>  
>>  	if (HAS_PCH_IBX(dev_priv))
>>  		ibx_set_fifo_underrun_reporting(&dev_priv->drm,
>> @@ -434,6 +458,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>>  			drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
>>  	}
>>  
>> +	intel_fifo_underrun_inc_count(crtc, true);
>>  	intel_fbc_handle_fifo_underrun_irq(dev_priv);
>>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915/display: Add a debugfs entry for fifo underruns

[Swati Sharma]

Thanks Andi and Jani N for the review comments.
Sorry for the delay.
Will send the next rev soon.

On 14-Feb-23 5:55 PM, Jani Nikula wrote:
> On Wed, 08 Feb 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote:
>> Hi Swati,
>>
>> [...]
>>
>>> +static void intel_fifo_underrun_inc_count(struct intel_crtc *crtc,
>>> +					  bool is_cpu_fifo)
>>
>> I'm not a big fan of the true/false parameters in functions. I
>> actually hate them because it's never clear from the caller what
>> the true/false means.
>>
>> Isn't it clear to just have some wrappers
>>
>> #define intel_fifo_underrun_inc_cpu_count(...)
>> #define intel_fifo_underrun_inc_cph_count(...)
>>
>> or similar?
>>
>>> +{
>>> +#ifdef CONFIG_DEBUG_FS
>>> +	if (is_cpu_fifo)
>>> +		crtc->cpu_fifo_underrun_count++;
>>> +	else
>>> +		crtc->pch_fifo_underrun_count++;
>>> +#endif
>>> +}
>>> +
>>>   static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>>>   {
>>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>> @@ -103,6 +114,7 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>>>   	intel_de_write(dev_priv, reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
>>>   	intel_de_posting_read(dev_priv, reg);
>>>   
>>> +	intel_fifo_underrun_inc_count(crtc, true);
>>>   	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
>>>   	drm_err(&dev_priv->drm, "pipe %c underrun\n", pipe_name(crtc->pipe));
>>>   }
>>> @@ -156,6 +168,7 @@ static void ivb_check_fifo_underruns(struct intel_crtc *crtc)
>>>   	intel_de_write(dev_priv, GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
>>>   	intel_de_posting_read(dev_priv, GEN7_ERR_INT);
>>>   
>>> +	intel_fifo_underrun_inc_count(crtc, true);
>>>   	trace_intel_cpu_fifo_underrun(dev_priv, pipe);
>>>   	drm_err(&dev_priv->drm, "fifo underrun on pipe %c\n", pipe_name(pipe));
>>>   }
>>> @@ -244,6 +257,7 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
>>>   		       SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
>>>   	intel_de_posting_read(dev_priv, SERR_INT);
>>>   
>>> +	intel_fifo_underrun_inc_count(crtc, false);
>>>   	trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);
>>>   	drm_err(&dev_priv->drm, "pch fifo underrun on pch transcoder %c\n",
>>>   		pipe_name(pch_transcoder));
>>> @@ -286,6 +300,11 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>>>   
>>>   	old = !crtc->cpu_fifo_underrun_disabled;
>>>   	crtc->cpu_fifo_underrun_disabled = !enable;
>>> +#ifdef CONFIG_DEBUG_FS
>>> +	/* don't reset count in fifo underrun irq path */
>>> +	if (!in_irq() && !enable)
>>> +		crtc->cpu_fifo_underrun_count = 0;
>>> +#endif
>>>   
>>>   	if (HAS_GMCH(dev_priv))
>>>   		i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
>>> @@ -365,6 +384,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
>>>   
>>>   	old = !crtc->pch_fifo_underrun_disabled;
>>>   	crtc->pch_fifo_underrun_disabled = !enable;
>>> +#ifdef CONFIG_DEBUG_FS
>>> +	/* don't reset count in fifo underrun irq path */
>>> +	if (!in_irq() && !enable)
>>> +		crtc->pch_fifo_underrun_count = 0;
>>> +#endif
>>
>> All these ifdefs are a bit ugly. Can we put all these stuff
>> inside the debugfs.c file that is compiled only if DEBUG_FS is
>> configured?
> 
> The opposite, the debugfs should be added in this file instead. :)
> 
> See my reply.
> 
> BR,
> Jani.
> 
> 
> 
> 
>>
>> Andi
>>
>>>   
>>>   	if (HAS_PCH_IBX(dev_priv))
>>>   		ibx_set_fifo_underrun_reporting(&dev_priv->drm,
>>> @@ -434,6 +458,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>>>   			drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
>>>   	}
>>>   
>>> +	intel_fifo_underrun_inc_count(crtc, true);
>>>   	intel_fbc_handle_fifo_underrun_irq(dev_priv);
>>>   }
> 

-- 
~Swati Sharma

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/display: Add a debugfs entry for fifo underruns

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

== Series Details ==

Series: drm/i915/display: Add a debugfs entry for fifo underruns
URL   : https://patchwork.freedesktop.org/series/113781/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12713 -> Patchwork_113781v1
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/index.html

Participating hosts (37 -> 36)
------------------------------

  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@reload:
    - fi-cfl-8700k:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12713/fi-cfl-8700k/igt@i915_module_load@reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/fi-cfl-8700k/igt@i915_module_load@reload.html

  
New tests
---------

  New tests have been introduced between CI_DRM_12713 and Patchwork_113781v1:

### New IGT tests (1) ###

  * igt@i915_selftest@live:
    - Statuses :
    - Exec time: [None] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-n3050:       [PASS][3] -> [FAIL][4] ([i915#6298])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12713/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - {bat-adlm-1}:       [ABORT][5] -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12713/bat-adlm-1/igt@i915_selftest@live@hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/bat-adlm-1/igt@i915_selftest@live@hangcheck.html
    - fi-skl-guc:         [DMESG-WARN][7] -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12713/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/fi-skl-guc/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@requests:
    - {bat-rpls-1}:       [ABORT][9] ([i915#7911]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12713/bat-rpls-1/igt@i915_selftest@live@requests.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/bat-rpls-1/igt@i915_selftest@live@requests.html

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

  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911


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

  * Linux: CI_DRM_12713 -> Patchwork_113781v1

  CI-20190529: 20190529
  CI_DRM_12713: 5180055794b438ce688a6804afb0af08e626ebab @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7153: f47f859f13376958a2bd199423b1f0ff53dddbe0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113781v1: 5180055794b438ce688a6804afb0af08e626ebab @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

44fc4892c967 drm/i915/display: Add a debugfs entry for fifo underruns

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113781v1/index.html

[-- Attachment #2: Type: text/html, Size: 4761 bytes --]

Re: [Intel-gfx] [PATCH] drm/i915/display: Add a debugfs entry for fifo underruns

[Jani Nikula]

On Wed, 08 Feb 2023, Swati Sharma <swati2.sharma@intel.com> wrote:
> From: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
>
> Add a debugfs entry i915_fifo_underruns to indicate the count of
> fifo underruns for each pipe.
>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Mohammed Khajapasha <mohammed.khajapasha@intel.com>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>  .../drm/i915/display/intel_display_debugfs.c  | 28 ++++++++++++++++++
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  .../drm/i915/display/intel_fifo_underrun.c    | 29 +++++++++++++++++++
>  3 files changed, 59 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 9e2fb8626c96..d64b4675766c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -9,6 +9,7 @@
>  #include <drm/drm_fourcc.h>
>  
>  #include "i915_debugfs.h"
> +#include "intel_crtc.h"
>  #include "i915_irq.h"
>  #include "i915_reg.h"
>  #include "intel_de.h"
> @@ -1057,6 +1058,32 @@ static int i915_lpsp_status(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> +static int i915_fifo_underruns(struct seq_file *m, void *unused)
> +{
> +	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> +	struct intel_crtc *crtc;
> +	enum pipe pipe;
> +	unsigned long flags;
> +	u32 cpu_fifo_underrun_count[I915_MAX_PIPES];
> +	u32 pch_fifo_underrun_count[I915_MAX_PIPES];
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +	for_each_pipe(dev_priv, pipe) {
> +		crtc = intel_crtc_for_pipe(dev_priv, pipe);

See the implementation of intel_crtc_for_pipe(). Looping over pipes and
then converting to crtcs is not great.

> +		cpu_fifo_underrun_count[pipe] = crtc->cpu_fifo_underrun_count;
> +		pch_fifo_underrun_count[pipe] = crtc->pch_fifo_underrun_count;
> +	}
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +
> +	seq_printf(m, "\t%-10s \t%10s\n", "cpu fifounderruns", "pch fifounderruns");
> +	for_each_pipe(dev_priv, pipe)
> +		seq_printf(m, "pipe:%c %10u %20u\n", pipe_name(pipe),
> +			   cpu_fifo_underrun_count[pipe],
> +			   pch_fifo_underrun_count[pipe]);

I would replace all of the above with a single for_each_intel_crtc()
loop, and ditch the local count arrays here. I'm not sure we care about
the counts being in sync across crtcs i.e. no need to take the irq lock
across the whole loop.

...

No wait. I think I'd actually replace all of that with a *crtc* specific
debugfs file instead. See below.

> +
> +	return 0;
> +}
> +
>  static int i915_dp_mst_info(struct seq_file *m, void *unused)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -1586,6 +1613,7 @@ static const struct drm_info_list intel_display_debugfs_list[] = {
>  	{"i915_dp_mst_info", i915_dp_mst_info, 0},
>  	{"i915_ddb_info", i915_ddb_info, 0},
>  	{"i915_lpsp_status", i915_lpsp_status, 0},
> +	{"i915_fifo_underruns", i915_fifo_underruns, 0},

The direction now is to add debugfs files next to the implementation.

So with the crtc specific files, you'd add

void intel_fifo_underrun_debugfs_add(struct intel_crtc *crtc)

in intel_fifo_underrun.[ch] and call that from intel_crtc_debugfs_add().

It handles exactly one crtc. See for example
intel_drrs_crtc_debugfs_add().

>  };
>  
>  static const struct {
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 9ccae7a46020..b0468ac70059 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1438,6 +1438,8 @@ struct intel_crtc {
>  
>  #ifdef CONFIG_DEBUG_FS
>  	struct intel_pipe_crc pipe_crc;
> +	u32 cpu_fifo_underrun_count;
> +	u32 pch_fifo_underrun_count;
>  #endif
>  };
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> index d636d21fa9ce..7131dd400ac3 100644
> --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> @@ -88,6 +88,17 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
>  	return true;
>  }
>  
> +static void intel_fifo_underrun_inc_count(struct intel_crtc *crtc,
> +					  bool is_cpu_fifo)

What Andy said, but please don't add #defines, just add two separate
functions like:

intel_cpu_fifo_underrun_count_inc()
intel_pch_fifo_underrun_count_inc()

Those go hand in hand with:

intel_cpu_fifo_underrun_count_reset()
intel_pch_fifo_underrun_count_reset()

which we'll also need instead of messing with the counts directly in
some places and via accessors in some others.

> +{
> +#ifdef CONFIG_DEBUG_FS

The #ifdefs go outside the function. See coding-style.rst.

> +	if (is_cpu_fifo)
> +		crtc->cpu_fifo_underrun_count++;
> +	else
> +		crtc->pch_fifo_underrun_count++;
> +#endif
> +}
> +
>  static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -103,6 +114,7 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>  	intel_de_write(dev_priv, reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
>  	intel_de_posting_read(dev_priv, reg);
>  
> +	intel_fifo_underrun_inc_count(crtc, true);
>  	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
>  	drm_err(&dev_priv->drm, "pipe %c underrun\n", pipe_name(crtc->pipe));
>  }
> @@ -156,6 +168,7 @@ static void ivb_check_fifo_underruns(struct intel_crtc *crtc)
>  	intel_de_write(dev_priv, GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
>  	intel_de_posting_read(dev_priv, GEN7_ERR_INT);
>  
> +	intel_fifo_underrun_inc_count(crtc, true);
>  	trace_intel_cpu_fifo_underrun(dev_priv, pipe);
>  	drm_err(&dev_priv->drm, "fifo underrun on pipe %c\n", pipe_name(pipe));
>  }
> @@ -244,6 +257,7 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
>  		       SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
>  	intel_de_posting_read(dev_priv, SERR_INT);
>  
> +	intel_fifo_underrun_inc_count(crtc, false);
>  	trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);
>  	drm_err(&dev_priv->drm, "pch fifo underrun on pch transcoder %c\n",
>  		pipe_name(pch_transcoder));
> @@ -286,6 +300,11 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>  
>  	old = !crtc->cpu_fifo_underrun_disabled;
>  	crtc->cpu_fifo_underrun_disabled = !enable;
> +#ifdef CONFIG_DEBUG_FS
> +	/* don't reset count in fifo underrun irq path */
> +	if (!in_irq() && !enable)
> +		crtc->cpu_fifo_underrun_count = 0;
> +#endif

Please, no #ifdefs inline in code. Use them to choose between defining
alternate versions of a function,
e.g. intel_cpu_fifo_underrun_count_reset()

The in_irq() part needs to be handled in some other way, can't use that.

As a tip, when using functions like this, see how much it's used
elsewhere in kernel. A kind of popularity contest. If it's not widely
used, figure out why, and think again.

$ git grep -w "in_irq()" | wc -l
12

>  
>  	if (HAS_GMCH(dev_priv))
>  		i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old);
> @@ -365,6 +384,11 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
>  
>  	old = !crtc->pch_fifo_underrun_disabled;
>  	crtc->pch_fifo_underrun_disabled = !enable;
> +#ifdef CONFIG_DEBUG_FS
> +	/* don't reset count in fifo underrun irq path */
> +	if (!in_irq() && !enable)
> +		crtc->pch_fifo_underrun_count = 0;
> +#endif
>  
>  	if (HAS_PCH_IBX(dev_priv))
>  		ibx_set_fifo_underrun_reporting(&dev_priv->drm,
> @@ -434,6 +458,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  			drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n", pipe_name(pipe));
>  	}
>  
> +	intel_fifo_underrun_inc_count(crtc, true);
>  	intel_fbc_handle_fifo_underrun_irq(dev_priv);
>  }
>  
> @@ -449,6 +474,10 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  					 enum pipe pch_transcoder)
>  {
> +	struct intel_crtc *crtc = intel_crtc_for_pipe(dev_priv, pch_transcoder);
> +
> +	intel_fifo_underrun_inc_count(crtc, false);
> +
>  	if (intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder,
>  						  false)) {
>  		trace_intel_pch_fifo_underrun(dev_priv, pch_transcoder);

-- 
Jani Nikula, Intel Open Source Graphics Center