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

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

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);
> 
>       
>