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!!
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.
@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;
