Hi Suraj,
+Ville - please comment if you see anything wrong with my explanation here
I think there could be a problem with the current logic and a possibility for underrun. As per the
bspec we need to configure highest latency to soc.
As per this patch, skl_program_dpkgc_latency() is called from skl_compute_wm() and
skl_program_dpkgc_latency() uses skl_watermark_max_latency() to get the maximum memory latency. As
per the current implementation this skl_watermark_max_latency() always return latency from the
highest level.
I think, that might not be the right approach. In skl_compute_wm() we calculate the basic wm levels
using skl_build_pipe_wm(). Then based on the ddb availability these initial wm levels are adjusted
again and some of the higher wm levels are pruned (skl_compute_ddb()). As far as I understood from
the purpose of the bspec 68986, we need to configure the latency of corresponding the available wm
level.
These wm levels are plane specific. So to find the best - highest wm levels used, you have to
iterate through all the planes and find out the "least" of highest enabled wm levels in these
planes. Then use the corresponding memory latency of that level to program this register.
Otherwise you might end up programming a higher latency value than what is used and can cause a FIFO
underrun I think.
In case of async flip cases, minimal wm0 is used and that makes wm level 1 to above not enabled and
in that case you need to set all 1's to this register. This is what I meant previously. This async
flip case will be covered automatically if the logic as per the above comments.
BR
Vinod
On Tue, 2024-02-13 at 11:58 +0530, Suraj Kandpal wrote:
> 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.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_dsb.c | 2 +-
> drivers/gpu/drm/i915/display/skl_watermark.c | 59 +++++++++++++++++++-
> drivers/gpu/drm/i915/display/skl_watermark.h | 4 +-
> 3 files changed, 60 insertions(+), 5 deletions(-)
>
> 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..1d423ce0f42d 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);
> @@ -2936,6 +2981,15 @@ skl_compute_wm(struct intel_atomic_state *state)
> return ret;
> }
>
> + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> + if (new_crtc_state->vrr.enable) {
> + vrr_enabled = true;
> + break;
can't we combine this to the previous crtc state iteration loop something like
vrr_enabled |= new_crtc_state->vrr.enable
> + }
> + }
> +
> + skl_program_dpkgc_latency(to_i915(state->base.dev), vrr_enabled);
> +
> skl_print_wm_changes(state);
>
> return 0;
> @@ -3415,6 +3469,7 @@ static void skl_setup_wm_latency(struct drm_i915_private *i915)
> skl_read_wm_latency(i915, i915->display.wm.skl_latency);
>
> intel_print_wm_latency(i915, "Gen9 Plane", i915->display.wm.skl_latency);
> +
extra blank line
> }
>
> static const struct intel_wm_funcs skl_wm_funcs = {
> @@ -3731,11 +3786,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 {
