| Message ID | 978418739-1927-3-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote: > @@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val) > return -ENODEV; > > drm_modeset_lock_all(dev); > + mutex_lock(&dev_priv->fbc.lock); > + > *val = dev_priv->fbc.false_color; > + > + mutex_unlock(&dev_priv->fbc.lock); > drm_modeset_unlock_all(dev); A read of of a single value doesn't need any of these locks. > > return 0;
On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote: > void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > unsigned int frontbuffer_bits, > enum fb_op_origin origin) > @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > if (origin == ORIGIN_GTT) > return; > > + mutex_lock(&dev_priv->fbc.lock); > + > if (dev_priv->fbc.enabled) > fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); > else if (dev_priv->fbc.fbc_work) > @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits); > > if (dev_priv->fbc.busy_bits) > - intel_fbc_disable(dev); > + __intel_fbc_disable(dev); > + > + mutex_unlock(&dev_priv->fbc.lock); > } > > void intel_fbc_flush(struct drm_i915_private *dev_priv, > @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, > { > struct drm_device *dev = dev_priv->dev; > > + mutex_lock(&dev_priv->fbc.lock); > + > if (!dev_priv->fbc.busy_bits) > - return; > + goto out; > > dev_priv->fbc.busy_bits &= ~frontbuffer_bits; > > if (!dev_priv->fbc.busy_bits) > - intel_fbc_update(dev); > + __intel_fbc_update(dev); > + > +out: > + mutex_unlock(&dev_priv->fbc.lock); > } These busy bits are locked higher up. In fact I want to migrate that lock to a spinlock, which has implications here. I didn't see anything that mandated using a mutex for fbc, right? -Chris
On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Make sure we're not gonna have weird races in really weird cases where > a lot of different CRTCs are doing rendering and modesets at the same > time. > > v2: > - Rebase (6 months later) > - Also lock debugfs and stolen. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem_stolen.c | 7 ++++ > drivers/gpu/drm/i915/intel_fbc.c | 73 +++++++++++++++++++++++++++------- > 4 files changed, 75 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index c49fe2a..c99be0e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1593,6 +1593,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > } > > intel_runtime_pm_get(dev_priv); > + mutex_lock(&dev_priv->fbc.lock); > > if (intel_fbc_enabled(dev)) > seq_puts(m, "FBC enabled\n"); > @@ -1605,6 +1606,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > yesno(I915_READ(FBC_STATUS2) & > FBC_COMPRESSION_MASK)); > > + mutex_unlock(&dev_priv->fbc.lock); > intel_runtime_pm_put(dev_priv); > > return 0; > @@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val) > return -ENODEV; > > drm_modeset_lock_all(dev); Modeset locks aren't required any more here. That was just to prevent concurrent modesets, but if we have a new unified fbc lock for all of this it's not needed any more. > + mutex_lock(&dev_priv->fbc.lock); > + > *val = dev_priv->fbc.false_color; > + > + mutex_unlock(&dev_priv->fbc.lock); > drm_modeset_unlock_all(dev); > > return 0; > @@ -1635,6 +1641,7 @@ static int i915_fbc_fc_set(void *data, u64 val) > return -ENODEV; > > drm_modeset_lock_all(dev); Same. > + mutex_lock(&dev_priv->fbc.lock); > > reg = I915_READ(ILK_DPFC_CONTROL); > dev_priv->fbc.false_color = val; > @@ -1643,6 +1650,7 @@ static int i915_fbc_fc_set(void *data, u64 val) > (reg | FBC_CTL_FALSE_COLOR) : > (reg & ~FBC_CTL_FALSE_COLOR)); > > + mutex_unlock(&dev_priv->fbc.lock); > drm_modeset_unlock_all(dev); > return 0; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 491ef0c..9ab04f7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -891,6 +891,7 @@ enum fb_op_origin { > }; > > struct i915_fbc { > + struct mutex lock; > unsigned long uncompressed_size; > unsigned threshold; > unsigned int fb_id; > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 348ed5a..947ddb4 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -250,6 +250,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + lockdep_assert_held(&dev_priv->fbc.lock); Please use WARN_ON(!mutex_locked). lockdep_assert_held is a no-op when lockdep is compiled out. And the benefit of also checking that the current task is the lock owner is imo negligible (it only matters when you have an actual race, which almost never happens). Also see my comment on the next patch, stolen memory management is protect by dev->struct_mutex actually. Locking looks good otherwise, but I guess we need to re-audit all this once it's all settled down into the new design. > + > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > return -ENODEV; > > @@ -266,6 +268,8 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + lockdep_assert_held(&dev_priv->fbc.lock); > + > if (dev_priv->fbc.uncompressed_size == 0) > return; > > @@ -286,7 +290,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > return; > > + mutex_lock(&dev_priv->fbc.lock); > i915_gem_stolen_cleanup_compression(dev); > + mutex_unlock(&dev_priv->fbc.lock); > + > drm_mm_takedown(&dev_priv->mm.stolen); > } > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 9e55b9b..31ff88b 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -336,6 +336,7 @@ static void intel_fbc_work_fn(struct work_struct *__work) > struct drm_i915_private *dev_priv = dev->dev_private; > > mutex_lock(&dev->struct_mutex); > + mutex_lock(&dev_priv->fbc.lock); > if (work == dev_priv->fbc.fbc_work) { > /* Double check that we haven't switched fb without cancelling > * the prior work. > @@ -350,6 +351,7 @@ static void intel_fbc_work_fn(struct work_struct *__work) > > dev_priv->fbc.fbc_work = NULL; > } > + mutex_unlock(&dev_priv->fbc.lock); > mutex_unlock(&dev->struct_mutex); > > kfree(work); > @@ -357,6 +359,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) > > static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) > { > + lockdep_assert_held(&dev_priv->fbc.lock); Small note for the future: The async worker cancelling scheme used by fbc doesn't match what psr/drrs use - they all cancel the work synchronously, but outside of any locked critical section. We couldn't do that with fbc because dev->struct_mutex is too big. But with the new locking it might make sense to switch to the same design for consistency. But we can only do that once fbc looks more like psr/drrs with dedicated enable/disable, and that's still a way off. Cheers, Daniel > + > if (dev_priv->fbc.fbc_work == NULL) > return; > > @@ -384,6 +388,8 @@ static void intel_fbc_enable(struct drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > + lockdep_assert_held(&dev_priv->fbc.lock); > + > if (!dev_priv->display.enable_fbc) > return; > > @@ -418,16 +424,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc) > schedule_delayed_work(&work->work, msecs_to_jiffies(50)); > } > > -/** > - * intel_fbc_disable - disable FBC > - * @dev: the drm_device > - * > - * This function disables FBC. > - */ > -void intel_fbc_disable(struct drm_device *dev) > +static void __intel_fbc_disable(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + lockdep_assert_held(&dev_priv->fbc.lock); > + > intel_fbc_cancel_work(dev_priv); > > if (!dev_priv->display.disable_fbc) > @@ -437,6 +439,21 @@ void intel_fbc_disable(struct drm_device *dev) > dev_priv->fbc.crtc = NULL; > } > > +/** > + * intel_fbc_disable - disable FBC > + * @dev: the drm_device > + * > + * This function disables FBC. > + */ > +void intel_fbc_disable(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&dev_priv->fbc.lock); > + __intel_fbc_disable(dev); > + mutex_unlock(&dev_priv->fbc.lock); > +} > + > const char *intel_no_fbc_reason_str(enum no_fbc_reason reason) > { > switch (reason) { > @@ -516,7 +533,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv) > } > > /** > - * intel_fbc_update - enable/disable FBC as needed > + * __intel_fbc_update - enable/disable FBC as needed, unlocked > * @dev: the drm_device > * > * Set up the framebuffer compression hardware at mode set time. We > @@ -534,7 +551,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv) > * > * We need to enable/disable FBC on a global basis. > */ > -void intel_fbc_update(struct drm_device *dev) > +static void __intel_fbc_update(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc = NULL; > @@ -544,6 +561,8 @@ void intel_fbc_update(struct drm_device *dev) > const struct drm_display_mode *adjusted_mode; > unsigned int max_width, max_height; > > + lockdep_assert_held(&dev_priv->fbc.lock); > + > if (!HAS_FBC(dev)) > return; > > @@ -665,7 +684,7 @@ void intel_fbc_update(struct drm_device *dev) > * some point. And we wait before enabling FBC anyway. > */ > DRM_DEBUG_KMS("disabling active FBC for update\n"); > - intel_fbc_disable(dev); > + __intel_fbc_disable(dev); > } > > intel_fbc_enable(crtc); > @@ -676,11 +695,26 @@ out_disable: > /* Multiple disables should be harmless */ > if (intel_fbc_enabled(dev)) { > DRM_DEBUG_KMS("unsupported config, disabling FBC\n"); > - intel_fbc_disable(dev); > + __intel_fbc_disable(dev); > } > i915_gem_stolen_cleanup_compression(dev); > } > > +/* > + * intel_fbc_update - enable/disable FBC as needed > + * @dev: the drm_device > + * > + * This function reevaluates the overall state and enables or disables FBC. > + */ > +void intel_fbc_update(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + mutex_lock(&dev_priv->fbc.lock); > + __intel_fbc_update(dev); > + mutex_unlock(&dev_priv->fbc.lock); > +} > + > void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > unsigned int frontbuffer_bits, > enum fb_op_origin origin) > @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > if (origin == ORIGIN_GTT) > return; > > + mutex_lock(&dev_priv->fbc.lock); > + > if (dev_priv->fbc.enabled) > fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); > else if (dev_priv->fbc.fbc_work) > @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, > dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits); > > if (dev_priv->fbc.busy_bits) > - intel_fbc_disable(dev); > + __intel_fbc_disable(dev); > + > + mutex_unlock(&dev_priv->fbc.lock); > } > > void intel_fbc_flush(struct drm_i915_private *dev_priv, > @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, > { > struct drm_device *dev = dev_priv->dev; > > + mutex_lock(&dev_priv->fbc.lock); > + > if (!dev_priv->fbc.busy_bits) > - return; > + goto out; > > dev_priv->fbc.busy_bits &= ~frontbuffer_bits; > > if (!dev_priv->fbc.busy_bits) > - intel_fbc_update(dev); > + __intel_fbc_update(dev); > + > +out: > + mutex_unlock(&dev_priv->fbc.lock); > } > > /** > @@ -729,6 +772,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv) > { > enum pipe pipe; > > + mutex_init(&dev_priv->fbc.lock); > + > if (!HAS_FBC(dev_priv)) { > dev_priv->fbc.enabled = false; > dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED; > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote: >> void intel_fbc_invalidate(struct drm_i915_private *dev_priv, >> unsigned int frontbuffer_bits, >> enum fb_op_origin origin) >> @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, >> if (origin == ORIGIN_GTT) >> return; >> >> + mutex_lock(&dev_priv->fbc.lock); >> + >> if (dev_priv->fbc.enabled) >> fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); >> else if (dev_priv->fbc.fbc_work) >> @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, >> dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits); >> >> if (dev_priv->fbc.busy_bits) >> - intel_fbc_disable(dev); >> + __intel_fbc_disable(dev); >> + >> + mutex_unlock(&dev_priv->fbc.lock); >> } >> >> void intel_fbc_flush(struct drm_i915_private *dev_priv, >> @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, >> { >> struct drm_device *dev = dev_priv->dev; >> >> + mutex_lock(&dev_priv->fbc.lock); >> + >> if (!dev_priv->fbc.busy_bits) >> - return; >> + goto out; >> >> dev_priv->fbc.busy_bits &= ~frontbuffer_bits; >> >> if (!dev_priv->fbc.busy_bits) >> - intel_fbc_update(dev); >> + __intel_fbc_update(dev); >> + >> +out: >> + mutex_unlock(&dev_priv->fbc.lock); >> } > > These busy bits are locked higher up. In fact I want to migrate that > lock to a spinlock, which has implications here. I didn't see anything > that mandated using a mutex for fbc, right? I didn't understand your idea. You want to replace the whole FBC mutex for a spinlock? Why? Please notice that we have dev_priv->fbc.busy_bits and also dev_priv->fb_tracking.busy_bits. The FBC busy bits are only handled in the intel_fbc.c functions. So maybe you want the spilock around the fb_tracking ones? That wouldn't require changing the FBC mutex to a spinlock, and it could be done today. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jun 17, 2015 at 04:39:32PM -0300, Paulo Zanoni wrote: > 2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > These busy bits are locked higher up. In fact I want to migrate that > > lock to a spinlock, which has implications here. I didn't see anything > > that mandated using a mutex for fbc, right? > > I didn't understand your idea. You want to replace the whole FBC mutex > for a spinlock? Why? I want to replace the frontbuffer mutex with a spinlock. You are inserting a mutex under my intended spinlock, which blows my idea of trying to speed up the normal operations. > Please notice that we have dev_priv->fbc.busy_bits and also > dev_priv->fb_tracking.busy_bits. The FBC busy bits are only handled in > the intel_fbc.c functions. So maybe you want the spilock around the > fb_tracking ones? That wouldn't require changing the FBC mutex to a > spinlock, and it could be done today. Somehow I need to avoid the mutex here, so kicking off the fbc enable/disable needs to be lockless (or spinlocked at most). Of course, if that is not practical, I will just have to live with not converting the higher mutex into the spinlock. -Chris
2015-06-17 17:25 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Wed, Jun 17, 2015 at 04:39:32PM -0300, Paulo Zanoni wrote: >> 2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: >> > These busy bits are locked higher up. In fact I want to migrate that >> > lock to a spinlock, which has implications here. I didn't see anything >> > that mandated using a mutex for fbc, right? >> >> I didn't understand your idea. You want to replace the whole FBC mutex >> for a spinlock? Why? > > I want to replace the frontbuffer mutex with a spinlock. You are > inserting a mutex under my intended spinlock, which blows my idea of > trying to speed up the normal operations. As far as I can see, fb_tracking.lock is not held when we call intel_fbc_flush and intel_fbc_invalidate. So your change could be possible now. > >> Please notice that we have dev_priv->fbc.busy_bits and also >> dev_priv->fb_tracking.busy_bits. The FBC busy bits are only handled in >> the intel_fbc.c functions. So maybe you want the spilock around the >> fb_tracking ones? That wouldn't require changing the FBC mutex to a >> spinlock, and it could be done today. > > Somehow I need to avoid the mutex here, so kicking off the fbc > enable/disable needs to be lockless (or spinlocked at most). Of course, > if that is not practical, I will just have to live with not converting > the higher mutex into the spinlock. I think we will reach a point where this doable. After we migrate some of the FBC stuff to pipe_config, and make FBC fully rely on the frontbuffer tracking infrastructure (i.e., no intel_fbc_something calls from intel_display.c), the FBC update/flush/invalidate functions will get much simpler, not needing to handle stolen memory, etc. Another current problem I can see is the delayed work stuff we have, but Daniel is already requesting to simplify/remove it, so that might die too. I'll try to keep the spinlock in mind when thinking about this. We can always try :). I hope you're fine with the idea of just converting the mutex to a spinlock later. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jun 17, 2015 at 05:47:10PM -0300, Paulo Zanoni wrote: > 2015-06-17 17:25 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Wed, Jun 17, 2015 at 04:39:32PM -0300, Paulo Zanoni wrote: > >> 2015-06-17 4:52 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > >> > These busy bits are locked higher up. In fact I want to migrate that > >> > lock to a spinlock, which has implications here. I didn't see anything > >> > that mandated using a mutex for fbc, right? > >> > >> I didn't understand your idea. You want to replace the whole FBC mutex > >> for a spinlock? Why? > > > > I want to replace the frontbuffer mutex with a spinlock. You are > > inserting a mutex under my intended spinlock, which blows my idea of > > trying to speed up the normal operations. > > As far as I can see, fb_tracking.lock is not held when we call > intel_fbc_flush and intel_fbc_invalidate. So your change could be > possible now. Hmm, right. My bad, I should have checked the extents of the lock first! Thanks, -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index c49fe2a..c99be0e 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1593,6 +1593,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused) } intel_runtime_pm_get(dev_priv); + mutex_lock(&dev_priv->fbc.lock); if (intel_fbc_enabled(dev)) seq_puts(m, "FBC enabled\n"); @@ -1605,6 +1606,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused) yesno(I915_READ(FBC_STATUS2) & FBC_COMPRESSION_MASK)); + mutex_unlock(&dev_priv->fbc.lock); intel_runtime_pm_put(dev_priv); return 0; @@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val) return -ENODEV; drm_modeset_lock_all(dev); + mutex_lock(&dev_priv->fbc.lock); + *val = dev_priv->fbc.false_color; + + mutex_unlock(&dev_priv->fbc.lock); drm_modeset_unlock_all(dev); return 0; @@ -1635,6 +1641,7 @@ static int i915_fbc_fc_set(void *data, u64 val) return -ENODEV; drm_modeset_lock_all(dev); + mutex_lock(&dev_priv->fbc.lock); reg = I915_READ(ILK_DPFC_CONTROL); dev_priv->fbc.false_color = val; @@ -1643,6 +1650,7 @@ static int i915_fbc_fc_set(void *data, u64 val) (reg | FBC_CTL_FALSE_COLOR) : (reg & ~FBC_CTL_FALSE_COLOR)); + mutex_unlock(&dev_priv->fbc.lock); drm_modeset_unlock_all(dev); return 0; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 491ef0c..9ab04f7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -891,6 +891,7 @@ enum fb_op_origin { }; struct i915_fbc { + struct mutex lock; unsigned long uncompressed_size; unsigned threshold; unsigned int fb_id; diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 348ed5a..947ddb4 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -250,6 +250,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c { struct drm_i915_private *dev_priv = dev->dev_private; + lockdep_assert_held(&dev_priv->fbc.lock); + if (!drm_mm_initialized(&dev_priv->mm.stolen)) return -ENODEV; @@ -266,6 +268,8 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + lockdep_assert_held(&dev_priv->fbc.lock); + if (dev_priv->fbc.uncompressed_size == 0) return; @@ -286,7 +290,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) if (!drm_mm_initialized(&dev_priv->mm.stolen)) return; + mutex_lock(&dev_priv->fbc.lock); i915_gem_stolen_cleanup_compression(dev); + mutex_unlock(&dev_priv->fbc.lock); + drm_mm_takedown(&dev_priv->mm.stolen); } diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 9e55b9b..31ff88b 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -336,6 +336,7 @@ static void intel_fbc_work_fn(struct work_struct *__work) struct drm_i915_private *dev_priv = dev->dev_private; mutex_lock(&dev->struct_mutex); + mutex_lock(&dev_priv->fbc.lock); if (work == dev_priv->fbc.fbc_work) { /* Double check that we haven't switched fb without cancelling * the prior work. @@ -350,6 +351,7 @@ static void intel_fbc_work_fn(struct work_struct *__work) dev_priv->fbc.fbc_work = NULL; } + mutex_unlock(&dev_priv->fbc.lock); mutex_unlock(&dev->struct_mutex); kfree(work); @@ -357,6 +359,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) { + lockdep_assert_held(&dev_priv->fbc.lock); + if (dev_priv->fbc.fbc_work == NULL) return; @@ -384,6 +388,8 @@ static void intel_fbc_enable(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; + lockdep_assert_held(&dev_priv->fbc.lock); + if (!dev_priv->display.enable_fbc) return; @@ -418,16 +424,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc) schedule_delayed_work(&work->work, msecs_to_jiffies(50)); } -/** - * intel_fbc_disable - disable FBC - * @dev: the drm_device - * - * This function disables FBC. - */ -void intel_fbc_disable(struct drm_device *dev) +static void __intel_fbc_disable(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + lockdep_assert_held(&dev_priv->fbc.lock); + intel_fbc_cancel_work(dev_priv); if (!dev_priv->display.disable_fbc) @@ -437,6 +439,21 @@ void intel_fbc_disable(struct drm_device *dev) dev_priv->fbc.crtc = NULL; } +/** + * intel_fbc_disable - disable FBC + * @dev: the drm_device + * + * This function disables FBC. + */ +void intel_fbc_disable(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + mutex_lock(&dev_priv->fbc.lock); + __intel_fbc_disable(dev); + mutex_unlock(&dev_priv->fbc.lock); +} + const char *intel_no_fbc_reason_str(enum no_fbc_reason reason) { switch (reason) { @@ -516,7 +533,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv) } /** - * intel_fbc_update - enable/disable FBC as needed + * __intel_fbc_update - enable/disable FBC as needed, unlocked * @dev: the drm_device * * Set up the framebuffer compression hardware at mode set time. We @@ -534,7 +551,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv) * * We need to enable/disable FBC on a global basis. */ -void intel_fbc_update(struct drm_device *dev) +static void __intel_fbc_update(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = NULL; @@ -544,6 +561,8 @@ void intel_fbc_update(struct drm_device *dev) const struct drm_display_mode *adjusted_mode; unsigned int max_width, max_height; + lockdep_assert_held(&dev_priv->fbc.lock); + if (!HAS_FBC(dev)) return; @@ -665,7 +684,7 @@ void intel_fbc_update(struct drm_device *dev) * some point. And we wait before enabling FBC anyway. */ DRM_DEBUG_KMS("disabling active FBC for update\n"); - intel_fbc_disable(dev); + __intel_fbc_disable(dev); } intel_fbc_enable(crtc); @@ -676,11 +695,26 @@ out_disable: /* Multiple disables should be harmless */ if (intel_fbc_enabled(dev)) { DRM_DEBUG_KMS("unsupported config, disabling FBC\n"); - intel_fbc_disable(dev); + __intel_fbc_disable(dev); } i915_gem_stolen_cleanup_compression(dev); } +/* + * intel_fbc_update - enable/disable FBC as needed + * @dev: the drm_device + * + * This function reevaluates the overall state and enables or disables FBC. + */ +void intel_fbc_update(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + mutex_lock(&dev_priv->fbc.lock); + __intel_fbc_update(dev); + mutex_unlock(&dev_priv->fbc.lock); +} + void intel_fbc_invalidate(struct drm_i915_private *dev_priv, unsigned int frontbuffer_bits, enum fb_op_origin origin) @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, if (origin == ORIGIN_GTT) return; + mutex_lock(&dev_priv->fbc.lock); + if (dev_priv->fbc.enabled) fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); else if (dev_priv->fbc.fbc_work) @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits); if (dev_priv->fbc.busy_bits) - intel_fbc_disable(dev); + __intel_fbc_disable(dev); + + mutex_unlock(&dev_priv->fbc.lock); } void intel_fbc_flush(struct drm_i915_private *dev_priv, @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, { struct drm_device *dev = dev_priv->dev; + mutex_lock(&dev_priv->fbc.lock); + if (!dev_priv->fbc.busy_bits) - return; + goto out; dev_priv->fbc.busy_bits &= ~frontbuffer_bits; if (!dev_priv->fbc.busy_bits) - intel_fbc_update(dev); + __intel_fbc_update(dev); + +out: + mutex_unlock(&dev_priv->fbc.lock); } /** @@ -729,6 +772,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv) { enum pipe pipe; + mutex_init(&dev_priv->fbc.lock); + if (!HAS_FBC(dev_priv)) { dev_priv->fbc.enabled = false; dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;