Skip to contentSkip to conversation

More simple FBC fixes

PATCH In review Paulo Zanoni 3 participants 21 messages Dec 23, 2014 – Jan 5, 2015

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Paulo Zanoni

author
#0 lore raw

cover More simple FBC fixes

From: Paulo Zanoni <[email protected]>

Hi

Since the previous FBC series is already entirely reviewed, it's time to submit a few more FBC patches to the mailing list. These patches just fix some simple bugs and annoyances: nothing really major. I tested them on my BDW and they pass the tests I have.

In theory we could even think about maybe enabling FBC on HSW+ since I can't think of any bugs remaining on these platforms. The biggest reason to prevent that is that I still didn't upstream the FBC tests I wrote since I still didn't integrate them to kms_fbc_crc - which I'm also running.

This doesn't mean FBC's TODO list is empty: there are still a lot of small improvements to do, and one of them is to _not_ disable FBC during page flips. I also didn't really do any power or performance tests yet: the focus is completely on bugs.

Regarding the previous platforms, I think there could be some hope to support FBC on ILK+, but I'm really only focused on HSW+, so I don't know if we're missing some checks and restrictions. For the older platforms, I think that, given all the conditions, the risk of enabling FBC and breaking these platforms even more is probably not worth it.

Thanks,
Paulo

Paulo Zanoni (9):
drm/i915: don't reallocate the compressed FB at every frame drm/i915: fix the FBC CFB size tracking
drm/i915: don't increment the FBC threshold at fbc_enable drm/i915: don't free the CFB while FBC is enabled drm/i915: don't set the FBC plane select bits on HSW+ drm/i915: add the FBC mutex
drm/i915: don't alloc/free fbc_work at every update drm/i915: print FBC compression status on debugfs drm/i915: FBC only supports 16bpp and 32bpp

 drivers/gpu/drm/i915/i915_debugfs.c    |   5 +
 drivers/gpu/drm/i915/i915_drv.h        |   5 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c |  16 ++--
 drivers/gpu/drm/i915/i915_reg.h        |   3 +
 drivers/gpu/drm/i915/intel_fbc.c       | 161 +++++++++++++++++++++------------
 5 files changed, 123 insertions(+), 67 deletions(-)
Signature · 6 lines
2.1.3

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

PATCH 1/9 drm/i915: don't reallocate the compressed FB at every frame

From: Paulo Zanoni <[email protected]>

With the current code we just reallocate the compressed FB at every FBC update: we have X in one frame, then in the other frame we need X again, but we check "needed < have" instead of "needed <= have".

There are still other problems with this code: we don't take the threshold into consideration and we also have cases where we reallocate the CFB while FBC is enabled, but let's leave these for later patches.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_gem_stolen.c +1 -1
index a204584..4797138 100644
@@ -253,7 +253,7 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
-	if (size < dev_priv->fbc.size)
+	if (size <= dev_priv->fbc.size)
 		return 0;
 
 	/* Release any current block */
Signature · 6 lines
2.1.3

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

PATCH 2/9 drm/i915: fix the FBC CFB size tracking

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We have dev_priv->fbc.size which is supposed to contain the compressed FB size, but it is not: at find_compression_threshold() we try to overallocate the CFB, but we don't consider this when we assign a value to dev_priv->fbc.size. Since the correct CFB size should already be stored at dev_priv->fbc.compressed_fb.size, just kill dev_priv->fbc.size and use the correct value isntead.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.h        |  1 -
 drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++--------
 drivers/gpu/drm/i915/intel_fbc.c       |  2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
index 3752040..f0419c8 100644
@@ -734,7 +734,6 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
-	unsigned long size;
 	unsigned threshold;
 	unsigned int fb_id;
 	unsigned int possible_framebuffer_bits;
drivers/gpu/drm/i915/i915_gem_stolen.c +5 -8
index 4797138..d02c102 100644
@@ -231,10 +231,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 			   dev_priv->mm.stolen_base + compressed_llb->start);
 	}
 
