[v2] drm/i915: Lock gmbus/aux mutexes while changing cdclk
diff mbox series

Message ID 20200302174442.5803-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • [v2] drm/i915: Lock gmbus/aux mutexes while changing cdclk
Related show

Commit Message

Ville Syrjälä March 2, 2020, 5:44 p.m. UTC
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>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Jani Nikula March 3, 2020, 9:11 a.m. UTC | #1
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")) {

Patch
diff mbox series

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")) {