On Thu, Oct 30, 2025 at 06:56:07PM -0300, Gustavo Sousa wrote:
> Quoting Matt Roper (2025-10-29 17:54:39-03:00)
> >On Tue, Oct 21, 2025 at 09:28:36PM -0300, Gustavo Sousa wrote:
> >> From: Sai Teja Pottumuttu <[email protected]>
> >>
> >> Starting with Xe3p_LPD, we get two new registers and some bits in
> >> existing registers that expose hardware state information at the time of
> >> underrun notification that can be relevant to debugging.
> >>
> >> Add the necessary logic in the driver to print the available debug
> >> information when an underrun happens.
> >>
> >> v2:
> >> - Use seq_buf to generate planes string. (Jani)
> >> - Move definition of FBC_DEBUG_STATUS to intel_fbc_regs.h. (Ville)
> >> - Change logic for processing UNDERRUN_DBG1 to use loops and move it
> >> to a separate function. (Gustavo)
> >> - Always print underrun error message, even if there wouldn't be any
> >> debug info associated to the underrun. (Gustavo)
> >>
> >> Bspec: 69111, 69561, 74411, 74412
> >> Cc: Jani Nikula <[email protected]>
> >> Cc: Ville Syrjälä <[email protected]>
> >> Signed-off-by: Sai Teja Pottumuttu <[email protected]>
> >> Co-developed-by: Gustavo Sousa <[email protected]>
> >> Signed-off-by: Gustavo Sousa <[email protected]>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_display_regs.h | 20 +++++
> >> drivers/gpu/drm/i915/display/intel_fbc_regs.h | 2 +
> >> drivers/gpu/drm/i915/display/intel_fifo_underrun.c | 87
> >> ++++++++++++++++++++++
> >> 3 files changed, 109 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h
> >> b/drivers/gpu/drm/i915/display/intel_display_regs.h
> >> index 9d71e26a4fa2..c9f8b90faa42 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_regs.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
> >> @@ -882,6 +882,25 @@
> >> #define PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK
> >> REG_GENMASK(2, 0) /* tgl+ */
> >> #define PIPE_MISC2_FLIP_INFO_PLANE_SEL(plane_id)
> >> REG_FIELD_PREP(PIPE_MISC2_FLIP_INFO_PLANE_SEL_MASK, (plane_id))
> >>
> >> +#define _UNDERRUN_DBG1_A 0x70064
> >> +#define _UNDERRUN_DBG1_B 0x71064
> >> +#define UNDERRUN_DBG1(pipe)
> >> _MMIO_PIPE(pipe, \
> >> +
> >> _UNDERRUN_DBG1_A, \
> >> +
> >> _UNDERRUN_DBG1_B)
> >> +#define UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK
> >> REG_GENMASK(29, 24)
> >> +#define UNDERRUN_DDB_EMPTY_MASK REG_GENMASK(21,
> >> 16)
> >> +#define UNDERRUN_DBUF_NOT_FILLED_MASK
> >> REG_GENMASK(13, 8)
> >> +#define UNDERRUN_BELOW_WM0_MASK REG_GENMASK(5, 0)
> >> +
> >> +#define _UNDERRUN_DBG2_A 0x70068
> >> +#define _UNDERRUN_DBG2_B 0x71068
> >> +#define UNDERRUN_DBG2(pipe)
> >> _MMIO_PIPE(pipe, \
> >> +
> >> _UNDERRUN_DBG2_A, \
> >> +
> >> _UNDERRUN_DBG2_B)
> >> +#define UNDERRUN_FRAME_LINE_COUNTERS_FROZEN REG_BIT(31)
> >> +#define UNDERRUN_PIPE_FRAME_COUNT_MASK REG_GENMASK(30,
> >> 20)
> >> +#define UNDERRUN_LINE_COUNT_MASK REG_GENMASK(19,
> >> 0)
> >> +
> >> #define DPINVGTT _MMIO(VLV_DISPLAY_BASE +
> >> 0x7002c) /* VLV/CHV only */
> >> #define DPINVGTT_EN_MASK_CHV
> >> REG_GENMASK(27, 16)
> >> #define DPINVGTT_EN_MASK_VLV
> >> REG_GENMASK(23, 16)
> >> @@ -1416,6 +1435,7 @@
> >>
> >> #define GEN12_DCPR_STATUS_1 _MMIO(0x46440)
> >> #define XELPDP_PMDEMAND_INFLIGHT_STATUS REG_BIT(26)
> >> +#define XE3P_UNDERRUN_PKGC REG_BIT(21)
> >>
> >> #define FUSE_STRAP _MMIO(0x42014)
> >> #define ILK_INTERNAL_GRAPHICS_DISABLE REG_BIT(31)
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> >> b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> >> index b1d0161a3196..272dba7f9695 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> >> @@ -88,6 +88,8 @@
> >> #define DPFC_FENCE_YOFF _MMIO(0x3218)
> >> #define ILK_DPFC_FENCE_YOFF(fbc_id) _MMIO_PIPE((fbc_id), 0x43218,
> >> 0x43258)
> >> #define DPFC_CHICKEN _MMIO(0x3224)
> >> +#define FBC_DEBUG_STATUS(pipe) _MMIO_PIPE(pipe, 0x43220,
> >> 0x43260)
> >
> >Is 'pipe' correct here? Most of the other FBC registers are
> >parameterized by FBC instance rather than pipe.
>
> Yeah, I just blindly copy/pasted the definition without realizing that
> the rest of the file uses fbc_id. I'll change it to use fbc_id as well.
>
> >
> >> +#define FBC_UNDERRUN_DECOMPRESSION REG_BIT(27)
> >> #define ILK_DPFC_CHICKEN(fbc_id) _MMIO_PIPE((fbc_id), 0x43224,
> >> 0x43264)
> >> #define DPFC_HT_MODIFY REG_BIT(31) /* pre-ivb */
> >> #define DPFC_NUKE_ON_ANY_MODIFICATION REG_BIT(23) /*
> >> bdw+ */
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> >> b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> >> index c2ce8461ac9e..43cf141a59ae 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> >> @@ -25,6 +25,8 @@
> >> *
> >> */
> >>
> >> +#include <linux/seq_buf.h>
> >> +
> >> #include <drm/drm_print.h>
> >>
> >> #include "i915_reg.h"
> >> @@ -34,6 +36,7 @@
> >> #include "intel_display_trace.h"
> >> #include "intel_display_types.h"
> >> #include "intel_fbc.h"
> >> +#include "intel_fbc_regs.h"
> >> #include "intel_fifo_underrun.h"
> >> #include "intel_pch_display.h"
> >>
> >> @@ -352,6 +355,87 @@ bool intel_set_pch_fifo_underrun_reporting(struct
> >> intel_display *display,
> >> return old;
> >> }
> >>
> >> +#define UNDERRUN_DBG1_NUM_PLANES 6
> >> +
> >> +static void process_underrun_dbg1(struct intel_display *display,
> >> + enum pipe pipe)
> >> +{
> >> + struct {
> >> + u32 mask;
> >> + const char *info;
> >> + } info_masks[] = {
> >> + { UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK, "DBUF block not
> >> valid" },
> >> + { UNDERRUN_DDB_EMPTY_MASK, "DDB empty" },
> >> + { UNDERRUN_DBUF_NOT_FILLED_MASK, "DBUF not completely
> >> filled" },
> >> + { UNDERRUN_BELOW_WM0_MASK, "DBUF below WM0" },
> >> + };
> >> + DECLARE_SEQ_BUF(planes_desc, 32);
> >> + u32 val;
> >> +
> >> + val = intel_de_read(display, UNDERRUN_DBG1(pipe));
> >> + intel_de_write(display, UNDERRUN_DBG1(pipe), val);
> >> +
> >> + for (int i = 0; i < ARRAY_SIZE(info_masks); i++) {
> >> + u32 plane_bits;
> >> +
> >> + plane_bits = val & info_masks[i].mask;
> >> +
> >> + if (!plane_bits)
> >> + continue;
> >> +
> >> + plane_bits >>= ffs(info_masks[i].mask) - 1;
> >
> >Nitpick: It seems like we're just open-coding REG_FIELD_GET here. Any
> >reason not to simplify down to something like this?
> >
> > u32 plane_bits = REG_FIELD_GET(info_masks[i].mask, val);
> >
> > if (!plane_bits)
> > continue;
>
> We can't use REG_FIELD_GET() because the mask parameter must be a
> constant expression. That's why I went with open-coded version.
Oh yeah, I always forget about that restriction. I'm fine with keeping
the open-coded version in that case, although you may want to move the
plane_bits assignment up onto the declaration line. And maybe use
__ffs() instead of ffs() to avoid the need to substract 1.
>
> We could change the current patch to use REG_FIELD_GET() with something
> like below. What do you think?
>
> |diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> |index 43cf141a59ae..34faedb9799c 100644
> |--- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> |+++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> |@@ -360,35 +360,28 @@ bool intel_set_pch_fifo_underrun_reporting(struct
> intel_display *display,
> | static void process_underrun_dbg1(struct intel_display *display,
> | enum pipe pipe)
> | {
> |+ u32 val = intel_de_read(display, UNDERRUN_DBG1(pipe));
> | struct {
> |- u32 mask;
> |+ u32 plane_mask;
> | const char *info;
> |- } info_masks[] = {
> |- { UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK, "DBUF block not
> valid" },
> |- { UNDERRUN_DDB_EMPTY_MASK, "DDB empty" },
> |- { UNDERRUN_DBUF_NOT_FILLED_MASK, "DBUF not completely
> filled" },
> |- { UNDERRUN_BELOW_WM0_MASK, "DBUF below WM0" },
> |+ } masks[] = {
> |+ { REG_FIELD_GET(UNDERRUN_DBUF_BLOCK_NOT_VALID_MASK,
> val), "DBUF block not valid" },
> |+ { REG_FIELD_GET(UNDERRUN_DDB_EMPTY_MASK, val), "DDB
> empty" },
> |+ { REG_FIELD_GET(UNDERRUN_DBUF_NOT_FILLED_MASK, val),
> "DBUF not completely filled" },
> |+ { REG_FIELD_GET(UNDERRUN_BELOW_WM0_MASK, val), "DBUF
> below WM0" },
> | };
> | DECLARE_SEQ_BUF(planes_desc, 32);
> |- u32 val;
> |
> |- val = intel_de_read(display, UNDERRUN_DBG1(pipe));
> | intel_de_write(display, UNDERRUN_DBG1(pipe), val);
> |
> |- for (int i = 0; i < ARRAY_SIZE(info_masks); i++) {
> |- u32 plane_bits;
> |-
> |- plane_bits = val & info_masks[i].mask;
> |-
> |- if (!plane_bits)
> |+ for (int i = 0; i < ARRAY_SIZE(masks); i++) {
> |+ if (!masks[i].plane_mask)
> | continue;
> |
> |- plane_bits >>= ffs(info_masks[i].mask) - 1;
> |-
> | seq_buf_clear(&planes_desc);
> |
> | for (int j = 0; j < UNDERRUN_DBG1_NUM_PLANES; j++) {
> |- if (!(plane_bits & REG_BIT(j)))
> |+ if (!(masks[i].plane_mask & REG_BIT(j)))
> | continue;
> |
> | if (j == 0)
> |@@ -399,7 +392,7 @@ static void process_underrun_dbg1(struct
> intel_display *display,
> |
> | drm_err(display->drm,
> | "Pipe %c FIFO underrun info: %s on planes:
> %s\n",
> |- pipe_name(pipe), info_masks[i].info,
> seq_buf_str(&planes_desc));
> |+ pipe_name(pipe), masks[i].info,
> seq_buf_str(&planes_desc));
> |
> | drm_WARN_ON(display->drm,
> seq_buf_has_overflowed(&planes_desc));
> | }
>
> >
> >> +
> >> + seq_buf_clear(&planes_desc);
> >> +
> >> + for (int j = 0; j < UNDERRUN_DBG1_NUM_PLANES; j++) {
> >> + if (!(plane_bits & REG_BIT(j)))
> >> + continue;
> >> +
> >> + if (j == 0)
> >> + seq_buf_puts(&planes_desc, "[C]");
> >> + else
> >> + seq_buf_printf(&planes_desc, "[%d]", j);
> >> + }
> >> +
> >> + drm_err(display->drm,
> >> + "Pipe %c FIFO underrun info: %s on planes: %s\n",
> >> + pipe_name(pipe), info_masks[i].info,
> >> seq_buf_str(&planes_desc));
> >> +
> >> + drm_WARN_ON(display->drm,
> >> seq_buf_has_overflowed(&planes_desc));
> >> + }
> >> +}
> >> +
> >> +static void xe3p_lpd_log_underrun(struct intel_display *display,
> >> + enum pipe pipe)
> >> +{
> >> + u32 val;
> >> +
> >> + process_underrun_dbg1(display, pipe);
> >> +
> >> + val = intel_de_read(display, UNDERRUN_DBG2(pipe));
> >> + if (val & UNDERRUN_FRAME_LINE_COUNTERS_FROZEN) {
> >> + intel_de_write(display, UNDERRUN_DBG2(pipe),
> >> UNDERRUN_FRAME_LINE_COUNTERS_FROZEN);
> >> + drm_err(display->drm, "Pipe %c FIFO underrun info: Frame
> >> count: %u, Line count: %u\n",
> >> + pipe_name(pipe),
> >> + REG_FIELD_GET(UNDERRUN_PIPE_FRAME_COUNT_MASK,
> >> val),
> >> + REG_FIELD_GET(UNDERRUN_LINE_COUNT_MASK, val));
> >> + }
> >> +
> >> + val = intel_de_read(display, FBC_DEBUG_STATUS(pipe));
> >> + if (val & FBC_UNDERRUN_DECOMPRESSION) {
> >> + intel_de_write(display, FBC_DEBUG_STATUS(pipe),
> >> FBC_UNDERRUN_DECOMPRESSION);
> >> + drm_err(display->drm, "Pipe %c FIFO underrun info: FBC
> >> decompression\n",
> >> + pipe_name(pipe));
> >> + }
> >
> >As noted above, I'm not sure if 'pipe' is technically correct for this
> >register. I think it always winds up with a 1:1 relationship on current
> >platforms, but would it make more sense to just move this check and
> >print into intel_fbc_handle_fifo_underrun_irq() where we're already
> >handling the FBC stuff on a per-FBC unit basis?
>
> Yeah. We probably want to check if there is a valid FBC instance
> respective to this pipe and then read the register.
>
> I see complications with moving this to
> intel_fbc_handle_fifo_underrun_irq():
>
> 1) The function intel_fbc_handle_fifo_underrun_irq() is more about
> disabling the FBC on an underrun. I think reporting that the FBC
> was decompressing when the there was an underrun makes more sense
> to be grouped together with the other info related to FIFO
> underruns. It could even be useful if, due to some hw/sw bug, FBC
> is still doing something after we disabled it (or so we thought)
> due to a previous underrun.
>
> 2) Logging underrun debug info is currently guarded by
> intel_set_cpu_fifo_underrun_reporting(). So, we would need to
> complicate the implementation a bit to ensure that
> intel_fbc_handle_fifo_underrun_irq() would only report when
> reporting was enabled.
>
>
> So, I was thinking about defining a new function
> intel_fbc_fifo_underrun_log_info() and calling it from
> xe3p_lpd_log_underrun(). What do you think?
Yeah, that sounds fine to me.
Matt
>
> --
> Gustavo Sousa
> >
> >
> >Matt
> >
> >> +
> >> + val = intel_de_read(display, GEN12_DCPR_STATUS_1);
> >> + if (val & XE3P_UNDERRUN_PKGC) {
> >> + intel_de_write(display, GEN12_DCPR_STATUS_1,
> >> XE3P_UNDERRUN_PKGC);
> >> + drm_err(display->drm, "Pipe %c FIFO underrun info: Pkgc
> >> blocking memory\n",
> >> + pipe_name(pipe));
> >> + }
> >> +}
> >> +
> >> /**
> >> * intel_cpu_fifo_underrun_irq_handler - handle CPU fifo underrun
> >> interrupt
> >> * @display: display device instance
> >> @@ -379,6 +463,9 @@ void intel_cpu_fifo_underrun_irq_handler(struct
> >> intel_display *display,
> >> trace_intel_cpu_fifo_underrun(display, pipe);
> >>
> >> drm_err(display->drm, "CPU pipe %c FIFO underrun\n",
> >> pipe_name(pipe));
> >> +
> >> + if (DISPLAY_VER(display) >= 35)
> >> + xe3p_lpd_log_underrun(display, pipe);
> >> }
> >>
> >> intel_fbc_handle_fifo_underrun_irq(display);
> >>
> >> --
> >> 2.51.0
> >>
> >
> >--
> >Matt Roper
> >Graphics Software Engineer
> >Linux GPU Platform Enablement
> >Intel Corporation
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
