FBC locking

PATCH In review Paulo Zanoni 3 participants 13 messages Jun 16, 2015 – Jun 18, 2015

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2015-06-16 22:17 UTC #0 lore raw

cover FBC locking

From: Paulo Zanoni <[email protected]>

Release early, release often!

So patch 1 is not really FBC locking but it's another trivial patch from December which I wanted to stop carrying around.

Patches 2 and 3 are the real locking thing. I know they could have been just a single patch, but I decided to split them for bisectability reasons. We probably shouldn't bisect anything to the "add the FBC mutex" patch, and if we bisect something to the other patch, it will be easier to fix the problem - or revert - if we already have fbc.lock. I can squash them if needed, no problem.

Notice that on patch 3 I remove the locking around a lot of calls from intel_display.c. On a future patch I will completely remove all those calls and rely only on the frontbuffer tracking infrastructure to update fbc through the invalidate/flush functions. But before we can do this, we need an additional fix which I'm already discussing with Daniel about.

The patches pass kms_frontbuffer_testing and kms_fbc_crc without any lockdep assertions.

Thanks,
Paulo

Paulo Zanoni (3):
drm/i915: don't increment the FBC threshold at fbc_enable drm/i915: add the FBC mutex
drm/i915: stop using struct_mutex for FBC

 drivers/gpu/drm/i915/i915_debugfs.c    |  10 ++--
 drivers/gpu/drm/i915/i915_drv.h        |   1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c |   7 +++
 drivers/gpu/drm/i915/intel_display.c   |  18 +-----
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 101 ++++++++++++++++++++++++++-------
 6 files changed, 99 insertions(+), 39 deletions(-)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-06-16 22:17 UTC #1 lore raw

PATCH 1/3 drm/i915: don't increment the FBC threshold at fbc_enable

From: Paulo Zanoni <[email protected]>

We first set the threshold value when we're allocating the CFB, and then later at {ilk,gen7}_fbc_enable() we increment it in case we're using 16bpp. While that is correct, it is dangerous: if we rework the code a little bit in a way that allows us to call intel_fbc_enable() without necessarily calling i915_gem_stolen_setup_compression() first, we might end up incrementing threshold more than once. To prevent that, increment a temporary variable instead.

v2: Rebase.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +6 -4

index 50ed333..9e55b9b 100644
@@ -188,14 +188,15 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
+	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
-		dev_priv->fbc.threshold++;
+		threshold++;
 
-	switch (dev_priv->fbc.threshold) {
+	switch (threshold) {
 	case 4:
 	case 3:
 		dpfc_ctl |= DPFC_CTL_LIMIT_4X;

@@ -259,6 +260,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
+	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;

@@ -267,9 +269,9 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
 
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
-		dev_priv->fbc.threshold++;
+		threshold++;
 
-	switch (dev_priv->fbc.threshold) {
+	switch (threshold) {
 	case 4:
 	case 3:
 		dpfc_ctl |= DPFC_CTL_LIMIT_4X;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-06-16 22:17 UTC #2 lore raw

PATCH 2/3 drm/i915: add the FBC mutex

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_debugfs.c +8 -0

index c49fe2a..c99be0e 100644
@@ -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;
 }

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 491ef0c..9ab04f7 100644
@@ -891,6 +891,7 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
+	struct mutex lock;
 	unsigned long uncompressed_size;
 	unsigned threshold;
 	unsigned int fb_id;

drivers/gpu/drm/i915/i915_gem_stolen.c +7 -0

index 348ed5a..947ddb4 100644
@@ -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);
 }

drivers/gpu/drm/i915/intel_fbc.c +59 ~~-14~~

175 lines not on this page · raw message

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2015-06-17 07:47 UTC #3 lore raw

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

drivers/gpu/drm/i915/i915_debugfs.c static int i915_fbc_fc_get(void *data, u64 *val) · line 1629

Quoted 10 lines · Paulo Zanoni

drivers/gpu/drm/i915/i915_debugfs.c +4 -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);

A read of of a single value doesn't need any of these locks.

Quoted 2 lines

(quoted diff) +0 -0
 	return 0;

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2015-06-17 07:52 UTC #4 lore raw

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

In reply to Paulo Zanoni

Quoted 32 lines · Paulo Zanoni

