| Message ID | 20230208105932.21681-1-swati2.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Add a debugfs entry for fifo underruns | expand |
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); > }
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);
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); >> }
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); >>> } >
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);