Re: [PATCH v3 (rebased) 4/4] drm/i915: Add locking to pll updates, v3.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Wed, 2016-03-23 at 14:51 +0100, Maarten Lankhorst wrote:
> With async modesets this is no longer protected with connection_mutex,
> so ensure that each pll has its own lock. The pll configuration state
> is still protected; it's only the pll updates that need locking against
> concurrency.
> 
> Changes since v1:
> - Rebased.
> - Fix locking to protect all accesses. (Durgadoss)
> Changes since v2:
> - Make the dpll_lock global to protect concurrent updates to the
>   same register, for example DPLL_CTRL1 on skl. (Ander)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 06e4773ae7f6..a66732744494 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1854,6 +1854,13 @@ struct drm_i915_private {
>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
>  	const struct intel_dpll_mgr *dpll_mgr;
>  
> +	/*
> +	 * dpll_lock serializes intel_{prepare,enable,disable}_shared_dpll.
> +	 * Must be global rather than per dpll, because on some platforms
> +	 * plls share registers.
> +	 */
> +	struct mutex dpll_lock;
> +
>  	unsigned int active_crtcs;
>  	unsigned int min_pixclk[I915_MAX_PIPES];
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 19bfe6743ef2..1175eebfe03b 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -89,14 +89,16 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> +	mutex_lock(&dev_priv->dpll_lock);
>  	WARN_ON(!pll->config.crtc_mask);
> -	if (pll->active_mask == 0) {
> +	if (!pll->active_mask) {
>  		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
>  		WARN_ON(pll->on);
>  		assert_shared_dpll_disabled(dev_priv, pll);
>  
>  		pll->funcs.mode_set(dev_priv, pll);
>  	}
> +	mutex_unlock(&dev_priv->dpll_lock);
>  }
>  
>  /**
> @@ -113,14 +115,17 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
>  	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
> -	unsigned old_mask = pll->active_mask;
> +	unsigned old_mask;
>  
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> +	mutex_lock(&dev_priv->dpll_lock);
> +	old_mask = pll->active_mask;
> +
>  	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)) ||
>  	    WARN_ON(pll->active_mask & crtc_mask))
> -		return;
> +		goto out;
>  
>  	pll->active_mask |= crtc_mask;
>  
> @@ -131,13 +136,16 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	if (old_mask) {
>  		WARN_ON(!pll->on);
>  		assert_shared_dpll_enabled(dev_priv, pll);
> -		return;
> +		goto out;
>  	}
>  	WARN_ON(pll->on);
>  
>  	DRM_DEBUG_KMS("enabling %s\n", pll->name);
>  	pll->funcs.enable(dev_priv, pll);
>  	pll->on = true;
> +
> +out:
> +	mutex_unlock(&dev_priv->dpll_lock);
>  }
>  
>  void intel_disable_shared_dpll(struct intel_crtc *crtc)
> @@ -154,8 +162,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  	if (pll == NULL)
>  		return;
>  
> +	mutex_lock(&dev_priv->dpll_lock);
>  	if (WARN_ON(!(pll->active_mask & crtc_mask)))
> -		return;
> +		goto out;
>  
>  	DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
>  		      pll->name, pll->active_mask, pll->on,
> @@ -166,11 +175,14 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  
>  	pll->active_mask &= ~crtc_mask;
>  	if (pll->active_mask)
> -		return;
> +		goto out;
>  
>  	DRM_DEBUG_KMS("disabling %s\n", pll->name);
>  	pll->funcs.disable(dev_priv, pll);
>  	pll->on = false;
> +
> +out:
> +	mutex_unlock(&dev_priv->dpll_lock);
>  }
>  
>  static struct intel_shared_dpll *
> @@ -1750,6 +1762,7 @@ void intel_shared_dpll_init(struct drm_device *dev)
>  
>  	dev_priv->dpll_mgr = dpll_mgr;
>  	dev_priv->num_shared_dpll = i;
> +	mutex_init(&dev_priv->dpll_lock);
>  
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx