Re: [PATCH v3 21/29] drm/i915/xe3p_lpd: Enable system caching for FBC

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Fri, 2025-11-07 at 20:22 -0300, Gustavo Sousa wrote:
> Quoting Govindapillai, Vinod (2025-11-04 13:35:43-03:00)
> > Hi Matt,
> > 
> > 
> > On Mon, 2025-11-03 at 16:15 -0800, Matt Roper wrote:
> > > On Mon, Nov 03, 2025 at 02:18:12PM -0300, Gustavo Sousa wrote:
> > > > From: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > > > 
> > > > Configure one of the FBC instances to use system caching. FBC
> > > > read/write requests are tagged as cacheable till a programmed
> > > > limit is reached by the hw.
> > > 
> > > What exactly is "system caching?"  We have lots of different
> > > caches
> > > in
> > > current platforms, and it's not really obvious to me from the
> > > description here (or the bspec page) exactly which cache(s) are
> > > involved
> > > here.
> > > 
> > > Is turning this on always a win or is it situational?  I.e., is
> > > there
> > > any potential for display memory traffic to fill a cache with FBC
> > > data
> > > by evicting data that was part of the CPU or GT's working set? 
> > > If
> > > so,
> > > that seems like it could potentially harm the performance of
> > > other
> > > workloads running on the platform.
> > > 
> > > Or is this whole thing about a completely new cache (unrelated to
> > > and unusable by anything else) which is devoted solely to FBC?
> > > 
> > > > 
> > > > Bspec: 74722
> > > 
> > > You might want to add 68881 here since it has a bit more
> > > information
> > > about how we're actually supposed to set the fields documented on
> > > 74722.
> > 
> > Okay I will include that. I guess, the HAS also talks about "system
> > cache" - no further explanation. But only a fixed portion is
> > allocated
> > specifically for the display use and is "configurable". Motivation
> > is
> > to reduce to memory subsystem power especially in idle scenarios.
> > 
> > > 
> > > > Signed-off-by: Vinod Govindapillai
> > > > <vinod.govindapillai@intel.com>
> > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_fbc.c      | 47
> > > > +++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_fbc_regs.h |  9 +++++
> > > >  2 files changed, 56 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > index 24b72951ea3c..e2e55c58ddbc 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > > @@ -127,6 +127,9 @@ struct intel_fbc {
> > > >           */
> > > >          struct intel_fbc_state state;
> > > >          const char *no_fbc_reason;
> > > > +
> > > > +        /* Only one of FBC instances can use the system cache
> > > > */
> > > > +        bool own_sys_cache;
> > > >  };
> > > >  
> > > >  static char fbc_name(enum intel_fbc_id fbc_id)
> > > > @@ -569,12 +572,51 @@ static bool ilk_fbc_is_compressing(struct
> > > > intel_fbc *fbc)
> > > >          return intel_de_read(fbc->display,
> > > > ILK_DPFC_STATUS(fbc-
> > > > > id)) & DPFC_COMP_SEG_MASK;
> > > >  }
> > > >  
> > > > +static void nvl_fbc_program_system_cache(struct intel_fbc
> > > > *fbc,
> > > > bool enable)
> > > > +{
> > > > +        struct intel_display *display = fbc->display;
> > > > +        u32 cfb_offset, usage;
> > > > +
> > > > +        lockdep_assert_held(&fbc->lock);
> > > > +
> > > > +        usage = intel_de_read(display,
> > > > NVL_FBC_SYS_CACHE_USAGE_CFG);
> > > > +
> > > > +        /* System cache already being used by another pipe */
> > > > +        if (enable && (usage &
> > > > FBC_SYS_CACHE_TAG_USE_RES_SPACE))
> > > > +                return;
> > > 
> > > Rather than relying on the current register contents, should we
> > > be
> > > sanitizing this on driver probe (in case the pre-OS firmware
> > > already
> > > set
> > > this up) and then making our own decisions (as part of an atomic
> > > transaction) about which pipe to prioritize after that?
> > 
> > 
> > > 
> > > > +
> > > > +        /* Only the fbc instance which owns system cache can
> > > > disable it */
> > > > +        if (!enable && !fbc->own_sys_cache)
> > > > +                return;
> > > > +
> > > > +        /*
> > > > +         * Not programming the cache limit and cache reading
> > > > enable bits explicitly
> > > > +         * here. The default values should take care of those
> > > > and
> > > > that could leave
> > > > +         * adjustments of those bits to the system hw policy
> > > > +         *
> > > > +         * TODO: check if we need to explicitly program these?
> > > 
> > > There's no hardware default documented for the range field, so
> > > unless
> > > the pre-OS firmware sets it up (which we probably shouldn't rely
> > > on),
> > > I'd expect this to be 0; I don't think that's what we want.
> > 
> > The Bspec says it is 2MB. But according to the HAS it is
> > "configurable"
> > and I clarified that this is at the moment 2MB but can change. So I
> > read it as something already configured and set as the default
> > value to
> > the register and it could be changed by the soc policy. Thats the
> > reason I thought it be kept untouched. Will forward on email
> > conversation I had.
> > 
> > 
> > > 
> > > > +         */
> > > > +        cfb_offset = enable ? i915_gem_stolen_node_offset(fbc-
> > > > > compressed_fb) : 0;
> > > > +        usage |= FBC_SYS_CACHE_START_BASE(cfb_offset);
> > > 
> > > And if something *did* set this up already, then OR'ing a new
> > > address
> > > over the old one isn't going to work.  We'd need "(old & ~mask) |
> > > new"
> > > to ensure we don't have leftover bits still set by accident.  But
> > > it
> > > would probably be better to just avoid RMW-style handling in
> > > general
> > > and
> > > build a complete register value with exactly what we want rather
> > > than
> > > trying to modify the pre-existing value.
> > > 
> > > > +        usage |= enable ? FBC_SYS_CACHE_TAG_USE_RES_SPACE :
> > > > FBC_SYS_CACHE_TAG_DONT_CACHE;
> > > > +
> > > > +        intel_de_write(display, NVL_FBC_SYS_CACHE_USAGE_CFG,
> > > > usage);
> > > > +
> > > > +        fbc->own_sys_cache = enable;
> > 
> > Okay. Thanks. Will fix that!
> > 
> > > 
> > > It feels like instead of having this as a boolean flag in fbc,
> > > this
> > > should be a pointer/ID tracked at the intel_display level.  E.g.,
> > > 
> > >         display->sys_cache_fbc = fbc;
> > > 
> > > or possibly converted over to something tracked with atomic state
> > > so
> > > that we can make better high-level decisions about which FBC we
> > > want
> > > to
> > > enable this on as various displays get enabled/disabled.
> > 
> > Okay. Will check this and get rid of the bool from the intel_fbc
> > structure! At the moment we can allocate only based on the firt
> > pipe
> > enabling the fbc. But may be in future we could have some logic for
> > this I guess.
> 
> 
> Vinod, based on your replies here, I'm assuming you are going to send
> an
> updated version of this patch.  I'll drop it in the v4 of this
> series.
> 
> --
> Gustavo Sousa

Yes. I am working on this. Will update this one separately.

Thanks
Vinod

> 
> > 
> > > Matt
> > > 
> > > > +
> > > > +        drm_dbg_kms(display->drm, "System caching for FBC[%d]
> > > > %s\n",
> > > > +                    fbc->id, enable ? "configured" :
> > > > "cleared");
> > > > +}
> > > > +
> > > >  static void ilk_fbc_program_cfb(struct intel_fbc *fbc)
> > > >  {
> > > >          struct intel_display *display = fbc->display;
> > > >  
> > > >          intel_de_write(display, ILK_DPFC_CB_BASE(fbc->id),
> > > >                         i915_gem_stolen_node_offset(fbc-
> > > > > compressed_fb));
> > > > +
> > > > +        if (DISPLAY_VER(display) >= 35)
> > > > +                nvl_fbc_program_system_cache(fbc, true);
> > > >  }
> > > >  
> > > >  static const struct intel_fbc_funcs ilk_fbc_funcs = {
> > > > @@ -952,6 +994,8 @@ static void
> > > > intel_fbc_program_workarounds(struct intel_fbc *fbc)
> > > >  
> > > >  static void __intel_fbc_cleanup_cfb(struct intel_fbc *fbc)
> > > >  {
> > > > +        struct intel_display *display = fbc->display;
> > > > +
> > > >          if (WARN_ON(intel_fbc_hw_is_active(fbc)))
> > > >                  return;
> > > >  
> > > > @@ -959,6 +1003,9 @@ static void __intel_fbc_cleanup_cfb(struct
> > > > intel_fbc *fbc)
> > > >                  i915_gem_stolen_remove_node(fbc-
> > > > >compressed_llb);
> > > >          if (i915_gem_stolen_node_allocated(fbc-
> > > > >compressed_fb))
> > > >                  i915_gem_stolen_remove_node(fbc-
> > > > >compressed_fb);
> > > > +
> > > > +        if (DISPLAY_VER(display) >= 35)
> > > > +                nvl_fbc_program_system_cache(fbc, false);
> > > >  }
> > > >  
> > > >  void intel_fbc_cleanup(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 77d8321c4fb3..592cd2384255 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > > > @@ -128,4 +128,13 @@
> > > >  #define   FBC_REND_NUKE                        REG_BIT(2)
> > > >  #define   FBC_REND_CACHE_CLEAN                REG_BIT(1)
> > > >  
> > > > +#define NVL_FBC_SYS_CACHE_USAGE_CFG            
> > > > _MMIO(0x1344E0)
> > > > +#define   FBC_SYS_CACHE_START_BASE_MASK        
> > > > REG_GENMASK(31,
> > > > 16)
> > > > +#define   FBC_SYS_CACHE_START_BASE(base)       
> > > > REG_FIELD_PREP(FBC_SYS_CACHE_START_BASE_MASK, (base))
> > > > +#define   FBC_SYS_CACHEABLE_RANGE_MASK         
> > > > REG_GENMASK(15, 4)
> > > > +#define   FBC_SYS_CACHEABLE_RANGE(range)       
> > > > REG_FIELD_PREP(FBC_SYS_CACHEABLE_RANGE_MASK, (range))
> > > > +#define   FBC_SYS_CACHE_TAG_MASK                REG_GENMASK(3,
> > > > 2)
> > > > +#define   FBC_SYS_CACHE_TAG_DONT_CACHE         
> > > > REG_FIELD_PREP(FBC_SYS_CACHE_TAG_MASK, 0)
> > > > +#define   FBC_SYS_CACHE_TAG_USE_RES_SPACE      
> > > > REG_FIELD_PREP(FBC_SYS_CACHE_TAG_MASK, 3)
> > > > +
> > > >  #endif /* __INTEL_FBC_REGS__ */
> > > > 
> > > > -- 
> > > > 2.51.0
> > > > 
> > > 
> >