Re: [Intel-gfx] [PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.
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(&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) */ ___ 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.
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.
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
[Intel-gfx] [PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.
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) 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(&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) */ -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx