On Mon, Feb 13, 2023 at 10:00:00PM +0200, Jani Nikula wrote:
> Move the generic sanitize_watermarks() to intel_wm.[ch] and rename as
It's not generic though. Only the ilk_ stuff uses it.
> intel_wm_sanitize(). The slightly unfortunate downside is having to
> expose intel_atomic_check() from intel_display.c, but this declutters
> intel_display.c nicely.
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 124 +------------------
> drivers/gpu/drm/i915/display/intel_display.h | 2 +
> drivers/gpu/drm/i915/display/intel_wm.c | 119 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_wm.h | 1 +
> 4 files changed, 125 insertions(+), 121 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 82efd96ace87..abb40112704b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6602,8 +6602,8 @@ static int intel_bigjoiner_add_affected_crtcs(struct intel_atomic_state *state)
> * @dev: drm device
> * @_state: state to validate
> */
> -static int intel_atomic_check(struct drm_device *dev,
> - struct drm_atomic_state *_state)
> +int intel_atomic_check(struct drm_device *dev,
> + struct drm_atomic_state *_state)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_atomic_state *state = to_intel_atomic_state(_state);
> @@ -8263,124 +8263,6 @@ void intel_modeset_init_hw(struct drm_i915_private *i915)
> cdclk_state->logical = cdclk_state->actual = i915->display.cdclk.hw;
> }
>
> -static int sanitize_watermarks_add_affected(struct drm_atomic_state *state)
> -{
> - struct drm_plane *plane;
> - struct intel_crtc *crtc;
> -
> - for_each_intel_crtc(state->dev, crtc) {
> - struct intel_crtc_state *crtc_state;
> -
> - crtc_state = intel_atomic_get_crtc_state(state, crtc);
> - if (IS_ERR(crtc_state))
> - return PTR_ERR(crtc_state);
> -
> - if (crtc_state->hw.active) {
> - /*
> - * Preserve the inherited flag to avoid
> - * taking the full modeset path.
> - */
> - crtc_state->inherited = true;
> - }
> - }
> -
> - drm_for_each_plane(plane, state->dev) {
> - struct drm_plane_state *plane_state;
> -
> - plane_state = drm_atomic_get_plane_state(state, plane);
> - if (IS_ERR(plane_state))
> - return PTR_ERR(plane_state);
> - }
> -
> - return 0;
> -}
> -
> -/*
> - * Calculate what we think the watermarks should be for the state we've read
> - * out of the hardware and then immediately program those watermarks so that
> - * we ensure the hardware settings match our internal state.
> - *
> - * We can calculate what we think WM's should be by creating a duplicate of the
> - * current state (which was constructed during hardware readout) and running it
> - * through the atomic check code to calculate new watermark values in the
> - * state object.
> - */
> -static void sanitize_watermarks(struct drm_i915_private *dev_priv)
> -{
> - struct drm_atomic_state *state;
> - struct intel_atomic_state *intel_state;
> - struct intel_crtc *crtc;
> - struct intel_crtc_state *crtc_state;
> - struct drm_modeset_acquire_ctx ctx;
> - int ret;
> - int i;
> -
> - /* Only supported on platforms that use atomic watermark design */
> - if (!dev_priv->display.funcs.wm->optimize_watermarks)
> - return;
> -
> - state = drm_atomic_state_alloc(&dev_priv->drm);
> - if (drm_WARN_ON(&dev_priv->drm, !state))
> - return;
> -
> - intel_state = to_intel_atomic_state(state);
> -
> - drm_modeset_acquire_init(&ctx, 0);
> -
> -retry:
> - state->acquire_ctx = &ctx;
> -
> - /*
> - * Hardware readout is the only time we don't want to calculate
> - * intermediate watermarks (since we don't trust the current
> - * watermarks).
> - */
> - if (!HAS_GMCH(dev_priv))
> - intel_state->skip_intermediate_wm = true;
> -
> - ret = sanitize_watermarks_add_affected(state);
> - if (ret)
> - goto fail;
> -
> - ret = intel_atomic_check(&dev_priv->drm, state);
> - if (ret)
> - goto fail;
> -
> - /* 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;
> - intel_optimize_watermarks(intel_state, crtc);
> -
> - to_intel_crtc_state(crtc->base.state)->wm = crtc_state->wm;
> - }
> -
> -fail:
> - if (ret == -EDEADLK) {
> - drm_atomic_state_clear(state);
> - drm_modeset_backoff(&ctx);
> - goto retry;
> - }
> -
> - /*
> - * If we fail here, it means that the hardware appears to be
> - * programmed in a way that shouldn't be possible, given our
> - * understanding of watermark requirements. This might mean a
> - * mistake in the hardware readout code or a mistake in the
> - * watermark calculations for a given platform. Raise a WARN
> - * so that this is noticeable.
> - *
> - * If this actually happens, we'll have to just leave the
> - * BIOS-programmed watermarks untouched and hope for the best.
> - */
> - drm_WARN(&dev_priv->drm, ret,
> - "Could not determine valid watermarks for inherited state\n");
> -
> - drm_atomic_state_put(state);
> -
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
> -}
> -
> static int intel_initial_commit(struct drm_device *dev)
> {
> struct drm_atomic_state *state = NULL;
> @@ -8657,7 +8539,7 @@ int intel_modeset_init_nogem(struct drm_i915_private *i915)
> * since the watermark calculation done here will use pstate->fb.
> */
> if (!HAS_GMCH(i915))
> - sanitize_watermarks(i915);
> + intel_wm_sanitize(i915);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index cb6f520cc575..ed852f62721d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -32,6 +32,7 @@
>
> enum drm_scaling_filter;
> struct dpll;
> +struct drm_atomic_state;
> struct drm_connector;
> struct drm_device;
> struct drm_display_mode;
> @@ -394,6 +395,7 @@ enum phy_fia {
> ((connector) = to_intel_connector((__state)->base.connectors[__i].ptr), \
> (new_connector_state) = to_intel_digital_connector_state((__state)->base.connectors[__i].new_state), 1))
>
> +int intel_atomic_check(struct drm_device *dev, struct drm_atomic_state *state);
> int intel_atomic_add_affected_planes(struct intel_atomic_state *state,
> struct intel_crtc *crtc);
> u8 intel_calc_active_pipes(struct intel_atomic_state *state,
> diff --git a/drivers/gpu/drm/i915/display/intel_wm.c b/drivers/gpu/drm/i915/display/intel_wm.c
> index c4d14a51869b..15fda0829c2f 100644
> --- a/drivers/gpu/drm/i915/display/intel_wm.c
> +++ b/drivers/gpu/drm/i915/display/intel_wm.c
> @@ -5,6 +5,7 @@
>
> #include "i915_drv.h"
> #include "i9xx_wm.h"
> +#include "intel_atomic.h"
> #include "intel_display_types.h"
> #include "intel_wm.h"
> #include "skl_watermark.h"
> @@ -173,6 +174,124 @@ void intel_print_wm_latency(struct drm_i915_private *dev_priv,
> }
> }
>
> +static int sanitize_watermarks_add_affected(struct drm_atomic_state *state)
> +{
> + struct drm_plane *plane;
> + struct intel_crtc *crtc;
> +
> + for_each_intel_crtc(state->dev, crtc) {
> + struct intel_crtc_state *crtc_state;
> +
> + crtc_state = intel_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + if (crtc_state->hw.active) {
> + /*
> + * Preserve the inherited flag to avoid
> + * taking the full modeset path.
> + */
> + crtc_state->inherited = true;
> + }
> + }
> +
> + drm_for_each_plane(plane, state->dev) {
> + struct drm_plane_state *plane_state;
> +
> + plane_state = drm_atomic_get_plane_state(state, plane);
> + if (IS_ERR(plane_state))
> + return PTR_ERR(plane_state);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Calculate what we think the watermarks should be for the state we've read
> + * out of the hardware and then immediately program those watermarks so that
> + * we ensure the hardware settings match our internal state.
> + *
> + * We can calculate what we think WM's should be by creating a duplicate of the
> + * current state (which was constructed during hardware readout) and running it
> + * through the atomic check code to calculate new watermark values in the
> + * state object.
> + */
> +void intel_wm_sanitize(struct drm_i915_private *dev_priv)
> +{
> + struct drm_atomic_state *state;
> + struct intel_atomic_state *intel_state;
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *crtc_state;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
> + int i;
> +
> + /* Only supported on platforms that use atomic watermark design */
> + if (!dev_priv->display.funcs.wm->optimize_watermarks)
> + return;
> +
> + state = drm_atomic_state_alloc(&dev_priv->drm);
> + if (drm_WARN_ON(&dev_priv->drm, !state))
> + return;
> +
> + intel_state = to_intel_atomic_state(state);
> +
> + drm_modeset_acquire_init(&ctx, 0);
> +
> +retry:
> + state->acquire_ctx = &ctx;
> +
> + /*
> + * Hardware readout is the only time we don't want to calculate
> + * intermediate watermarks (since we don't trust the current
> + * watermarks).
> + */
> + if (!HAS_GMCH(dev_priv))
> + intel_state->skip_intermediate_wm = true;
> +
> + ret = sanitize_watermarks_add_affected(state);
> + if (ret)
> + goto fail;
> +
> + ret = intel_atomic_check(&dev_priv->drm, state);
> + if (ret)
> + goto fail;
> +
> + /* 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;
> + intel_optimize_watermarks(intel_state, crtc);
> +
> + to_intel_crtc_state(crtc->base.state)->wm = crtc_state->wm;
> + }
> +
> +fail:
> + if (ret == -EDEADLK) {
> + drm_atomic_state_clear(state);
> + drm_modeset_backoff(&ctx);
> + goto retry;
> + }
> +
> + /*
> + * If we fail here, it means that the hardware appears to be
> + * programmed in a way that shouldn't be possible, given our
> + * understanding of watermark requirements. This might mean a
> + * mistake in the hardware readout code or a mistake in the
> + * watermark calculations for a given platform. Raise a WARN
> + * so that this is noticeable.
> + *
> + * If this actually happens, we'll have to just leave the
> + * BIOS-programmed watermarks untouched and hope for the best.
> + */
> + drm_WARN(&dev_priv->drm, ret,
> + "Could not determine valid watermarks for inherited state\n");
> +
> + drm_atomic_state_put(state);
> +
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +}
> +
> void intel_wm_init(struct drm_i915_private *i915)
> {
> if (DISPLAY_VER(i915) >= 9)
> diff --git a/drivers/gpu/drm/i915/display/intel_wm.h b/drivers/gpu/drm/i915/display/intel_wm.h
> index dc582967a25e..a5233e7e5e8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_wm.h
> +++ b/drivers/gpu/drm/i915/display/intel_wm.h
> @@ -31,6 +31,7 @@ bool intel_wm_plane_visible(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state);
> void intel_print_wm_latency(struct drm_i915_private *i915,
> const char *name, const u16 wm[]);
> +void intel_wm_sanitize(struct drm_i915_private *i915);
> void intel_wm_init(struct drm_i915_private *i915);
>
> #endif /* __INTEL_WM_H__ */
> --
> 2.34.1
--
Ville Syrjälä
Intel
