Re: [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 
> ---
>  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(>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(>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(>base);
> - unsigned old_mask = pll->active_mask;
> + unsigned old_mask;
>  
>   if (WARN_ON(pll == NULL))
>   return;
>  
> + mutex_lock(>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(>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(>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(>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(_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) */
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 16-03-16 om 17:19 schreef 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.
>
Would making the dpll lock global help? I don't think in practice the locks 
will be contended much,
it's not a performance sensitive path.

~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ander Conselvan De Oliveira]

On Wed, 2016-03-16 at 18:48 +0100, Maarten Lankhorst wrote:
> Op 16-03-16 om 17:19 schreef 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.
> > 
> Would making the dpll lock global help? I don't think in practice the locks
> will be contended much,
> it's not a performance sensitive path.

Yeah, I think that should be enough.

Ander
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx