| Message ID | 1381933553-19529-4-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/16 Imre Deak <imre.deak@intel.com>: > There is no hard need for this to be a spin lock, as we don't take these > locks in irq context from anywhere. An upcoming patch will add calls to > punit read/write functions from within regions protected by this lock > and those functions need a mutex in turn. As a solution for that convert > the spin lock to be a mutex. Not even from snd_hda_intel? Did we check this? > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++------------- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ca05f3a..e4354dd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt { > /* Power well structure for haswell */ > struct i915_power_well { > struct drm_device *device; > - spinlock_t lock; > + struct mutex lock; > /* power well enable/disable usage count */ > int count; > int i915_request; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 57d08a2..f7363a8 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev, > if (is_always_on_power_domain(dev, domain)) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > __intel_power_well_get(power_well); > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > void intel_display_power_put(struct drm_device *dev, > @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev, > if (is_always_on_power_domain(dev, domain)) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > __intel_power_well_put(power_well); > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > static struct i915_power_well *hsw_pwr; > @@ -5506,9 +5506,9 @@ void i915_request_power_well(void) > if (WARN_ON(!hsw_pwr)) > return; > > - spin_lock_irq(&hsw_pwr->lock); > + mutex_lock(&hsw_pwr->lock); > __intel_power_well_get(hsw_pwr); > - spin_unlock_irq(&hsw_pwr->lock); > + mutex_unlock(&hsw_pwr->lock); > } > EXPORT_SYMBOL_GPL(i915_request_power_well); > > @@ -5518,9 +5518,9 @@ void i915_release_power_well(void) > if (WARN_ON(!hsw_pwr)) > return; > > - spin_lock_irq(&hsw_pwr->lock); > + mutex_lock(&hsw_pwr->lock); > __intel_power_well_put(hsw_pwr); > - spin_unlock_irq(&hsw_pwr->lock); > + mutex_unlock(&hsw_pwr->lock); > } > EXPORT_SYMBOL_GPL(i915_release_power_well); > > @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev) > hsw_pwr = &dev_priv->power_well; > > hsw_pwr->device = dev; > - spin_lock_init(&hsw_pwr->lock); > + mutex_init(&hsw_pwr->lock); > hsw_pwr->count = 0; > > return 0; > @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) > if (!i915_disable_power_well && !enable) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > > /* > * This function will only ever contribute one > @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) > __intel_power_well_put(power_well); > > out: > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > static void intel_resume_power_well(struct drm_device *dev) > @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev) > if (!HAS_POWER_WELL(dev)) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > __intel_set_power_well(dev, power_well->count > 0); > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > /* > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2013-10-16 at 13:19 -0300, Paulo Zanoni wrote: > 2013/10/16 Imre Deak <imre.deak@intel.com>: > > There is no hard need for this to be a spin lock, as we don't take these > > locks in irq context from anywhere. An upcoming patch will add calls to > > punit read/write functions from within regions protected by this lock > > and those functions need a mutex in turn. As a solution for that convert > > the spin lock to be a mutex. > > Not even from snd_hda_intel? Did we check this? Yea, at least I tried to check this at all call sites and haven't found any place where they request the power well in atomic context. In any case I think it's a design problem if they do .. --Imre > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++------------- > > 2 files changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index ca05f3a..e4354dd 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt { > > /* Power well structure for haswell */ > > struct i915_power_well { > > struct drm_device *device; > > - spinlock_t lock; > > + struct mutex lock; > > /* power well enable/disable usage count */ > > int count; > > int i915_request; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 57d08a2..f7363a8 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev, > > if (is_always_on_power_domain(dev, domain)) > > return; > > > > - spin_lock_irq(&power_well->lock); > > + mutex_lock(&power_well->lock); > > __intel_power_well_get(power_well); > > - spin_unlock_irq(&power_well->lock); > > + mutex_unlock(&power_well->lock); > > } > > > > void intel_display_power_put(struct drm_device *dev, > > @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev, > > if (is_always_on_power_domain(dev, domain)) > > return; > > > > - spin_lock_irq(&power_well->lock); > > + mutex_lock(&power_well->lock); > > __intel_power_well_put(power_well); > > - spin_unlock_irq(&power_well->lock); > > + mutex_unlock(&power_well->lock); > > } > > > > static struct i915_power_well *hsw_pwr; > > @@ -5506,9 +5506,9 @@ void i915_request_power_well(void) > > if (WARN_ON(!hsw_pwr)) > > return; > > > > - spin_lock_irq(&hsw_pwr->lock); > > + mutex_lock(&hsw_pwr->lock); > > __intel_power_well_get(hsw_pwr); > > - spin_unlock_irq(&hsw_pwr->lock); > > + mutex_unlock(&hsw_pwr->lock); > > } > > EXPORT_SYMBOL_GPL(i915_request_power_well); > > > > @@ -5518,9 +5518,9 @@ void i915_release_power_well(void) > > if (WARN_ON(!hsw_pwr)) > > return; > > > > - spin_lock_irq(&hsw_pwr->lock); > > + mutex_lock(&hsw_pwr->lock); > > __intel_power_well_put(hsw_pwr); > > - spin_unlock_irq(&hsw_pwr->lock); > > + mutex_unlock(&hsw_pwr->lock); > > } > > EXPORT_SYMBOL_GPL(i915_release_power_well); > > > > @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev) > > hsw_pwr = &dev_priv->power_well; > > > > hsw_pwr->device = dev; > > - spin_lock_init(&hsw_pwr->lock); > > + mutex_init(&hsw_pwr->lock); > > hsw_pwr->count = 0; > > > > return 0; > > @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) > > if (!i915_disable_power_well && !enable) > > return; > > > > - spin_lock_irq(&power_well->lock); > > + mutex_lock(&power_well->lock); > > > > /* > > * This function will only ever contribute one > > @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) > > __intel_power_well_put(power_well); > > > > out: > > - spin_unlock_irq(&power_well->lock); > > + mutex_unlock(&power_well->lock); > > } > > > > static void intel_resume_power_well(struct drm_device *dev) > > @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev) > > if (!HAS_POWER_WELL(dev)) > > return; > > > > - spin_lock_irq(&power_well->lock); > > + mutex_lock(&power_well->lock); > > __intel_set_power_well(dev, power_well->count > 0); > > - spin_unlock_irq(&power_well->lock); > > + mutex_unlock(&power_well->lock); > > } > > > > /* > > -- > > 1.8.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > >
On Wed, 16 Oct 2013 17:25:50 +0300 Imre Deak <imre.deak@intel.com> wrote: > There is no hard need for this to be a spin lock, as we don't take these > locks in irq context from anywhere. An upcoming patch will add calls to > punit read/write functions from within regions protected by this lock > and those functions need a mutex in turn. As a solution for that convert > the spin lock to be a mutex. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++------------- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ca05f3a..e4354dd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt { > /* Power well structure for haswell */ > struct i915_power_well { > struct drm_device *device; > - spinlock_t lock; > + struct mutex lock; > /* power well enable/disable usage count */ > int count; > int i915_request; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 57d08a2..f7363a8 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev, > if (is_always_on_power_domain(dev, domain)) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > __intel_power_well_get(power_well); > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > void intel_display_power_put(struct drm_device *dev, > @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev, > if (is_always_on_power_domain(dev, domain)) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > __intel_power_well_put(power_well); > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > static struct i915_power_well *hsw_pwr; > @@ -5506,9 +5506,9 @@ void i915_request_power_well(void) > if (WARN_ON(!hsw_pwr)) > return; > > - spin_lock_irq(&hsw_pwr->lock); > + mutex_lock(&hsw_pwr->lock); > __intel_power_well_get(hsw_pwr); > - spin_unlock_irq(&hsw_pwr->lock); > + mutex_unlock(&hsw_pwr->lock); > } > EXPORT_SYMBOL_GPL(i915_request_power_well); > > @@ -5518,9 +5518,9 @@ void i915_release_power_well(void) > if (WARN_ON(!hsw_pwr)) > return; > > - spin_lock_irq(&hsw_pwr->lock); > + mutex_lock(&hsw_pwr->lock); > __intel_power_well_put(hsw_pwr); > - spin_unlock_irq(&hsw_pwr->lock); > + mutex_unlock(&hsw_pwr->lock); > } > EXPORT_SYMBOL_GPL(i915_release_power_well); > > @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev) > hsw_pwr = &dev_priv->power_well; > > hsw_pwr->device = dev; > - spin_lock_init(&hsw_pwr->lock); > + mutex_init(&hsw_pwr->lock); > hsw_pwr->count = 0; > > return 0; > @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) > if (!i915_disable_power_well && !enable) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > > /* > * This function will only ever contribute one > @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) > __intel_power_well_put(power_well); > > out: > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > static void intel_resume_power_well(struct drm_device *dev) > @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev) > if (!HAS_POWER_WELL(dev)) > return; > > - spin_lock_irq(&power_well->lock); > + mutex_lock(&power_well->lock); > __intel_set_power_well(dev, power_well->count > 0); > - spin_unlock_irq(&power_well->lock); > + mutex_unlock(&power_well->lock); > } > > /* Are there ordering requirements we should document? E.g. always take this after the mode config lock or something? Otherwise: Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Fri, Oct 18, 2013 at 11:50:47AM -0700, Jesse Barnes wrote: > Are there ordering requirements we should document? E.g. always take > this after the mode config lock or something? The mode_config lock is pretty much the outermost thing, and for getting it right we have lockdpe. As long as we ensure that we have full coverage with our tests and as long as QA doesn't fumble running the debug kernel builds we should be fine. So imo no need to document the locking. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ca05f3a..e4354dd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt { /* Power well structure for haswell */ struct i915_power_well { struct drm_device *device; - spinlock_t lock; + struct mutex lock; /* power well enable/disable usage count */ int count; int i915_request; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 57d08a2..f7363a8 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev, if (is_always_on_power_domain(dev, domain)) return; - spin_lock_irq(&power_well->lock); + mutex_lock(&power_well->lock); __intel_power_well_get(power_well); - spin_unlock_irq(&power_well->lock); + mutex_unlock(&power_well->lock); } void intel_display_power_put(struct drm_device *dev, @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev, if (is_always_on_power_domain(dev, domain)) return; - spin_lock_irq(&power_well->lock); + mutex_lock(&power_well->lock); __intel_power_well_put(power_well); - spin_unlock_irq(&power_well->lock); + mutex_unlock(&power_well->lock); } static struct i915_power_well *hsw_pwr; @@ -5506,9 +5506,9 @@ void i915_request_power_well(void) if (WARN_ON(!hsw_pwr)) return; - spin_lock_irq(&hsw_pwr->lock); + mutex_lock(&hsw_pwr->lock); __intel_power_well_get(hsw_pwr); - spin_unlock_irq(&hsw_pwr->lock); + mutex_unlock(&hsw_pwr->lock); } EXPORT_SYMBOL_GPL(i915_request_power_well); @@ -5518,9 +5518,9 @@ void i915_release_power_well(void) if (WARN_ON(!hsw_pwr)) return; - spin_lock_irq(&hsw_pwr->lock); + mutex_lock(&hsw_pwr->lock); __intel_power_well_put(hsw_pwr); - spin_unlock_irq(&hsw_pwr->lock); + mutex_unlock(&hsw_pwr->lock); } EXPORT_SYMBOL_GPL(i915_release_power_well); @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev) hsw_pwr = &dev_priv->power_well; hsw_pwr->device = dev; - spin_lock_init(&hsw_pwr->lock); + mutex_init(&hsw_pwr->lock); hsw_pwr->count = 0; return 0; @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) if (!i915_disable_power_well && !enable) return; - spin_lock_irq(&power_well->lock); + mutex_lock(&power_well->lock); /* * This function will only ever contribute one @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable) __intel_power_well_put(power_well); out: - spin_unlock_irq(&power_well->lock); + mutex_unlock(&power_well->lock); } static void intel_resume_power_well(struct drm_device *dev) @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev) if (!HAS_POWER_WELL(dev)) return; - spin_lock_irq(&power_well->lock); + mutex_lock(&power_well->lock); __intel_set_power_well(dev, power_well->count > 0); - spin_unlock_irq(&power_well->lock); + mutex_unlock(&power_well->lock); } /*
There is no hard need for this to be a spin lock, as we don't take these locks in irq context from anywhere. An upcoming patch will add calls to punit read/write functions from within regions protected by this lock and those functions need a mutex in turn. As a solution for that convert the spin lock to be a mutex. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-)