void intel_fbc_invalidate(struct drm_i915_private *dev_priv,

drivers/gpu/drm/i915/intel_fbc.c +8 -2

 			  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;

drivers/gpu/drm/i915/intel_fbc.c void intel_fbc_flush(struct drm_i915_private *dev_priv, · line 763

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +4 -1

 
 	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

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-06-17 19:39 UTC #5 lore raw

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

2015-06-17 4:52 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 36 lines · Paulo Zanoni

Quoted 35 lines

void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
unsigned int frontbuffer_bits, enum fb_op_origin origin)

drivers/gpu/drm/i915/intel_fbc.c +8 -2

@@ -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;

Quoted 11 lines

drivers/gpu/drm/i915/intel_fbc.c void intel_fbc_flush(struct drm_i915_private *dev_priv, · line 763
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbc.c +4 -1
      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.

Quoted 4 lines

-Chris
--
Chris Wilson, Intel Open Source Technology Centre

Signature · 5 lines

Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2015-06-17 20:25 UTC #6 lore raw

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

Quoted 7 lines · Paulo Zanoni

2015-06-17 4:52 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 3 lines
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.

Quoted 5 lines

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

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-06-17 20:47 UTC #7 lore raw

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

2015-06-17 17:25 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni

Quoted 5 lines
2015-06-17 4:52 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 3 lines
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?

Quoted 6 lines

Quoted 2 lines
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.

Quoted 11 lines

Quoted 6 lines
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.

Quoted 4 lines

-Chris
--
Chris Wilson, Intel Open Source Technology Centre

Signature · 5 lines

Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2015-06-18 08:37 UTC #8 lore raw

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

Quoted 10 lines · Paulo Zanoni

2015-06-17 17:25 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 9 lines · Paulo Zanoni
Quoted 7 lines
2015-06-17 4:52 GMT-03:00 Chris Wilson <[email protected]>:
Quoted 3 lines
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?

Quoted 7 lines

Quoted 3 lines
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

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter

2015-06-17 08:18 UTC #9 lore raw

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

In reply to Paulo Zanoni

Quoted 39 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_debugfs.c +2 -0

index c49fe2a..c99be0e 100644
@@ -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;

drivers/gpu/drm/i915/i915_debugfs.c static int i915_fbc_fc_get(void *data, u64 *val) · line 1623

Quoted 3 lines

drivers/gpu/drm/i915/i915_debugfs.c +0 -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.

drivers/gpu/drm/i915/i915_debugfs.c static int i915_fbc_fc_set(void *data, u64 val) · line 1643

Quoted 12 lines

drivers/gpu/drm/i915/i915_debugfs.c +4 -0

+	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.

Quoted 29 lines

drivers/gpu/drm/i915/i915_debugfs.c +2 -0

+	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;
 }

drivers/gpu/drm/i915/i915_drv.h +1 -0

index 491ef0c..9ab04f7 100644
@@ -891,6 +891,7 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
+	struct mutex lock;
 	unsigned long uncompressed_size;
 	unsigned threshold;
 	unsigned int fb_id;

drivers/gpu/drm/i915/i915_gem_stolen.c +0 -0

index 348ed5a..947ddb4 100644

drivers/gpu/drm/i915/i915_gem_stolen.c int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c · line 253

Quoted 4 lines

drivers/gpu/drm/i915/i915_gem_stolen.c +1 -0

@@ -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.

Quoted 45 lines

drivers/gpu/drm/i915/i915_gem_stolen.c +6 -0

+
 	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);
 }

drivers/gpu/drm/i915/intel_fbc.c +2 -0

index 9e55b9b..31ff88b 100644
@@ -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);

drivers/gpu/drm/i915/intel_fbc.c static void intel_fbc_work_fn(struct work_struct *__work) · line 362

Quoted 4 lines

drivers/gpu/drm/i915/intel_fbc.c +1 -0

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

Quoted 176 lines

drivers/gpu/drm/i915/intel_fbc.c +56 ~~-14~~

+
 	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;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-06-16 22:17 UTC #10 lore raw

PATCH 3/3 drm/i915: stop using struct_mutex for FBC

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We now have dev_priv->fbc.lock and it should be enough to protect everything related to FBC.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c  |  6 ------
 drivers/gpu/drm/i915/intel_display.c | 18 ++----------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbc.c     | 22 +++++++++++++++++++---
 4 files changed, 22 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -6

index c99be0e..d36aa639 100644
@@ -1620,13 +1620,9 @@ static int i915_fbc_fc_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
 		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;
 }

@@ -1640,7 +1636,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
 		return -ENODEV;
 
-	drm_modeset_lock_all(dev);
 	mutex_lock(&dev_priv->fbc.lock);
 
 	reg = I915_READ(ILK_DPFC_CONTROL);

