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.
> +#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;
> +
> + 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?
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
