| Message ID | 20200302174442.5803-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Lock gmbus/aux mutexes while changing cdclk | expand |
On Mon, 02 Mar 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. > > v2: Use mutex_lock_nest_lock() (Chris) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I don't think my review comments were invalid in general, but I don't want to block this fairly straightforward fix either. Acked-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 0741d643455b..979a0241fdcb 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1868,6 +1868,8 @@ 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; > + > if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config)) > return; > > @@ -1876,8 +1878,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_nest_lock(&intel_dp->aux.hw_mutex, > + &dev_priv->gmbus_mutex); > + } > + > 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")) {
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 0741d643455b..979a0241fdcb 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -1868,6 +1868,8 @@ 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; + if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_config)) return; @@ -1876,8 +1878,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_nest_lock(&intel_dp->aux.hw_mutex, + &dev_priv->gmbus_mutex); + } + 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")) {