-	dev_priv->fbc.size = size / dev_priv->fbc.threshold;
-
-	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
-		      size);
+	DRM_DEBUG_KMS("reserved %lu bytes of contiguous stolen space for FBC\n",
+		      dev_priv->fbc.compressed_fb.size);
 
 	return 0;
 

@@ -253,7 +251,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
-	if (size <= dev_priv->fbc.size)
+	if (dev_priv->fbc.compressed_fb.allocated &&
+	    size <= dev_priv->fbc.compressed_fb.size)
 		return 0;
 
 	/* Release any current block */

@@ -266,7 +265,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->fbc.size == 0)
+	if (dev_priv->fbc.compressed_fb.allocated == 0)
 		return;
 
 	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);

@@ -275,8 +274,6 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 		drm_mm_remove_node(dev_priv->fbc.compressed_llb);
 		kfree(dev_priv->fbc.compressed_llb);
 	}
-
-	dev_priv->fbc.size = 0;
 }
 
 void i915_gem_cleanup_stolen(struct drm_device *dev)
drivers/gpu/drm/i915/intel_fbc.c +1 -1
index d3ff2c1..5270dc4 100644
@@ -78,7 +78,7 @@ static void i8xx_fbc_enable(struct drm_crtc *crtc)
 
 	dev_priv->fbc.enabled = true;
 
-	cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE;
+	cfb_pitch = dev_priv->fbc.compressed_fb.size / FBC_LL_SIZE;
 	if (fb->pitches[0] < cfb_pitch)
 		cfb_pitch = fb->pitches[0];
Signature · 6 lines
2.1.3

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

