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.
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?
--
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