@@ -1651,7 +1646,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
 		   (reg & ~FBC_CTL_FALSE_COLOR));
 
 	mutex_unlock(&dev_priv->fbc.lock);
-	drm_modeset_unlock_all(dev);
 	return 0;
 }

drivers/gpu/drm/i915/intel_display.c +2 ~~-16~~

index 3f48917..cf3a86d 100644
@@ -4682,9 +4682,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 */
 	hsw_enable_ips(intel_crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.

@@ -4740,10 +4738,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		intel_set_memory_cxsr(dev_priv, false);
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->fbc.crtc == intel_crtc)
-		intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
+	intel_fbc_disable_crtc(intel_crtc);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is

@@ -5045,9 +5040,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)

@@ -5100,9 +5093,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	if (intel_crtc_to_shared_dpll(intel_crtc))
 		intel_disable_shared_dpll(intel_crtc);

@@ -6199,9 +6190,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)

@@ -13786,11 +13775,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
 
-	if (intel_crtc->atomic.update_fbc) {
-		mutex_lock(&dev->struct_mutex);
+	if (intel_crtc->atomic.update_fbc)
 		intel_fbc_update(dev);
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	if (intel_crtc->atomic.post_enable_primary)
 		intel_post_enable_primary(crtc);

drivers/gpu/drm/i915/intel_drv.h +1 -0

index bcafefc..ac34041 100644
@@ -1251,6 +1251,7 @@ bool intel_fbc_enabled(struct drm_device *dev);
 void intel_fbc_update(struct drm_device *dev);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
 void intel_fbc_disable(struct drm_device *dev);
+void intel_fbc_disable_crtc(struct intel_crtc *crtc);
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin);

drivers/gpu/drm/i915/intel_fbc.c +19 -3

index 31ff88b..ef52847 100644
@@ -335,7 +335,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct drm_device *dev = work->crtc->dev;
 	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

@@ -352,7 +351,6 @@ 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);
 }

@@ -366,7 +364,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
 
-	/* Synchronisation is provided by struct_mutex and checking of
+	/* Synchronisation is provided by fbc.lock and checking of
 	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */

@@ -454,6 +452,24 @@ void intel_fbc_disable(struct drm_device *dev)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
+/**
+ * intel_fbc_disable_crtc - disable FBC for CRTC
+ * @crtc: the CRTC
+ *
+ * This function is called when the CRTC is being disabled. If it is the FBC
+ * CRTC, then FBC will also be disabled.
+ */
+void intel_fbc_disable_crtc(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (dev_priv->fbc.crtc == crtc)
+		__intel_fbc_disable(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 {
 	switch (reason) {

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter

2015-06-17 07:48 UTC #11 lore raw

Re: [PATCH 3/3] drm/i915: stop using struct_mutex for FBC

Quoted 6 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
We now have dev_priv->fbc.lock and it should be enough to protect everything related to FBC.
Signed-off-by Paulo Zanoni <[email protected]>

Managing stolen memory still needs dev->struct_mutex which means you can only push down the mutex lock/unlock calls down into intel_fbc_update right around the compressed memory reallocation. Well maybe do even more and push it down into the fbc functions in i915_gem_stolen.c. -Daniel

Quoted 183 lines

---
 drivers/gpu/drm/i915/i915_debugfs.c  |  6 ------
 drivers/gpu/drm/i915/intel_display.c | 18 ++----------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbc.c     | 22 +++++++++++++++++++---
 4 files changed, 22 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -6

index c99be0e..d36aa639 100644
@@ -1620,13 +1620,9 @@ static int i915_fbc_fc_get(void *data, u64 *val)
 	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
 		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;
 }

@@ -1640,7 +1636,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
 	if (INTEL_INFO(dev)->gen < 7 || !HAS_FBC(dev))
 		return -ENODEV;
 
-	drm_modeset_lock_all(dev);
 	mutex_lock(&dev_priv->fbc.lock);
 
 	reg = I915_READ(ILK_DPFC_CONTROL);

@@ -1651,7 +1646,6 @@ static int i915_fbc_fc_set(void *data, u64 val)
 		   (reg & ~FBC_CTL_FALSE_COLOR));
 
 	mutex_unlock(&dev_priv->fbc.lock);
-	drm_modeset_unlock_all(dev);
 	return 0;
 }

drivers/gpu/drm/i915/intel_display.c +2 ~~-16~~

index 3f48917..cf3a86d 100644
@@ -4682,9 +4682,7 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 */
 	hsw_enable_ips(intel_crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.

@@ -4740,10 +4738,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		intel_set_memory_cxsr(dev_priv, false);
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->fbc.crtc == intel_crtc)
-		intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
+	intel_fbc_disable_crtc(intel_crtc);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is

@@ -5045,9 +5040,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)

