Re: [PATCH v4 05/11] drm/i915/fbc: Add intel_fbc_id_for_pipe()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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 <gustavo.sousa@intel.com>
> > ---
> >  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