Re: [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane

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

On Tue, Aug 29, 2023 at 01:34:56PM +0000, Govindapillai, Vinod wrote:
> On Tue, 2023-08-29 at 10:50 +0300, Ville Syrjälä wrote:
> > On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> > > In LNL onwards, FBC can be associated to the first three planes.
> > > The FBC will be enabled for first FBC capable visible plane
> > > until the userspace can select one of these FBC capable plane
> > > explicitly
> > > 
> > > Bspec: 69560
> > > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c      | 29 +++++++++++++++++++
> > >  .../drm/i915/display/skl_universal_plane.c    |  5 +++-
> > >  drivers/gpu/drm/i915/i915_reg.h               |  4 +++
> > >  3 files changed, 37 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 45e205a0f740..62f59630d410 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct intel_fbc *fbc)
> > >                      CHICKEN_FBC_STRIDE_MASK, val);
> > >  }
> > >  
> > > +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> > > +{
> > > +       switch (fbc->state.plane->id) {
> > > +       default:
> > > +               MISSING_CASE(fbc->state.plane->id);
> > > +               fallthrough;
> > > +       case 0:
> > > +               return DPFC_CTL_PLANE_BINDING_1;
> > > +       case 1:
> > > +               return DPFC_CTL_PLANE_BINDING_2;
> > > +       case 2:
> > > +               return DPFC_CTL_PLANE_BINDING_3;
> > > +       }
> > > +}
> > > +
> > >  static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > >  {
> > >         const struct intel_fbc_state *fbc_state = &fbc->state;
> > > @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > >         if (IS_IVYBRIDGE(i915))
> > >                 dpfc_ctl |= DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
> > >  
> > > +       if (DISPLAY_VER(i915) >= 20)
> > > +               dpfc_ctl |= lnl_plane_binding(fbc);
> > > +
> > >         if (fbc_state->fence_id >= 0)
> > >                 dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
> > >  
> > > @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
> > >                 }
> > >         }
> > >  
> > > +       /*
> > > +        * From LNL, FBC can be assigned on any plane. Until a provision is
> > > +        * provided for the userspace to select a plane for FBC, lets select
> > > +        * the first visible plane that is FBC capable.
> > > +        */
> > > +       if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&
> > > +           fbc->state.plane != plane) {
> > > +               plane_state->no_fbc_reason = "fbc enabled on another plane";
> > > +               return 0;
> > > +       }
> > 
> > This isn't right. fbc->state.plane is an internal state thing
> > that chanes willy nilly, so we don't want to check it here.
> > 
> > But we shouldn't need this kind of check anyway. The code 
> > should already work just fine if you just do the tweak to
> > skl_plane_has_fbc((). Which plane actually gets FBC will be
> > a bit random, but I think that's fine for the moment.
> 
> Currently, intel_crtc_init() creates the primary plane and other planes. Then the "fbc" is
> associated with the primary plane based on skl_plane_has_fbc() 
> 
> In Lnl, the first 3 planes are FBC capable. So my thinking has been that those three planes can have
> the fbc pointers. 
> 
> And because at the moment we don't have a logic to select which frame should get this FBC,

It's there (in intel_fbc_update()/__intel_fbc_enable() more or
less). It's just "first come first serve".

> I used
> the above snippet to avoid FBC being assoicated with multiple planes in a pipe. So the first visible
> fbc capable plane will had no_fbc_reason as NULL and the other 2 planes will have "fbc on another
> plane".
> 
> BR
> Vinod
> 
> > 
> > > +
> > >         plane_state->no_fbc_reason = NULL;
> > >  
> > >         return 0;
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index 4d01c7ae4485..1291351c9941 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -1962,7 +1962,10 @@ static bool skl_plane_has_fbc(struct drm_i915_private *dev_priv,
> > >         if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
> > >                 return false;
> > >  
> > > -       return plane_id == PLANE_PRIMARY;
> > > +       if (DISPLAY_VER(dev_priv) >= 20)
> > > +               return plane_id <= PLANE_SPRITE1;
> > > +       else
> > > +               return plane_id == PLANE_PRIMARY;
> > >  }
> > >  
> > >  static struct intel_fbc *skl_plane_fbc(struct drm_i915_private *dev_priv,
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index aefad14ab27a..b207774f3c33 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1327,6 +1327,10 @@
> > >  #define   DPFC_CTL_PLANE_IVB(i9xx_plane)       REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB,
> > > (i9xx_plane))
> > >  #define   DPFC_CTL_FENCE_EN_IVB                        REG_BIT(28) /* ivb+ */
> > >  #define   DPFC_CTL_PERSISTENT_MODE             REG_BIT(25) /* g4x-snb */
> > > +#define   DPFC_CTL_PLANE_BINDING_MASK          REG_GENMASK(12, 11) /* XE */
> > > +#define   DPFC_CTL_PLANE_BINDING_1             REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 0)
> > > /* XE */
> > > +#define   DPFC_CTL_PLANE_BINDING_2             REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 1)
> > > /* XE */
> > > +#define   DPFC_CTL_PLANE_BINDING_3             REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 2)
> > > /* XE */
> > >  #define   DPFC_CTL_FALSE_COLOR                 REG_BIT(10) /* ivb+ */
> > >  #define   DPFC_CTL_SR_EN                       REG_BIT(10) /* g4x only */
> > >  #define   DPFC_CTL_SR_EXIT_DIS                 REG_BIT(9) /* g4x only */
> > > -- 
> > > 2.34.1
> > 
> 

-- 
Ville Syrjälä
Intel