Re: [PATCH v5 2/3] drm/i915/xe3p_lpd: Enable display use of system cache for FBC

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

Hi Jouni

Thanks for the review.

On Wed, 2025-12-03 at 13:12 +0000, Hogander, Jouni wrote:
> On Fri, 2025-11-28 at 13:35 +0200, Vinod Govindapillai wrote:
> > One of the FBC instances can utilize the reserved area of SoC
> > level cache for the fbc transactions to benefit reduced memory
> > system power especially in idle scenarios. Reserved area of the
> > system cache can be assigned to an fbc instance by configuring
> > the cacheability configuration register with offset of the
> > compressed frame buffer in stolen memoty of that fbc. There is
> > a limit to this reserved area which is programmable and for
> > xe3p_lpd the limit is defined as 2MB.
> 
> Maybe you could mention here wow it is decided which instance can use
> the cache?
> 
Ack
> > 
> > v2: - better to track fbc sys cache usage from intel_display level,
> >       sanitize the cacheability config register on probe (Matt)
> >     - limit this for integrated graphics solutions, confirmed that
> >       no default value set for cache range by hw (Gustavo)
> > 
> > v3: - changes related to the use of fbc substruct in intel_display
> >     - use intel_de_write() instead of intel_rmw() by hardcoding the
> >       default value fields
> > 
> > v4: - protect sys cache config accesses, sys cache usage status in
> >       debugfs per fbc instance (Jani)
> > 
> > v5: - mutex_init and missing mutex_lock in sanitize call
> > 
> > Bspec: 68881, 74722
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_core.h |  7 ++
> >  .../drm/i915/display/intel_display_device.h   |  1 +
> >  drivers/gpu/drm/i915/display/intel_fbc.c      | 87
> > +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_fbc_regs.h | 10 +++
> >  4 files changed, 105 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index 58325f530670..0a1744b3b440 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -400,6 +400,13 @@ struct intel_display {
> >  
> >  	struct {
> >  		struct intel_fbc *instances[I915_MAX_FBCS];
> > +
> > +		/* xe3p_lpd+: FBC instance utilizing the system
> > cache */
> > +		struct sys_cache_cfg {
> > +			/* Protect concurrecnt access to system
> > cache configuration */
> > +			struct mutex lock;
> > +			enum intel_fbc_id id;
> > +		} sys_cache;
> >  	} fbc;
> >  
> >  	struct {
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> > b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index b559ef43d547..b74cb69ccc85 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -173,6 +173,7 @@ struct intel_display_platforms {
> >  #define HAS_DSC_MST(__display)		(DISPLAY_VER(__display) >=
> > 12 && HAS_DSC(__display))
> >  #define
> > HAS_FBC(__display)		(DISPLAY_RUNTIME_INFO(__display)-
> > >fbc_mask != 0)
> >  #define HAS_FBC_DIRTY_RECT(__display)	(DISPLAY_VER(__display) >=
> > 30)
> > +#define HAS_FBC_SYS_CACHE(__display)	(DISPLAY_VER(__display) >=
> > 35 && !(__display)->platform.dgfx)
> >  #define
> > HAS_FPGA_DBG_UNCLAIMED(__display)	(DISPLAY_INFO(__display)-
> > >has_fpga_dbg)
> >  #define HAS_FW_BLC(__display)		(DISPLAY_VER(__display) >=
> > 3)
> >  #define
> > HAS_GMBUS_BURST_READ(__display)	(DISPLAY_VER(__display) >= 10 ||
> > (__display)->platform.kabylake)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index dcdfcff80de3..cebde5db3dd7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -71,6 +71,8 @@
> >  	for_each_fbc_id((__display), (__fbc_id)) \
> >  		for_each_if((__fbc) = (__display)-
> > > fbc.instances[(__fbc_id)])
> >  
> > +#define FBC_SYS_CACHE_ID_NONE	I915_MAX_FBCS
> > +
> >  struct intel_fbc_funcs {
> >  	void (*activate)(struct intel_fbc *fbc);
> >  	void (*deactivate)(struct intel_fbc *fbc);
> > @@ -941,6 +943,69 @@ static void
> > intel_fbc_program_workarounds(struct
> > intel_fbc *fbc)
> >  		fbc_compressor_clkgate_disable_wa(fbc, true);
> >  }
> >  
> > +static void fbc_sys_cache_update_config(struct intel_display
> > *display, u32 reg,
> > +					enum intel_fbc_id id)
> > +{
> > +	if (!HAS_FBC_SYS_CACHE(display))
> > +		return;
> > +
> > +	lockdep_assert_held(&display->fbc.sys_cache.lock);
> > +
> > +	/* Cache read enable is set by default */
> > +	reg |= FBC_SYS_CACHE_READ_ENABLE;
> > +
> > +	intel_de_write(display, XE3P_LPD_FBC_SYS_CACHE_USAGE_CFG,
> > reg);
> > +
> > +	display->fbc.sys_cache.id = id;
> > +}
> > +
> > +static void fbc_sys_cache_disable(const struct intel_fbc *fbc)
> > +{
> > +	struct intel_display *display = fbc->display;
> > +	struct sys_cache_cfg *sys_cache = &display->fbc.sys_cache;
> 
> How about early return in here as well? :
> 
> if (!HAS_FBC_SYS_CACHE(display))
> 		return;

Here (sys_cache->id == fbc->id) check should handle the same condition
as sys_cache->id will never be fbc-id if FBC_SYS_CACHE feature is not
supported. Jani was suggesting this in his comments in the previous
iteration.  

> > +
> > +	mutex_lock(&sys_cache->lock);
> > +	/* clear only if "fbc" reserved the cache */
> > +	if (sys_cache->id == fbc->id)
> > +		fbc_sys_cache_update_config(display, 0,
> > FBC_SYS_CACHE_ID_NONE);
> > +	mutex_unlock(&sys_cache->lock);
> > +}
> > +
> > +static int fbc_sys_cache_limit(struct intel_display *display)
> > +{
> > +	/* Default 2MB for xe3p_lpd */
> 
> You could review all added comments in this patch and consider
> dropping
> if not all but at least some of them. E.g. Do we really need
> clarification saying 2 * 1024 * 1024 is 2MB? On the other hand you
> are
> saying xe3p_lpd but then checking display version below. How xe3p_lpd
> is related to display version 35?

Yeah.. my intention was to explain that magic number 2MB as the default
for NVL. But you are right I could remove some of those comments as
those are anyway found from bspec.

Thanks
Vinod

> 
> Anyways the patch looks ok to me:
> 
> Reviewed-by: Jouni Högander <jouni.hogander@intel.com>
> 
> > +	if (DISPLAY_VER(display) == 35)
> > +		return 2 * 1024 * 1024;
> > +
> > +	return 0;
> > +}
> > +
> > +static void fbc_sys_cache_enable(const struct intel_fbc *fbc)
> > +{
> > +	struct intel_display *display = fbc->display;
> > +	struct sys_cache_cfg *sys_cache = &display->fbc.sys_cache;
> > +	int range, offset;
> > +	u32 cfg;
> > +
> > +	if (!HAS_FBC_SYS_CACHE(display))
> > +		return;
> > +
> > +	/* limit to be configured to the register in 64k byte
> > chunks
> > */
> > +	range = fbc_sys_cache_limit(display) / (64 * 1024);
> > +
> > +	/* offset to be configured to the register in 4K byte
> > chunks
> > */
> > +	offset = i915_gem_stolen_node_offset(fbc->compressed_fb) /
> > (4 * 1024);
> > +
> > +	cfg = FBC_SYS_CACHE_TAG_USE_RES_SPACE |
> > FBC_SYS_CACHEABLE_RANGE(range) |
> > +	      FBC_SYS_CACHE_START_BASE(offset);
> > +
> > +	mutex_lock(&sys_cache->lock);
> > +	/* update sys cache config only if sys cache is unassigned
> > */
> > +	if (sys_cache->id == FBC_SYS_CACHE_ID_NONE)
> > +		fbc_sys_cache_update_config(display, cfg, fbc-
> > >id);
> > +	mutex_unlock(&sys_cache->lock);
> > +}
> > +
> >  static void __intel_fbc_cleanup_cfb(struct intel_fbc *fbc)
> >  {
> >  	if (WARN_ON(intel_fbc_hw_is_active(fbc)))
> > @@ -967,6 +1032,11 @@ void intel_fbc_cleanup(struct intel_display
> > *display)
> >  
> >  		kfree(fbc);
> >  	}
> > +
> > +	mutex_lock(&display->fbc.sys_cache.lock);
> > +	drm_WARN_ON(display->drm,
> > +		    display->fbc.sys_cache.id !=
> > FBC_SYS_CACHE_ID_NONE);
> > +	mutex_unlock(&display->fbc.sys_cache.lock);
> >  }
> >  
> >  static bool i8xx_fbc_stride_is_valid(const struct
> > intel_plane_state
> > *plane_state)
> > @@ -1780,6 +1850,8 @@ static void __intel_fbc_disable(struct
> > intel_fbc *fbc)
> >  
> >  	__intel_fbc_cleanup_cfb(fbc);
> >  
> > +	fbc_sys_cache_disable(fbc);
> > +
> >  	/* wa_18038517565 Enable DPFC clock gating after FBC
> > disable
> > */
> >  	if (display->platform.dg2 || DISPLAY_VER(display) >= 14)
> >  		fbc_compressor_clkgate_disable_wa(fbc, false);
> > @@ -1972,6 +2044,8 @@ static void __intel_fbc_enable(struct
> > intel_atomic_state *state,
> >  
> >  	intel_fbc_program_workarounds(fbc);
> >  	intel_fbc_program_cfb(fbc);
> > +
> > +	fbc_sys_cache_enable(fbc);
> >  }
> >  
> >  /**
> > @@ -2212,6 +2286,9 @@ void intel_fbc_init(struct intel_display
> > *display)
> >  
> >  	for_each_fbc_id(display, fbc_id)
> >  		display->fbc.instances[fbc_id] =
> > intel_fbc_create(display, fbc_id);
> > +
> > +	mutex_init(&display->fbc.sys_cache.lock);
> > +	display->fbc.sys_cache.id = FBC_SYS_CACHE_ID_NONE;
> >  }
> >  
> >  /**
> > @@ -2231,6 +2308,11 @@ void intel_fbc_sanitize(struct intel_display
> > *display)
> >  		if (intel_fbc_hw_is_active(fbc))
> >  			intel_fbc_hw_deactivate(fbc);
> >  	}
> > +
> > +	/* Ensure the sys cache usage config is clear as well */
> > +	mutex_lock(&display->fbc.sys_cache.lock);
> > +	fbc_sys_cache_update_config(display, 0,
> > FBC_SYS_CACHE_ID_NONE);
> > +	mutex_unlock(&display->fbc.sys_cache.lock);
> >  }
> >  
> >  static int intel_fbc_debugfs_status_show(struct seq_file *m, void
> > *unused)
> > @@ -2249,6 +2331,11 @@ static int
> > intel_fbc_debugfs_status_show(struct seq_file *m, void *unused)
> >  		seq_puts(m, "FBC enabled\n");
> >  		seq_printf(m, "Compressing: %s\n",
> >  			  
> > str_yes_no(intel_fbc_is_compressing(fbc)));
> > +
> > +		mutex_lock(&display->fbc.sys_cache.lock);
> > +		seq_printf(m, "Using system cache: %s\n",
> > +			   str_yes_no(display->fbc.sys_cache.id ==
> > fbc->id));
> > +		mutex_unlock(&display->fbc.sys_cache.lock);
> >  	} else {
> >  		seq_printf(m, "FBC disabled: %s\n", fbc-
> > > no_fbc_reason);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > index b1d0161a3196..d2d889fa4bed 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc_regs.h
> > @@ -126,4 +126,14 @@
> >  #define   FBC_REND_NUKE			REG_BIT(2)
> >  #define   FBC_REND_CACHE_CLEAN		REG_BIT(1)
> >  
> > +#define XE3P_LPD_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_R
> > ANGE_MASK,(range))
> > +#define   FBC_SYS_CACHE_TAG_MASK		REG_GENMASK(3, 2)
> > +#define  
> > FBC_SYS_CACHE_TAG_DONT_CACHE		REG_FIELD_PREP(FBC_SYS_CAC
> > HE_TAG_MASK, 0)
> > +#define  
> > FBC_SYS_CACHE_TAG_USE_RES_SPACE	REG_FIELD_PREP(FBC_SYS_CACHE_TAG_M
> > ASK,3)
> > +#define   FBC_SYS_CACHE_READ_ENABLE		REG_BIT(0)
> > +
> >  #endif /* __INTEL_FBC_REGS__ */
>