Re: [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

Quoted 8 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We have dev_priv->fbc.size which is supposed to contain the compressed FB size, but it is not: at find_compression_threshold() we try to overallocate the CFB, but we don't consider this when we assign a value to dev_priv->fbc.size. Since the correct CFB size should already be stored at dev_priv->fbc.compressed_fb.size, just kill dev_priv->fbc.size and use the correct value isntead.

They should not be equivalent though. We actually want fbc.size to compensate for the compression in the allocation so that the simple check for enough space succeeds even if we are compressing. -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
author
#4 lore raw

Re: [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

2014-12-25 8:16 GMT-02:00 Chris Wilson <[email protected]>:

Quoted 10 lines · Paulo Zanoni
Quoted 8 lines

From: Paulo Zanoni <[email protected]>

We have dev_priv->fbc.size which is supposed to contain the compressed FB size, but it is not: at find_compression_threshold() we try to overallocate the CFB, but we don't consider this when we assign a value to dev_priv->fbc.size. Since the correct CFB size should already be stored at dev_priv->fbc.compressed_fb.size, just kill dev_priv->fbc.size and use the correct value isntead.

Quoted 3 lines

They should not be equivalent though. We actually want fbc.size to compensate for the compression in the allocation so that the simple check for enough space succeeds even if we are compressing.

Can you please elaborate more on that? Are you talking about the threshold? As far as I can see, we're failing to properly take it into consideration both before and after this patch, so it wouldn't be a valid reason.

I was planning to fix the "take threshold into consideration when checking the size" problem later: I wanted to think on a way that would guarantee that we'd always get the best possible threshold to prevent cases where we'd keep reusing 1:4 compression even while we could just free+realloc to have 1:1 compression back.

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
#5 lore raw

Re: [PATCH 2/9] drm/i915: fix the FBC CFB size tracking

Quoted 11 lines · Paulo Zanoni

2014-12-25 8:16 GMT-02:00 Chris Wilson <[email protected]>:

Quoted 10 lines · Paulo Zanoni
Quoted 8 lines

From: Paulo Zanoni <[email protected]>

We have dev_priv->fbc.size which is supposed to contain the compressed FB size, but it is not: at find_compression_threshold() we try to overallocate the CFB, but we don't consider this when we assign a value to dev_priv->fbc.size. Since the correct CFB size should already be stored at dev_priv->fbc.compressed_fb.size, just kill dev_priv->fbc.size and use the correct value isntead.

Quoted 8 lines
Quoted 3 lines

They should not be equivalent though. We actually want fbc.size to compensate for the compression in the allocation so that the simple check for enough space succeeds even if we are compressing.

Can you please elaborate more on that? Are you talking about the threshold? As far as I can see, we're failing to properly take it into consideration both before and after this patch, so it wouldn't be a valid reason.

We have both identified the problem, and the fix to the current code looks rather easy as it seems a simple bug in

commit 5e59f7175f96550ede91f58d267d2b551cb6fbba

Author: Ben Widawsky <[email protected]>
Date:   Mon Jun 30 10:41:24 2014 -0700

drm/i915: Try harder to get FBC

that fluffs the compare of uncompressed request sizes.

Your subject line claims to be a fix, so fix something. -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
author
#6 lore raw

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

In reply to Paulo Zanoni

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.

We're going to need this patch for a later rework.

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 5270dc4..1b10b06 100644
@@ -187,14 +187,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;

@@ -258,14 +259,15 @@ 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;
 
 	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.3

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

PATCH 4/9 drm/i915: don't free the CFB while FBC is enabled

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Because that is probably not very a good idea: if we used the stolen memory for more things, there could be a risk that someone would "allocate" the memory that the HW is still using as the CFB while FBC was still enabled.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c |  3 +++
 drivers/gpu/drm/i915/intel_fbc.c       | 14 +++++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/i915_gem_stolen.c +3 -0
index d02c102..f84c5f3 100644
@@ -198,6 +198,8 @@ static int i915_setup_compression(struct drm_device *dev, int size, int fb_cpp)
 	struct drm_mm_node *uninitialized_var(compressed_llb);
 	int ret;
 
+	WARN_ON(dev_priv->fbc.enabled);
+
 	ret = find_compression_threshold(dev, &dev_priv->fbc.compressed_fb,
 					 size, fb_cpp);
 	if (!ret)

@@ -268,6 +270,7 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
 	if (dev_priv->fbc.compressed_fb.allocated == 0)
 		return;
 
+	WARN_ON(dev_priv->fbc.enabled);
 	drm_mm_remove_node(&dev_priv->fbc.compressed_fb);
 
 	if (dev_priv->fbc.compressed_llb) {
drivers/gpu/drm/i915/intel_fbc.c +7 -7
index 1b10b06..83d3c8a 100644
@@ -593,13 +593,6 @@ void intel_fbc_update(struct drm_device *dev)
 	if (in_dbg_master())
 		goto out_disable;
 
-	if (i915_gem_stolen_setup_compression(dev, obj->base.size,
-					      drm_format_plane_cpp(fb->pixel_format, 0))) {
-		if (set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL))
-			DRM_DEBUG_KMS("framebuffer too large, disabling compression\n");
-		goto out_disable;
-	}
-
 	/* If the scanout has not changed, don't modify the FBC settings.
 	 * Note that we make the fundamental assumption that the fb->obj
 	 * cannot be unpinned (and have its GTT offset and fence revoked)

@@ -638,6 +631,13 @@ void intel_fbc_update(struct drm_device *dev)
 		intel_fbc_disable(dev);
 	}
 
+	if (i915_gem_stolen_setup_compression(dev, obj->base.size,
+					      drm_format_plane_cpp(fb->pixel_format, 0))) {
+		if (set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL))
+			DRM_DEBUG_KMS("framebuffer too large, disabling compression\n");
+		return;
+	}
+
 	intel_fbc_enable(crtc);
 	dev_priv->fbc.no_fbc_reason = FBC_OK;
 	return;
Signature · 6 lines
2.1.3

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

Re: [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

Quoted 6 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Because that is probably not very a good idea: if we used the stolen memory for more things, there could be a risk that someone would "allocate" the memory that the HW is still using as the CFB while FBC was still enabled.

Not in the code as it currently is due to the struct mutex lock. You need to explain how and why you intend to remove that invariant. -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
author
#9 lore raw

Re: [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

2014-12-25 8:20 GMT-02:00 Chris Wilson <[email protected]>:

Quoted 10 lines · Paulo Zanoni
Quoted 6 lines

From: Paulo Zanoni <[email protected]>

Because that is probably not very a good idea: if we used the stolen memory for more things, there could be a risk that someone would "allocate" the memory that the HW is still using as the CFB while FBC was still enabled.

Not in the code as it currently is due to the struct mutex lock. You need to explain how and why you intend to remove that invariant.

Ok, maybe this point is wrong, but I still wee we freeing and then reallocating the CFB while FBC is active, which is still a problem, even if we end up just reallocating the same chunk of memory 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
#10 lore raw

Re: [PATCH 4/9] drm/i915: don't free the CFB while FBC is enabled

Quoted 9 lines · Paulo Zanoni

2014-12-25 8:20 GMT-02:00 Chris Wilson <[email protected]>:

Quoted 8 lines · Paulo Zanoni
Quoted 6 lines

From: Paulo Zanoni <[email protected]>

Because that is probably not very a good idea: if we used the stolen memory for more things, there could be a risk that someone would "allocate" the memory that the HW is still using as the CFB while FBC was still enabled.

Quoted 6 lines
Quoted 2 lines

Not in the code as it currently is due to the struct mutex lock. You need to explain how and why you intend to remove that invariant.

Ok, maybe this point is wrong, but I still wee we freeing and then reallocating the CFB while FBC is active, which is still a problem, even if we end up just reallocating the same chunk of memory later.

No. If you read the current code it will disable FBC using the old location before it can be reallocated. That it may continue to use the old address for a frame after we have marked it free isn't clean, but it is not the bug you describe.
-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
author
#11 lore raw

PATCH 5/9 drm/i915: don't set the FBC plane select bits on HSW+

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

This commit is just to make the intentions explicit: on HSW+ these bits are MBZ, but since we only support plane A and the macro evaluates to zero when plane A is the parameter, we're not fixing any bug.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_fbc.c +5 -2
index 83d3c8a..c6e688c 100644
@@ -258,12 +258,15 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 	struct drm_framebuffer *fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	u32 dpfc_ctl;
+	u32 dpfc_ctl = 0;
 	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
-	dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
+
+	if (IS_IVYBRIDGE(dev))
+		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
+
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
 		threshold++;
Signature · 6 lines
2.1.3

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

Re: [PATCH 5/9] drm/i915: don't set the FBC plane select bits on HSW+

Quoted 19 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

This commit is just to make the intentions explicit: on HSW+ these bits are MBZ, but since we only support plane A and the macro evaluates to zero when plane A is the parameter, we're not fixing any bug.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_fbc.c +0 -0
index 83d3c8a..c6e688c 100644
@@ -258,12 +258,15 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
 	struct drm_framebuffer *fb = crtc->primary->fb;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);

drivers/gpu/drm/i915/intel_fbc.c static void gen7_fbc_enable(struct drm_crtc *crtc) · line 267

Quoted 10 lines
drivers/gpu/drm/i915/intel_fbc.c +3 -2
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	u32 dpfc_ctl;
+	u32 dpfc_ctl = 0;
 	int threshold = dev_priv->fbc.threshold;
 
 	dev_priv->fbc.enabled = true;
 
-	dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
+
+	if (IS_IVYBRIDGE(dev))

What about baytrail?
-Daniel

Quoted 12 lines
drivers/gpu/drm/i915/intel_fbc.c +2 -0
+		dpfc_ctl |= IVB_DPFC_CTL_PLANE(intel_crtc->plane);
+
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
 		threshold++;
Signature · 6 lines
2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#13 lore raw

PATCH 6/9 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.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 80 +++++++++++++++++++++++++++++++---------
 2 files changed, 64 insertions(+), 17 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
index f0419c8..18fcce4 100644
@@ -734,6 +734,7 @@ enum fb_op_origin {
 };
 
 struct i915_fbc {
+	struct mutex lock;
 	unsigned threshold;
 	unsigned int fb_id;
 	unsigned int possible_framebuffer_bits;
drivers/gpu/drm/i915/intel_fbc.c +63 -17

195 lines not on this page · raw message

Signature · 6 lines
2.1.3

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

Re: [PATCH 6/9] drm/i915: add the FBC mutex

Quoted 5 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.

You are not actually reducing the partial coverage from other locks, particularly struct_mutex, so this doesn't look like the full improvement it should be.
-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
author
#15 lore raw

Re: [PATCH 6/9] drm/i915: add the FBC mutex

2014-12-25 8:25 GMT-02:00 Chris Wilson <[email protected]>:

Quoted 10 lines · Paulo Zanoni
Quoted 5 lines

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.

You are not actually reducing the partial coverage from other locks, particularly struct_mutex, so this doesn't look like the full improvement it should be.

Good point. I'll take a look at that.

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
author
#16 lore raw

PATCH 7/9 drm/i915: don't alloc/free fbc_work at every update

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Because there's no need for it. Just use a static structure with a bool field to tell if it's in use or not. The big advantage here is not saving kzalloc/kfree calls, it's cutting the ugly "failed to allocate FBC work structure" code path: in this path we call enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and fbc.y - they are updated in intel_fbc_work_fn(), which we're not calling. And since testing out-of-memory cases like this is really hard, getting rid of the code path is a major relief.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++-
 drivers/gpu/drm/i915/intel_fbc.c | 41 +++++++++++++++-------------------------
 2 files changed, 17 insertions(+), 27 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +2 -1
index 18fcce4..40bc864 100644
@@ -751,10 +751,11 @@ struct i915_fbc {
 	bool enabled;
 
 	struct intel_fbc_work {
+		bool scheduled;
 		struct delayed_work work;
 		struct drm_crtc *crtc;
 		struct drm_framebuffer *fb;
-	} *fbc_work;
+	} work;
 
 	enum no_fbc_reason {
 		FBC_OK, /* FBC is enabled */
drivers/gpu/drm/i915/intel_fbc.c +15 -26

104 lines not on this page · raw message

Signature · 6 lines
2.1.3

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

Re: [PATCH 7/9] drm/i915: don't alloc/free fbc_work at every update

Quoted 10 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Because there's no need for it. Just use a static structure with a bool field to tell if it's in use or not. The big advantage here is not saving kzalloc/kfree calls, it's cutting the ugly "failed to allocate FBC work structure" code path: in this path we call enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and fbc.y - they are updated in intel_fbc_work_fn(), which we're not calling. And since testing out-of-memory cases like this is really hard, getting rid of the code path is a major relief.

No, it is not that hard to test.

The complaint here should be addressed by a function to call dev_priv->display.enable_fbc, which would do the common task of setting dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

That would remove duplicated code first. And then you can argue about the merits of replacing the kmalloc by growing our global state. -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
author
#18 lore raw

Re: [PATCH 7/9] drm/i915: don't alloc/free fbc_work at every update

2014-12-25 8:33 GMT-02:00 Chris Wilson <[email protected]>:

Quoted 12 lines · Paulo Zanoni
Quoted 10 lines

From: Paulo Zanoni <[email protected]>

Because there's no need for it. Just use a static structure with a bool field to tell if it's in use or not. The big advantage here is not saving kzalloc/kfree calls, it's cutting the ugly "failed to allocate FBC work structure" code path: in this path we call enable_fbc() directly but we don't update fbc.crtc, fbc.fb_id and fbc.y - they are updated in intel_fbc_work_fn(), which we're not calling. And since testing out-of-memory cases like this is really hard, getting rid of the code path is a major relief.

Quoted 1 line

No, it is not that hard to test.

Yeah, I know we have tons of checks for out-of-memory stuff, but it takes time to test and I think we just don't have the FBC-specific test.

Quoted 4 lines

The complaint here should be addressed by a function to call dev_priv->display.enable_fbc, which would do the common task of setting dev_priv->fbc.crtc, .fb_id, .y and *.enabled*.

Ok, I'll do that. I thought that by keeping a single code path we'd be able to avoid it :)

Quoted 3 lines

That would remove duplicated code first. And then you can argue about the merits of replacing the kmalloc by growing our global state.

And what is your opinion on this? Do you think it's an improvement to the codebase?

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
author
#19 lore raw

PATCH 8/9 drm/i915: print FBC compression status on debugfs

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We already had a few bugs in the past where FBC was compressing nothing when it was enabled, which makes the feature quite useless. Add this information on debugfs so the test suites can check for regressions in this piece of the code.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
 drivers/gpu/drm/i915/i915_reg.h     | 3 +++
 2 files changed, 8 insertions(+)
drivers/gpu/drm/i915/i915_debugfs.c +5 -0
index e515aad..e34e021 100644
@@ -1510,6 +1510,11 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		seq_putc(m, '\n');
 	}
 
