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.
Hehe. I ended up applying the diff from below to my local v3; it is not
too late to go back to the open-coded version though. I could go with
either versions, so let me know what you prefer. :-)
--
Gustavo Sousa
>
>>
>> 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
