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 <[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; 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 <[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_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__ */ >
