On Mon, 13 Feb 2023, Ville Syrjälä <[email protected]> wrote: > 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.
Okay, so the caller side requires HAS_GMCH() and the callee side requires .optimize_watermarks != NULL. That indeed leaves us with PCH split platforms before display version 9. However, the implementation of sanitize_watermarks() seems pretty generic, right? I guess the question is, do you suggest moving the whole thing to i9xx_wm.c and specifically not calling it on display 9+, or do you just want the commit message to reflect the above? BR, Jani. > >> 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 -- Jani Nikula, Intel Open Source Graphics Center
