[Intel-gfx] [PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.

[Ander Conselvan De Oliveira]

On Mon, 2016-03-14 at 09:27 +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.

I think I need to look at your async branch, since I'm not really sure how async
will work. But locking the individual plls might fail in SKL with the current
code. The register DPLL_CTRL1 controls all 4 plls, and currently it is updated
with a read-modify-write in the enable hook, so we can't update two plls
concurrently.

Ander


> Changes since v1:
> - Rebased.
> - Fix locking to protect all accesses. (Durgadoss)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 25 +++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a9084c7c3a36..e730b2001c07 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(&pll->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(&pll->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(&pll->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(&pll->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(&pll->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(&pll->lock);
>  }
>  
>  static struct intel_shared_dpll *
> @@ -1742,6 +1754,7 @@ void intel_shared_dpll_init(struct drm_device *dev)
>  	for (i = 0; dpll_info[i].id >= 0; i++) {
>  		WARN_ON(i != dpll_info[i].id);
>  
> +		mutex_init(&dev_priv->shared_dplls[i].lock);
>  		dev_priv->shared_dplls[i].id = dpll_info[i].id;
>  		dev_priv->shared_dplls[i].name = dpll_info[i].name;
>  		dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs;
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 89c5ada1a315..fba8cd36ce0a 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -113,6 +113,8 @@ struct intel_shared_dpll_funcs {
>  };
>  
>  struct intel_shared_dpll {
> +	struct mutex lock;
> +
>  	struct intel_shared_dpll_config config;
>  
>  	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */