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ä <[email protected]> > Signed-off-by: Jani Nikula <[email protected]> > --- > 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
