| Message ID | 20241108085048.919665-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/watermark: Modify latency programmed into PKG_C_LATENCY | expand |
Hi Suraj, Some comments, please have a look.. On Fri, 2024-11-08 at 14:20 +0530, Suraj Kandpal wrote: > > 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] > > > > WA: 22020299601 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/skl_watermark.c | 26 ++++++++++++++------ > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > > > b/drivers/gpu/drm/i915/display/skl_watermark.c > > index d3bbf335c749..9e208db55abb 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 = 0; > > + u32 adjusted_latency = 0; I guess, if you initialize adjusted_latency to LNL_PKG_C_LATENCY_MASK, you could get rid of the else branch below. May be you could have a preparatory patch which get rid of this else branch? > > u32 clear = 0, val = 0; > > u32 added_wake_time = 0; > > > > @@ -2858,18 +2860,23 @@ 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); > > + if (adjusted_latency == 0) > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > + Also, here I guess you need to avoid this WA if adjusted_latency is 0 ie. invalid level 1 latency. Also use intel_display instead of i915 wherever possible related to this change. > > + /* Wa_22020299601 */ > > + if (DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == 30) > > + adjusted_latency = max_linetime * > > + DIV_ROUND_UP(adjusted_latency, max_linetime); > > added_wake_time = DSB_EXE_TIME + > > i915->display.sagv.block_time_us; > > } else { > > - max_latency = LNL_PKG_C_LATENCY_MASK; > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > added_wake_time = 0; > > } > > > > 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); > > @@ -2882,6 +2889,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); > > @@ -2911,9 +2919,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); > >
> -----Original Message----- > From: Govindapillai, Vinod <vinod.govindapillai@intel.com> > Sent: Monday, November 11, 2024 4:29 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org; > intel-gfx@lists.freedesktop.org > Cc: Pottumuttu, Sai Teja <sai.teja.pottumuttu@intel.com> > Subject: Re: [PATCH] drm/i915/watermark: Modify latency programmed into > PKG_C_LATENCY > > Hi Suraj, > > Some comments, please have a look.. > > On Fri, 2024-11-08 at 14:20 +0530, Suraj Kandpal wrote: > > > 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] > > > > > > WA: 22020299601 > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/skl_watermark.c | 26 > > > ++++++++++++++------ > > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > b/drivers/gpu/drm/i915/display/skl_watermark.c > > > index d3bbf335c749..9e208db55abb 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 = 0; > > > + u32 adjusted_latency = 0; > I guess, if you initialize adjusted_latency to LNL_PKG_C_LATENCY_MASK, you > could get rid of the else branch below. May be you could have a preparatory > patch which get rid of this else branch? You're right will get that done. > > > > u32 clear = 0, val = 0; > > > u32 added_wake_time = 0; > > > > > > @@ -2858,18 +2860,23 @@ 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); > > > + if (adjusted_latency == 0) > > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > > + > Also, here I guess you need to avoid this WA if adjusted_latency is 0 ie. invalid > level 1 latency. Sure will get this fixed > > Also use intel_display instead of i915 wherever possible related to this > change. Sure you are right ill update the function itself to use intel_de_rmw instead on uncore_rmw too will have a separate patch for it. Regards, Suraj Kandpal > > > > + /* Wa_22020299601 */ > > > + if (DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == > > > +30) > > > + adjusted_latency = max_linetime * > > > + DIV_ROUND_UP(adjusted_latency, > > > +max_linetime); > > > added_wake_time = DSB_EXE_TIME + > > > i915->display.sagv.block_time_us; > > > } else { > > > - max_latency = LNL_PKG_C_LATENCY_MASK; > > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > > added_wake_time = 0; > > > } > > > > > > 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); @@ -2882,6 +2889,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); @@ -2911,9 > > > +2919,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); > > >
> -----Original Message----- > From: Govindapillai, Vinod <vinod.govindapillai@intel.com> > Sent: Monday, November 11, 2024 4:29 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org; > intel-gfx@lists.freedesktop.org > Cc: Pottumuttu, Sai Teja <sai.teja.pottumuttu@intel.com> > Subject: Re: [PATCH] drm/i915/watermark: Modify latency programmed into > PKG_C_LATENCY > > Hi Suraj, > > Some comments, please have a look.. > > On Fri, 2024-11-08 at 14:20 +0530, Suraj Kandpal wrote: > > > 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] > > > > > > WA: 22020299601 > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/skl_watermark.c | 26 > > > ++++++++++++++------ > > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > b/drivers/gpu/drm/i915/display/skl_watermark.c > > > index d3bbf335c749..9e208db55abb 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 = 0; > > > + u32 adjusted_latency = 0; > I guess, if you initialize adjusted_latency to LNL_PKG_C_LATENCY_MASK, you > could get rid of the else branch below. May be you could have a preparatory > patch which get rid of this else branch? Just one issue here we assign and reassign this value when we call skl_watermark_max_latency() which Can also make adjusted_latency 0 so I still don’t see the else branch being eliminated. Regards, Suraj Kandpal > > > > u32 clear = 0, val = 0; > > > u32 added_wake_time = 0; > > > > > > @@ -2858,18 +2860,23 @@ 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); > > > + if (adjusted_latency == 0) > > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > > + > Also, here I guess you need to avoid this WA if adjusted_latency is 0 ie. invalid > level 1 latency. > > Also use intel_display instead of i915 wherever possible related to this > change. > > > > + /* Wa_22020299601 */ > > > + if (DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == > > > +30) > > > + adjusted_latency = max_linetime * > > > + DIV_ROUND_UP(adjusted_latency, > > > +max_linetime); > > > added_wake_time = DSB_EXE_TIME + > > > i915->display.sagv.block_time_us; > > > } else { > > > - max_latency = LNL_PKG_C_LATENCY_MASK; > > > + adjusted_latency = LNL_PKG_C_LATENCY_MASK; > > > added_wake_time = 0; > > > } > > > > > > 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); @@ -2882,6 +2889,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); @@ -2911,9 > > > +2919,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); > > >
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index d3bbf335c749..9e208db55abb 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 = 0; + u32 adjusted_latency = 0; u32 clear = 0, val = 0; u32 added_wake_time = 0; @@ -2858,18 +2860,23 @@ 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); + if (adjusted_latency == 0) + adjusted_latency = LNL_PKG_C_LATENCY_MASK; + + /* Wa_22020299601 */ + if (DISPLAY_VER(i915) == 20 || DISPLAY_VER(i915) == 30) + adjusted_latency = max_linetime * + DIV_ROUND_UP(adjusted_latency, max_linetime); added_wake_time = DSB_EXE_TIME + i915->display.sagv.block_time_us; } else { - max_latency = LNL_PKG_C_LATENCY_MASK; + adjusted_latency = LNL_PKG_C_LATENCY_MASK; added_wake_time = 0; } 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); @@ -2882,6 +2889,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); @@ -2911,9 +2919,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] WA: 22020299601 Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/skl_watermark.c | 26 ++++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-)