@@ -5100,9 +5093,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	if (intel_crtc_to_shared_dpll(intel_crtc))
 		intel_disable_shared_dpll(intel_crtc);

@@ -6199,9 +6190,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)

@@ -13786,11 +13775,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
 
-	if (intel_crtc->atomic.update_fbc) {
-		mutex_lock(&dev->struct_mutex);
+	if (intel_crtc->atomic.update_fbc)
 		intel_fbc_update(dev);
-		mutex_unlock(&dev->struct_mutex);
-	}
 
 	if (intel_crtc->atomic.post_enable_primary)
 		intel_post_enable_primary(crtc);

drivers/gpu/drm/i915/intel_drv.h +1 -0

index bcafefc..ac34041 100644
@@ -1251,6 +1251,7 @@ bool intel_fbc_enabled(struct drm_device *dev);
 void intel_fbc_update(struct drm_device *dev);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
 void intel_fbc_disable(struct drm_device *dev);
+void intel_fbc_disable_crtc(struct intel_crtc *crtc);
 void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  unsigned int frontbuffer_bits,
 			  enum fb_op_origin origin);

drivers/gpu/drm/i915/intel_fbc.c +19 -3

index 31ff88b..ef52847 100644
@@ -335,7 +335,6 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct drm_device *dev = work->crtc->dev;
 	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

@@ -352,7 +351,6 @@ 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);
 }

@@ -366,7 +364,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
 
-	/* Synchronisation is provided by struct_mutex and checking of
+	/* Synchronisation is provided by fbc.lock and checking of
 	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */

@@ -454,6 +452,24 @@ void intel_fbc_disable(struct drm_device *dev)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
+/**
+ * intel_fbc_disable_crtc - disable FBC for CRTC
+ * @crtc: the CRTC
+ *
+ * This function is called when the CRTC is being disabled. If it is the FBC
+ * CRTC, then FBC will also be disabled.
+ */
+void intel_fbc_disable_crtc(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (dev_priv->fbc.crtc == crtc)
+		__intel_fbc_disable(dev);
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 {
 	switch (reason) {

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni

author

2015-06-16 22:23 UTC #12 lore raw

In reply to Paulo Zanoni

2001-01-02 4:58 GMT-02:00 Paulo Zanoni <[email protected]>:

Quoted 13 lines

From: Paulo Zanoni <[email protected]>
Release early, release often!
So patch 1 is not really FBC locking but it's another trivial patch from December which I wanted to stop carrying around.
Patches 2 and 3 are the real locking thing. I know they could have been just a single patch, but I decided to split them for bisectability reasons. We probably shouldn't bisect anything to the "add the FBC mutex" patch, and if we bisect something to the other patch, it will be easier to fix the problem - or revert - if we already have fbc.lock. I can squash them if needed, no problem.

Quoted 8 lines

Notice that on patch 3 I remove the locking around a lot of calls from intel_display.c. On a future patch I will completely remove all those calls and rely only on the frontbuffer tracking infrastructure to update fbc through the invalidate/flush functions. But before we can do this, we need an additional fix which I'm already discussing with Daniel about.
The patches pass kms_frontbuffer_testing and kms_fbc_crc without any lockdep assertions.

For some reason the email came with a date of 2001. So let's bump the thread to the top of people's inboxes using this reply. Sorry.

Quoted 20 lines

Thanks,
Paulo

Paulo Zanoni (3):
drm/i915: don't increment the FBC threshold at fbc_enable drm/i915: add the FBC mutex
drm/i915: stop using struct_mutex for FBC

 drivers/gpu/drm/i915/i915_debugfs.c    |  10 ++--
 drivers/gpu/drm/i915/i915_drv.h        |   1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c |   7 +++
 drivers/gpu/drm/i915/intel_display.c   |  18 +-----
 drivers/gpu/drm/i915/intel_drv.h       |   1 +
 drivers/gpu/drm/i915/intel_fbc.c       | 101 ++++++++++++++++++++++++++-------
 6 files changed, 99 insertions(+), 39 deletions(-)

--
2.1.4

Signature · 5 lines

Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx