FBC trivial patches

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 to debugfs so the test suites can check for regressions in this piece of the code.

Our igt/tests/kms_frontbuffer_tracking already has support for this message.

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

index 92cf273..7358f6d 100644

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

+		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 40a3a64..0c0b12a 100644
@@ -1951,6 +1951,9 @@ enum skl_disp_power_wells {
 #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.4

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

From: Paulo Zanoni <[email protected]>

This commit has two main advantages: simplify intel_fbc_update() and deduplicate the strings.

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

---
 drivers/gpu/drm/i915/i915_debugfs.c | 49 +++------------------------
 drivers/gpu/drm/i915/intel_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_fbc.c    | 66 +++++++++++++++++++++++++------------
 3 files changed, 50 insertions(+), 66 deletions(-)

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

index 405022b..eaa567c 100644
@@ -1594,52 +1594,11 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	if (intel_fbc_enabled(dev)) {
+	if (intel_fbc_enabled(dev))
 		seq_puts(m, "FBC enabled\n");
-	} else {
-		seq_puts(m, "FBC disabled: ");
-		switch (dev_priv->fbc.no_fbc_reason) {
-		case FBC_OK:
-			seq_puts(m, "FBC actived, but currently disabled in hardware");
-			break;
-		case FBC_UNSUPPORTED:
-			seq_puts(m, "unsupported by this chipset");
-			break;
-		case FBC_NO_OUTPUT:
-			seq_puts(m, "no outputs");
-			break;
-		case FBC_STOLEN_TOO_SMALL:
-			seq_puts(m, "not enough stolen memory");
-			break;
-		case FBC_UNSUPPORTED_MODE:
-			seq_puts(m, "mode not supported");
-			break;
-		case FBC_MODE_TOO_LARGE:
-			seq_puts(m, "mode too large");
-			break;
-		case FBC_BAD_PLANE:
-			seq_puts(m, "FBC unsupported on plane");
-			break;
-		case FBC_NOT_TILED:
-			seq_puts(m, "scanout buffer not tiled");
-			break;
-		case FBC_MULTIPLE_PIPES:
-			seq_puts(m, "multiple pipes are enabled");
-			break;
-		case FBC_MODULE_PARAM:
-			seq_puts(m, "disabled per module param (default off)");
-			break;
-		case FBC_CHIP_DEFAULT:
-			seq_puts(m, "disabled per chip default");
-			break;
-		case FBC_ROTATION:
-			seq_puts(m, "rotation not supported");
-			break;
-		default:
-			seq_puts(m, "unknown reason");
-		}
-		seq_putc(m, '\n');
-	}
+	else
+		seq_printf(m, "FBC disabled: %s\n",
+			  intel_no_fbc_reason_str(dev_priv->fbc.no_fbc_reason));
 
 	if (INTEL_INFO(dev_priv)->gen >= 7 && !IS_VALLEYVIEW(dev_priv))
 		seq_printf(m, "Compressing: %s\n",

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

index b28029a..77f24e0 100644
@@ -1255,6 +1255,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  enum fb_op_origin origin);
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits);
+const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
 
 /* intel_hdmi.c */
 void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);

drivers/gpu/drm/i915/intel_fbc.c +45 -21

index 43704a4..9b300bd 100644
@@ -432,6 +432,39 @@ void intel_fbc_disable(struct drm_device *dev)
 	dev_priv->fbc.crtc = NULL;
 }
 
+const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
+{
+	switch (reason) {
+	case FBC_OK:
+		return "FBC enabled but currently disabled in hardware";
+	case FBC_UNSUPPORTED:
+		return "unsupported by this chipset";
+	case FBC_NO_OUTPUT:
+		return "no output";
+	case FBC_STOLEN_TOO_SMALL:
+		return "not enough stolen memory";
+	case FBC_UNSUPPORTED_MODE:
+		return "mode incompatible with compression";
+	case FBC_MODE_TOO_LARGE:
+		return "mode too large for compression";
+	case FBC_BAD_PLANE:
+		return "FBC unsupported on plane";
+	case FBC_NOT_TILED:
+		return "framebuffer not tiled or fenced";
+	case FBC_MULTIPLE_PIPES:
+		return "more than one pipe active";
+	case FBC_MODULE_PARAM:
+		return "disabled per module param";
+	case FBC_CHIP_DEFAULT:
+		return "disabled per chip default";
+	case FBC_ROTATION:
+		return "rotation unsupported";
+	default:
+		MISSING_CASE(reason);
+		return "unknown reason";
+	}
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {

@@ -439,6 +472,8 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 		return false;
 
 	dev_priv->fbc.no_fbc_reason = reason;
+	DRM_DEBUG_KMS("Disabling FBC: %s\n", intel_no_fbc_reason_str(reason));
+
 	return true;
 }
 

@@ -459,8 +494,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 		if (intel_crtc_active(tmp_crtc) &&
 		    to_intel_plane_state(tmp_crtc->primary->state)->visible) {
 			if (one_pipe_only && crtc) {
-				if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
-					DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
+				set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES);
 				return NULL;
 			}
 			crtc = tmp_crtc;

@@ -471,8 +505,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 	}
 
 	if (!crtc || crtc->primary->fb == NULL) {
-		if (set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT))
-			DRM_DEBUG_KMS("no output, disabling\n");
+		set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT);
 		return NULL;
 	}
 

