On Mon, Nov 03, 2025 at 02:18:08PM -0300, Gustavo Sousa wrote:
> Xe3p_LPD added registers and bits to provide debug information at the
> time a FIFO underrun happens. We have already handled most of them,
> with FBC being left out. Let's handle it now.
>
> For FBC, a bit was added to FBC_DEBUG_STATUS that indicates that the
> respective FBC was decompressing when a FIFO underrun happened. Add the
> logic log that info.
>
> Bspec: 69561
> Cc: Matt Roper <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> Signed-off-by: Gustavo Sousa <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_fbc.c | 45
> ++++++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_fbc.h | 2 +
> drivers/gpu/drm/i915/display/intel_fbc_regs.h | 2 +
> drivers/gpu/drm/i915/display/intel_fifo_underrun.c | 1 +
> 4 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index a1e3083022ee..24b72951ea3c 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -129,6 +129,11 @@ struct intel_fbc {
> const char *no_fbc_reason;
> };
>
> +static char fbc_name(enum intel_fbc_id fbc_id)
> +{
> + return 'A' + fbc_id;
> +}
> +
> /* plane stride in pixels */
> static unsigned int intel_fbc_plane_stride(const struct intel_plane_state
> *plane_state)
> {
> @@ -2119,6 +2124,46 @@ void intel_fbc_handle_fifo_underrun_irq(struct
> intel_display *display)
> __intel_fbc_handle_fifo_underrun_irq(fbc);
> }
>
> +static unsigned int pipe_to_fbc_mask(struct intel_display *display, enum
> pipe pipe)
> +{
> + struct intel_crtc *crtc = intel_crtc_for_pipe(display, pipe);
> + struct intel_plane *plane;
> + unsigned int mask = 0;
> +
> + for_each_intel_plane_on_crtc(display->drm, crtc, plane) {
> + if (plane->fbc)
> + mask |= BIT(plane->fbc->id);
> + }
> +
> + return mask;
> +}
I notice that there's also a skl_fbc_id_for_pipe in
skl_universal_plane.c that relies on the current hardware design of one
FBC unit per pipe. What you have here is a lot more general and
future-proof if the hardware someday starts allowing multiple units per
pipe. I don't have strong feelings about whether the simple approach or
the future-proof approach is better, but we might want to consolidate to
a single function that can be used in both places so that there isn't
confusion about why there are two ways to do the same thing in different
parts of the driver.
Matt
> +
> +static void __intel_fbc_log_fifo_underrun(struct intel_fbc *fbc, enum pipe
> pipe)
> +{
> + struct intel_display *display = fbc->display;
> + u32 val;
> +
> + val = intel_de_read(display, FBC_DEBUG_STATUS(fbc->id));
> + if (val & FBC_UNDERRUN_DECMPR) {
> + intel_de_write(display, FBC_DEBUG_STATUS(fbc->id),
> FBC_UNDERRUN_DECMPR);
> + drm_err(display->drm, "Pipe %c FIFO underrun info: FBC %c
> decompressing\n",
> + pipe_name(pipe), fbc_name(fbc->id));
> + }
> +}
> +
> +void intel_fbc_log_fifo_underrun(struct intel_display *display, enum pipe
> pipe)
> +{
> + struct intel_fbc *fbc;
> + enum intel_fbc_id fbc_id;
> + unsigned int fbc_mask;
> +
> + fbc_mask = pipe_to_fbc_mask(display, pipe);
> +
> + for_each_intel_fbc(display, fbc, fbc_id)
> + if (fbc_mask & BIT(fbc_id))
> + __intel_fbc_log_fifo_underrun(fbc, pipe);
> +}
> +
> /*
> * The DDX driver changes its behavior depending on the value it reads from
> * i915.enable_fbc, so sanitize it by translating the default value into
> either
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h
> b/drivers/gpu/drm/i915/display/intel_fbc.h
> index 91424563206a..d34282cbe971 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
>
> enum fb_op_origin;
> +enum pipe;
> struct intel_atomic_state;
> struct intel_crtc;
> struct intel_crtc_state;
> @@ -46,6 +47,7 @@ void intel_fbc_flush(struct intel_display *display,
> unsigned int frontbuffer_bits, enum fb_op_origin origin);
> void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane);
> void intel_fbc_handle_fifo_underrun_irq(struct intel_display *display);
> +void intel_fbc_log_fifo_underrun(struct intel_display *display, enum pipe
> pipe);
> void intel_fbc_reset_underrun(struct intel_display *display);
> void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
> void intel_fbc_debugfs_register(struct intel_display *display);
> 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 1da90c99f93f..d0dbc4faa3f4 100644
> --- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
> @@ -403,6 +403,7 @@ static void xe3p_lpd_log_underrun(struct intel_display
> *display,
> u32 val;
>
> process_underrun_dbg1(display, pipe);
> + intel_fbc_log_fifo_underrun(display, pipe);
>
> val = intel_de_read(display, UNDERRUN_DBG2(pipe));
> if (val & UNDERRUN_FRAME_LINE_COUNTERS_FROZEN) {
>
> --
> 2.51.0
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