+	if (INTEL_INFO(dev_priv)->gen >= 7 && !IS_VALLEYVIEW(dev_priv))
+		seq_printf(m, "Compressing: %s\n",
+			   yesno(I915_READ(FBC_STATUS2) &
+				 FBC_COMPRESSION_MASK));
+
 	intel_runtime_pm_put(dev_priv);
 
 	return 0;
drivers/gpu/drm/i915/i915_reg.h +3 -0
index 40ca873..b6db966 100644
@@ -1600,6 +1600,9 @@ enum punit_power_well {
 #define FBC_FENCE_OFF		0x03218 /* BSpec typo has 321Bh */
 #define FBC_TAG			0x03300
 
+#define FBC_STATUS2		0x43214
+#define  FBC_COMPRESSION_MASK	0x7ff
+
 #define FBC_LL_SIZE		(1536)
 
 /* Framebuffer compression for GM45+ */
Signature · 6 lines
2.1.3

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

PATCH 9/9 drm/i915: FBC only supports 16bpp and 32bpp

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

So check for this.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_fbc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_fbc.c +9 -2
index 80bdbde..8c6fb1d 100644
@@ -515,6 +515,7 @@ static void __intel_fbc_update(struct drm_device *dev)
 	struct drm_i915_gem_object *obj;
 	const struct drm_display_mode *adjusted_mode;
 	unsigned int max_width, max_height;
+	int fb_cpp;
 
 	lockdep_assert_held(&dev_priv->fbc.lock);
 

@@ -598,6 +599,13 @@ static void __intel_fbc_update(struct drm_device *dev)
 		goto out_disable;
 	}
 
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+	if (!(fb_cpp == 2 || fb_cpp == 4)) {
+		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE));
+			DRM_DEBUG_KMS("FBC only supports 16bpp and 32bpp\n");
+		goto out_disable;
+	}
+
 	/* If the kernel debugger is active, always disable compression */
 	if (in_dbg_master())
 		goto out_disable;

@@ -640,8 +648,7 @@ static void __intel_fbc_update(struct drm_device *dev)
 		__intel_fbc_disable(dev);
 	}
 
-	if (i915_gem_stolen_setup_compression(dev, obj->base.size,
-					      drm_format_plane_cpp(fb->pixel_format, 0))) {
+	if (i915_gem_stolen_setup_compression(dev, obj->base.size, fb_cpp)) {
 		if (set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL))
 			DRM_DEBUG_KMS("framebuffer too large, disabling compression\n");
 		return;
Signature · 6 lines
2.1.3

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