drm/i915: Lock gmbus/aux mutexes while changing cdclk
diff mbox series

Message ID 20200227193954.5585-1-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: Lock gmbus/aux mutexes while changing cdclk
Related show

Commit Message

Ville Syrjälä Feb. 27, 2020, 7:39 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

gmbus/aux may be clocked by cdclk, thus we should make sure no
transfers are ongoing while the cdclk frequency is being changed.
We do that by simply grabbing all the gmbus/aux mutexes. No one
else should be holding any more than one of those at a time so
the lock ordering here shouldn't matter.

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

Comments

Chris Wilson Feb. 28, 2020, 8:28 a.m. UTC | #1
Quoting Ville Syrjala (2020-02-27 19:39:54)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> gmbus/aux may be clocked by cdclk, thus we should make sure no
> transfers are ongoing while the cdclk frequency is being changed.
> We do that by simply grabbing all the gmbus/aux mutexes. No one
> else should be holding any more than one of those at a time so
> the lock ordering here shouldn't matter.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 0741d643455b..f69bf4a4eb1c 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>                             const struct intel_cdclk_config *cdclk_config,
>                             enum pipe pipe)
>  {
> +       struct intel_encoder *encoder;
> +       unsigned int aux_mutex_lockclass = 0;
> +
>         if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
>                 return;
>  
> @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  
>         intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
>  
> +       /*
> +        * Lock aux/gmbus while we change cdclk in case those
> +        * functions use cdclk. Not all platforms/ports do,
> +        * but we'll lock them all for simplicity.
> +        */
> +       mutex_lock(&dev_priv->gmbus_mutex);
> +       for_each_intel_dp(&dev_priv->drm, encoder) {
> +               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +               mutex_lock_nested(&intel_dp->aux.hw_mutex,
> +                                 aux_mutex_lockclass++);

mutex_lock_nest_lock(&intel_dp->aux.hw_mutex, &dev_priv->gmbus_mutex);
?
-Chris
Jani Nikula Feb. 28, 2020, 9:06 a.m. UTC | #2
On Thu, 27 Feb 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> gmbus/aux may be clocked by cdclk, thus we should make sure no
> transfers are ongoing while the cdclk frequency is being changed.
> We do that by simply grabbing all the gmbus/aux mutexes. No one
> else should be holding any more than one of those at a time so
> the lock ordering here shouldn't matter.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 0741d643455b..f69bf4a4eb1c 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  			    const struct intel_cdclk_config *cdclk_config,
>  			    enum pipe pipe)
>  {
> +	struct intel_encoder *encoder;
> +	unsigned int aux_mutex_lockclass = 0;
> +
>  	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
>  		return;
>  
> @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  
>  	intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
>  
> +	/*
> +	 * Lock aux/gmbus while we change cdclk in case those
> +	 * functions use cdclk. Not all platforms/ports do,
> +	 * but we'll lock them all for simplicity.
> +	 */
> +	mutex_lock(&dev_priv->gmbus_mutex);
> +	for_each_intel_dp(&dev_priv->drm, encoder) {
> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		mutex_lock_nested(&intel_dp->aux.hw_mutex,
> +				  aux_mutex_lockclass++);
> +	}
> +
>  	dev_priv->display.set_cdclk(dev_priv, cdclk_config, pipe);
>  
> +	for_each_intel_dp(&dev_priv->drm, encoder) {
> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		mutex_unlock(&intel_dp->aux.hw_mutex);
> +	}
> +	mutex_unlock(&dev_priv->gmbus_mutex);
> +

I'm becoming increasingly sensitive to directly touching the private
parts of other modules... gmbus_mutex is really for intel_gmbus.c and
aux.hw_mutex for drm_dp_helper.c.

One could also argue that the cdclk is a lower level function used by
higher level functions aux/gmbus, and it seems like the higher level
function should lock the cdclk while it depends on it, not the other way
around.

BR,
Jani.



>  	if (drm_WARN(&dev_priv->drm,
>  		     intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config),
>  		     "cdclk state doesn't match!\n")) {
Ville Syrjälä Feb. 28, 2020, 11:10 a.m. UTC | #3
On Fri, Feb 28, 2020 at 11:06:41AM +0200, Jani Nikula wrote:
> On Thu, 27 Feb 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > gmbus/aux may be clocked by cdclk, thus we should make sure no
> > transfers are ongoing while the cdclk frequency is being changed.
> > We do that by simply grabbing all the gmbus/aux mutexes. No one
> > else should be holding any more than one of those at a time so
> > the lock ordering here shouldn't matter.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 0741d643455b..f69bf4a4eb1c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >  			    const struct intel_cdclk_config *cdclk_config,
> >  			    enum pipe pipe)
> >  {
> > +	struct intel_encoder *encoder;
> > +	unsigned int aux_mutex_lockclass = 0;
> > +
> >  	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
> >  		return;
> >  
> > @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >  
> >  	intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
> >  
> > +	/*
> > +	 * Lock aux/gmbus while we change cdclk in case those
> > +	 * functions use cdclk. Not all platforms/ports do,
> > +	 * but we'll lock them all for simplicity.
> > +	 */
> > +	mutex_lock(&dev_priv->gmbus_mutex);
> > +	for_each_intel_dp(&dev_priv->drm, encoder) {
> > +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +		mutex_lock_nested(&intel_dp->aux.hw_mutex,
> > +				  aux_mutex_lockclass++);
> > +	}
> > +
> >  	dev_priv->display.set_cdclk(dev_priv, cdclk_config, pipe);
> >  
> > +	for_each_intel_dp(&dev_priv->drm, encoder) {
> > +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +		mutex_unlock(&intel_dp->aux.hw_mutex);
> > +	}
> > +	mutex_unlock(&dev_priv->gmbus_mutex);
> > +
> 
> I'm becoming increasingly sensitive to directly touching the private
> parts of other modules... gmbus_mutex is really for intel_gmbus.c and
> aux.hw_mutex for drm_dp_helper.c.
> 
> One could also argue that the cdclk is a lower level function used by
> higher level functions aux/gmbus, and it seems like the higher level
> function should lock the cdclk while it depends on it, not the other way
> around.

That would require a rwsem. Otherwise it all gets serialized needlessly.
Not sure what's the state of rwsems these days, but IIRC at some point
the rt patches converted them all to normal mutexes.
Ville Syrjälä Feb. 28, 2020, 1:37 p.m. UTC | #4
On Fri, Feb 28, 2020 at 08:28:36AM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2020-02-27 19:39:54)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > gmbus/aux may be clocked by cdclk, thus we should make sure no
> > transfers are ongoing while the cdclk frequency is being changed.
> > We do that by simply grabbing all the gmbus/aux mutexes. No one
> > else should be holding any more than one of those at a time so
> > the lock ordering here shouldn't matter.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 0741d643455b..f69bf4a4eb1c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >                             const struct intel_cdclk_config *cdclk_config,
> >                             enum pipe pipe)
> >  {
> > +       struct intel_encoder *encoder;
> > +       unsigned int aux_mutex_lockclass = 0;
> > +
> >         if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
> >                 return;
> >  
> > @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >  
> >         intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
> >  
> > +       /*
> > +        * Lock aux/gmbus while we change cdclk in case those
> > +        * functions use cdclk. Not all platforms/ports do,
> > +        * but we'll lock them all for simplicity.
> > +        */
> > +       mutex_lock(&dev_priv->gmbus_mutex);
> > +       for_each_intel_dp(&dev_priv->drm, encoder) {
> > +               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +               mutex_lock_nested(&intel_dp->aux.hw_mutex,
> > +                                 aux_mutex_lockclass++);
> 
> mutex_lock_nest_lock(&intel_dp->aux.hw_mutex, &dev_priv->gmbus_mutex);
> ?

That does seems to work. Not sure what it means though since no docs
and I was too lazy to read the code. Does it say "as long as we hold
nest_lock the order doesn't matter"?
Chris Wilson Feb. 28, 2020, 1:45 p.m. UTC | #5
Quoting Ville Syrjälä (2020-02-28 13:37:56)
> On Fri, Feb 28, 2020 at 08:28:36AM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2020-02-27 19:39:54)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > gmbus/aux may be clocked by cdclk, thus we should make sure no
> > > transfers are ongoing while the cdclk frequency is being changed.
> > > We do that by simply grabbing all the gmbus/aux mutexes. No one
> > > else should be holding any more than one of those at a time so
> > > the lock ordering here shouldn't matter.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index 0741d643455b..f69bf4a4eb1c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >                             const struct intel_cdclk_config *cdclk_config,
> > >                             enum pipe pipe)
> > >  {
> > > +       struct intel_encoder *encoder;
> > > +       unsigned int aux_mutex_lockclass = 0;
> > > +
> > >         if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
> > >                 return;
> > >  
> > > @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >  
> > >         intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
> > >  
> > > +       /*
> > > +        * Lock aux/gmbus while we change cdclk in case those
> > > +        * functions use cdclk. Not all platforms/ports do,
> > > +        * but we'll lock them all for simplicity.
> > > +        */
> > > +       mutex_lock(&dev_priv->gmbus_mutex);
> > > +       for_each_intel_dp(&dev_priv->drm, encoder) {
> > > +               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +               mutex_lock_nested(&intel_dp->aux.hw_mutex,
> > > +                                 aux_mutex_lockclass++);
> > 
> > mutex_lock_nest_lock(&intel_dp->aux.hw_mutex, &dev_priv->gmbus_mutex);
> > ?
> 
> That does seems to work. Not sure what it means though since no docs
> and I was too lazy to read the code. Does it say "as long as we hold
> nest_lock the order doesn't matter"?

Yes, that's exactly what it means.
-Chris
Ville Syrjälä Feb. 28, 2020, 2:10 p.m. UTC | #6
On Fri, Feb 28, 2020 at 01:10:45PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 28, 2020 at 11:06:41AM +0200, Jani Nikula wrote:
> > On Thu, 27 Feb 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > gmbus/aux may be clocked by cdclk, thus we should make sure no
> > > transfers are ongoing while the cdclk frequency is being changed.
> > > We do that by simply grabbing all the gmbus/aux mutexes. No one
> > > else should be holding any more than one of those at a time so
> > > the lock ordering here shouldn't matter.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index 0741d643455b..f69bf4a4eb1c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >  			    const struct intel_cdclk_config *cdclk_config,
> > >  			    enum pipe pipe)
> > >  {
> > > +	struct intel_encoder *encoder;
> > > +	unsigned int aux_mutex_lockclass = 0;
> > > +
> > >  	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
> > >  		return;
> > >  
> > > @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >  
> > >  	intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
> > >  
> > > +	/*
> > > +	 * Lock aux/gmbus while we change cdclk in case those
> > > +	 * functions use cdclk. Not all platforms/ports do,
> > > +	 * but we'll lock them all for simplicity.
> > > +	 */
> > > +	mutex_lock(&dev_priv->gmbus_mutex);
> > > +	for_each_intel_dp(&dev_priv->drm, encoder) {
> > > +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +		mutex_lock_nested(&intel_dp->aux.hw_mutex,
> > > +				  aux_mutex_lockclass++);
> > > +	}
> > > +
> > >  	dev_priv->display.set_cdclk(dev_priv, cdclk_config, pipe);
> > >  
> > > +	for_each_intel_dp(&dev_priv->drm, encoder) {
> > > +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +		mutex_unlock(&intel_dp->aux.hw_mutex);
> > > +	}
> > > +	mutex_unlock(&dev_priv->gmbus_mutex);
> > > +
> > 
> > I'm becoming increasingly sensitive to directly touching the private
> > parts of other modules... gmbus_mutex is really for intel_gmbus.c and
> > aux.hw_mutex for drm_dp_helper.c.
> > 
> > One could also argue that the cdclk is a lower level function used by
> > higher level functions aux/gmbus, and it seems like the higher level
> > function should lock the cdclk while it depends on it, not the other way
> > around.
> 
> That would require a rwsem. Otherwise it all gets serialized needlessly.
> Not sure what's the state of rwsems these days, but IIRC at some point
> the rt patches converted them all to normal mutexes.

Some googling suggests that my infromation may be out of date. So we
could introduce an rwsem for this I guess. Would add a bit more cost
to the common codepaths though (not that they're really performance
critical) whereas this version only adds extra cost to the much more
rare .set_cdclk() path.
Ville Syrjälä March 2, 2020, 5:47 p.m. UTC | #7
On Fri, Feb 28, 2020 at 04:10:42PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 28, 2020 at 01:10:45PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 28, 2020 at 11:06:41AM +0200, Jani Nikula wrote:
> > > On Thu, 27 Feb 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > gmbus/aux may be clocked by cdclk, thus we should make sure no
> > > > transfers are ongoing while the cdclk frequency is being changed.
> > > > We do that by simply grabbing all the gmbus/aux mutexes. No one
> > > > else should be holding any more than one of those at a time so
> > > > the lock ordering here shouldn't matter.
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++++++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index 0741d643455b..f69bf4a4eb1c 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -1868,6 +1868,9 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > >  			    const struct intel_cdclk_config *cdclk_config,
> > > >  			    enum pipe pipe)
> > > >  {
> > > > +	struct intel_encoder *encoder;
> > > > +	unsigned int aux_mutex_lockclass = 0;
> > > > +
> > > >  	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
> > > >  		return;
> > > >  
> > > > @@ -1876,8 +1879,28 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > >  
> > > >  	intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
> > > >  
> > > > +	/*
> > > > +	 * Lock aux/gmbus while we change cdclk in case those
> > > > +	 * functions use cdclk. Not all platforms/ports do,
> > > > +	 * but we'll lock them all for simplicity.
> > > > +	 */
> > > > +	mutex_lock(&dev_priv->gmbus_mutex);
> > > > +	for_each_intel_dp(&dev_priv->drm, encoder) {
> > > > +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > +
> > > > +		mutex_lock_nested(&intel_dp->aux.hw_mutex,
> > > > +				  aux_mutex_lockclass++);
> > > > +	}
> > > > +
> > > >  	dev_priv->display.set_cdclk(dev_priv, cdclk_config, pipe);
> > > >  
> > > > +	for_each_intel_dp(&dev_priv->drm, encoder) {
> > > > +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > +
> > > > +		mutex_unlock(&intel_dp->aux.hw_mutex);
> > > > +	}
> > > > +	mutex_unlock(&dev_priv->gmbus_mutex);
> > > > +
> > > 
> > > I'm becoming increasingly sensitive to directly touching the private
> > > parts of other modules... gmbus_mutex is really for intel_gmbus.c and
> > > aux.hw_mutex for drm_dp_helper.c.
> > > 
> > > One could also argue that the cdclk is a lower level function used by
> > > higher level functions aux/gmbus, and it seems like the higher level
> > > function should lock the cdclk while it depends on it, not the other way
> > > around.
> > 
> > That would require a rwsem. Otherwise it all gets serialized needlessly.
> > Not sure what's the state of rwsems these days, but IIRC at some point
> > the rt patches converted them all to normal mutexes.
> 
> Some googling suggests that my infromation may be out of date. So we
> could introduce an rwsem for this I guess. Would add a bit more cost
> to the common codepaths though (not that they're really performance
> critical) whereas this version only adds extra cost to the much more
> rare .set_cdclk() path.

Decided to not bother with the rwsem and just sent a v2 with
the mutex_lock_nest_lock().
Chris Wilson March 3, 2020, 8:22 a.m. UTC | #8
Quoting Patchwork (2020-03-03 08:20:02)
> == Series Details ==
> 
> Series: drm/i915: Lock gmbus/aux mutexes while changing cdclk (rev2)
> URL   : https://patchwork.freedesktop.org/series/74039/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_8046_full -> Patchwork_16785_full
> ====================================================
> 
> Summary
> -------
> 
>   **SUCCESS**
> 
>   No regressions found.

v2: Fell foul of SPF, so I'll reply here instead
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 0741d643455b..f69bf4a4eb1c 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1868,6 +1868,9 @@  static void intel_set_cdclk(struct drm_i915_private *dev_priv,
 			    const struct intel_cdclk_config *cdclk_config,
 			    enum pipe pipe)
 {
+	struct intel_encoder *encoder;
+	unsigned int aux_mutex_lockclass = 0;
+
 	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config))
 		return;
 
@@ -1876,8 +1879,28 @@  static void intel_set_cdclk(struct drm_i915_private *dev_priv,
 
 	intel_dump_cdclk_config(cdclk_config, "Changing CDCLK to");
 
+	/*
+	 * Lock aux/gmbus while we change cdclk in case those
+	 * functions use cdclk. Not all platforms/ports do,
+	 * but we'll lock them all for simplicity.
+	 */
+	mutex_lock(&dev_priv->gmbus_mutex);
+	for_each_intel_dp(&dev_priv->drm, encoder) {
+		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+		mutex_lock_nested(&intel_dp->aux.hw_mutex,
+				  aux_mutex_lockclass++);
+	}
+
 	dev_priv->display.set_cdclk(dev_priv, cdclk_config, pipe);
 
+	for_each_intel_dp(&dev_priv->drm, encoder) {
+		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+		mutex_unlock(&intel_dp->aux.hw_mutex);
+	}
+	mutex_unlock(&dev_priv->gmbus_mutex);
+
 	if (drm_WARN(&dev_priv->drm,
 		     intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config),
 		     "cdclk state doesn't match!\n")) {