Re: [Intel-gfx] [PATCH v2] drm/i915: Lock gmbus/aux mutexes while changing cdclk

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 02 Mar 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> gmbus/aux may be clocked by cdclk, thus we should make sure no
> transfers are ongoing while the cdclk frequency is being changed.
> We do that by simply grabbing all the gmbus/aux mutexes. No one
> else should be holding any more than one of those at a time so
> the lock ordering here shouldn't matter.
>
> v2: Use mutex_lock_nest_lock() (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I don't think my review comments were invalid in general, but I don't
want to block this fairly straightforward fix either.

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 0741d643455b..979a0241fdcb 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1868,6 +1868,8 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  			    const struct intel_cdclk_config *cdclk_config,
>  			    enum pipe pipe)
>  {
> +	struct intel_encoder *encoder;
> +
>  	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
>  		return;
>  
> @@ -1876,8 +1878,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  
>  	intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
>  
> +	/*
> +	 * Lock aux/gmbus while we change cdclk in case those
> +	 * functions use cdclk. Not all platforms/ports do,
> +	 * but we'll lock them all for simplicity.
> +	 */
> +	mutex_lock(&dev_priv->gmbus_mutex);
> +	for_each_intel_dp(&dev_priv->drm, encoder) {
> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		mutex_lock_nest_lock(&intel_dp->aux.hw_mutex,
> +				     &dev_priv->gmbus_mutex);
> +	}
> +
>  	dev_priv->display.set_cdclk(dev_priv, cdclk_config, pipe);
>  
> +	for_each_intel_dp(&dev_priv->drm, encoder) {
> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		mutex_unlock(&intel_dp->aux.hw_mutex);
> +	}
> +	mutex_unlock(&dev_priv->gmbus_mutex);
> +
>  	if (drm_WARN(&dev_priv->drm,
>  		     intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config),
>  		     "cdclk state doesn't match!\n")) {

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx