diff mbox

[2/3] drm/i915: add the FBC mutex

Message ID 978418739-1927-3-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Jan. 2, 2001, 6:58 a.m. UTC
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(-)

Comments

Chris Wilson June 17, 2015, 7:47 a.m. UTC | #1
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;
Chris Wilson June 17, 2015, 7:52 a.m. UTC | #2
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
Daniel Vetter June 17, 2015, 8:21 a.m. UTC | #3
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
Paulo Zanoni June 17, 2015, 7:39 p.m. UTC | #4
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
Chris Wilson June 17, 2015, 8:25 p.m. UTC | #5
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
Paulo Zanoni June 17, 2015, 8:47 p.m. UTC | #6
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
Chris Wilson June 18, 2015, 8:37 a.m. UTC | #7
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 mbox

Patch

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;