Re: [PATCH] drm/i914/watermark: Modify latency programmed into PKG_C_LATENCY

["Pottumuttu, Sai Teja" <sai.teja.pottumuttu@xxxxxxxxx>]

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@xxxxxxxxx>
> *Sent:* Friday, November 8, 2024 12:20 PM
> *To:* Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel- 
> xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> *Cc:* Govindapillai, Vinod <vinod.govindapillai@xxxxxxxxx>
> *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@xxxxxxxxx> <mailto:suraj.kandpal@xxxxxxxxx>
>
>     ---
>
>       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);
>
>