On Fri, 2024-11-15 at 10:28 +0000, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > > Sent: Friday, November 15, 2024 3:50 PM > > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx; > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Cc: Govindapillai, Vinod <vinod.govindapillai@xxxxxxxxx>; Syrjala, Ville > > <ville.syrjala@xxxxxxxxx>; Kandpal, Suraj <suraj.kandpal@xxxxxxxxx> > > Subject: Re: [PATCH 1/2] drm/i915/display: Refactor handling of dpkgc latency > > programming > > > > On Fri, 15 Nov 2024, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote: > > > - 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 do not want to write into any registers during compute_config > > > phase While we are at it do some more refactors in the function like: > > > -Use intel_display wherever possible > > > -Move away from using enable_dpkgc bool and call it fixed_refresh_rate > > > -Optimize value prepration > > > > Please write proper sentences instead of bullet points. And the general advice > > is to do one thing per patch. > > Sure will fix that. > It was previously divided into 6 patches but squashed it after Vinod suggested to do you want me > to go back to that convention > https://patchwork.freedesktop.org/series/141200/ Most of the old function handling was changed - from where and when it is called etc. Thats why I suggested to have one patch as refactoring the whole handling. > > > > > > > > > --v2 > > > -No need to save anything in intel_display structure [Vinod] -Move > > > computation and writing into register to intel_atomic_commit_tail > > > [Vinod] > > > > > > --v3 > > > -Rename the subject [Vinod] > > > -Rearrange the variable initialization and declaration [Vinod] > > > -Reaarange condition evaluation for fixed_refresh_rate [Vinod] > > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 2 + > > > drivers/gpu/drm/i915/display/intel_wm.c | 56 ++++++++++++++++++++ > > > drivers/gpu/drm/i915/display/intel_wm.h | 1 + > > > drivers/gpu/drm/i915/display/skl_watermark.c | 52 ------------------ > > > 4 files changed, 59 insertions(+), 52 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index e790a2de5b3d..d1880e0a5d29 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -7826,6 +7826,8 @@ static void intel_atomic_commit_tail(struct > > intel_atomic_state *state) > > > /* Now enable the clocks, plane, pipe, and connectors that we set up. > > */ > > > dev_priv->display.funcs.display->commit_modeset_enables(state); > > > > > > + intel_program_dpkgc_latency(state); > > > + > > > if (state->modeset) > > > intel_set_cdclk_post_plane_update(state); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_wm.c > > > b/drivers/gpu/drm/i915/display/intel_wm.c > > > index d7dc49aecd27..4a2caf9cb03e 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,53 @@ 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 */ void > > > +intel_program_dpkgc_latency(struct intel_atomic_state *state) > > > > The function prefix should match the file name i.e. a function in intel_foo.[ch] > > should be named intel_foo_bar(). > > > > Well it was previously intel_wm_program_dpkgc_latency but Vinod had asked to rename the function Ah.. sorry! I didnt notice that you moved this to intel_wm.c. I guess it has mainly for old generation wm handling. I am not sure if "skl_watermarks.c" the right place. But atleast it is better than intel_wm.c. > > > > But why is the function being relocated at all? That's also not explained in the > > commit message. > > The reason is in the commit message > "- 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." > > Will reword the commit message into sentences to make it more clearer. > > Regards, > Suraj Kandpal > > > > > > +{ > > > + struct intel_display *display = to_intel_display(state); > > > + struct drm_i915_private *i915 = to_i915(display->drm); > > > + struct intel_crtc *crtc; > > > + struct intel_crtc_state *new_crtc_state; > > > + u32 max_latency = LNL_PKG_C_LATENCY_MASK, added_waketime = 0; > > > + bool fixed_refresh_rate = false; > > > + u32 clear, val; > > > + int i; > > > + > > > + if (DISPLAY_VER(display) < 20) > > > + return; > > > + > > > + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > > > + if (!new_crtc_state->vrr.enable || > > > + (new_crtc_state->vrr.vmin == new_crtc_state->vrr.vmax && > > > + new_crtc_state->vrr.vmin == new_crtc_state->vrr.flipline)) > > > + 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; > > > + } > > > + > > > + 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_waketime); > > > + > > > + intel_de_rmw(display, LNL_PKG_C_LATENCY, clear, val); } > > > + > > > 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..07e3d9359d73 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_wm.h > > > +++ b/drivers/gpu/drm/i915/display/intel_wm.h > > > @@ -31,5 +31,6 @@ 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); > > > +void intel_program_dpkgc_latency(struct intel_atomic_state *state); > > > > > > #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 1a4c1fa24820..d419edb196c6 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 */ @@ -2844,51 +2838,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 > > > enable_dpkgc) -{ > > > - u32 max_latency = 0; > > > - u32 clear = 0, val = 0; > > > - u32 added_wake_time = 0; > > > - > > > - if (DISPLAY_VER(i915) < 20) > > > - return; > > > - > > > - if (enable_dpkgc) { > > > - 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 + > > > - i915->display.sagv.block_time_us; > > > - } else { > > > - max_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_ADDED_WAKE_TIME_MASK, > > added_wake_time); > > > - > > > - intel_uncore_rmw(&i915->uncore, 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 enable_dpkgc = false; > > > > > > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { > > > ret = skl_build_pipe_wm(state, crtc); @@ -2913,15 +2868,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) > > > - enable_dpkgc = true; > > > } > > > > > > - skl_program_dpkgc_latency(to_i915(state->base.dev), enable_dpkgc); > > > - > > > skl_print_wm_changes(state); > > > > > > return 0; > > > > -- > > Jani Nikula, Intel
