[Intel-gfx] [PATCH] drm/i915: Lock gmbus/aux mutexes while changing cdclk

[Chris Wilson]

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 at 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 at 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