Hi Maarten, >-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Maarten Lankhorst >Sent: Monday, February 29, 2016 6:22 PM >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Subject: [PATCH 4/4] drm/i915: Add locking to pll updates. > >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. > >Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >--- > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > drivers/gpu/drm/i915/intel_ddi.c | 4 ++++ > drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++++---------- > 3 files changed, 30 insertions(+), 11 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >index c7401b50818c..e20dae7b1fa9 100644 >--- a/drivers/gpu/drm/i915/i915_drv.h >+++ b/drivers/gpu/drm/i915/i915_drv.h >@@ -391,9 +391,10 @@ struct intel_shared_dpll_config { > > struct intel_shared_dpll { > struct intel_shared_dpll_config config; >+ struct mutex lock; > > unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */ >- bool on; /* is the PLL actually active? Disabled during modeset */ >+ bool on, prepared; /* is the PLL actually active? Disabled during modeset */ > const char *name; > /* should match the index in the dev_priv->shared_dplls array */ > enum intel_dpll_id id; >diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >index 21a9b83f3bfc..e23be6e1b846 100644 >--- a/drivers/gpu/drm/i915/intel_ddi.c >+++ b/drivers/gpu/drm/i915/intel_ddi.c >@@ -2514,6 +2514,7 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv) > dev_priv->num_shared_dpll = 3; > > for (i = 0; i < 2; i++) { >+ mutex_init(&dev_priv->shared_dplls[i].lock); > dev_priv->shared_dplls[i].id = i; > dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; > dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable; >@@ -2523,6 +2524,7 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv) > } > > /* SPLL is special, but needs to be initialized anyway.. */ >+ mutex_init(&dev_priv->shared_dplls[i].lock); > dev_priv->shared_dplls[i].id = i; > dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i]; > dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable; >@@ -2650,6 +2652,7 @@ static void skl_shared_dplls_init(struct drm_i915_private *dev_priv) > dev_priv->num_shared_dpll = 3; > > for (i = 0; i < dev_priv->num_shared_dpll; i++) { >+ mutex_init(&dev_priv->shared_dplls[i].lock); > dev_priv->shared_dplls[i].id = i; > dev_priv->shared_dplls[i].name = skl_ddi_pll_names[i]; > dev_priv->shared_dplls[i].disable = skl_ddi_pll_disable; >@@ -2978,6 +2981,7 @@ static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv) > dev_priv->num_shared_dpll = 3; > > for (i = 0; i < dev_priv->num_shared_dpll; i++) { >+ mutex_init(&dev_priv->shared_dplls[i].lock); > dev_priv->shared_dplls[i].id = i; > dev_priv->shared_dplls[i].name = bxt_ddi_pll_names[i]; > dev_priv->shared_dplls[i].disable = bxt_ddi_pll_disable; >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >index 6f2dd3192bac..6b17b77106d2 100644 >--- a/drivers/gpu/drm/i915/intel_display.c >+++ b/drivers/gpu/drm/i915/intel_display.c >@@ -1865,14 +1865,18 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc) > if (WARN_ON(pll == NULL)) > return; > >- WARN_ON(!pll->config.crtc_mask); >- if (pll->active_mask == 0) { >+ mutex_lock(&pll->lock); >+ if (pll->active_mask == 0 && !pll->prepared) { > DRM_DEBUG_DRIVER("setting up %s\n", pll->name); >+ >+ pll->prepared = true; > WARN_ON(pll->on); >+ > assert_shared_dpll_disabled(dev_priv, pll); > > pll->mode_set(dev_priv, pll); > } >+ mutex_unlock(&pll->lock); > } > > /** >@@ -1903,18 +1907,22 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc) > pll->name, hweight32(pll->active_mask), pll->on, Don't we need the lock for this access of 'active_mask' and the WARN_ON() before also ? Thanks, Durga > crtc->base.base.id); > >+ mutex_lock(&pll->lock); > pll->active_mask |= crtc_mask; > > if (pll->active_mask != crtc_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->enable(dev_priv, pll); > pll->on = true; >+ >+out: >+ mutex_unlock(&pll->lock); > } > > static void intel_disable_shared_dpll(struct intel_crtc *crtc) >@@ -1938,18 +1946,21 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc) > pll->name, pll->active_mask, pll->on, > crtc->base.base.id); > >+ mutex_lock(&pll->lock); > pll->active_mask &= ~crtc_mask; >- if (pll->active_mask) { >- assert_shared_dpll_disabled(dev_priv, pll); >- return; >- } >- > assert_shared_dpll_enabled(dev_priv, pll); >+ >+ if (pll->active_mask) >+ goto out; >+ > WARN_ON(!pll->on); > > DRM_DEBUG_KMS("disabling %s\n", pll->name); > pll->disable(dev_priv, pll); >- pll->on = false; >+ pll->on = pll->prepared = false; >+ >+out: >+ mutex_unlock(&pll->lock); > } > > static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv, >@@ -13772,6 +13783,8 @@ static void ibx_pch_dpll_init(struct drm_device *dev) > dev_priv->num_shared_dpll = 2; > > for (i = 0; i < dev_priv->num_shared_dpll; i++) { >+ mutex_init(&dev_priv->shared_dplls[i].lock); >+ > dev_priv->shared_dplls[i].id = i; > dev_priv->shared_dplls[i].name = ibx_pch_dpll_names[i]; > dev_priv->shared_dplls[i].mode_set = ibx_pch_dpll_mode_set; >@@ -15764,6 +15777,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > pll->on = pll->get_hw_state(dev_priv, pll, > &pll->config.hw_state); >+ pll->prepared = pll->on; > pll->active_mask = 0; > pll->config.crtc_mask = 0; > for_each_intel_crtc(dev, crtc) { >@@ -15895,7 +15909,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev) > DRM_DEBUG_KMS("%s enabled but not in use, disabling\n", pll->name); > > pll->disable(dev_priv, pll); >- pll->on = false; >+ pll->prepared = pll->on = false; > } > > if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) >-- >2.1.0 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
