Quoting Matt Roper (2025-11-03 19:30:28-03:00)
>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.
For today's needs, I think it is probably simpler to just promote
skl_fbc_id_for_pipe() to a function exported by intel_fbc.h. I'll do
that.
Thanks!
--
Gustavo Sousa
>
>
>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
