Re: [PATCH v6 2/2] drm/i915: Skip modeset for cdclk changes if possible

[Imre Deak <imre.deak@intel.com>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx