| Message ID | 20241107113240.887316-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i914/watermark: Modify latency programmed into PKG_C_LATENCY | expand |
On 07-11-2024 17:02, 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. The commit subject prefix should be drm/i915/watermark (its i914 currently) > 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..856b20a683fd 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) Nit: This ^ argument can fit in the line before. > { > - 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 (IS_DISPLAY_VERx100(i915, 2000, 3000)) This wa is applicable only to IP versions 2000 and 3000. So, shouldn't we be limiting this to only those IP versions instead of the full range? > + 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; max_linetime should be initialized to 0 I believe so that the max comparison works correctly below. Thanks Sai Teja > > 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); >
Hi Teja, I have revised this patch based on your feedback but please don’t reply/comment on patch series in the below format we don’t really support it this also makes the subsequent replies to you comment come in the same format. Please make sure you use the option to include the original message prefixed with a “>” instead of a space. Regards, Suraj Kandpal From: Pottumuttu, Sai Teja <sai.teja.pottumuttu@intel.com> Sent: Friday, November 8, 2024 12:20 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> Subject: Re: [PATCH] drm/i914/watermark: Modify latency programmed into PKG_C_LATENCY On 07-11-2024 17:02, 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. The commit subject prefix should be drm/i915/watermark (its i914 currently) WA: 22020299601 Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com><mailto: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..856b20a683fd 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) Nit: This ^ argument can fit in the line before. { - 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 (IS_DISPLAY_VERx100(i915, 2000, 3000)) This wa is applicable only to IP versions 2000 and 3000. So, shouldn't we be limiting this to only those IP versions instead of the full range? + 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; max_linetime should be initialized to 0 I believe so that the max comparison works correctly below. Thanks Sai Teja 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);
On 11-11-2024 08:49, Kandpal, Suraj wrote: > Hi Teja, > > I have revised this patch based on your feedback but please don’t reply/ > comment on patch series in the below format we don’t really support it > this also makes the subsequent replies to you comment come in the same > format. Please make sure you use the option to include the original > message prefixed with a “>” instead of a space. Hi Suraj, Thanks for pointing it out. Sorry for the wrong format. I am not really sure how did it come in that format for this patch as my other reviews went in with the ">" prefix only. Thanks for the v2, I will take a look. For others reference, leaving a link to v2 here (as its a different series now): https://patchwork.freedesktop.org/series/141091/ Regards, Sai Teja > > Regards, > Suraj Kandpal > > *From:*Pottumuttu, Sai Teja <sai.teja.pottumuttu@intel.com> > *Sent:* Friday, November 8, 2024 12:20 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> > *Subject:* Re: [PATCH] drm/i914/watermark: Modify latency programmed > into PKG_C_LATENCY > > On 07-11-2024 17:02, 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. > > The commit subject prefix should be drm/i915/watermark (its i914 currently) > > WA: 22020299601 > > Signed-off-by: Suraj Kandpal<suraj.kandpal@intel.com> <mailto: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..856b20a683fd 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) > > Nit: This ^ argument can fit in the line before. > > { > > - 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 (IS_DISPLAY_VERx100(i915, 2000, 3000)) > > This wa is applicable only to IP versions 2000 and 3000. So, shouldn't > we be limiting this to only those IP versions instead of the full range? > > + 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; > > max_linetime should be initialized to 0 I believe so that the max > comparison works correctly below. > > Thanks > Sai Teja > > > > 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..856b20a683fd 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 (IS_DISPLAY_VERx100(i915, 2000, 3000)) + 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; 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. 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(-)