On Mon, Mar 18, 2019 at 08:37:33PM +0200, Ville Syrjälä wrote:
> 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.
Yep, screwed this up..
> 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?
I suppose you mean in intel_atomic_commit() already, where the swap is
done already halfway, so in the end state->cdclk would become
old_cdclk_state. That sounds fine to me.
> 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
