| Message ID | 20241111123259.1072534-2-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] drm/i915/watermark: Initialize latency variable to appropriate value | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Suraj > Kandpal > Sent: 11 November 2024 18:03 > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: [PATCH 2/3] drm/i915/watermark: Modify latency programmed into > PKG_C_LATENCY > > Increase the latency programmed into PKG_C_LATENCY latency to be a > multiple of line time which is written into WM_LINETIME. > > --v2 > -Fix commit subject line [Sai Teja] > -Use individual DISPLAY_VER checks instead of range [Sai Teja] -Initialize > max_linetime [Sai Teja] > > --v3 > -take into account the scenario when adjusted_latency is 0 [Vinod] > > WA: 22020299601 > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/i915/display/skl_watermark.c | 26 ++++++++++++++------ > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > b/drivers/gpu/drm/i915/display/skl_watermark.c > index a97e90ac643f..e061015a89b0 100644 > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > @@ -2848,9 +2848,11 @@ static int skl_wm_add_affected_planes(struct > intel_atomic_state *state, > * Program PKG_C_LATENCY Added Wake Time = 0 > */ > static void > -skl_program_dpkgc_latency(struct drm_i915_private *i915, bool > enable_dpkgc) > +skl_program_dpkgc_latency(struct drm_i915_private *i915, > + bool enable_dpkgc, > + u32 max_linetime) > { > - u32 max_latency = LNL_PKG_C_LATENCY_MASK; > + u32 adjusted_latency = LNL_PKG_C_LATENCY_MASK; > u32 clear = 0, val = 0; > u32 added_wake_time = 0; > > @@ -2858,15 +2860,22 @@ skl_program_dpkgc_latency(struct > drm_i915_private *i915, bool enable_dpkgc) > return; > > if (enable_dpkgc) { > - max_latency = skl_watermark_max_latency(i915, 1); > - if (max_latency == 0) > - max_latency = LNL_PKG_C_LATENCY_MASK; > + adjusted_latency = skl_watermark_max_latency(i915, 1); > + > + /* Wa_22020299601 */ > + if ((DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == 30) && > + adjusted_latency != 0) > + adjusted_latency = max_linetime * > + DIV_ROUND_UP(adjusted_latency, > max_linetime); > + else > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; You are already initialized it on the first instance, and wrote a patch #1 to get rid of duplicate of initialization. But why again ? Also any reason to move away from 'max_latency' to 'adjusted_latency' ? all I can read through your commit message is, you are making this latency as multiple of linetime, any adjustment we are making ? > + > 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_PKG_C_LATENCY_MASK, > adjusted_latency); Also you can think of this combine with below line for simplification ? > val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, > added_wake_time); > > intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); > @@ -2879,6 +2888,7 @@ skl_compute_wm(struct intel_atomic_state *state) > struct intel_crtc_state __maybe_unused *new_crtc_state; > int ret, i; > bool enable_dpkgc = false; > + u32 max_linetime = 0; > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > ret = skl_build_pipe_wm(state, crtc); @@ -2908,9 +2918,11 > @@ skl_compute_wm(struct intel_atomic_state *state) > new_crtc_state->vrr.vmin == new_crtc_state->vrr.flipline) > || > !new_crtc_state->vrr.enable) > enable_dpkgc = true; > + > + max_linetime = max(new_crtc_state->linetime, > max_linetime); > } > > - skl_program_dpkgc_latency(to_i915(state->base.dev), > enable_dpkgc); > + skl_program_dpkgc_latency(to_i915(state->base.dev), enable_dpkgc, > +max_linetime); > > skl_print_wm_changes(state); > > -- > 2.34.1
> -----Original Message----- > From: Golani, Mitulkumar Ajitkumar > <mitulkumar.ajitkumar.golani@intel.com> > Sent: Monday, November 11, 2024 6:56 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org; > intel-gfx@lists.freedesktop.org > Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: RE: [PATCH 2/3] drm/i915/watermark: Modify latency programmed > into PKG_C_LATENCY > > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Suraj Kandpal > > Sent: 11 November 2024 18:03 > > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Kandpal, > > Suraj <suraj.kandpal@intel.com> > > Subject: [PATCH 2/3] drm/i915/watermark: Modify latency programmed > > into PKG_C_LATENCY > > > > Increase the latency programmed into PKG_C_LATENCY latency to be a > > multiple of line time which is written into WM_LINETIME. > > > > --v2 > > -Fix commit subject line [Sai Teja] > > -Use individual DISPLAY_VER checks instead of range [Sai Teja] > > -Initialize max_linetime [Sai Teja] > > > > --v3 > > -take into account the scenario when adjusted_latency is 0 [Vinod] > > > > WA: 22020299601 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/skl_watermark.c | 26 > > ++++++++++++++------ > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > > b/drivers/gpu/drm/i915/display/skl_watermark.c > > index a97e90ac643f..e061015a89b0 100644 > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > @@ -2848,9 +2848,11 @@ static int skl_wm_add_affected_planes(struct > > intel_atomic_state *state, > > * Program PKG_C_LATENCY Added Wake Time = 0 > > */ > > static void > > -skl_program_dpkgc_latency(struct drm_i915_private *i915, bool > > enable_dpkgc) > > +skl_program_dpkgc_latency(struct drm_i915_private *i915, > > + bool enable_dpkgc, > > + u32 max_linetime) > > { > > - u32 max_latency = LNL_PKG_C_LATENCY_MASK; > > + u32 adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > u32 clear = 0, val = 0; > > u32 added_wake_time = 0; > > > > @@ -2858,15 +2860,22 @@ skl_program_dpkgc_latency(struct > > drm_i915_private *i915, bool enable_dpkgc) > > return; > > > > if (enable_dpkgc) { > > - max_latency = skl_watermark_max_latency(i915, 1); > > - if (max_latency == 0) > > - max_latency = LNL_PKG_C_LATENCY_MASK; > > + adjusted_latency = skl_watermark_max_latency(i915, 1); > > + > > + /* Wa_22020299601 */ > > + if ((DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == 30) && > > + adjusted_latency != 0) > > + adjusted_latency = max_linetime * > > + DIV_ROUND_UP(adjusted_latency, > > max_linetime); > > + else > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > You are already initialized it on the first instance, and wrote a patch #1 to get > rid of duplicate of initialization. But why again ? The reason i added the first patch to remove the else block for enable dpkgc variable which tells us if it's fixed refresh rate or not. In the second patch we add that else block to make sure that the wa is not applied in cases adjusted latency is 0. Also adjusted_latency = skl_watermark_max_latency(i915, 1); Makes it so we will reassign adjusted latency which may become 0 too due to which the above else block was added. > Also any reason to move away from 'max_latency' to 'adjusted_latency' ? > all I can read through your commit message is, you are making this latency as > multiple of linetime, any adjustment we are making ? Yes original value would have been the simple max latency now the new value is linetime * ceil(max latency/linetime) And if we are not taking the wa into picture we still program the max latency. Also the idea of keeping it adjusted latency is that we write to the pkgc_latency register's Latency bits. But we can rename it to latency if it seems confusing. > > > + > > 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_PKG_C_LATENCY_MASK, > > adjusted_latency); > > Also you can think of this combine with below line for simplification ? Sure ill send this refactor as a separate patch. Regards, Suraj Kandpal > > > val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, > > added_wake_time); > > > > intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); > @@ > > -2879,6 +2888,7 @@ skl_compute_wm(struct intel_atomic_state *state) > > struct intel_crtc_state __maybe_unused *new_crtc_state; > > int ret, i; > > bool enable_dpkgc = false; > > + u32 max_linetime = 0; > > > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > > ret = skl_build_pipe_wm(state, crtc); @@ -2908,9 +2918,11 > @@ > > skl_compute_wm(struct intel_atomic_state *state) > > new_crtc_state->vrr.vmin == new_crtc_state->vrr.flipline) > > || > > !new_crtc_state->vrr.enable) > > enable_dpkgc = true; > > + > > + max_linetime = max(new_crtc_state->linetime, > > max_linetime); > > } > > > > - skl_program_dpkgc_latency(to_i915(state->base.dev), > > enable_dpkgc); > > + skl_program_dpkgc_latency(to_i915(state->base.dev), enable_dpkgc, > > +max_linetime); > > > > skl_print_wm_changes(state); > > > > -- > > 2.34.1
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index a97e90ac643f..e061015a89b0 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -2848,9 +2848,11 @@ static int skl_wm_add_affected_planes(struct intel_atomic_state *state, * Program PKG_C_LATENCY Added Wake Time = 0 */ static void -skl_program_dpkgc_latency(struct drm_i915_private *i915, bool enable_dpkgc) +skl_program_dpkgc_latency(struct drm_i915_private *i915, + bool enable_dpkgc, + u32 max_linetime) { - u32 max_latency = LNL_PKG_C_LATENCY_MASK; + u32 adjusted_latency = LNL_PKG_C_LATENCY_MASK; u32 clear = 0, val = 0; u32 added_wake_time = 0; @@ -2858,15 +2860,22 @@ skl_program_dpkgc_latency(struct drm_i915_private *i915, bool enable_dpkgc) return; if (enable_dpkgc) { - max_latency = skl_watermark_max_latency(i915, 1); - if (max_latency == 0) - max_latency = LNL_PKG_C_LATENCY_MASK; + adjusted_latency = skl_watermark_max_latency(i915, 1); + + /* Wa_22020299601 */ + if ((DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == 30) && + adjusted_latency != 0) + adjusted_latency = max_linetime * + DIV_ROUND_UP(adjusted_latency, max_linetime); + else + adjusted_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_PKG_C_LATENCY_MASK, adjusted_latency); val |= REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK, added_wake_time); intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val); @@ -2879,6 +2888,7 @@ skl_compute_wm(struct intel_atomic_state *state) struct intel_crtc_state __maybe_unused *new_crtc_state; int ret, i; bool enable_dpkgc = false; + u32 max_linetime = 0; for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { ret = skl_build_pipe_wm(state, crtc); @@ -2908,9 +2918,11 @@ skl_compute_wm(struct intel_atomic_state *state) new_crtc_state->vrr.vmin == new_crtc_state->vrr.flipline) || !new_crtc_state->vrr.enable) enable_dpkgc = true; + + max_linetime = max(new_crtc_state->linetime, max_linetime); } - skl_program_dpkgc_latency(to_i915(state->base.dev), enable_dpkgc); + skl_program_dpkgc_latency(to_i915(state->base.dev), enable_dpkgc, max_linetime); skl_print_wm_changes(state);
Increase the latency programmed into PKG_C_LATENCY latency to be a multiple of line time which is written into WM_LINETIME. --v2 -Fix commit subject line [Sai Teja] -Use individual DISPLAY_VER checks instead of range [Sai Teja] -Initialize max_linetime [Sai Teja] --v3 -take into account the scenario when adjusted_latency is 0 [Vinod] WA: 22020299601 Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/skl_watermark.c | 26 ++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-)