On Fri, 10 Sep 2021, Dave Airlie <airlied@xxxxxxxxx> wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> This moves one wrapper from the pm->display side, and creates
> wrappers for all the others, this should simplify things later.
>
> One thing to note is that the code checks the existance of some
> of these ptrs, so the wrappers are a bit complicated by that.
>
> Suggested by Jani.
>
> v2: fixup warnings in wrong place error.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 187 ++++++++++++-------
> drivers/gpu/drm/i915/intel_pm.c | 39 ----
> drivers/gpu/drm/i915/intel_pm.h | 1 -
> 3 files changed, 123 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e62f8317cbda..a1380ce02861 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -126,6 +126,101 @@ static void ilk_pfit_enable(const struct intel_crtc_state *crtc_state);
> static void intel_modeset_setup_hw_state(struct drm_device *dev,
> struct drm_modeset_acquire_ctx *ctx);
>
> +
> +/**
> + * intel_update_watermarks - update FIFO watermark values based on current modes
> + * @dev_priv: i915 device
> + *
> + * Calculate watermark values for the various WM regs based on current mode
> + * and plane configuration.
> + *
> + * There are several cases to deal with here:
> + * - normal (i.e. non-self-refresh)
> + * - self-refresh (SR) mode
> + * - lines are large relative to FIFO size (buffer can hold up to 2)
> + * - lines are small relative to FIFO size (buffer can hold more than 2
> + * lines), so need to account for TLB latency
> + *
> + * The normal calculation is:
> + * watermark = dotclock * bytes per pixel * latency
> + * where latency is platform & configuration dependent (we assume pessimal
> + * values here).
> + *
> + * The SR calculation is:
> + * watermark = (trunc(latency/line time)+1) * surface width *
> + * bytes per pixel
> + * where
> + * line time = htotal / dotclock
> + * surface width = hdisplay for normal plane and 64 for cursor
> + * and latency is assumed to be high, as above.
> + *
> + * The final value programmed to the register should always be rounded up,
> + * and include an extra 2 entries to account for clock crossings.
> + *
> + * We don't use the sprite, so we can ignore that. And on Crestline we have
> + * to set the non-SR watermarks to 8.
> + */
> +static void intel_update_watermarks(struct drm_i915_private *dev_priv)
> +{
> + if (dev_priv->display.update_wm)
> + dev_priv->display.update_wm(dev_priv);
> +}
> +
> +static int intel_compute_pipe_wm(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + if (dev_priv->display.compute_pipe_wm)
> + return dev_priv->display.compute_pipe_wm(state, crtc);
> + return 0;
> +}
> +
> +static int intel_compute_intermediate_wm(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + if (!dev_priv->display.compute_intermediate_wm)
> + return 0;
> + if (drm_WARN_ON(&dev_priv->drm,
> + !dev_priv->display.compute_pipe_wm))
> + return 0;
> + return dev_priv->display.compute_intermediate_wm(state, crtc);
> +}
> +
> +static bool intel_initial_watermarks(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + if (dev_priv->display.initial_watermarks) {
> + dev_priv->display.initial_watermarks(state, crtc);
> + return true;
> + }
> + return false;
> +}
> +
> +static void intel_atomic_update_watermarks(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + if (dev_priv->display.atomic_update_watermarks)
> + dev_priv->display.atomic_update_watermarks(state, crtc);
> +}
> +
> +static void intel_optimize_watermarks(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + if (dev_priv->display.optimize_watermarks)
> + dev_priv->display.optimize_watermarks(state, crtc);
> +}
> +
> +static void intel_compute_global_watermarks(struct intel_atomic_state *state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + if (dev_priv->display.compute_global_watermarks)
> + dev_priv->display.compute_global_watermarks(state);
> +}
> +
> /* returns HPLL frequency in kHz */
> int vlv_get_hpll_vco(struct drm_i915_private *dev_priv)
> {
> @@ -2528,9 +2623,8 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
> * we'll continue to update watermarks the old way, if flags tell
> * us to.
> */
> - if (dev_priv->display.initial_watermarks)
> - dev_priv->display.initial_watermarks(state, crtc);
> - else if (new_crtc_state->update_wm_pre)
> + if (!intel_initial_watermarks(state, crtc))
> + if (new_crtc_state->update_wm_pre)
> intel_update_watermarks(dev_priv);
> }
>
> @@ -2903,8 +2997,7 @@ static void ilk_crtc_enable(struct intel_atomic_state *state,
> /* update DSPCNTR to configure gamma for pipe bottom color */
> intel_disable_primary_plane(new_crtc_state);
>
> - if (dev_priv->display.initial_watermarks)
> - dev_priv->display.initial_watermarks(state, crtc);
> + intel_initial_watermarks(state, crtc);
> intel_enable_pipe(new_crtc_state);
>
> if (new_crtc_state->has_pch_encoder)
> @@ -3114,8 +3207,7 @@ static void hsw_crtc_enable(struct intel_atomic_state *state,
> if (DISPLAY_VER(dev_priv) >= 11)
> icl_set_pipe_chicken(new_crtc_state);
>
> - if (dev_priv->display.initial_watermarks)
> - dev_priv->display.initial_watermarks(state, crtc);
> + intel_initial_watermarks(state, crtc);
>
> if (DISPLAY_VER(dev_priv) >= 11) {
> const struct intel_dbuf_state *dbuf_state =
> @@ -3532,7 +3624,7 @@ static void valleyview_crtc_enable(struct intel_atomic_state *state,
> /* update DSPCNTR to configure gamma for pipe bottom color */
> intel_disable_primary_plane(new_crtc_state);
>
> - dev_priv->display.initial_watermarks(state, crtc);
> + intel_initial_watermarks(state, crtc);
> intel_enable_pipe(new_crtc_state);
>
> intel_crtc_vblank_on(new_crtc_state);
> @@ -3575,10 +3667,8 @@ static void i9xx_crtc_enable(struct intel_atomic_state *state,
> /* update DSPCNTR to configure gamma for pipe bottom color */
> intel_disable_primary_plane(new_crtc_state);
>
> - if (dev_priv->display.initial_watermarks)
> - dev_priv->display.initial_watermarks(state, crtc);
> - else
> - intel_update_watermarks(dev_priv);
> + if (!intel_initial_watermarks(state, crtc))
> + intel_update_watermarks(dev_priv);
> intel_enable_pipe(new_crtc_state);
>
> intel_crtc_vblank_on(new_crtc_state);
> @@ -6752,32 +6842,23 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state,
> return ret;
> }
>
> - if (dev_priv->display.compute_pipe_wm) {
> - ret = dev_priv->display.compute_pipe_wm(state, crtc);
> - if (ret) {
> - drm_dbg_kms(&dev_priv->drm,
> - "Target pipe watermarks are invalid\n");
> - return ret;
> - }
> -
> + ret = intel_compute_pipe_wm(state, crtc);
> + if (ret) {
> + drm_dbg_kms(&dev_priv->drm,
> + "Target pipe watermarks are invalid\n");
> + return ret;
> }
>
> - if (dev_priv->display.compute_intermediate_wm) {
> - if (drm_WARN_ON(&dev_priv->drm,
> - !dev_priv->display.compute_pipe_wm))
> - return 0;
> -
> - /*
> - * Calculate 'intermediate' watermarks that satisfy both the
> - * old state and the new state. We can program these
> - * immediately.
> - */
> - ret = dev_priv->display.compute_intermediate_wm(state, crtc);
> - if (ret) {
> - drm_dbg_kms(&dev_priv->drm,
> - "No valid intermediate pipe watermarks are possible\n");
> - return ret;
> - }
> + /*
> + * Calculate 'intermediate' watermarks that satisfy both the
> + * old state and the new state. We can program these
> + * immediately.
> + */
> + ret = intel_compute_intermediate_wm(state, crtc);
> + if (ret) {
> + drm_dbg_kms(&dev_priv->drm,
> + "No valid intermediate pipe watermarks are possible\n");
> + return ret;
> }
>
> if (DISPLAY_VER(dev_priv) >= 9) {
> @@ -8870,23 +8951,6 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
> return 0;
> }
>
> -/*
> - * Handle calculation of various watermark data at the end of the atomic check
> - * phase. The code here should be run after the per-crtc and per-plane 'check'
> - * handlers to ensure that all derived state has been updated.
> - */
> -static int calc_watermark_data(struct intel_atomic_state *state)
> -{
> - struct drm_device *dev = state->base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> -
> - /* Is there platform-specific watermark information to calculate? */
> - if (dev_priv->display.compute_global_watermarks)
> - return dev_priv->display.compute_global_watermarks(state);
> -
> - return 0;
> -}
> -
> static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state,
> struct intel_crtc_state *new_crtc_state)
> {
> @@ -9533,9 +9597,7 @@ static int intel_atomic_check(struct drm_device *dev,
> goto fail;
>
> intel_fbc_choose_crtc(dev_priv, state);
> - ret = calc_watermark_data(state);
> - if (ret)
> - goto fail;
> + intel_compute_global_watermarks(state);
>
> ret = intel_bw_atomic_check(state);
> if (ret)
> @@ -9707,8 +9769,7 @@ static void commit_pipe_pre_planes(struct intel_atomic_state *state,
> intel_psr2_program_trans_man_trk_ctl(new_crtc_state);
> }
>
> - if (dev_priv->display.atomic_update_watermarks)
> - dev_priv->display.atomic_update_watermarks(state, crtc);
> + intel_atomic_update_watermarks(state, crtc);
> }
>
> static void commit_pipe_post_planes(struct intel_atomic_state *state,
> @@ -9835,9 +9896,8 @@ static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
>
> /* FIXME unify this for all platforms */
> if (!new_crtc_state->hw.active &&
> - !HAS_GMCH(dev_priv) &&
> - dev_priv->display.initial_watermarks)
> - dev_priv->display.initial_watermarks(state, crtc);
> + !HAS_GMCH(dev_priv))
> + intel_initial_watermarks(state, crtc);
> }
>
> static void intel_commit_modeset_disables(struct intel_atomic_state *state)
> @@ -10259,8 +10319,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> if (DISPLAY_VER(dev_priv) == 2 && planes_enabling(old_crtc_state, new_crtc_state))
> intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
>
> - if (dev_priv->display.optimize_watermarks)
> - dev_priv->display.optimize_watermarks(state, crtc);
> + intel_optimize_watermarks(state, crtc);
> }
>
> intel_dbuf_post_plane_update(state);
> @@ -11361,7 +11420,7 @@ static void sanitize_watermarks(struct drm_i915_private *dev_priv)
> /* Write calculated watermark values back */
> for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
> crtc_state->wm.need_postvbl_update = true;
> - dev_priv->display.optimize_watermarks(intel_state, crtc);
> + intel_optimize_watermarks(intel_state, crtc);
>
> to_intel_crtc_state(crtc->base.state)->wm = crtc_state->wm;
> }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index be6520756aae..4054c6f7a2f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7132,45 +7132,6 @@ void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv)
> !(intel_uncore_read(&dev_priv->uncore, DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> }
>
> -/**
> - * intel_update_watermarks - update FIFO watermark values based on current modes
> - * @dev_priv: i915 device
> - *
> - * Calculate watermark values for the various WM regs based on current mode
> - * and plane configuration.
> - *
> - * There are several cases to deal with here:
> - * - normal (i.e. non-self-refresh)
> - * - self-refresh (SR) mode
> - * - lines are large relative to FIFO size (buffer can hold up to 2)
> - * - lines are small relative to FIFO size (buffer can hold more than 2
> - * lines), so need to account for TLB latency
> - *
> - * The normal calculation is:
> - * watermark = dotclock * bytes per pixel * latency
> - * where latency is platform & configuration dependent (we assume pessimal
> - * values here).
> - *
> - * The SR calculation is:
> - * watermark = (trunc(latency/line time)+1) * surface width *
> - * bytes per pixel
> - * where
> - * line time = htotal / dotclock
> - * surface width = hdisplay for normal plane and 64 for cursor
> - * and latency is assumed to be high, as above.
> - *
> - * The final value programmed to the register should always be rounded up,
> - * and include an extra 2 entries to account for clock crossings.
> - *
> - * We don't use the sprite, so we can ignore that. And on Crestline we have
> - * to set the non-SR watermarks to 8.
> - */
> -void intel_update_watermarks(struct drm_i915_private *dev_priv)
> -{
> - if (dev_priv->display.update_wm)
> - dev_priv->display.update_wm(dev_priv);
> -}
> -
> void intel_enable_ipc(struct drm_i915_private *dev_priv)
> {
> u32 val;
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 99bce0b4f5fb..990cdcaf85ce 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -29,7 +29,6 @@ struct skl_wm_level;
> void intel_init_clock_gating(struct drm_i915_private *dev_priv);
> void intel_suspend_hw(struct drm_i915_private *dev_priv);
> int ilk_wm_max_level(const struct drm_i915_private *dev_priv);
> -void intel_update_watermarks(struct drm_i915_private *dev_priv);
> void intel_init_pm(struct drm_i915_private *dev_priv);
> void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);
> void intel_pm_setup(struct drm_i915_private *dev_priv);
--
Jani Nikula, Intel Open Source Graphics Center
