Quoting Ville Syrjälä (2025-11-10 14:03:03-03:00)
>On Mon, Nov 10, 2025 at 08:35:03AM -0800, Matt Roper wrote:
>> On Fri, Nov 07, 2025 at 09:05:38PM -0300, Gustavo Sousa wrote:
>> > We will need to know the FBC id respective to the pipe in other parts of
>> > the driver. Let's promote the static function skl_fbc_id_for_pipe() to a
>> > public one named intel_fbc_id_for_pipe().
>> >
>> > Signed-off-by: Gustavo Sousa <[email protected]>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_fbc.c | 5 +++++
>> > drivers/gpu/drm/i915/display/intel_fbc.h | 2 ++
>> > drivers/gpu/drm/i915/display/skl_universal_plane.c | 9 ++-------
>> > 3 files changed, 9 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
>> > b/drivers/gpu/drm/i915/display/intel_fbc.c
>> > index a1e3083022ee..435bfd05109c 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;
>> > };
>> >
>> > +enum intel_fbc_id intel_fbc_id_for_pipe(enum pipe pipe)
>> > +{
>> > + return pipe - PIPE_A + INTEL_FBC_A;
>>
>> The existing usage of skl_fbc_id_for_pipe() was to call this function to
>> receive a (possibly bogus) FBC ID, and then follow up with a call to
>> skl_plane_has_fbc() which had checks to make sure the returned FBC ID
>> actually existed on the platform. So, for example, calling
>> skl_fbc_id_for_pipe(PIPE_B) on something like an ICL would return
>> INTEL_FBC_B here, but then the subsequent call to skl_plane_has_fbc()
>> would realize that there is no FBC_B on that platform and bail out.
>> It's only relatively recently (MTL and beyond I think?) that FBC has
>> become usable on pipes other than A.
>>
>> Now that we're promoting this function to be more general, should we
>> also adjust the logic so that this function either returns a *valid* FBC
>> ID or and error? Otherwise it may not be apparent to people writing new
>> code that the result returned here can't be immediately trusted without
>> additional checking.
>
>The simples way to find the FBC instance for a pipe is to grab it from
>the primary plane. That is already used elsewhere so won't make things
>any less generic.
And do that internally without a public function, right? Because the
feedback in the next patch is that the part that handles the "FBC
decompressing" bit should be in intel_fbc.c.
How should the primary plane be found? Loop with
for_each_intel_plane_on_crtc() and get the one with type
DRM_PLANE_TYPE_PRIMARY? For example:
static struct intel_fbc *intel_fbc_for_pipe(struct intel_display *display,
enum pipe pipe)
{
struct intel_crtc *crtc = intel_crtc_for_pipe(display, pipe);
struct intel_plane *primary = NULL;
struct intel_plane *plane;
for_each_intel_plane_on_crtc(display->drm, crtc, plane) {
if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) {
primary = plane;
break;
}
}
if (drm_WARN_ON(display->drm, primary == NULL))
return NULL;
return primary->fbc;
}
I saw that the DRM layer keeps a "primary" plane in struct drm_crtc,
but, reading the kerneldoc for that member, I get the feeling that we
should not use it.
--
Gustavo Sousa
>
>>
>>
>> Matt
>>
>> > +}
>> > +
>> > /* plane stride in pixels */
>> > static unsigned int intel_fbc_plane_stride(const struct intel_plane_state
>> > *plane_state)
>> > {
>> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h
>> > b/drivers/gpu/drm/i915/display/intel_fbc.h
>> > index 91424563206a..3d02f3fe5630 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;
>> > @@ -27,6 +28,7 @@ enum intel_fbc_id {
>> > I915_MAX_FBCS,
>> > };
>> >
>> > +enum intel_fbc_id intel_fbc_id_for_pipe(enum pipe pipe);
>> > int intel_fbc_atomic_check(struct intel_atomic_state *state);
>> > int intel_fbc_min_cdclk(const struct intel_crtc_state *crtc_state);
>> > bool intel_fbc_pre_update(struct intel_atomic_state *state,
>> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > index bc55fafe9ce3..275ee2903219 100644
>> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > @@ -439,11 +439,6 @@ static int skl_plane_max_height(const struct
>> > drm_framebuffer *fb,
>> > return 4096;
>> > }
>> >
>> > -static enum intel_fbc_id skl_fbc_id_for_pipe(enum pipe pipe)
>> > -{
>> > - return pipe - PIPE_A + INTEL_FBC_A;
>> > -}
>> > -
>> > static bool skl_plane_has_fbc(struct intel_display *display,
>> > enum intel_fbc_id fbc_id, enum plane_id
>> > plane_id)
>> > {
>> > @@ -896,7 +891,7 @@ static void
>> > x3p_lpd_plane_update_pixel_normalizer(struct intel_dsb *dsb,
>> > bool enable)
>> > {
>> > struct intel_display *display = to_intel_display(plane);
>> > - enum intel_fbc_id fbc_id = skl_fbc_id_for_pipe(plane->pipe);
>> > + enum intel_fbc_id fbc_id = intel_fbc_id_for_pipe(plane->pipe);
>> > u32 val;
>> >
>> > /* Only HDR planes have pixel normalizer and don't matter if no
>> > FBC */
>> > @@ -2442,7 +2437,7 @@ void icl_link_nv12_planes(struct intel_plane_state
>> > *uv_plane_state,
>> > static struct intel_fbc *skl_plane_fbc(struct intel_display *display,
>> > enum pipe pipe, enum plane_id
>> > plane_id)
>> > {
>> > - enum intel_fbc_id fbc_id = skl_fbc_id_for_pipe(pipe);
>> > + enum intel_fbc_id fbc_id = intel_fbc_id_for_pipe(pipe);
>> >
>> > if (skl_plane_has_fbc(display, fbc_id, plane_id))
>> > return display->fbc[fbc_id];
>> >
>> > --
>> > 2.51.0
>> >
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation
>
>--
>Ville Syrjälä
>Intel
