Quoting Matt Roper (2025-11-03 18:51:11-03:00) >On Mon, Nov 03, 2025 at 02:18:07PM -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. >> >> Note that the register FBC_DEBUG_STATUS also got a bit to indicate that >> the respective FBC was decompressing when the underrun happened, but we >> leave that one to be handled in an upcoming change. >> >> 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) >> v3: >> - Use REG_FIELD_GET() for fields of UNDERRUN_DBG1. (Matt) >> - Move handling of FBC_DEBUG_STATUS to a separate patch that adds >> extra logic to match FBCs by pipe. (Gustavo) >> >> Bspec: 69111, 69561, 74411, 74412 >> Cc: Jani Nikula <[email protected]> >> Cc: Matt Roper <[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_fifo_underrun.c | 72 >> ++++++++++++++++++++++ >> 2 files changed, 92 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_fifo_underrun.c >> b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c >> index c2ce8461ac9e..1da90c99f93f 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" >> @@ -352,6 +354,73 @@ 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) >> +{ >> + u32 val = intel_de_read(display, UNDERRUN_DBG1(pipe)); >> + struct { >> + u32 plane_mask; >> + const char *info; >> + } 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); >> + >> + intel_de_write(display, UNDERRUN_DBG1(pipe), val); >> + >> + for (int i = 0; i < ARRAY_SIZE(masks); i++) { >> + if (!masks[i].plane_mask) >> + continue; >> + >> + seq_buf_clear(&planes_desc); >> + >> + for (int j = 0; j < UNDERRUN_DBG1_NUM_PLANES; j++) { >> + if (!(masks[i].plane_mask & 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), 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, 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)); > >This is a global (not per-pipe) register here. So if memory is >unavailable due to a package C-state, then presumably all of our active >pipes are going to be failing to fetch data (and hitting underruns) >because of this. If we clear the sticky bit here, then the message may >only appear once. I'd remove the "Pipe %c" prefix here so that we're >not blaming one specific pipe.
Yeah, good point. Now, one thing that is bothering me is that we would be showing this message in between pipe i and pipe (i+k) underrun error messages. But I'm not sure refactoring the current code so that we "isolate" this message is worth the trouble. What if we replace "Pipe %c" with "General", so that we are explicit that this is general and not specific to the pipe? -- Gustavo Sousa > > >Matt > >> + } >> +} >> + >> /** >> * intel_cpu_fifo_underrun_irq_handler - handle CPU fifo underrun interrupt >> * @display: display device instance >> @@ -379,6 +448,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