@@ -516,14 +549,12 @@ void intel_fbc_update(struct drm_device *dev)
 		i915.enable_fbc = 0;
 
 	if (i915.enable_fbc < 0) {
-		if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
-			DRM_DEBUG_KMS("disabled per chip default\n");
+		set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT);
 		goto out_disable;
 	}
 
 	if (!i915.enable_fbc) {
-		if (set_no_fbc_reason(dev_priv, FBC_MODULE_PARAM))
-			DRM_DEBUG_KMS("fbc disabled per module param\n");
+		set_no_fbc_reason(dev_priv, FBC_MODULE_PARAM);
 		goto out_disable;
 	}
 

@@ -547,9 +578,7 @@ void intel_fbc_update(struct drm_device *dev)
 
 	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
 	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
-		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
-			DRM_DEBUG_KMS("mode incompatible with compression, "
-				      "disabling\n");
+		set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE);
 		goto out_disable;
 	}
 

@@ -565,14 +594,12 @@ void intel_fbc_update(struct drm_device *dev)
 	}
 	if (intel_crtc->config->pipe_src_w > max_width ||
 	    intel_crtc->config->pipe_src_h > max_height) {
-		if (set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE))
-			DRM_DEBUG_KMS("mode too large for compression, disabling\n");
+		set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE);
 		goto out_disable;
 	}
 	if ((INTEL_INFO(dev)->gen < 4 || HAS_DDI(dev)) &&
 	    intel_crtc->plane != PLANE_A) {
-		if (set_no_fbc_reason(dev_priv, FBC_BAD_PLANE))
-			DRM_DEBUG_KMS("plane not A, disabling compression\n");
+		set_no_fbc_reason(dev_priv, FBC_BAD_PLANE);
 		goto out_disable;
 	}
 

@@ -581,14 +608,12 @@ void intel_fbc_update(struct drm_device *dev)
 	 */
 	if (obj->tiling_mode != I915_TILING_X ||
 	    obj->fence_reg == I915_FENCE_REG_NONE) {
-		if (set_no_fbc_reason(dev_priv, FBC_NOT_TILED))
-			DRM_DEBUG_KMS("framebuffer not tiled or fenced, disabling compression\n");
+		set_no_fbc_reason(dev_priv, FBC_NOT_TILED);
 		goto out_disable;
 	}
 	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
 	    crtc->primary->state->rotation != BIT(DRM_ROTATE_0)) {
-		if (set_no_fbc_reason(dev_priv, FBC_ROTATION))
-			DRM_DEBUG_KMS("Rotation unsupported, disabling\n");
+		set_no_fbc_reason(dev_priv, FBC_ROTATION);
 		goto out_disable;
 	}
 

@@ -598,8 +623,7 @@ void intel_fbc_update(struct drm_device *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");
+		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
 

Signature · 6 lines

2.1.4

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

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 9b300bd..8b980e5 100644

From: Paulo Zanoni <[email protected]>

Let's try to get the easy stuff merged while the rest is not ready yet.

This is a nice opportunity for you to easily increase your patch review count!

From: Paulo Zanoni <[email protected]>

Thanks for the V1 reviews and comments!

Paulo Zanoni (4):
drm/i915: print FBC compression status on debugfs drm/i915: add FBC_ROTATION to enum no_fbc_reason drm/i915: unify no_fbc_reason message printing drm/i915: don't set the FBC plane select bits on HSW+

Quoted 9 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Thanks for the V1 reviews and comments!

Paulo Zanoni (4):
drm/i915: print FBC compression status on debugfs drm/i915: add FBC_ROTATION to enum no_fbc_reason drm/i915: unify no_fbc_reason message printing drm/i915: don't set the FBC plane select bits on HSW+

Reviewed-by Chris Wilson <[email protected]>