Re: [PATCH 4/4] drm/i915: Add locking to pll updates.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux