On Mon, Mar 18, 2019 at 04:34:35PM +0200, Imre Deak wrote:
> @@ -2137,6 +2194,44 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
> }
> }
>
> +/**
> + * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> + * @dev_priv: i915 device
> + * @cdclk_state: new CDCLK state
> + * @pipe: pipe with which to synchronize the update
> + *
> + * Program the hardware before updating the HW plane state based on the passed
> + * in CDCLK state, if necessary.
> + */
> +void
> +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *cdclk_state,
> + enum pipe pipe)
> +{
> + if (pipe == INVALID_PIPE ||
> + dev_priv->cdclk.actual.cdclk >= cdclk_state->cdclk)
cdclk_state == &dev_priv->cdclk.actual
so these look a bit buggered.
Would be somewhat nice to use the private obj stuff for this but
the locking now enforced by that make it difficult. Perhaps a
swap(dev_priv->cdlk, state->cdclk) would be possible? A quick scan
through the code didn't reveal any uses of state->cdclk outside the
compute code paths, so looks safeish. Another option is just
comparing against dev_priv->cdclk.hw since we still have that.
> + intel_set_cdclk(dev_priv, cdclk_state, pipe);
> +}
> +
> +/**
> + * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware
> + * @dev_priv: i915 device
> + * @cdclk_state: new CDCLK state
> + * @pipe: pipe with which to synchronize the update
> + *
> + * Program the hardware after updating the HW plane state based on the passed
> + * in CDCLK state, if necessary.
> + */
> +void
> +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *cdclk_state,
> + enum pipe pipe)
> +{
> + if (pipe != INVALID_PIPE &&
> + dev_priv->cdclk.actual.cdclk < cdclk_state->cdclk)
> + intel_set_cdclk(dev_priv, cdclk_state, pipe);
> +}
> +
> static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
> int pixel_rate)
> {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b4199cd53349..0f9884ec7e8e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12982,6 +12982,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> intel_state->active_crtcs = dev_priv->active_crtcs;
> intel_state->cdclk.logical = dev_priv->cdclk.logical;
> intel_state->cdclk.actual = dev_priv->cdclk.actual;
> + intel_state->cdclk.pipe = INVALID_PIPE;
>
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> if (new_crtc_state->active)
> @@ -13001,6 +13002,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> * adjusted_mode bits in the crtc directly.
> */
> if (dev_priv->display.modeset_calc_cdclk) {
> + enum pipe pipe;
> +
> ret = dev_priv->display.modeset_calc_cdclk(state);
> if (ret < 0)
> return ret;
> @@ -13017,12 +13020,36 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> return ret;
> }
>
> + if (is_power_of_2(intel_state->active_crtcs)) {
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> +
> + pipe = ilog2(intel_state->active_crtcs);
> + crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base;
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + if (crtc_state && needs_modeset(crtc_state))
> + pipe = INVALID_PIPE;
> + } else {
> + pipe = INVALID_PIPE;
> + }
> +
> /* All pipes must be switched off while we change the cdclk. */
> - if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> - &intel_state->cdclk.actual)) {
> + if (pipe != INVALID_PIPE &&
> + intel_cdclk_needs_cd2x_update(dev_priv,
> + &dev_priv->cdclk.actual,
> + &intel_state->cdclk.actual)) {
> + ret = intel_lock_all_pipes(state);
> + if (ret < 0)
> + return ret;
> +
> + intel_state->cdclk.pipe = pipe;
> + } else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> + &intel_state->cdclk.actual)) {
> ret = intel_modeset_all_pipes(state);
> if (ret < 0)
> return ret;
> +
> + intel_state->cdclk.pipe = INVALID_PIPE;
> }
>
> DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",
> @@ -13433,7 +13460,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> if (intel_state->modeset) {
> drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>
> - intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> + intel_set_cdclk_pre_plane_update(dev_priv,
> + &dev_priv->cdclk.actual,
> + intel_state->cdclk.pipe);
>
> /*
> * SKL workaround: bspec recommends we disable the SAGV when we
> @@ -13462,6 +13491,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> dev_priv->display.update_crtcs(state);
>
> + intel_set_cdclk_post_plane_update(dev_priv,
> + &dev_priv->cdclk.actual,
> + intel_state->cdclk.pipe);
> +
> /* FIXME: We should call drm_atomic_helper_commit_hw_done() here
> * already, but still need the state for the delayed optimization. To
> * fix this:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0b84e557c267..e5a1e245ee65 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -559,6 +559,8 @@ struct intel_atomic_state {
>
> int force_min_cdclk;
> bool force_min_cdclk_changed;
> + /* pipe to which cd2x update is synchronized */
> + enum pipe pipe;
> } cdclk;
>
> bool dpll_set, modeset;
> @@ -1694,12 +1696,21 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
> void intel_update_cdclk(struct drm_i915_private *dev_priv);
> void intel_update_rawclk(struct drm_i915_private *dev_priv);
> +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *a,
> + const struct intel_cdclk_state *b);
> bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> const struct intel_cdclk_state *b);
> bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> const struct intel_cdclk_state *b);
> -void intel_set_cdclk(struct drm_i915_private *dev_priv,
> - const struct intel_cdclk_state *cdclk_state);
> +void
> +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *cdclk_state,
> + enum pipe pipe);
> +void
> +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv,
> + const struct intel_cdclk_state *cdclk_state,
> + enum pipe pipe);
> void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> const char *context);
>
> --
> 2.13.2
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
