diff mbox series

[10/15] drm/i915: Sprinkle a few missing locks around shared DDI clock registers

Message ID 20210201183343.15292-11-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Clean up the DDI clock routing mess | expand

Commit Message

Ville Syrjala Feb. 1, 2021, 6:33 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current code attempts to protect the RMWs into global
clock routing registers with a mutex, but forgets to do so
in a few places. Let's remedy that.

Note that at the moment we serialize all modesets onto single
wq, so this shouldn't actually matter. But maybe one day we
wish to attempt parallel modesets again...

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Lucas De Marchi Feb. 1, 2021, 7:15 p.m. UTC | #1 
On Mon, Feb 01, 2021 at 08:33:38PM +0200, Ville Syrjälä wrote:
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>The current code attempts to protect the RMWs into global
>clock routing registers with a mutex, but forgets to do so
>in a few places. Let's remedy that.

Forgets or doesn't do on purpose? In the first patches in this series I
was actually wondering why do we even have to take the lock you were
adding for some platforms.

>
>Note that at the moment we serialize all modesets onto single
>wq, so this shouldn't actually matter. But maybe one day we
>wish to attempt parallel modesets again...

and this answers that. But why don't we just stop getting the lock and
reintroduce them if/when we attempt parallel modeset?

Lucas De Marchi

>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/display/intel_ddi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>index 7137929f58bd..93552f3c2c43 100644
>--- a/drivers/gpu/drm/i915/display/intel_ddi.c
>+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>@@ -3463,8 +3463,12 @@ static void cnl_ddi_disable_clock(struct intel_encoder *encoder)
> 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> 	enum port port = encoder->port;
>
>+	mutex_lock(&dev_priv->dpll.lock);
>+
> 	intel_de_rmw(dev_priv, DPCLKA_CFGCR0,
> 		     0, DPCLKA_CFGCR0_DDI_CLK_OFF(port));
>+
>+	mutex_unlock(&dev_priv->dpll.lock);
> }
>
> static void skl_ddi_enable_clock(struct intel_encoder *encoder,
>@@ -3493,8 +3497,12 @@ static void skl_ddi_disable_clock(struct intel_encoder *encoder)
> 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> 	enum port port = encoder->port;
>
>+	mutex_lock(&dev_priv->dpll.lock);
>+
> 	intel_de_rmw(dev_priv, DPLL_CTRL2,
> 		     0, DPLL_CTRL2_DDI_CLK_OFF(port));
>+
>+	mutex_unlock(&dev_priv->dpll.lock);
> }
>
> static void hsw_ddi_enable_clock(struct intel_encoder *encoder,
>-- 
>2.26.2
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala Feb. 1, 2021, 7:21 p.m. UTC | #2 
On Mon, Feb 01, 2021 at 11:15:01AM -0800, Lucas De Marchi wrote:
> On Mon, Feb 01, 2021 at 08:33:38PM +0200, Ville Syrjälä wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >The current code attempts to protect the RMWs into global
> >clock routing registers with a mutex, but forgets to do so
> >in a few places. Let's remedy that.
> 
> Forgets or doesn't do on purpose? In the first patches in this series I
> was actually wondering why do we even have to take the lock you were
> adding for some platforms.
> 
> >
> >Note that at the moment we serialize all modesets onto single
> >wq, so this shouldn't actually matter. But maybe one day we
> >wish to attempt parallel modesets again...
> 
> and this answers that. But why don't we just stop getting the lock and
> reintroduce them if/when we attempt parallel modeset?

I suspect that will pretty much guarantee that it'll never
happen, because last time it was tried no one bothered to
do any kind of code review for these kinds of problems. So
I think we'll just have to keep sprinkling the locks as we
find such code, and then maybe one day we can attempt to
throw the switch again.

> 
> Lucas De Marchi
> 
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >index 7137929f58bd..93552f3c2c43 100644
> >--- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >@@ -3463,8 +3463,12 @@ static void cnl_ddi_disable_clock(struct intel_encoder *encoder)
> > 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > 	enum port port = encoder->port;
> >
> >+	mutex_lock(&dev_priv->dpll.lock);
> >+
> > 	intel_de_rmw(dev_priv, DPCLKA_CFGCR0,
> > 		     0, DPCLKA_CFGCR0_DDI_CLK_OFF(port));
> >+
> >+	mutex_unlock(&dev_priv->dpll.lock);
> > }
> >
> > static void skl_ddi_enable_clock(struct intel_encoder *encoder,
> >@@ -3493,8 +3497,12 @@ static void skl_ddi_disable_clock(struct intel_encoder *encoder)
> > 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > 	enum port port = encoder->port;
> >
> >+	mutex_lock(&dev_priv->dpll.lock);
> >+
> > 	intel_de_rmw(dev_priv, DPLL_CTRL2,
> > 		     0, DPLL_CTRL2_DDI_CLK_OFF(port));
> >+
> >+	mutex_unlock(&dev_priv->dpll.lock);
> > }
> >
> > static void hsw_ddi_enable_clock(struct intel_encoder *encoder,
> >-- 
> >2.26.2
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 7137929f58bd..93552f3c2c43 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3463,8 +3463,12 @@  static void cnl_ddi_disable_clock(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = encoder->port;
 
+	mutex_lock(&dev_priv->dpll.lock);
+
 	intel_de_rmw(dev_priv, DPCLKA_CFGCR0,
 		     0, DPCLKA_CFGCR0_DDI_CLK_OFF(port));
+
+	mutex_unlock(&dev_priv->dpll.lock);
 }
 
 static void skl_ddi_enable_clock(struct intel_encoder *encoder,
@@ -3493,8 +3497,12 @@  static void skl_ddi_disable_clock(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = encoder->port;
 
+	mutex_lock(&dev_priv->dpll.lock);
+
 	intel_de_rmw(dev_priv, DPLL_CTRL2,
 		     0, DPLL_CTRL2_DDI_CLK_OFF(port));
+
+	mutex_unlock(&dev_priv->dpll.lock);
 }
 
 static void hsw_ddi_enable_clock(struct intel_encoder *encoder,