> -----Original Message-----
> From: Govindapillai, Vinod <vinod.govindapillai@xxxxxxxxx>
> Sent: Wednesday, November 13, 2024 3:43 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
> intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>
> Subject: Re: [PATCH 5/6] drm/i915/watermark: Break and move DPKGC
> latency programming function
>
> Hi Suraj,
>
> I guess, patches 1 to 5 can be squashed into one as "Refactoring of dpkgc
> handling" as the whole dpkgc handling is being changed!!
>
Sure
> Bspec reference would be useful.
>
> Now coming to this patch,
>
> And then I think, probably there is no benefit in declaring dpkgc_latency,
> dpkgc_added_waketime as a way to store those during atomic_check as no
> global object association or any comparisons with the previous values are
> being done. So the split of intel_wm_compute_dpkgc_latency() and
> intel_wm_write_dpkgc_latency might not be needed.
>
> Instead you could just have "intel_program_dpkgc_latency() where you
> compute and program the values at once and get it called from
> intel_atomic_commit_tail() may be just after commit_modeset_enables().
> Also not sure if "wm" refrence is needed as it is not directly relate to wm.
>
Sure I can do that .
@Syrjala, Ville do you think what Vinod suggested would be okay to go with
Regards,
Suraj Kandpal
> @Ville, could you please comment?
>
>
>
> On Tue, 2024-11-12 at 14:15 +0530, Suraj Kandpal wrote:
> > We break our dpkgc functions into two parts computation and writing to
> > its respective register. This is for mainly due to the reason stated
> > below
> > - We do not want to compute and write to the register in the compute
> > config phase itself.
> > - We want to make sure we have all the required values specially
> > linetime which is computed after intel_wm_compute, this will also help
> > implement some WA's which require linetime.
> > We also move it from its initial place because of the first point
> > state above.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 7 ++
> > .../gpu/drm/i915/display/intel_display_core.h | 3 +
> > drivers/gpu/drm/i915/display/intel_wm.c | 71
> > +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_wm.h | 4 ++
> > drivers/gpu/drm/i915/display/skl_watermark.c | 50 -------------
> > 5 files changed, 85 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 46f6643d8d0f..484681f4e023 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4600,6 +4600,10 @@ static int intel_crtc_atomic_check(struct
> > intel_atomic_state *state,
> >
> > }
> >
> > + ret = intel_wm_compute_dpkgc_latency(state, dev_priv);
> > + if (ret)
> > + return ret;
> > +
> > ret = intel_psr2_sel_fetch_update(state, crtc);
> > if (ret)
> > return ret;
> > @@ -7124,6 +7128,9 @@ static void commit_pipe_post_planes(struct
> > intel_atomic_state *state,
> >
> > if (intel_crtc_vrr_enabling(state, crtc))
> > intel_vrr_enable(new_crtc_state);
> > +
> > + if (DISPLAY_VER(dev_priv) >= 20)
> > + intel_wm_write_dpkgc_latency(to_intel_display(crtc));
>
> In any case I think this is not the right place to call this as this will be called
> per crtc and also I see commit_pipe_post_planes() is based on "use_dsb".
>
> > }
> >
> > static void intel_enable_crtc(struct intel_atomic_state *state, diff
> > --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index 45b7c6900adc..8cfd8ab725ce 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -278,6 +278,9 @@ struct intel_wm {
> > struct mutex wm_mutex;
> >
> > bool ipc_enabled;
> > +
> > + u32 dpkgc_latency;
> > + u32 dpkgc_added_waketime;
> > };
> >
> > struct intel_display {
> > diff --git a/drivers/gpu/drm/i915/display/intel_wm.c
> > b/drivers/gpu/drm/i915/display/intel_wm.c
> > index d7dc49aecd27..ffc7dde86629 100644
> > --- a/drivers/gpu/drm/i915/display/intel_wm.c
> > +++ b/drivers/gpu/drm/i915/display/intel_wm.c
> > @@ -7,9 +7,18 @@
> >
> > #include "i915_drv.h"
> > #include "i9xx_wm.h"
> > +#include "intel_de.h"
> > #include "intel_display_types.h"
> > #include "intel_wm.h"
> > #include "skl_watermark.h"
> > +#include "skl_watermark_regs.h"
> > +
> > +/*
> > + * It is expected that DSB can do posted writes to every register in
> > + * the pipe and planes within 100us. For flip queue use case, the
> > + * recommended DSB execution time is 100us + one SAGV block time.
> > + */
> > +#define DSB_EXE_TIME 100
> >
> > /**
> > * intel_update_watermarks - update FIFO watermark values based on
> > current modes @@ -131,6 +140,68 @@ bool intel_wm_plane_visible(const
> > struct intel_crtc_state *crtc_state,
> > return plane_state->uapi.visible;
> > }
> >
> > +/*
> > + * If Fixed Refresh Rate or For VRR case Vmin = Vmax = Flipline:
> > + * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from
> > + * watermark level1 and up and above. If watermark level 1 is
> > + * invalid program it with all 1's.
> > + * Program PKG_C_LATENCY Added Wake Time = DSB execution time
> > + * If Variable Refresh Rate where Vmin != Vmax != Flipline:
> > + * Program DEEP PKG_C_LATENCY Pkg C with all 1's.
> > + * Program PKG_C_LATENCY Added Wake Time = 0 */ int
> > +intel_wm_compute_dpkgc_latency(struct intel_atomic_state *state,
> > + struct drm_i915_private *i915) {
> > + struct intel_display *display = to_intel_display(state);
> > + struct intel_crtc *crtc;
> > + struct intel_crtc_state *new_crtc_state;
> > + u32 max_latency = LNL_PKG_C_LATENCY_MASK;
> > + u32 added_waketime = 0;
> > + int i;
> > + bool fixed_refresh_rate = false;
> > +
> > + if (DISPLAY_VER(display) < 20)
> > + return 0;
> > +
> > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > +i) {
> > + if ((new_crtc_state->vrr.vmin ==
> > +new_crtc_state->vrr.vmax &&
> > + new_crtc_state->vrr.vmin ==
> > +new_crtc_state->vrr.flipline) ||
> > + !new_crtc_state->vrr.enable)
> > + fixed_refresh_rate = true;
> > + }
> > +
> > + if (fixed_refresh_rate) {
> > + max_latency = skl_watermark_max_latency(i915, 1);
> > + if (max_latency == 0)
> > + max_latency = LNL_PKG_C_LATENCY_MASK;
> > + added_waketime = DSB_EXE_TIME +
> > + display->sagv.block_time_us;
> > + }
> > +
> > + display->wm.dpkgc_latency = max_latency;
> > + display->wm.dpkgc_added_waketime = added_waketime;
> > +
> > + return 0;
> > +}
> > +
> > +int
> > +intel_wm_write_dpkgc_latency(struct intel_display *display) {
> > + u32 clear = 0, val = 0;
> > +
> > + if (DISPLAY_VER(display) < 20)
> > + return 0;
> > +
> > + clear |= LNL_ADDED_WAKE_TIME_MASK |
> LNL_PKG_C_LATENCY_MASK;
> > + val |= REG_FIELD_PREP(LNL_PKG_C_LATENCY_MASK,
> > +display->wm.dpkgc_latency) |
> > + REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> > +display->wm.dpkgc_added_waketime);
> > +
> > + intel_de_rmw(display, LNL_PKG_C_LATENCY, clear, val);
> > + return 0;
> > +}
> > +
> > void intel_print_wm_latency(struct drm_i915_private *dev_priv,
> > const char *name, const u16 wm[])
> > {
> > diff --git a/drivers/gpu/drm/i915/display/intel_wm.h
> > b/drivers/gpu/drm/i915/display/intel_wm.h
> > index e97cdca89a5c..1d3c2653168f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_wm.h
> > +++ b/drivers/gpu/drm/i915/display/intel_wm.h
> > @@ -9,6 +9,7 @@
> > #include <linux/types.h>
> >
> > struct drm_i915_private;
> > +struct intel_display;
> > struct intel_atomic_state;
> > struct intel_crtc;
> > struct intel_crtc_state;
> > @@ -31,5 +32,8 @@ void intel_print_wm_latency(struct drm_i915_private
> > *i915,
> > const char *name, const u16 wm[]);
> > void intel_wm_init(struct drm_i915_private *i915);
> > void intel_wm_debugfs_register(struct drm_i915_private *i915);
> > +int intel_wm_compute_dpkgc_latency(struct intel_atomic_state *state,
> > + struct drm_i915_private *i915); int
> > +intel_wm_write_dpkgc_latency(struct intel_display *display);
> >
> > #endif /* __INTEL_WM_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 714bfcd83015..70f1d4f66f2d 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -28,12 +28,6 @@
> > #include "skl_watermark.h"
> > #include "skl_watermark_regs.h"
> >
> > -/*It is expected that DSB can do posted writes to every register in
> > - * the pipe and planes within 100us. For flip queue use case, the
> > - * recommended DSB execution time is 100us + one SAGV block time.
> > - */
> > -#define DSB_EXE_TIME 100
> > -
> > static void skl_sagv_disable(struct drm_i915_private *i915);
> >
> > /* Stores plane specific WM parameters */ @@ -2837,49 +2831,12 @@
> > static int skl_wm_add_affected_planes(struct intel_atomic_state
> > *state,
> > return 0;
> > }
> >
> > -/*
> > - * If Fixed Refresh Rate or For VRR case Vmin = Vmax = Flipline:
> > - * Program DEEP PKG_C_LATENCY Pkg C with highest valid latency from
> > - * watermark level1 and up and above. If watermark level 1 is
> > - * invalid program it with all 1's.
> > - * Program PKG_C_LATENCY Added Wake Time = DSB execution time
> > - * If Variable Refresh Rate where Vmin != Vmax != Flipline:
> > - * Program DEEP PKG_C_LATENCY Pkg C with all 1's.
> > - * Program PKG_C_LATENCY Added Wake Time = 0
> > - */
> > -static void
> > -skl_program_dpkgc_latency(struct drm_i915_private *i915, bool
> > fixed_refresh_rate) -{
> > - struct intel_display *display = to_intel_display(&i915->drm);
> > - u32 max_latency = LNL_PKG_C_LATENCY_MASK;
> > - u32 clear = 0, val = 0;
> > - u32 added_wake_time = 0;
> > -
> > - if (DISPLAY_VER(display) < 20)
> > - return;
> > -
> > - if (fixed_refresh_rate) {
> > - max_latency = skl_watermark_max_latency(i915, 1);
> > - if (max_latency == 0)
> > - max_latency = LNL_PKG_C_LATENCY_MASK;
> > - added_wake_time = DSB_EXE_TIME +
> > - 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) |
> > - REG_FIELD_PREP(LNL_ADDED_WAKE_TIME_MASK,
> > added_wake_time);
> > -
> > - intel_de_rmw(display, LNL_PKG_C_LATENCY, clear, val); -}
> > -
> > static int
> > skl_compute_wm(struct intel_atomic_state *state)
> > {
> > struct intel_crtc *crtc;
> > struct intel_crtc_state __maybe_unused *new_crtc_state;
> > int ret, i;
> > - bool fixed_refresh_rate = false;
> >
> > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i) {
> > ret = skl_build_pipe_wm(state, crtc); @@ -2904,15
> > +2861,8 @@ skl_compute_wm(struct intel_atomic_state *state)
> > ret = skl_wm_add_affected_planes(state, crtc);
> > if (ret)
> > return ret;
> > -
> > - if ((new_crtc_state->vrr.vmin ==
> > new_crtc_state->vrr.vmax &&
> > - new_crtc_state->vrr.vmin ==
> > new_crtc_state->vrr.flipline) ||
> > - !new_crtc_state->vrr.enable)
> > - fixed_refresh_rate = true;
> > }
> >
> > - skl_program_dpkgc_latency(to_i915(state->base.dev),
> > fixed_refresh_rate);
> > -
> > skl_print_wm_changes(state);
> >
> > return 0;
