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? > > 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 <[email protected]> > --- > .../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; > + > + 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? Anyways the patch looks ok to me: Reviewed-by: Jouni Högander <[email protected]> > + 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_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) > +#define FBC_SYS_CACHE_READ_ENABLE REG_BIT(0) > + > #endif /* __INTEL_FBC_REGS__ */
