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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Aug 27, 2018 at 11:50:32AM -0700, Abhay Kumar wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If we have only a single active pipe and the cdclk change only requires
> the cd2x divider to be updated bxt+ can do the update with forcing a full
> modeset on the pipe. Try to hook that up.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Abhay Kumar <abhay.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +-
>  drivers/gpu/drm/i915/i915_reg.h      |   3 +-
>  drivers/gpu/drm/i915/intel_cdclk.c   | 105 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  20 ++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |   9 ++-
>  5 files changed, 105 insertions(+), 35 deletions(-)
> 
<snip>
> @@ -12252,12 +12253,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  				return ret;
>  		}
>  
> +
>  		/* 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 (is_power_of_2(intel_state->active_crtcs) &&
> +		    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 = ilog2(intel_state->active_crtcs);

BTW on further reflection this probably isn't quite sufficient. Let's
say we have a commit with allow_modeset=true, but we aren't actually
required to do a modeset based on any of the state changes. If we still
have to change cdclk we should actually be doing the cd2x update
atomically with the plane updates, or we should do it before or after
the plane updates depending on whether the cdclk freq is going up or
down.

Doing the update atomically with the plane updates might be nicer in the
end, but for that we would likely need to split the .set_cdclk() hooks
into three parts (pre+commit+post). Whereas just doing the update before
or after the plane updates as needed would probably be a little simpler.


> +		} 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",

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx