Quoting Jani Nikula (2025-11-10 08:45:48-03:00) >On Fri, 07 Nov 2025, Gustavo Sousa <[email protected]> wrote: >> Xe3p_LPD added several bits containing information that can be relevant >> to debugging FIFO underruns. Add the logic necessary to handle them >> when reporting underruns. >> >> This was adapted from the initial patch[1] from Sai Teja Pottumuttu. >> >> [1] >> https://lore.kernel.org/all/[email protected]/ >> >> Bspec: 69111, 69561, 74411, 74412 >> Cc: Jani Nikula <[email protected]> >> Cc: Matt Roper <[email protected]> >> Cc: Ville Syrjälä <[email protected]> >> Signed-off-by: Gustavo Sousa <[email protected]> >> --- >> I tested this by adding a change on top of this series that updates >> Xe3p_LPD's CDCLK table to use bad values and I got the following >> messages: >> >> [ +0.000237] xe 0000:00:02.0: [drm:intel_modeset_verify_crtc [xe]] >> [CRTC:88:pipe A] >> [ +0.000674] xe 0000:00:02.0: [drm] *ERROR* CPU pipe A FIFO underrun >> [ +0.000015] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: >> DBUF block not valid on planes: [1] >> [ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: >> DDB empty on planes: [1] >> [ +0.000001] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: >> DBUF below WM0 on planes: [1] >> [ +0.000004] xe 0000:00:02.0: [drm] *ERROR* Pipe A FIFO underrun info: >> frame count: 1890, line count: 44 >> --- >> .../gpu/drm/i915/display/intel_display_device.h | 1 + >> drivers/gpu/drm/i915/display/intel_display_regs.h | 16 +++ >> drivers/gpu/drm/i915/display/intel_fbc_regs.h | 2 + >> drivers/gpu/drm/i915/display/intel_fifo_underrun.c | 128 >> +++++++++++++++++++++ >> 4 files changed, 147 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h >> b/drivers/gpu/drm/i915/display/intel_display_device.h >> index b559ef43d547..91d8cfac5eff 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_device.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h >> @@ -197,6 +197,7 @@ struct intel_display_platforms { >> #define HAS_TRANSCODER(__display, trans) >> ((DISPLAY_RUNTIME_INFO(__display)->cpu_transcoder_mask & \ >> BIT(trans)) != 0) >> #define HAS_UNCOMPRESSED_JOINER(__display) (DISPLAY_VER(__display) >> >= 13) >> +#define HAS_UNDERRUN_DBG_INFO(__display) (DISPLAY_VER(__display) >= >> 35) >> #define HAS_ULTRAJOINER(__display) (((__display)->platform.dgfx && \ >> DISPLAY_VER(__display) == 14) && >> HAS_DSC(__display)) >> #define HAS_VRR(__display) (DISPLAY_VER(__display) >= 11) >> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h >> b/drivers/gpu/drm/i915/display/intel_display_regs.h >> index 9d71e26a4fa2..89ea0156ee06 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_regs.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h >> @@ -882,6 +882,21 @@ >> #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 +1431,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..77d8321c4fb3 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(fbc_id) _MMIO_PIPE((fbc_id), 0x43220, >> 0x43260) >> +#define FBC_UNDERRUN_DECMPR 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..8a05b5c5fccd 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" >> >> @@ -57,6 +60,118 @@ >> * The code also supports underrun detection on the PCH transcoder. >> */ >> >> +#define UNDERRUN_DBG1_NUM_PLANES 6 >> + >> +static void read_underrun_dbg1(struct intel_display *display, enum pipe >> pipe, bool log) >> +{ >> + u32 val = intel_de_read(display, UNDERRUN_DBG1(pipe)); > >Nitpick, I really don't like the style of using "functional" (for want >of a better word) initializers. Complicated is fine, like below for >masks[], but doing something with the hardware or something that can >fail, feels iffy.
Alright. I'll update this to do as you suggested below. One little annoyance is that we will need to open code REG_FIELD_GET() (as done in [1]), because the mask parameter needs to be a constant expression. [1] https://lore.kernel.org/all/[email protected]/ > >> + 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); >> + > >IMO there's also a lot more clarity in having the assignment and the >check back to back here: > > val = intel_de_read(display, UNDERRUN_DBG1(pipe)); > if (!val) > return; > >Ditto below. I'll update those as well. That said, for curiosity: Do you prefer to always use the pattern of declaring a variable for register values and assigning the result of the reading as separate statements? Or is this a preference for specific cases, like this one? I did git grep -W 'u32\s\+\w\+\s*=\s*intel_de_read' -- drivers/gpu/drm/i915/display/ and found cases where the pattern "u32 val = intel_de_read(...)" appears to make the code a bit more concise IMO. While here in read_underrun_dbg1() the other variables and initializers do get in the way of readability/clarity, I see cases like: u32 val = intel_de_read(...); do_something_with(val); to be still clear, and each saves up 1 line of editor real estate. -- Gustavo Sousa > >BR, >Jani. > > >> + if (!val) >> + return; >> + >> + intel_de_write(display, UNDERRUN_DBG1(pipe), val); >> + >> + if (!log) >> + return; >> + >> + 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 read_underrun_dbg2(struct intel_display *display, enum pipe >> pipe, bool log) >> +{ >> + u32 val = intel_de_read(display, UNDERRUN_DBG2(pipe)); >> + >> + if (!(val & UNDERRUN_FRAME_LINE_COUNTERS_FROZEN)) >> + return; >> + >> + intel_de_write(display, UNDERRUN_DBG2(pipe), >> UNDERRUN_FRAME_LINE_COUNTERS_FROZEN); >> + >> + if (log) >> + 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)); >> +} >> + >> +static void read_underrun_dbg_fbc(struct intel_display *display, enum pipe >> pipe, bool log) >> +{ >> + enum intel_fbc_id fbc_id = intel_fbc_id_for_pipe(pipe); >> + u32 val = intel_de_read(display, FBC_DEBUG_STATUS(fbc_id)); >> + >> + if (!(val & FBC_UNDERRUN_DECMPR)) >> + return; >> + >> + intel_de_write(display, FBC_DEBUG_STATUS(fbc_id), >> FBC_UNDERRUN_DECMPR); >> + >> + if (log) >> + drm_err(display->drm, >> + "Pipe %c FIFO underrun info: FBC decompressing\n", >> + pipe_name(pipe)); >> +} >> + >> +static void read_underrun_dbg_pkgc(struct intel_display *display, bool log) >> +{ >> + u32 val = intel_de_read(display, GEN12_DCPR_STATUS_1); >> + >> + if (!(val & XE3P_UNDERRUN_PKGC)) >> + return; >> + >> + /* >> + * Note: If there are multiple pipes enabled, only one of them will >> see >> + * XE3P_UNDERRUN_PKGC set. >> + */ >> + intel_de_write(display, GEN12_DCPR_STATUS_1, XE3P_UNDERRUN_PKGC); >> + >> + if (log) >> + drm_err(display->drm, >> + "General FIFO underrun info: Package C-state >> blocking memory\n"); >> +} >> + >> +static void read_underrun_dbg_info(struct intel_display *display, >> + enum pipe pipe, >> + bool log) >> +{ >> + if (!HAS_UNDERRUN_DBG_INFO(display)) >> + return; >> + >> + read_underrun_dbg1(display, pipe, log); >> + read_underrun_dbg2(display, pipe, log); >> + read_underrun_dbg_fbc(display, pipe, log); >> + read_underrun_dbg_pkgc(display, log); >> +} >> + >> static bool ivb_can_enable_err_int(struct intel_display *display) >> { >> struct intel_crtc *crtc; >> @@ -262,6 +377,17 @@ static bool >> __intel_set_cpu_fifo_underrun_reporting(struct intel_display *displa >> old = !crtc->cpu_fifo_underrun_disabled; >> crtc->cpu_fifo_underrun_disabled = !enable; >> >> + /* >> + * The debug bits get latched at the time of the FIFO underrun ISR >> bit >> + * getting set. That means that any existing debug bit that is set >> when >> + * handling a FIFO underrun interrupt has the potential to belong to >> + * another underrun event (past or future). To alleviate this >> problem, >> + * let's clear existing bits before enabling the interrupt, so that >> at >> + * least we don't get information that is too out-of-date. >> + */ >> + if (enable && !old) >> + read_underrun_dbg_info(display, pipe, false); >> + >> if (HAS_GMCH(display)) >> i9xx_set_fifo_underrun_reporting(display, pipe, enable, >> old); >> else if (display->platform.ironlake || >> display->platform.sandybridge) >> @@ -379,6 +505,8 @@ 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)); >> + >> + read_underrun_dbg_info(display, pipe, true); >> } >> >> intel_fbc_handle_fifo_underrun_irq(display); > >-- >Jani Nikula, Intel
