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