[Intel-gfx] [PATCH 4/4] drm/i915: Add locking to pll updates.
R, Durgadoss
durgadoss.r at intel.com
Tue Mar 1 17:08:07 UTC 2016
Hi Maarten,
>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Maarten Lankhorst
>Sent: Monday, February 29, 2016 6:22 PM
>To: intel-gfx at lists.freedesktop.org
>Subject: [Intel-gfx] [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 at linux.intel.com>
>---
> 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 at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list