Quoting Matt Roper (2025-10-31 19:17:23-03:00) >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.
I'm going back to the open-coded version after feedback[1] on v4. With that, I'll move the assignment up onto the declaration. Now, for the suggestion of using __ffs(), while i915 (non-display part) and xe do use it, it appears display code has the preference of using "ffs(...) - 1", so I'm a bit hesitant on introducing its usage to display code. Maybe display maintainers could give their view on this. [1] https://lore.kernel.org/all/[email protected]/ > >> >> 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
