From 6bc41f9cf252385d3a24e63ce6e2c955dd35c0b2 Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Wed, 13 Dec 2023 12:25:19 +0200 Subject: Revert "drm/i915/xe2lpd: Treat cursor plane as regular plane for DDB allocation" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28. A core design consideration with legacy cursor updates is that the cursor must not touch any other plane, even if we were to force it to take the slow path. That is the real reason why the cursor uses a fixed ddb allocation, not because bspec says so. Treating cursors as any other plane during ddb allocation violates that, which means we can now pull other planes into fully unsynced legacy cursor mailbox commits. That is definitely not something we've ever considered when designing the rest of the code. The noarm+arm register write split in particular makes that dangerous as previous updates can get disarmed pretty much at any random time, and not necessarily in an order that is actually safe (eg. against ddb overlaps). So if we were to do this then: - someone needs to expend the appropriate amount of brain cells thinking through all the tricky details - we should do it for all skl+ platforms since all of those have double buffered wm/ddb registers. The current arbitrary mtl+ cutoff doesn't really make sense For the moment just go back to the original behaviour where the cursor's ddb alloation does not change outside of modeset/fastset. As of now anything else isn't safe. Cc: Stanislav Lisovskiy Cc: Matt Roper Cc: Lucas De Marchi Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20231213102519.13500-10-ville.syrjala@linux.intel.com Reviewed-by: Stanislav Lisovskiy Reviewed-by: Uma Shankar --- drivers/gpu/drm/i915/display/skl_watermark.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) (limited to 'drivers/gpu/drm/i915/display/skl_watermark.c') diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index 56588d6e24ae..051a02ac01a4 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -1367,7 +1367,7 @@ skl_total_relative_data_rate(const struct intel_crtc_state *crtc_state) u64 data_rate = 0; for_each_plane_id_on_crtc(crtc, plane_id) { - if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) + if (plane_id == PLANE_CURSOR) continue; data_rate += crtc_state->rel_data_rate[plane_id]; @@ -1514,12 +1514,10 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state, return 0; /* Allocate fixed number of blocks for cursor. */ - if (DISPLAY_VER(i915) < 20) { - cursor_size = skl_cursor_allocation(crtc_state, num_active); - iter.size -= cursor_size; - skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR], - alloc->end - cursor_size, alloc->end); - } + cursor_size = skl_cursor_allocation(crtc_state, num_active); + iter.size -= cursor_size; + skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR], + alloc->end - cursor_size, alloc->end); iter.data_rate = skl_total_relative_data_rate(crtc_state); @@ -1533,7 +1531,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state, const struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id]; - if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) { + if (plane_id == PLANE_CURSOR) { const struct skl_ddb_entry *ddb = &crtc_state->wm.skl.plane_ddb[plane_id]; @@ -1581,7 +1579,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state, const struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id]; - if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) + if (plane_id == PLANE_CURSOR) continue; if (DISPLAY_VER(i915) < 11 && -- cgit v1.2.3 From 1e41fa9452039beea297105fb6f7f68cb2774e1a Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Tue, 19 Dec 2023 15:07:54 +0200 Subject: drm/i915: Compute use_sagv_wm differently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit drm_atomic_check_only() gets upset if we try to add extra crtcs to any commit that isn't flagged with DRM_MODE_ATOMIC_ALLOW_MODESET. This conflicts with how SAGV watermarks work on pre-ADL as we need to manually switch over the SAGV watermarks before we can safely enable SAGV. So in order to make SAGV usage possible we need to compute each pipe's use of SAGV watermarks as if there aren't any other active pipes. Ie. if the current pipe isn't the one blocking SAGV then we make it use the SAGV watermarks, even if some other pipe prevents SAGV from actually being used. Otherwise we could end up with a pipes using the normal watermarks (but not blocking SAGV), and some other pipe in parallel enabling SAGV, which would likely cause underruns. The alternative approach of preventing SAGV usage until all pipes simultanously end up using SAGV watermarks would only really work if userspace always adds all pipes to every commits, which isn't the case typically. The downside of this is that we will end up using the less optimal SAGV watermarks even if some other pipe prevents SAGV from actually being enabled. In which case the system won't achieve the minimum possible power consumption. Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20231219130756.25986-2-ville.syrjala@linux.intel.com Reviewed-by: Stanislav Lisovskiy --- drivers/gpu/drm/i915/display/skl_watermark.c | 38 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 15 deletions(-) (limited to 'drivers/gpu/drm/i915/display/skl_watermark.c') diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index 051a02ac01a4..614f319d754e 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -443,12 +443,35 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; + new_bw_state = intel_atomic_get_bw_state(state); if (IS_ERR(new_bw_state)) return PTR_ERR(new_bw_state); old_bw_state = intel_atomic_get_old_bw_state(state); + /* + * We store use_sagv_wm in the crtc state rather than relying on + * that bw state since we have no convenient way to get at the + * latter from the plane commit hooks (especially in the legacy + * cursor case). + * + * drm_atomic_check_only() gets upset if we pull more crtcs + * into the state, so we have to calculate this based on the + * individual intel_crtc_can_enable_sagv() rather than + * the overall intel_can_enable_sagv(). Otherwise the + * crtcs not included in the commit would not switch to the + * SAGV watermarks when we are about to enable SAGV, and that + * would lead to underruns. This does mean extra power draw + * when only a subset of the crtcs are blocking SAGV as the + * other crtcs can't be allowed to use the more optimal + * normal (ie. non-SAGV) watermarks. + */ + pipe_wm->use_sagv_wm = !HAS_HW_SAGV_WM(i915) && + DISPLAY_VER(i915) >= 12 && + intel_crtc_can_enable_sagv(new_crtc_state); + if (intel_crtc_can_enable_sagv(new_crtc_state)) new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); else @@ -478,21 +501,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) return ret; } - for_each_new_intel_crtc_in_state(state, crtc, - new_crtc_state, i) { - struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; - - /* - * We store use_sagv_wm in the crtc state rather than relying on - * that bw state since we have no convenient way to get at the - * latter from the plane commit hooks (especially in the legacy - * cursor case) - */ - pipe_wm->use_sagv_wm = !HAS_HW_SAGV_WM(i915) && - DISPLAY_VER(i915) >= 12 && - intel_can_enable_sagv(i915, new_bw_state); - } - return 0; } -- cgit v1.2.3 From 131288c4681bbc2727f20c4b31c89a93464aa9ca Mon Sep 17 00:00:00 2001 From: Suraj Kandpal Date: Mon, 19 Feb 2024 12:06:38 +0530 Subject: drm/i915/lnl: Program PKGC_LATENCY register If fixed refresh rate program the PKGC_LATENCY register with the highest latency from level 1 and above LP registers and program ADDED_WAKE_TIME = DSB execution time. else program PKGC_LATENCY with all 1's and ADDED_WAKE_TIME as 0. This is used to improve package C residency by sending the highest latency tolerance requirement (LTR) when the planes are done with the frame until the next frame programming window (set context latency, window 2) starts. Bspec: 68986 --v2 -Fix indentation [Chaitanya] --v3 -Take into account if fixed refrersh rate or not [Vinod] -Added wake time dependengt on DSB execution time [Vinod] -Use REG_FIELD_PREP [Jani] -Call program_pkgc_latency from appropriate place [Jani] -no need for the ~0 while setting max latency [Jani] -change commit message to add the new changes made in. --v4 -Remove extra blank line [Vinod] -move the vrr.enable check to previous loop [Vinod] Signed-off-by: Suraj Kandpal Reviewed-by: Chaitanya Kumar Borah Reviewed-by: Vinod Govindapillai Signed-off-by: Animesh Manna Link: https://patchwork.freedesktop.org/patch/msgid/20240219063638.1467114-1-suraj.kandpal@intel.com --- drivers/gpu/drm/i915/display/intel_dsb.c | 2 +- drivers/gpu/drm/i915/display/skl_watermark.c | 54 ++++++++++++++++++++++++++-- drivers/gpu/drm/i915/display/skl_watermark.h | 4 +-- 3 files changed, 55 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/i915/display/skl_watermark.c') diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c index a6c7122fd671..d62e050185e7 100644 --- a/drivers/gpu/drm/i915/display/intel_dsb.c +++ b/drivers/gpu/drm/i915/display/intel_dsb.c @@ -325,7 +325,7 @@ static int intel_dsb_dewake_scanline(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode; - unsigned int latency = skl_watermark_max_latency(i915); + unsigned int latency = skl_watermark_max_latency(i915, 0); int vblank_start; if (crtc_state->vrr.enable) { diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index 614f319d754e..c6b9be80d83c 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -23,6 +23,12 @@ #include "skl_watermark.h" #include "skl_watermark_regs.h" +/*It is expected that DSB can do posted writes to every register in + * the pipe and planes within 100us. For flip queue use case, the + * recommended DSB execution time is 100us + one SAGV block time. + */ +#define DSB_EXE_TIME 100 + static void skl_sagv_disable(struct drm_i915_private *i915); /* Stores plane specific WM parameters */ @@ -2904,12 +2910,51 @@ static int skl_wm_add_affected_planes(struct intel_atomic_state *state, return 0; } +/* + * If Fixed Refresh Rate: + * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from + * watermark level1 and up and above. If watermark level 1 is + * invalid program it with all 1's. + * Program PKG_C_LATENCY Added Wake Time = DSB execution time + * If Variable Refresh Rate: + * Program DEEP PKG_C_LATENCY Pkg C with all 1's. + * Program PKG_C_LATENCY Added Wake Time = 0 + */ +static void +skl_program_dpkgc_latency(struct drm_i915_private *i915, bool vrr_enabled) +{ + u32 max_latency = 0; + u32 clear = 0, val = 0; + u32 added_wake_time = 0; + + if (DISPLAY_VER(i915) < 20) + return; + + if (vrr_enabled) { + max_latency = LNL_PKG_C_LATENCY_MASK; + added_wake_time = 0; + } else { + max_latency = skl_watermark_max_latency(i915, 1); + if (max_latency == 0) + max_latency = LNL_PKG_C_LATENCY_MASK; + added_wake_time = DSB_EXE_TIME + + i915->display.sagv.block_time_us; + } + + clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK; + val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK, max_latency); + val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time); + + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); +} + static int skl_compute_wm(struct intel_atomic_state *state) { struct intel_crtc *crtc; struct intel_crtc_state __maybe_unused *new_crtc_state; int ret, i; + bool vrr_enabled = false; for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { ret = skl_build_pipe_wm(state, crtc); @@ -2934,8 +2979,13 @@ skl_compute_wm(struct intel_atomic_state *state) ret = skl_wm_add_affected_planes(state, crtc); if (ret) return ret; + + if (new_crtc_state->vrr.enable) + vrr_enabled = true; } + skl_program_dpkgc_latency(to_i915(state->base.dev), vrr_enabled); + skl_print_wm_changes(state); return 0; @@ -3731,11 +3781,11 @@ void skl_watermark_debugfs_register(struct drm_i915_private *i915) &intel_sagv_status_fops); } -unsigned int skl_watermark_max_latency(struct drm_i915_private *i915) +unsigned int skl_watermark_max_latency(struct drm_i915_private *i915, int initial_wm_level) { int level; - for (level = i915->display.wm.num_levels - 1; level >= 0; level--) { + for (level = i915->display.wm.num_levels - 1; level >= initial_wm_level; level--) { unsigned int latency = skl_wm_latency(i915, level, NULL); if (latency) diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h index fb0da36fd3ec..e3d1d74a7b17 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.h +++ b/drivers/gpu/drm/i915/display/skl_watermark.h @@ -46,8 +46,8 @@ void skl_watermark_ipc_update(struct drm_i915_private *i915); bool skl_watermark_ipc_enabled(struct drm_i915_private *i915); void skl_watermark_debugfs_register(struct drm_i915_private *i915); -unsigned int skl_watermark_max_latency(struct drm_i915_private *i915); - +unsigned int skl_watermark_max_latency(struct drm_i915_private *i915, + int initial_wm_level); void skl_wm_init(struct drm_i915_private *i915); struct intel_dbuf_state { -- cgit v1.2.3