[PATCH 00/16] FBC bug fixes

[PATCH 00/16] FBC bug fixes

[Paulo Zanoni]

Hello

This series contains tons of bug fixes for FBC. Some of the patches on this
series have seen the mailing list a few times already. With this series applied,
my BDW machine passes all the FBC tests that are on IGT.

This means we could even try to enable FBC on BDW by default, but I won't put
this patch as part of the series since I want QA to confirm everything passes
before we make this move. In the meantime, there are some optimizations I want
to do, and SKL to fix.

Thanks,
Paulo

Paulo Zanoni (16):
  drm/i915: make sure we're not changing the FBC CFB with FBC enabled
  drm/i915: fix the FBC work allocation failure path
  drm/i915: fix FBC for cases where crtc->base.y is non-zero
  drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB
  drm/i915: check for the supported strides on HSW+ FBC
  drm/i915: try a little harder to find an FBC CRTC
  drm/i915: disable FBC on FIFO underruns
  drm/i915: avoid the last 8mb of stolen on BDW/SKL
  drm/i915: print the correct amount of bytes allocated for the CFB
  drm/i915: fix CFB size calculation
  drm/i915/bdw: don't enable FBC when pixel rate exceeds 95%
  drm/i915: apply WaFbcAsynchFlipDisableFbcQueue earlier
  drm/i915: don't use the first stolen page on Broadwell
  drm/i915: don't apply WaFbcAsynchFlipDisableFbcQueue on SKL
  Revert "drm/i915: Allocate fbcon from stolen memory"
  drm/i915: reject invalid formats for FBC

 drivers/gpu/drm/i915/i915_drv.h            |  12 ++
 drivers/gpu/drm/i915/i915_gem_gtt.h        |   1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c     |  29 +++-
 drivers/gpu/drm/i915/intel_drv.h           |   1 +
 drivers/gpu/drm/i915/intel_fbc.c           | 233 +++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_fbdev.c         |   4 +-
 drivers/gpu/drm/i915/intel_fifo_underrun.c |   2 +
 7 files changed, 242 insertions(+), 40 deletions(-)

-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/16] drm/i915: make sure we're not changing the FBC CFB with FBC enabled

[Paulo Zanoni]

We used to have this bug in the past, but now that we properly track
the size of the CFB, we don't have it anymore. Still, add the WARN
just to make sure we don't go back to the bad state.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1f97fb5..c97aba2 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -589,6 +589,8 @@ static int intel_fbc_alloc_cfb(struct drm_i915_private *dev_priv, int size,
 
 	dev_priv->fbc.threshold = ret;
 
+	WARN_ON(dev_priv->fbc.enabled);
+
 	if (INTEL_INFO(dev_priv)->gen >= 5)
 		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
 	else if (IS_GM45(dev_priv)) {
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Paulo Zanoni]

Always update the currrent crtc, fb and vertical offset after calling
enable_fbc. We were forgetting to do so along the failure paths when
enabling fbc synchronously. Fix this with a new helper to enable_fbc()
and update the state simultaneously.

v2: Improve commit message (Chris).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index c97aba2..fa9b004 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
 	return dev_priv->fbc.enabled;
 }
 
+static void intel_fbc_enable(struct intel_crtc *crtc,
+			     struct drm_framebuffer *fb)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	dev_priv->fbc.enable_fbc(crtc);
+
+	dev_priv->fbc.crtc = crtc;
+	dev_priv->fbc.fb_id = fb->base.id;
+	dev_priv->fbc.y = crtc->base.y;
+}
+
 static void intel_fbc_work_fn(struct work_struct *__work)
 {
 	struct intel_fbc_work *work =
@@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
 		 */
-		if (crtc_fb == work->fb) {
-			dev_priv->fbc.enable_fbc(work->crtc);
-
-			dev_priv->fbc.crtc = work->crtc;
-			dev_priv->fbc.fb_id = crtc_fb->base.id;
-			dev_priv->fbc.y = work->crtc->base.y;
-		}
+		if (crtc_fb == work->fb)
+			intel_fbc_enable(work->crtc, work->fb);
 
 		dev_priv->fbc.fbc_work = NULL;
 	}
@@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 	dev_priv->fbc.fbc_work = NULL;
 }
 
-static void intel_fbc_enable(struct intel_crtc *crtc)
+static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
 {
 	struct intel_fbc_work *work;
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc)
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
-		dev_priv->fbc.enable_fbc(crtc);
+		intel_fbc_enable(crtc, crtc->base.primary->fb);
 		return;
 	}
 
@@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		__intel_fbc_disable(dev_priv);
 	}
 
-	intel_fbc_enable(intel_crtc);
+	intel_fbc_schedule_enable(intel_crtc);
 	dev_priv->fbc.no_fbc_reason = FBC_OK;
 	return;
 
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/16] drm/i915: fix FBC for cases where crtc->base.y is non-zero

[Paulo Zanoni]

I only tested this on BDW, but since the register description is the
same ever since gen4, let's assume that all gens take the same
register format. If that's not true, then hopefully someone will
bisect a bug to this patch and we'll fix it.

Notice that the wrong fence offset register just means that the
hardware tracking will be wrong.

Testcases:
 - igt/kms_frontbuffer_tracking/fbc-1p-primscrn-pri-shrfb-draw-mmap-gtt
 - igt/kms_frontbuffer_tracking/fbc-2p-primscrn-pri-shrfb-draw-mmap-gtt

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fa9b004..9ffa7dc 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -64,6 +64,20 @@ static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_KMS("disabled FBC\n");
 }
 
+static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	unsigned int tile_height;
+
+	tile_height = intel_tile_height(dev_priv->dev, fb->pixel_format,
+					fb->modifier[0]);
+
+	/* The value we want is the line number of the first tile that contains
+	 * the first vertical line of the CRTC. */
+	return crtc->base.y - (crtc->base.y % tile_height);
+}
+
 static void i8xx_fbc_enable(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -97,7 +111,7 @@ static void i8xx_fbc_enable(struct intel_crtc *crtc)
 		fbc_ctl2 = FBC_CTL_FENCE_DBL | FBC_CTL_IDLE_IMM | FBC_CTL_CPU_FENCE;
 		fbc_ctl2 |= FBC_CTL_PLANE(crtc->plane);
 		I915_WRITE(FBC_CONTROL2, fbc_ctl2);
-		I915_WRITE(FBC_FENCE_OFF, crtc->base.y);
+		I915_WRITE(FBC_FENCE_OFF, get_crtc_fence_y_offset(crtc));
 	}
 
 	/* enable it... */
@@ -135,7 +149,7 @@ static void g4x_fbc_enable(struct intel_crtc *crtc)
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
 	dpfc_ctl |= DPFC_CTL_FENCE_EN | obj->fence_reg;
 
-	I915_WRITE(DPFC_FENCE_YOFF, crtc->base.y);
+	I915_WRITE(DPFC_FENCE_YOFF, get_crtc_fence_y_offset(crtc));
 
 	/* enable it... */
 	I915_WRITE(DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
@@ -177,6 +191,7 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	u32 dpfc_ctl;
 	int threshold = dev_priv->fbc.threshold;
+	unsigned int y_offset;
 
 	dev_priv->fbc.enabled = true;
 
@@ -200,7 +215,8 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 	if (IS_GEN5(dev_priv))
 		dpfc_ctl |= obj->fence_reg;
 
-	I915_WRITE(ILK_DPFC_FENCE_YOFF, crtc->base.y);
+	y_offset = get_crtc_fence_y_offset(crtc);
+	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
 	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
@@ -208,7 +224,7 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 	if (IS_GEN6(dev_priv)) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
-		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->base.y);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, y_offset);
 	}
 
 	intel_fbc_nuke(dev_priv);
@@ -288,7 +304,7 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 
 	I915_WRITE(SNB_DPFC_CTL_SA,
 		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
-	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->base.y);
+	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
 
 	intel_fbc_nuke(dev_priv);
 
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/16] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Paulo Zanoni]

The doc is pretty clear that this register should be set to 0 on SNB.
We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9ffa7dc..f7be9ab8 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 		dpfc_ctl |= obj->fence_reg;
 
 	y_offset = get_crtc_fence_y_offset(crtc);
-	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
+
+	if (IS_GEN5(dev_priv))
+		I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
+	else
+		I915_WRITE(ILK_DPFC_FENCE_YOFF, 0);
+
 	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/16] drm/i915: check for the supported strides on HSW+ FBC

[Paulo Zanoni]

I could only find the restrictions for HSW+, but I think it's safe to
assume that the older platforms also can't support the configurations
HSW can't support. The older platforms probably have additional
restrictions, so we need to figure out those and implement them later.
Let's not block HSW+ FBC based on missing information for the older
platforms.

v2:
  - Just WARN_ON() the strides that should have been caught earlier
    (Daniel)
  - Make it a new function since I expect this to grow more.
v3:
  - Document which IGT test is exercised by this.

Testcase: igt/kms_frontbuffer_tracking/fbc-badstride
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e0f3f05..4fd7858 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -940,6 +940,7 @@ struct i915_fbc {
 		FBC_CHIP_DEFAULT, /* disabled by default on this chip */
 		FBC_ROTATION, /* rotation is not supported */
 		FBC_IN_DBG_MASTER, /* kernel debugger is active */
+		FBC_BAD_STRIDE, /* stride is not supported */
 	} no_fbc_reason;
 
 	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index f7be9ab8..73bd559 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -501,6 +501,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 		return "rotation unsupported";
 	case FBC_IN_DBG_MASTER:
 		return "Kernel debugger is active";
+	case FBC_BAD_STRIDE:
+		return "framebuffer stride not supported";
 	default:
 		MISSING_CASE(reason);
 		return "unknown reason";
@@ -694,6 +696,22 @@ static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
 	return intel_fbc_alloc_cfb(dev_priv, size, fb_cpp);
 }
 
+static bool stride_is_valid(unsigned int stride)
+{
+	/* TODO: we need to figure out what are the stride restrictions for the
+	 * older platforms. */
+
+	/* These should have been caught earlier. */
+	WARN_ON(stride < 512);
+	WARN_ON((stride & (64 - 1)) != 0);
+
+	/* These are additional FBC restrictions. */
+	if (stride > 16385)
+		return false;
+
+	return true;
+}
+
 /**
  * __intel_fbc_update - enable/disable FBC as needed, unlocked
  * @dev_priv: i915 device instance
@@ -804,6 +822,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
+	if (!stride_is_valid(fb->pitches[0])) {
+		set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE);
+		goto out_disable;
+	}
+
 	/* If the kernel debugger is active, always disable compression */
 	if (in_dbg_master()) {
 		set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/16] drm/i915: try a little harder to find an FBC CRTC

[Paulo Zanoni]

Keep searching in case the candidate has a NULL primary fb. This is
only relevant for the platforms that don't have the pipe_a_only
restriction.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 73bd559..a63d10a 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -532,16 +532,14 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
 		if (intel_crtc_active(tmp_crtc) &&
-		    to_intel_plane_state(tmp_crtc->primary->state)->visible)
+		    to_intel_plane_state(tmp_crtc->primary->state)->visible &&
+		    tmp_crtc->primary->fb != NULL)
 			crtc = tmp_crtc;
 
 		if (pipe_a_only)
 			break;
 	}
 
-	if (!crtc || crtc->primary->fb == NULL)
-		return NULL;
-
 	return crtc;
 }
 
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Paulo Zanoni]

If we want to try to enable FBC by default on any platform we need to
be on the safe side and disable it in case we get an underrun while
FBC is enabled on the corresponding pipe. We currently already have
other reasons for FIFO underruns on our driver, but the ones I saw
with FBC lead to black screens that only go away when you disable FBC.

The current FIFO underrun I see happens when the CFB is using the top
8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
on a system with 32mb of stolen memory. On this case, all the IGT
tests fail while checking the screen CRC. A later patch on this series
will fix this problem for real.

With this patch, the tests will start failing while checking if FBC is
enabled instead of failing while comparing the CRC of the black screen
against the correct CRC. So this patch is not hiding any IGT bugs: the
tests still fail, but now they fail with a different reason.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |  5 +++
 drivers/gpu/drm/i915/intel_drv.h           |  1 +
 drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
 4 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4fd7858..e74a844 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -926,6 +926,11 @@ struct i915_fbc {
 		struct drm_framebuffer *fb;
 	} *fbc_work;
 
+	struct intel_fbc_underrun_work {
+		struct work_struct work;
+		struct intel_crtc *crtc;
+	} underrun_work;
+
 	enum no_fbc_reason {
 		FBC_OK, /* FBC is enabled */
 		FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81b7d77..246925d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
 const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
+void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
 
 /* intel_hdmi.c */
 void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index a63d10a..28e569c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
+static void intel_fbc_underrun_work_fn(struct work_struct *__work)
+{
+	struct intel_fbc_underrun_work *work =
+		container_of(__work, struct intel_fbc_underrun_work, work);
+	struct intel_crtc *crtc = work->crtc;
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	mutex_lock(&dev_priv->fbc.lock);
+	if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
+		goto out;
+
+	DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
+	i915.enable_fbc = 0;
+	__intel_fbc_disable(dev_priv);
+
+out:
+	work->crtc = NULL;
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
+/**
+ * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
+ * @crtc: the CRTC that caused the underrun
+ *
+ * Although we can't know for sure what caused an underrun, one of the possible
+ * reasons is FBC. And on the FBC case, the user may have a black screen until
+ * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
+ * disable FBC just because it may help.
+ *
+ * We disable FBC by changing the i915 param, so FBC won't come back on the next
+ * frame just to cause another underrun. Test suites can force FBC back by
+ * changing the module parameter again through sysfs.
+ */
+void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
+
+	if (!dev_priv->fbc.enable_fbc)
+		return;
+
+	/* These checks are unlocked. We can't grab the lock since we're in the
+	 * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
+	 * interrupt is asynchronous. Still, this a "try to recover because we
+	 * have already failed" function, so let's at least try, and if we fail,
+	 * we'll probably be able to try again next frame when another underrun
+	 * happens. We'll also do the checks again in the work function, where
+	 * we can grab the lock. */
+	if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
+		return;
+
+	/* Something already scheduled it. */
+	if (work->crtc != NULL)
+		return;
+
+	work->crtc = crtc;
+	schedule_work(&work->work);
+}
+
 /**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
@@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	enum pipe pipe;
 
 	mutex_init(&dev_priv->fbc.lock);
+	INIT_WORK(&dev_priv->fbc.underrun_work.work,
+		  intel_fbc_underrun_work_fn);
 
 	if (!HAS_FBC(dev_priv)) {
 		dev_priv->fbc.enabled = false;
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 54daa66..90e60fb 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 	if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
 		DRM_ERROR("CPU pipe %c FIFO underrun\n",
 			  pipe_name(pipe));
+
+	intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
 }
 
 /**
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/16] drm/i915: avoid the last 8mb of stolen on BDW/SKL

[Paulo Zanoni]

The FBC hardware for these platforms doesn't have access to the
bios_reserved range, so it always assumes the maximum (8mb) is used.
So avoid this range while allocating.

This solves a bunch of FIFO underruns that happen if you end up
putting the CFB in that memory range. On my machine, with 32mb of
stolen, I need a 2560x1440 mode for that.

Testcase: igt/kms_frontbuffer_tracking/fbc-* (given the right setup)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  4 ++++
 drivers/gpu/drm/i915/i915_gem_gtt.h    |  1 +
 drivers/gpu/drm/i915/i915_gem_stolen.c | 26 +++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_fbc.c       | 16 ++++++++++++++--
 4 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e74a844..f44d101 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3159,6 +3159,10 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
 int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 				struct drm_mm_node *node, u64 size,
 				unsigned alignment);
+int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
+					 struct drm_mm_node *node, u64 size,
+					 unsigned alignment, u64 start,
+					 u64 end);
 void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 				 struct drm_mm_node *node);
 int i915_gem_init_stolen(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8275007..96ebb98 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -341,6 +341,7 @@ struct i915_gtt {
 	struct i915_address_space base;
 
 	size_t stolen_size;		/* Total size of stolen memory */
+	size_t stolen_usable_size;	/* Total size minus BIOS reserved */
 	u64 mappable_end;		/* End offset that we can CPU map */
 	struct io_mapping *mappable;	/* Mapping to our CPU mappable region */
 	phys_addr_t mappable_base;	/* PA of our GMADR */
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a36cb95..824334d 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -42,9 +42,9 @@
  * for is a boon.
  */
 
-int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
-				struct drm_mm_node *node, u64 size,
-				unsigned alignment)
+int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
+					 struct drm_mm_node *node, u64 size,
+					 unsigned alignment, u64 start, u64 end)
 {
 	int ret;
 
@@ -52,13 +52,23 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
 		return -ENODEV;
 
 	mutex_lock(&dev_priv->mm.stolen_lock);
-	ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment,
-				 DRM_MM_SEARCH_DEFAULT);
+	ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size,
+					  alignment, start, end,
+					  DRM_MM_SEARCH_DEFAULT);
 	mutex_unlock(&dev_priv->mm.stolen_lock);
 
 	return ret;
 }
 
+int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
+				struct drm_mm_node *node, u64 size,
+				unsigned alignment)
+{
+	return i915_gem_stolen_insert_node_in_range(dev_priv, node, size,
+					alignment, 0,
+					dev_priv->gtt.stolen_usable_size);
+}
+
 void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 				 struct drm_mm_node *node)
 {
@@ -352,9 +362,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
 		      dev_priv->gtt.stolen_size >> 10,
 		      (dev_priv->gtt.stolen_size - reserved_total) >> 10);
 
+	dev_priv->gtt.stolen_usable_size = dev_priv->gtt.stolen_size -
+					   reserved_total;
+
 	/* Basic memrange allocator for stolen space */
-	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
-		    reserved_total);
+	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_usable_size);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 28e569c..9fd7b74 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -570,6 +570,16 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv,
 {
 	int compression_threshold = 1;
 	int ret;
+	u64 end;
+
+	/* The FBC hardware for BDW/SKL doesn't have access to the stolen
+	 * reserved range size, so it always assumes the maximum (8mb) is used.
+	 * If we enable FBC using a CFB on that memory range we'll get FIFO
+	 * underruns, even if that range is not reserved by the BIOS. */
+	if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv))
+		end = dev_priv->gtt.stolen_size - 8 * 1024 * 1024;
+	else
+		end = dev_priv->gtt.stolen_usable_size;
 
 	/* HACK: This code depends on what we will do in *_enable_fbc. If that
 	 * code changes, this code needs to change as well.
@@ -579,7 +589,8 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv,
 	 */
 
 	/* Try to over-allocate to reduce reallocations and fragmentation. */
-	ret = i915_gem_stolen_insert_node(dev_priv, node, size <<= 1, 4096);
+	ret = i915_gem_stolen_insert_node_in_range(dev_priv, node, size <<= 1,
+						   4096, 0, end);
 	if (ret == 0)
 		return compression_threshold;
 
@@ -589,7 +600,8 @@ again:
 	    (fb_cpp == 2 && compression_threshold == 2))
 		return 0;
 
-	ret = i915_gem_stolen_insert_node(dev_priv, node, size >>= 1, 4096);
+	ret = i915_gem_stolen_insert_node_in_range(dev_priv, node, size >>= 1,
+						   4096, 0, end);
 	if (ret && INTEL_INFO(dev_priv)->gen <= 4) {
 		return 0;
 	} else if (ret) {
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/16] drm/i915: print the correct amount of bytes allocated for the CFB

[Paulo Zanoni]

And also print the threshold. I was surprised to see a log message
claiming the CFB size was 32mb when there was less than 24mb available
for it.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9fd7b74..dc84e67 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -655,8 +655,9 @@ static int intel_fbc_alloc_cfb(struct drm_i915_private *dev_priv, int size,
 
 	dev_priv->fbc.uncompressed_size = size;
 
-	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
-		      size);
+	DRM_DEBUG_KMS("reserved %llu bytes of contiguous stolen space for FBC, threshold: %d\n",
+		      dev_priv->fbc.compressed_fb.size,
+		      dev_priv->fbc.threshold);
 
 	return 0;
 
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/16] drm/i915: fix CFB size calculation

[Paulo Zanoni]

We were considering the whole framebuffer height, but the spec clearly
says that we should only consider the active display height size.

On my current testing machine, this moves us from 124 successes and
502 skips to 209 successes and 417 skips on "kms_frontbuffer_tracking
--fbc-only". The high amount of skips is due to the --fbc-only
argument. We had those skips due to not enough stolen memory for the
tests. We're now passing the maximum possible amount: 209.

Note: when this patch was written, the amount of tests we had for FBC
was different than what we have now.

Testcase: igt/kms_frontbuffer_tracking/fbc-*
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index dc84e67..cfd4cba 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -695,9 +695,15 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->fbc.lock);
 }
 
-static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
-			       int fb_cpp)
+static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
 {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	int size, fb_cpp;
+
+	size = crtc->config->pipe_src_h * fb->pitches[0];
+	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+
 	if (size <= dev_priv->fbc.uncompressed_size)
 		return 0;
 
@@ -844,8 +850,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
-				drm_format_plane_cpp(fb->pixel_format, 0))) {
+	if (intel_fbc_setup_cfb(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
 	}
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/16] drm/i915/bdw: don't enable FBC when pixel rate exceeds 95%

[Paulo Zanoni]

BSpec says we shouldn't enable FBC on BDW when the pipe pixel rate
exceeds 95% of the core display clock.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 1 +
 drivers/gpu/drm/i915/intel_fbc.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f44d101..f8f3ed3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -946,6 +946,7 @@ struct i915_fbc {
 		FBC_ROTATION, /* rotation is not supported */
 		FBC_IN_DBG_MASTER, /* kernel debugger is active */
 		FBC_BAD_STRIDE, /* stride is not supported */
+		FBC_PIXEL_RATE, /* pixel rate is too big */
 	} no_fbc_reason;
 
 	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index cfd4cba..9dee0b5 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -503,6 +503,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 		return "Kernel debugger is active";
 	case FBC_BAD_STRIDE:
 		return "framebuffer stride not supported";
+	case FBC_PIXEL_RATE:
+		return "pixel rate is too big";
 	default:
 		MISSING_CASE(reason);
 		return "unknown reason";
@@ -850,6 +852,12 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
+	if (IS_BROADWELL(dev_priv) && ilk_pipe_pixel_rate(intel_crtc->config) >=
+	    dev_priv->max_cdclk_freq * 95 / 100) {
+		set_no_fbc_reason(dev_priv, FBC_PIXEL_RATE);
+		goto out_disable;
+	}
+
 	if (intel_fbc_setup_cfb(intel_crtc)) {
 		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
 		goto out_disable;
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/16] drm/i915: apply WaFbcAsynchFlipDisableFbcQueue earlier

[Paulo Zanoni]

The spec says the register should have that value for the entire time
that FBC is enabled, so apply the WA before we enable FBC.

Notice that we also have this WA for ILK/SNB, but it is implemented at
init_clock_gating(). I could move the IVB/HSW/BDW WA code to
init_clock_gating() too, but since we recently had some complaints
about WAs not staying after being set, I'm going to play safe and keep
this here for now.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9dee0b5..b76c19f 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -293,8 +293,6 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 	if (dev_priv->fbc.false_color)
 		dpfc_ctl |= FBC_CTL_FALSE_COLOR;
 
-	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
-
 	if (IS_IVYBRIDGE(dev_priv)) {
 		/* WaFbcAsynchFlipDisableFbcQueue:ivb */
 		I915_WRITE(ILK_DISPLAY_CHICKEN1,
@@ -307,6 +305,8 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 			   HSW_FBCQ_DIS);
 	}
 
+	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
+
 	I915_WRITE(SNB_DPFC_CTL_SA,
 		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/16] drm/i915: don't use the first stolen page on Broadwell

[Paulo Zanoni]

The spec says we just can't use it.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 824334d..a292ea1 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -51,6 +51,9 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return -ENODEV;
 
+	if (IS_BROADWELL(dev_priv) && start < 4096)
+		start = 4096;
+
 	mutex_lock(&dev_priv->mm.stolen_lock);
 	ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node, size,
 					  alignment, start, end,
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/16] drm/i915: don't apply WaFbcAsynchFlipDisableFbcQueue on SKL

[Paulo Zanoni]

This WA is only for HSW/BDW.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b76c19f..5dfe460 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -298,7 +298,7 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 		I915_WRITE(ILK_DISPLAY_CHICKEN1,
 			   I915_READ(ILK_DISPLAY_CHICKEN1) |
 			   ILK_FBCQ_DIS);
-	} else {
+	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
 		I915_WRITE(CHICKEN_PIPESL_1(crtc->pipe),
 			   I915_READ(CHICKEN_PIPESL_1(crtc->pipe)) |
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen memory"

[Paulo Zanoni]

This reverts commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb.

Technology has evolved and now we have eDP panels with 3200x1800
resolution. In the meantime, the BIOS guys didn't change the default
32mb for stolen memory. And we can't assume our users will be able to
increase the default stolen memory size to more than 32mb - I'm not
even sure all BIOSes allow that.

So just the fbcon buffer alone eats 22mb of my stolen memroy, and due
to the BDW/SKL restriction of not using the last 8mb of stolen memory,
all that's left for FBC is 2mb! Since fbcon is not the coolest feature
ever, I think it's better to save our precious stolen resource to FBC
and the other guys.

Besides, neither the original commit message nor the review comments
showed any arguments in favor of actually allocating fbcon from
stolen.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 96476d7..f90bf72 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -139,9 +139,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
-	obj = i915_gem_object_create_stolen(dev, size);
-	if (obj == NULL)
-		obj = i915_gem_alloc_object(dev, size);
+	obj = i915_gem_alloc_object(dev, size);
 	if (!obj) {
 		DRM_ERROR("failed to allocate framebuffer\n");
 		ret = -ENOMEM;
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 16/16] drm/i915: reject invalid formats for FBC

[Paulo Zanoni]

This commit is essentially a rewrite of "drm/i915: Check pixel format
for fbc" from Ville Syrjälä. The idea is the same, but the code is
different due to all the changes that happened since his original
patch. So any bugs are due to my bad rewrite.

Testcases: igt/kms_frontbuffer_tracking/*fbc*-${format_name}-draw-*
Credits-to: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f8f3ed3..938f69f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -947,6 +947,7 @@ struct i915_fbc {
 		FBC_IN_DBG_MASTER, /* kernel debugger is active */
 		FBC_BAD_STRIDE, /* stride is not supported */
 		FBC_PIXEL_RATE, /* pixel rate is too big */
+		FBC_PIXEL_FORMAT /* pixel format is invalid */
 	} no_fbc_reason;
 
 	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 5dfe460..cd3ed90 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -505,6 +505,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
 		return "framebuffer stride not supported";
 	case FBC_PIXEL_RATE:
 		return "pixel rate is too big";
+	case FBC_PIXEL_FORMAT:
+		return "pixel format is invalid";
 	default:
 		MISSING_CASE(reason);
 		return "unknown reason";
@@ -731,6 +733,34 @@ static bool stride_is_valid(unsigned int stride)
 	return true;
 }
 
+static bool pixel_format_is_valid(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Primary planes don't support alpha, so the "A" formats and "X"
+	 * formats are one and the same. */
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		return true;
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+	case DRM_FORMAT_RGB565:
+		/* 16bpp not supported on gen2 */
+		if (IS_GEN2(dev))
+			return false;
+		/* WaFbcOnly1to1Ratio:ctg */
+		if (IS_G4X(dev_priv))
+			return false;
+		return true;
+	default:
+		return false;
+	}
+}
+
 /**
  * __intel_fbc_update - enable/disable FBC as needed, unlocked
  * @dev_priv: i915 device instance
@@ -846,6 +876,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
+	if (!pixel_format_is_valid(fb)) {
+		set_no_fbc_reason(dev_priv, FBC_PIXEL_FORMAT);
+		goto out_disable;
+	}
+
 	/* If the kernel debugger is active, always disable compression */
 	if (in_dbg_master()) {
 		set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Chris Wilson]

On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
> If we want to try to enable FBC by default on any platform we need to
> be on the safe side and disable it in case we get an underrun while
> FBC is enabled on the corresponding pipe. We currently already have
> other reasons for FIFO underruns on our driver, but the ones I saw
> with FBC lead to black screens that only go away when you disable FBC.
> 
> The current FIFO underrun I see happens when the CFB is using the top
> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
> on a system with 32mb of stolen memory. On this case, all the IGT
> tests fail while checking the screen CRC. A later patch on this series
> will fix this problem for real.
> 
> With this patch, the tests will start failing while checking if FBC is
> enabled instead of failing while comparing the CRC of the black screen
> against the correct CRC. So this patch is not hiding any IGT bugs: the
> tests still fail, but now they fail with a different reason.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
>  4 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4fd7858..e74a844 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -926,6 +926,11 @@ struct i915_fbc {
>  		struct drm_framebuffer *fb;
>  	} *fbc_work;
>  
> +	struct intel_fbc_underrun_work {
> +		struct work_struct work;
> +		struct intel_crtc *crtc;
> +	} underrun_work;

It's a singleshot (perhaps a couple in flight during the switch off), so
there's no need for a permanent allocation. Fight the bloat!
-Chris

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

Re: [PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen memory"

[Chris Wilson]

On Fri, Aug 14, 2015 at 06:34:20PM -0300, Paulo Zanoni wrote:
> This reverts commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb.
> 
> Technology has evolved and now we have eDP panels with 3200x1800
> resolution. In the meantime, the BIOS guys didn't change the default
> 32mb for stolen memory. And we can't assume our users will be able to
> increase the default stolen memory size to more than 32mb - I'm not
> even sure all BIOSes allow that.
> 
> So just the fbcon buffer alone eats 22mb of my stolen memroy, and due
> to the BDW/SKL restriction of not using the last 8mb of stolen memory,
> all that's left for FBC is 2mb! Since fbcon is not the coolest feature
> ever, I think it's better to save our precious stolen resource to FBC
> and the other guys.
> 
> Besides, neither the original commit message nor the review comments
> showed any arguments in favor of actually allocating fbcon from
> stolen.

Pardon?
-Chris

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

Re: [PATCH 08/16] drm/i915: avoid the last 8mb of stolen on BDW/SKL

[Chris Wilson]

On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote:
> The FBC hardware for these platforms doesn't have access to the
> bios_reserved range, so it always assumes the maximum (8mb) is used.
> So avoid this range while allocating.
> 
> This solves a bunch of FIFO underruns that happen if you end up
> putting the CFB in that memory range. On my machine, with 32mb of
> stolen, I need a 2560x1440 mode for that.

If the restriction applies only to FBC, apply it to only fbc.
-Chris

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

Re: [PATCH 13/16] drm/i915: don't use the first stolen page on Broadwell

[Chris Wilson]

On Fri, Aug 14, 2015 at 06:34:18PM -0300, Paulo Zanoni wrote:
> The spec says we just can't use it.

But what about when we inherit a framebuffer at that address?
-Chris

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

Re: [PATCH 08/16] drm/i915: avoid the last 8mb of stolen on BDW/SKL

[Zanoni, Paulo R]

Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu:
> On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote:
> > The FBC hardware for these platforms doesn't have access to the
> > bios_reserved range, so it always assumes the maximum (8mb) is 
> > used.
> > So avoid this range while allocating.
> > 
> > This solves a bunch of FIFO underruns that happen if you end up
> > putting the CFB in that memory range. On my machine, with 32mb of
> > stolen, I need a 2560x1440 mode for that.
> 
> If the restriction applies only to FBC, apply it to only fbc.

That's what the patch is doing. The part where we set the unusual
start/end is at intel_fbc.c:find_compression_threshold(). Everything
else should be behaving just as before.

Maybe you're being confused by the addition of the stolen_usable_size
variable.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen memory"

[Zanoni, Paulo R]

Em Sáb, 2015-08-15 às 09:24 +0100, Chris Wilson escreveu:
> On Fri, Aug 14, 2015 at 06:34:20PM -0300, Paulo Zanoni wrote:
> > This reverts commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb.
> > 
> > Technology has evolved and now we have eDP panels with 3200x1800
> > resolution. In the meantime, the BIOS guys didn't change the 
> > default
> > 32mb for stolen memory. And we can't assume our users will be able 
> > to
> > increase the default stolen memory size to more than 32mb - I'm not
> > even sure all BIOSes allow that.
> > 
> > So just the fbcon buffer alone eats 22mb of my stolen memroy, and 
> > due
> > to the BDW/SKL restriction of not using the last 8mb of stolen 
> > memory,
> > all that's left for FBC is 2mb! Since fbcon is not the coolest 
> > feature
> > ever, I think it's better to save our precious stolen resource to 
> > FBC
> > and the other guys.
> > 
> > Besides, neither the original commit message nor the review 
> > comments
> > showed any arguments in favor of actually allocating fbcon from
> > stolen.
> 
> Pardon?


pzanoni@panetone:~/git/kernel/kernel$ git show
0ffb0ff283cca16f72caf29c44496d83b0c291fb | head -n 12
commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:27 2012 +0000

    drm/i915: Allocate fbcon from stolen memory
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
    Acked-by: Ben Widawsky <ben@bwidawsk.net>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/intel_fb.c
b/drivers/gpu/drm/i915/intel_fb.c
pzanoni@panetone:~/git/kernel/kernel$ 

And the only review I could find was: 

http://lists.freedesktop.org/archives/intel-gfx/2012
-October/021025.html

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen memory"

[chris]

On Tue, Aug 18, 2015 at 09:54:57PM +0000, Zanoni, Paulo R wrote:
> Em Sáb, 2015-08-15 às 09:24 +0100, Chris Wilson escreveu:
> > On Fri, Aug 14, 2015 at 06:34:20PM -0300, Paulo Zanoni wrote:
> > > This reverts commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb.
> > > 
> > > Technology has evolved and now we have eDP panels with 3200x1800
> > > resolution. In the meantime, the BIOS guys didn't change the 
> > > default
> > > 32mb for stolen memory. And we can't assume our users will be able 
> > > to
> > > increase the default stolen memory size to more than 32mb - I'm not
> > > even sure all BIOSes allow that.
> > > 
> > > So just the fbcon buffer alone eats 22mb of my stolen memroy, and 
> > > due
> > > to the BDW/SKL restriction of not using the last 8mb of stolen 
> > > memory,
> > > all that's left for FBC is 2mb! Since fbcon is not the coolest 
> > > feature
> > > ever, I think it's better to save our precious stolen resource to 
> > > FBC
> > > and the other guys.
> > > 
> > > Besides, neither the original commit message nor the review 
> > > comments
> > > showed any arguments in favor of actually allocating fbcon from
> > > stolen.
> > 
> > Pardon?
> 
> 
> pzanoni@panetone:~/git/kernel/kernel$ git show
> 0ffb0ff283cca16f72caf29c44496d83b0c291fb | head -n 12
> commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Nov 15 11:32:27 2012 +0000
> 
>     drm/i915: Allocate fbcon from stolen memory
>     
>     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>     Acked-by: Ben Widawsky <ben@bwidawsk.net>
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> diff --git a/drivers/gpu/drm/i915/intel_fb.c
> b/drivers/gpu/drm/i915/intel_fb.c
> pzanoni@panetone:~/git/kernel/kernel$ 

Sorry, I though it was pretty self explanatory that we want to reuse as
much as stolen as is possible on all machines, epecially those where
even 8MiB reserved is about 10% of available memory. So not using it for
pinned memory is a double whammy.

That would have been said in the preceeding patches to enable stolen
memory.
-Chris

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

Re: [PATCH 08/16] drm/i915: avoid the last 8mb of stolen on BDW/SKL

[chris]

On Tue, Aug 18, 2015 at 09:49:34PM +0000, Zanoni, Paulo R wrote:
> Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu:
> > On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote:
> > > The FBC hardware for these platforms doesn't have access to the
> > > bios_reserved range, so it always assumes the maximum (8mb) is 
> > > used.
> > > So avoid this range while allocating.
> > > 
> > > This solves a bunch of FIFO underruns that happen if you end up
> > > putting the CFB in that memory range. On my machine, with 32mb of
> > > stolen, I need a 2560x1440 mode for that.
> > 
> > If the restriction applies only to FBC, apply it to only fbc.
> 
> That's what the patch is doing. The part where we set the unusual
> start/end is at intel_fbc.c:find_compression_threshold(). Everything
> else should be behaving just as before.
> 
> Maybe you're being confused by the addition of the stolen_usable_size
> variable.

Hmm, I expected to see the constaint being passed into the search
routine.a It looked like you were adjusting the stolen_size probably the
root of my confusion.

Anyway we have a quandary. You want to reserve stolen space, and I want
to make sure as much of it gets used as possible (especially for long
lived objects).

What I have in mind is adding a priority to the search and allow us to
evict anything of lower priority. We can use a page replacement
algorithm even for the pinned fbcon (since only the GGTT itself is
pinned and we can swap the pages underneath). This of course requires a
callback for low priority evictable objects. I have 3 priorities in mind
plus the evictable flag: USER, KERNEL, POWER (with FBC taking the
highest priority of POWER). That will allow us to do the BIOS takeover
and only if we run out of space force the copy.
-Chris

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

Re: [PATCH 13/16] drm/i915: don't use the first stolen page on Broadwell

[Ville Syrjälä]

On Sat, Aug 15, 2015 at 09:30:18AM +0100, Chris Wilson wrote:
> On Fri, Aug 14, 2015 at 06:34:18PM -0300, Paulo Zanoni wrote:
> > The spec says we just can't use it.
> 
> But what about when we inherit a framebuffer at that address?

Indeed. I asked the same question several times during the past attempts
at this, and even tried to outline a potential solution at least once [1]

Also limiting it to BDW is insufficient. CHV needs it too.

[1] http://lists.freedesktop.org/archives/intel-gfx/2014-July/049924.html

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
> If we want to try to enable FBC by default on any platform we need to
> be on the safe side and disable it in case we get an underrun while
> FBC is enabled on the corresponding pipe. We currently already have
> other reasons for FIFO underruns on our driver, but the ones I saw
> with FBC lead to black screens that only go away when you disable FBC.

We don't try to deal with underruns on other platforms either, and yes
on some, cough, chv, cough, they can definitely blank out the screen
until the next modeset. On even older platforms it's even worse and an
underrun can kill the display engine until display reset/reboot :(
Especially annoying on gen2 where we have no reset support. So I'm not
entirely convinced FBC deserves special treatment here.

> 
> The current FIFO underrun I see happens when the CFB is using the top
> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
> on a system with 32mb of stolen memory. On this case, all the IGT
> tests fail while checking the screen CRC. A later patch on this series
> will fix this problem for real.
> 
> With this patch, the tests will start failing while checking if FBC is
> enabled instead of failing while comparing the CRC of the black screen
> against the correct CRC. So this patch is not hiding any IGT bugs: the
> tests still fail, but now they fail with a different reason.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
>  4 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4fd7858..e74a844 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -926,6 +926,11 @@ struct i915_fbc {
>  		struct drm_framebuffer *fb;
>  	} *fbc_work;
>  
> +	struct intel_fbc_underrun_work {
> +		struct work_struct work;
> +		struct intel_crtc *crtc;
> +	} underrun_work;
> +
>  	enum no_fbc_reason {
>  		FBC_OK, /* FBC is enabled */
>  		FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81b7d77..246925d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
>  
>  /* intel_hdmi.c */
>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index a63d10a..28e569c 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> +static void intel_fbc_underrun_work_fn(struct work_struct *__work)
> +{
> +	struct intel_fbc_underrun_work *work =
> +		container_of(__work, struct intel_fbc_underrun_work, work);
> +	struct intel_crtc *crtc = work->crtc;
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +
> +	mutex_lock(&dev_priv->fbc.lock);
> +	if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> +		goto out;
> +
> +	DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> +	i915.enable_fbc = 0;
> +	__intel_fbc_disable(dev_priv);
> +
> +out:
> +	work->crtc = NULL;
> +	mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
> +/**
> + * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
> + * @crtc: the CRTC that caused the underrun
> + *
> + * Although we can't know for sure what caused an underrun, one of the possible
> + * reasons is FBC. And on the FBC case, the user may have a black screen until
> + * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
> + * disable FBC just because it may help.
> + *
> + * We disable FBC by changing the i915 param, so FBC won't come back on the next
> + * frame just to cause another underrun. Test suites can force FBC back by
> + * changing the module parameter again through sysfs.
> + */
> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
> +
> +	if (!dev_priv->fbc.enable_fbc)
> +		return;
> +
> +	/* These checks are unlocked. We can't grab the lock since we're in the
> +	 * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
> +	 * interrupt is asynchronous. Still, this a "try to recover because we
> +	 * have already failed" function, so let's at least try, and if we fail,
> +	 * we'll probably be able to try again next frame when another underrun
> +	 * happens. We'll also do the checks again in the work function, where
> +	 * we can grab the lock. */
> +	if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> +		return;
> +
> +	/* Something already scheduled it. */
> +	if (work->crtc != NULL)
> +		return;
> +
> +	work->crtc = crtc;
> +	schedule_work(&work->work);
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	enum pipe pipe;
>  
>  	mutex_init(&dev_priv->fbc.lock);
> +	INIT_WORK(&dev_priv->fbc.underrun_work.work,
> +		  intel_fbc_underrun_work_fn);
>  
>  	if (!HAS_FBC(dev_priv)) {
>  		dev_priv->fbc.enabled = false;
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 54daa66..90e60fb 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  	if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
>  		DRM_ERROR("CPU pipe %c FIFO underrun\n",
>  			  pipe_name(pipe));
> +
> +	intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
>  }
>  
>  /**
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Paulo Zanoni]

2015-08-19 9:06 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
>> If we want to try to enable FBC by default on any platform we need to
>> be on the safe side and disable it in case we get an underrun while
>> FBC is enabled on the corresponding pipe. We currently already have
>> other reasons for FIFO underruns on our driver, but the ones I saw
>> with FBC lead to black screens that only go away when you disable FBC.
>
> We don't try to deal with underruns on other platforms either, and yes
> on some, cough, chv, cough, they can definitely blank out the screen
> until the next modeset. On even older platforms it's even worse and an
> underrun can kill the display engine until display reset/reboot :(
> Especially annoying on gen2 where we have no reset support. So I'm not
> entirely convinced FBC deserves special treatment here.

I don't understand your logic. To me, it sounds like "you're proposing
adding airbags to new cars, but that is not going to protect against
every type of accident, and it's also not going to help the cars that
are already manufactured, so I don't think front-collisions of new
cars deserve special treatment".

>
>>
>> The current FIFO underrun I see happens when the CFB is using the top
>> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
>> on a system with 32mb of stolen memory. On this case, all the IGT
>> tests fail while checking the screen CRC. A later patch on this series
>> will fix this problem for real.
>>
>> With this patch, the tests will start failing while checking if FBC is
>> enabled instead of failing while comparing the CRC of the black screen
>> against the correct CRC. So this patch is not hiding any IGT bugs: the
>> tests still fail, but now they fail with a different reason.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
>>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
>>  4 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 4fd7858..e74a844 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -926,6 +926,11 @@ struct i915_fbc {
>>               struct drm_framebuffer *fb;
>>       } *fbc_work;
>>
>> +     struct intel_fbc_underrun_work {
>> +             struct work_struct work;
>> +             struct intel_crtc *crtc;
>> +     } underrun_work;
>> +
>>       enum no_fbc_reason {
>>               FBC_OK, /* FBC is enabled */
>>               FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 81b7d77..246925d 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>>                    unsigned int frontbuffer_bits, enum fb_op_origin origin);
>>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
>>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
>> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
>>
>>  /* intel_hdmi.c */
>>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index a63d10a..28e569c 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>>       mutex_unlock(&dev_priv->fbc.lock);
>>  }
>>
>> +static void intel_fbc_underrun_work_fn(struct work_struct *__work)
>> +{
>> +     struct intel_fbc_underrun_work *work =
>> +             container_of(__work, struct intel_fbc_underrun_work, work);
>> +     struct intel_crtc *crtc = work->crtc;
>> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +
>> +     mutex_lock(&dev_priv->fbc.lock);
>> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
>> +             goto out;
>> +
>> +     DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
>> +     i915.enable_fbc = 0;
>> +     __intel_fbc_disable(dev_priv);
>> +
>> +out:
>> +     work->crtc = NULL;
>> +     mutex_unlock(&dev_priv->fbc.lock);
>> +}
>> +
>> +/**
>> + * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
>> + * @crtc: the CRTC that caused the underrun
>> + *
>> + * Although we can't know for sure what caused an underrun, one of the possible
>> + * reasons is FBC. And on the FBC case, the user may have a black screen until
>> + * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
>> + * disable FBC just because it may help.
>> + *
>> + * We disable FBC by changing the i915 param, so FBC won't come back on the next
>> + * frame just to cause another underrun. Test suites can force FBC back by
>> + * changing the module parameter again through sysfs.
>> + */
>> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
>> +{
>> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +     struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
>> +
>> +     if (!dev_priv->fbc.enable_fbc)
>> +             return;
>> +
>> +     /* These checks are unlocked. We can't grab the lock since we're in the
>> +      * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
>> +      * interrupt is asynchronous. Still, this a "try to recover because we
>> +      * have already failed" function, so let's at least try, and if we fail,
>> +      * we'll probably be able to try again next frame when another underrun
>> +      * happens. We'll also do the checks again in the work function, where
>> +      * we can grab the lock. */
>> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
>> +             return;
>> +
>> +     /* Something already scheduled it. */
>> +     if (work->crtc != NULL)
>> +             return;
>> +
>> +     work->crtc = crtc;
>> +     schedule_work(&work->work);
>> +}
>> +
>>  /**
>>   * intel_fbc_init - Initialize FBC
>>   * @dev_priv: the i915 device
>> @@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>>       enum pipe pipe;
>>
>>       mutex_init(&dev_priv->fbc.lock);
>> +     INIT_WORK(&dev_priv->fbc.underrun_work.work,
>> +               intel_fbc_underrun_work_fn);
>>
>>       if (!HAS_FBC(dev_priv)) {
>>               dev_priv->fbc.enabled = false;
>> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
>> index 54daa66..90e60fb 100644
>> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
>> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
>> @@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>>       if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
>>               DRM_ERROR("CPU pipe %c FIFO underrun\n",
>>                         pipe_name(pipe));
>> +
>> +     intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
>>  }
>>
>>  /**
>> --
>> 2.4.6
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Ville Syrjälä]

On Thu, Aug 20, 2015 at 10:30:17AM -0300, Paulo Zanoni wrote:
> 2015-08-19 9:06 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
> >> If we want to try to enable FBC by default on any platform we need to
> >> be on the safe side and disable it in case we get an underrun while
> >> FBC is enabled on the corresponding pipe. We currently already have
> >> other reasons for FIFO underruns on our driver, but the ones I saw
> >> with FBC lead to black screens that only go away when you disable FBC.
> >
> > We don't try to deal with underruns on other platforms either, and yes
> > on some, cough, chv, cough, they can definitely blank out the screen
> > until the next modeset. On even older platforms it's even worse and an
> > underrun can kill the display engine until display reset/reboot :(
> > Especially annoying on gen2 where we have no reset support. So I'm not
> > entirely convinced FBC deserves special treatment here.
> 
> I don't understand your logic. To me, it sounds like "you're proposing
> adding airbags to new cars, but that is not going to protect against
> every type of accident, and it's also not going to help the cars that
> are already manufactured, so I don't think front-collisions of new
> cars deserve special treatment".

Currently FBC is more like like "driving backwards on one wheel". I'm
not sure there's much point in trying to make it fault tolerant while we
know the code is still broken. Once it's otherwise known to be solid,
then it might make sense, although a much cooler thing would be if we
could actually detect when the display has failed entirely and recover
it somehow.

Oh and to make the protection mechanism actually kick in reliably you
would somehow need to address the problems with the underrun interrupts.
At the moment, to continue your car analogy, it's like the airbags would
refuse to deploy in a real crash if you happened to ding the mailbox
while pulling out of your driveway.

> 
> >
> >>
> >> The current FIFO underrun I see happens when the CFB is using the top
> >> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
> >> on a system with 32mb of stolen memory. On this case, all the IGT
> >> tests fail while checking the screen CRC. A later patch on this series
> >> will fix this problem for real.
> >>
> >> With this patch, the tests will start failing while checking if FBC is
> >> enabled instead of failing while comparing the CRC of the black screen
> >> against the correct CRC. So this patch is not hiding any IGT bugs: the
> >> tests still fail, but now they fail with a different reason.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
> >>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
> >>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
> >>  4 files changed, 69 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 4fd7858..e74a844 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -926,6 +926,11 @@ struct i915_fbc {
> >>               struct drm_framebuffer *fb;
> >>       } *fbc_work;
> >>
> >> +     struct intel_fbc_underrun_work {
> >> +             struct work_struct work;
> >> +             struct intel_crtc *crtc;
> >> +     } underrun_work;
> >> +
> >>       enum no_fbc_reason {
> >>               FBC_OK, /* FBC is enabled */
> >>               FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 81b7d77..246925d 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >>                    unsigned int frontbuffer_bits, enum fb_op_origin origin);
> >>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
> >>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
> >>
> >>  /* intel_hdmi.c */
> >>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index a63d10a..28e569c 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >>       mutex_unlock(&dev_priv->fbc.lock);
> >>  }
> >>
> >> +static void intel_fbc_underrun_work_fn(struct work_struct *__work)
> >> +{
> >> +     struct intel_fbc_underrun_work *work =
> >> +             container_of(__work, struct intel_fbc_underrun_work, work);
> >> +     struct intel_crtc *crtc = work->crtc;
> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> +
> >> +     mutex_lock(&dev_priv->fbc.lock);
> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> >> +             goto out;
> >> +
> >> +     DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> >> +     i915.enable_fbc = 0;
> >> +     __intel_fbc_disable(dev_priv);
> >> +
> >> +out:
> >> +     work->crtc = NULL;
> >> +     mutex_unlock(&dev_priv->fbc.lock);
> >> +}
> >> +
> >> +/**
> >> + * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
> >> + * @crtc: the CRTC that caused the underrun
> >> + *
> >> + * Although we can't know for sure what caused an underrun, one of the possible
> >> + * reasons is FBC. And on the FBC case, the user may have a black screen until
> >> + * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
> >> + * disable FBC just because it may help.
> >> + *
> >> + * We disable FBC by changing the i915 param, so FBC won't come back on the next
> >> + * frame just to cause another underrun. Test suites can force FBC back by
> >> + * changing the module parameter again through sysfs.
> >> + */
> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
> >> +{
> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> +     struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
> >> +
> >> +     if (!dev_priv->fbc.enable_fbc)
> >> +             return;
> >> +
> >> +     /* These checks are unlocked. We can't grab the lock since we're in the
> >> +      * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
> >> +      * interrupt is asynchronous. Still, this a "try to recover because we
> >> +      * have already failed" function, so let's at least try, and if we fail,
> >> +      * we'll probably be able to try again next frame when another underrun
> >> +      * happens. We'll also do the checks again in the work function, where
> >> +      * we can grab the lock. */
> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> >> +             return;
> >> +
> >> +     /* Something already scheduled it. */
> >> +     if (work->crtc != NULL)
> >> +             return;
> >> +
> >> +     work->crtc = crtc;
> >> +     schedule_work(&work->work);
> >> +}
> >> +
> >>  /**
> >>   * intel_fbc_init - Initialize FBC
> >>   * @dev_priv: the i915 device
> >> @@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> >>       enum pipe pipe;
> >>
> >>       mutex_init(&dev_priv->fbc.lock);
> >> +     INIT_WORK(&dev_priv->fbc.underrun_work.work,
> >> +               intel_fbc_underrun_work_fn);
> >>
> >>       if (!HAS_FBC(dev_priv)) {
> >>               dev_priv->fbc.enabled = false;
> >> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> index 54daa66..90e60fb 100644
> >> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> @@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> >>       if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> >>               DRM_ERROR("CPU pipe %c FIFO underrun\n",
> >>                         pipe_name(pipe));
> >> +
> >> +     intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
> >>  }
> >>
> >>  /**
> >> --
> >> 2.4.6
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Paulo Zanoni]

2015-08-20 10:58 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Aug 20, 2015 at 10:30:17AM -0300, Paulo Zanoni wrote:
>> 2015-08-19 9:06 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>> > On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
>> >> If we want to try to enable FBC by default on any platform we need to
>> >> be on the safe side and disable it in case we get an underrun while
>> >> FBC is enabled on the corresponding pipe. We currently already have
>> >> other reasons for FIFO underruns on our driver, but the ones I saw
>> >> with FBC lead to black screens that only go away when you disable FBC.
>> >
>> > We don't try to deal with underruns on other platforms either, and yes
>> > on some, cough, chv, cough, they can definitely blank out the screen
>> > until the next modeset. On even older platforms it's even worse and an
>> > underrun can kill the display engine until display reset/reboot :(
>> > Especially annoying on gen2 where we have no reset support. So I'm not
>> > entirely convinced FBC deserves special treatment here.
>>
>> I don't understand your logic. To me, it sounds like "you're proposing
>> adding airbags to new cars, but that is not going to protect against
>> every type of accident, and it's also not going to help the cars that
>> are already manufactured, so I don't think front-collisions of new
>> cars deserve special treatment".
>
> Currently FBC is more like like "driving backwards on one wheel". I'm
> not sure there's much point in trying to make it fault tolerant while we
> know the code is still broken.

That's why this series contains other patches fixing the other
problems. That's also why I recently added a few thousands of lines of
code to IGT. Besides, no one is proposing to enable FBC by default on
every single platform. I don't think "feature X doesn't work at the
moment" is a reason to reject a patch that improves feature X.

> Once it's otherwise known to be solid,
> then it might make sense, although a much cooler thing would be if we
> could actually detect when the display has failed entirely and recover
> it somehow.
>
> Oh and to make the protection mechanism actually kick in reliably you
> would somehow need to address the problems with the underrun interrupts.

Which problems?

When I tested this patch, it was working very reliably: we detected
the underrun and disabled FBC 100% of the time.

It's also worth noticing that the cause of the underrun is actually
fixed by a later patch of this series, so we don't really need this
patch as part of the whole "fix FBC" task, but I really think it's
better to have it than not have it, just in case we regress somehow.

> At the moment, to continue your car analogy, it's like the airbags would
> refuse to deploy in a real crash if you happened to ding the mailbox
> while pulling out of your driveway.
>
>>
>> >
>> >>
>> >> The current FIFO underrun I see happens when the CFB is using the top
>> >> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
>> >> on a system with 32mb of stolen memory. On this case, all the IGT
>> >> tests fail while checking the screen CRC. A later patch on this series
>> >> will fix this problem for real.
>> >>
>> >> With this patch, the tests will start failing while checking if FBC is
>> >> enabled instead of failing while comparing the CRC of the black screen
>> >> against the correct CRC. So this patch is not hiding any IGT bugs: the
>> >> tests still fail, but now they fail with a different reason.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
>> >>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>> >>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
>> >>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
>> >>  4 files changed, 69 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> >> index 4fd7858..e74a844 100644
>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> @@ -926,6 +926,11 @@ struct i915_fbc {
>> >>               struct drm_framebuffer *fb;
>> >>       } *fbc_work;
>> >>
>> >> +     struct intel_fbc_underrun_work {
>> >> +             struct work_struct work;
>> >> +             struct intel_crtc *crtc;
>> >> +     } underrun_work;
>> >> +
>> >>       enum no_fbc_reason {
>> >>               FBC_OK, /* FBC is enabled */
>> >>               FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
>> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> >> index 81b7d77..246925d 100644
>> >> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> @@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>> >>                    unsigned int frontbuffer_bits, enum fb_op_origin origin);
>> >>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
>> >>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
>> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
>> >>
>> >>  /* intel_hdmi.c */
>> >>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
>> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> >> index a63d10a..28e569c 100644
>> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> >> @@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>> >>       mutex_unlock(&dev_priv->fbc.lock);
>> >>  }
>> >>
>> >> +static void intel_fbc_underrun_work_fn(struct work_struct *__work)
>> >> +{
>> >> +     struct intel_fbc_underrun_work *work =
>> >> +             container_of(__work, struct intel_fbc_underrun_work, work);
>> >> +     struct intel_crtc *crtc = work->crtc;
>> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> >> +
>> >> +     mutex_lock(&dev_priv->fbc.lock);
>> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
>> >> +             goto out;
>> >> +
>> >> +     DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
>> >> +     i915.enable_fbc = 0;
>> >> +     __intel_fbc_disable(dev_priv);
>> >> +
>> >> +out:
>> >> +     work->crtc = NULL;
>> >> +     mutex_unlock(&dev_priv->fbc.lock);
>> >> +}
>> >> +
>> >> +/**
>> >> + * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
>> >> + * @crtc: the CRTC that caused the underrun
>> >> + *
>> >> + * Although we can't know for sure what caused an underrun, one of the possible
>> >> + * reasons is FBC. And on the FBC case, the user may have a black screen until
>> >> + * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
>> >> + * disable FBC just because it may help.
>> >> + *
>> >> + * We disable FBC by changing the i915 param, so FBC won't come back on the next
>> >> + * frame just to cause another underrun. Test suites can force FBC back by
>> >> + * changing the module parameter again through sysfs.
>> >> + */
>> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
>> >> +{
>> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> >> +     struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
>> >> +
>> >> +     if (!dev_priv->fbc.enable_fbc)
>> >> +             return;
>> >> +
>> >> +     /* These checks are unlocked. We can't grab the lock since we're in the
>> >> +      * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
>> >> +      * interrupt is asynchronous. Still, this a "try to recover because we
>> >> +      * have already failed" function, so let's at least try, and if we fail,
>> >> +      * we'll probably be able to try again next frame when another underrun
>> >> +      * happens. We'll also do the checks again in the work function, where
>> >> +      * we can grab the lock. */
>> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
>> >> +             return;
>> >> +
>> >> +     /* Something already scheduled it. */
>> >> +     if (work->crtc != NULL)
>> >> +             return;
>> >> +
>> >> +     work->crtc = crtc;
>> >> +     schedule_work(&work->work);
>> >> +}
>> >> +
>> >>  /**
>> >>   * intel_fbc_init - Initialize FBC
>> >>   * @dev_priv: the i915 device
>> >> @@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>> >>       enum pipe pipe;
>> >>
>> >>       mutex_init(&dev_priv->fbc.lock);
>> >> +     INIT_WORK(&dev_priv->fbc.underrun_work.work,
>> >> +               intel_fbc_underrun_work_fn);
>> >>
>> >>       if (!HAS_FBC(dev_priv)) {
>> >>               dev_priv->fbc.enabled = false;
>> >> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
>> >> index 54daa66..90e60fb 100644
>> >> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
>> >> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
>> >> @@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>> >>       if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
>> >>               DRM_ERROR("CPU pipe %c FIFO underrun\n",
>> >>                         pipe_name(pipe));
>> >> +
>> >> +     intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
>> >>  }
>> >>
>> >>  /**
>> >> --
>> >> 2.4.6
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Ville Syrjälä
>> > Intel OTC
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Ville Syrjälä]

On Thu, Aug 20, 2015 at 11:29:29AM -0300, Paulo Zanoni wrote:
> 2015-08-20 10:58 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Thu, Aug 20, 2015 at 10:30:17AM -0300, Paulo Zanoni wrote:
> >> 2015-08-19 9:06 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> >> > On Fri, Aug 14, 2015 at 06:34:12PM -0300, Paulo Zanoni wrote:
> >> >> If we want to try to enable FBC by default on any platform we need to
> >> >> be on the safe side and disable it in case we get an underrun while
> >> >> FBC is enabled on the corresponding pipe. We currently already have
> >> >> other reasons for FIFO underruns on our driver, but the ones I saw
> >> >> with FBC lead to black screens that only go away when you disable FBC.
> >> >
> >> > We don't try to deal with underruns on other platforms either, and yes
> >> > on some, cough, chv, cough, they can definitely blank out the screen
> >> > until the next modeset. On even older platforms it's even worse and an
> >> > underrun can kill the display engine until display reset/reboot :(
> >> > Especially annoying on gen2 where we have no reset support. So I'm not
> >> > entirely convinced FBC deserves special treatment here.
> >>
> >> I don't understand your logic. To me, it sounds like "you're proposing
> >> adding airbags to new cars, but that is not going to protect against
> >> every type of accident, and it's also not going to help the cars that
> >> are already manufactured, so I don't think front-collisions of new
> >> cars deserve special treatment".
> >
> > Currently FBC is more like like "driving backwards on one wheel". I'm
> > not sure there's much point in trying to make it fault tolerant while we
> > know the code is still broken.
> 
> That's why this series contains other patches fixing the other
> problems.

Well the whole intel_update_fbc() thing is still very much a fail wrt.
locking. But maybe you're planning on resurrecting my fbc_score idea?

Also now I that I glanced at the code, intel_fbc_flush() looks busted.
To actually nuke the FBC state you need to wait at least until the next
vblank before re-enabling FBC. That was one reason I had the vblank
notify thingy in my "fix FBC" series.

> That's also why I recently added a few thousands of lines of
> code to IGT. Besides, no one is proposing to enable FBC by default on
> every single platform. I don't think "feature X doesn't work at the
> moment" is a reason to reject a patch that improves feature X.
> 
> > Once it's otherwise known to be solid,
> > then it might make sense, although a much cooler thing would be if we
> > could actually detect when the display has failed entirely and recover
> > it somehow.
> >
> > Oh and to make the protection mechanism actually kick in reliably you
> > would somehow need to address the problems with the underrun interrupts.
> 
> Which problems?

The fact that we disable them as soon as one occurs, and even worse the
error interrupt is a shared one on many platforms, so one underrun any
pipe or some other unrelated error kills underrun detection everywhere.

> 
> When I tested this patch, it was working very reliably: we detected
> the underrun and disabled FBC 100% of the time.
> 
> It's also worth noticing that the cause of the underrun is actually
> fixed by a later patch of this series, so we don't really need this
> patch as part of the whole "fix FBC" task, but I really think it's
> better to have it than not have it, just in case we regress somehow.
> 
> > At the moment, to continue your car analogy, it's like the airbags would
> > refuse to deploy in a real crash if you happened to ding the mailbox
> > while pulling out of your driveway.
> >
> >>
> >> >
> >> >>
> >> >> The current FIFO underrun I see happens when the CFB is using the top
> >> >> 8mb of stolen memory. This is reproducible with a 2560x1440 DP Monitor
> >> >> on a system with 32mb of stolen memory. On this case, all the IGT
> >> >> tests fail while checking the screen CRC. A later patch on this series
> >> >> will fix this problem for real.
> >> >>
> >> >> With this patch, the tests will start failing while checking if FBC is
> >> >> enabled instead of failing while comparing the CRC of the black screen
> >> >> against the correct CRC. So this patch is not hiding any IGT bugs: the
> >> >> tests still fail, but now they fail with a different reason.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
> >> >>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
> >> >>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
> >> >>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
> >> >>  4 files changed, 69 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> >> index 4fd7858..e74a844 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >> @@ -926,6 +926,11 @@ struct i915_fbc {
> >> >>               struct drm_framebuffer *fb;
> >> >>       } *fbc_work;
> >> >>
> >> >> +     struct intel_fbc_underrun_work {
> >> >> +             struct work_struct work;
> >> >> +             struct intel_crtc *crtc;
> >> >> +     } underrun_work;
> >> >> +
> >> >>       enum no_fbc_reason {
> >> >>               FBC_OK, /* FBC is enabled */
> >> >>               FBC_UNSUPPORTED, /* FBC is not supported by this chipset */
> >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> >> index 81b7d77..246925d 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> >> @@ -1247,6 +1247,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >> >>                    unsigned int frontbuffer_bits, enum fb_op_origin origin);
> >> >>  const char *intel_no_fbc_reason_str(enum no_fbc_reason reason);
> >> >>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> >> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc);
> >> >>
> >> >>  /* intel_hdmi.c */
> >> >>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> >> index a63d10a..28e569c 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> >> @@ -955,6 +955,65 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >> >>       mutex_unlock(&dev_priv->fbc.lock);
> >> >>  }
> >> >>
> >> >> +static void intel_fbc_underrun_work_fn(struct work_struct *__work)
> >> >> +{
> >> >> +     struct intel_fbc_underrun_work *work =
> >> >> +             container_of(__work, struct intel_fbc_underrun_work, work);
> >> >> +     struct intel_crtc *crtc = work->crtc;
> >> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> >> +
> >> >> +     mutex_lock(&dev_priv->fbc.lock);
> >> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> >> >> +             goto out;
> >> >> +
> >> >> +     DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");
> >> >> +     i915.enable_fbc = 0;
> >> >> +     __intel_fbc_disable(dev_priv);
> >> >> +
> >> >> +out:
> >> >> +     work->crtc = NULL;
> >> >> +     mutex_unlock(&dev_priv->fbc.lock);
> >> >> +}
> >> >> +
> >> >> +/**
> >> >> + * intel_fbc_handle_fifo_underrun - handle FIFO underruns if FBC is enabled
> >> >> + * @crtc: the CRTC that caused the underrun
> >> >> + *
> >> >> + * Although we can't know for sure what caused an underrun, one of the possible
> >> >> + * reasons is FBC. And on the FBC case, the user may have a black screen until
> >> >> + * FBC is disabled. So whenever a FIFO underrun happens while FBC is enabled,
> >> >> + * disable FBC just because it may help.
> >> >> + *
> >> >> + * We disable FBC by changing the i915 param, so FBC won't come back on the next
> >> >> + * frame just to cause another underrun. Test suites can force FBC back by
> >> >> + * changing the module parameter again through sysfs.
> >> >> + */
> >> >> +void intel_fbc_handle_fifo_underrun(struct intel_crtc *crtc)
> >> >> +{
> >> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> >> +     struct intel_fbc_underrun_work *work = &dev_priv->fbc.underrun_work;
> >> >> +
> >> >> +     if (!dev_priv->fbc.enable_fbc)
> >> >> +             return;
> >> >> +
> >> >> +     /* These checks are unlocked. We can't grab the lock since we're in the
> >> >> +      * IRQ handler. OTOH, grabbing it wouldn't help much since the underrun
> >> >> +      * interrupt is asynchronous. Still, this a "try to recover because we
> >> >> +      * have already failed" function, so let's at least try, and if we fail,
> >> >> +      * we'll probably be able to try again next frame when another underrun
> >> >> +      * happens. We'll also do the checks again in the work function, where
> >> >> +      * we can grab the lock. */
> >> >> +     if (!intel_fbc_enabled(dev_priv) || dev_priv->fbc.crtc != crtc)
> >> >> +             return;
> >> >> +
> >> >> +     /* Something already scheduled it. */
> >> >> +     if (work->crtc != NULL)
> >> >> +             return;
> >> >> +
> >> >> +     work->crtc = crtc;
> >> >> +     schedule_work(&work->work);
> >> >> +}
> >> >> +
> >> >>  /**
> >> >>   * intel_fbc_init - Initialize FBC
> >> >>   * @dev_priv: the i915 device
> >> >> @@ -966,6 +1025,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> >> >>       enum pipe pipe;
> >> >>
> >> >>       mutex_init(&dev_priv->fbc.lock);
> >> >> +     INIT_WORK(&dev_priv->fbc.underrun_work.work,
> >> >> +               intel_fbc_underrun_work_fn);
> >> >>
> >> >>       if (!HAS_FBC(dev_priv)) {
> >> >>               dev_priv->fbc.enabled = false;
> >> >> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> >> index 54daa66..90e60fb 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> >> >> @@ -356,6 +356,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> >> >>       if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> >> >>               DRM_ERROR("CPU pipe %c FIFO underrun\n",
> >> >>                         pipe_name(pipe));
> >> >> +
> >> >> +     intel_fbc_handle_fifo_underrun(to_intel_crtc(crtc));
> >> >>  }
> >> >>
> >> >>  /**
> >> >> --
> >> >> 2.4.6
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/16] drm/i915: disable FBC on FIFO underruns

[Daniel Vetter]

On Thu, Aug 20, 2015 at 06:00:02PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 20, 2015 at 11:29:29AM -0300, Paulo Zanoni wrote:
> > 2015-08-20 10:58 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > Once it's otherwise known to be solid,
> > > then it might make sense, although a much cooler thing would be if we
> > > could actually detect when the display has failed entirely and recover
> > > it somehow.
> > >
> > > Oh and to make the protection mechanism actually kick in reliably you
> > > would somehow need to address the problems with the underrun interrupts.
> > 
> > Which problems?
> 
> The fact that we disable them as soon as one occurs, and even worse the
> error interrupt is a shared one on many platforms, so one underrun any
> pipe or some other unrelated error kills underrun detection everywhere.

E.g. on hsw with vga + hdmi/dp (which now happens easily since igt creates
a fake vga output, just run kms_pipe_crc_basic) underruns happen reliably
on modeset and then don't work anywhere. We also seem to fail to restore
them somehow :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/16] drm/i915: don't use the first stolen page on Broadwell

[Daniel Vetter]

On Wed, Aug 19, 2015 at 02:55:34PM +0300, Ville Syrjälä wrote:
> On Sat, Aug 15, 2015 at 09:30:18AM +0100, Chris Wilson wrote:
> > On Fri, Aug 14, 2015 at 06:34:18PM -0300, Paulo Zanoni wrote:
> > > The spec says we just can't use it.
> > 
> > But what about when we inherit a framebuffer at that address?
> 
> Indeed. I asked the same question several times during the past attempts
> at this, and even tried to outline a potential solution at least once [1]

Paulo's only patching the insert_node_in_range allocator, for fbcon
takeover we use reserve_node. I think this patch seems to do what you want
it to do, no need for fancier schemes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/16] drm/i915: don't use the first stolen page on Broadwell

[Ville Syrjälä]

On Wed, Aug 26, 2015 at 09:48:51AM +0200, Daniel Vetter wrote:
> On Wed, Aug 19, 2015 at 02:55:34PM +0300, Ville Syrjälä wrote:
> > On Sat, Aug 15, 2015 at 09:30:18AM +0100, Chris Wilson wrote:
> > > On Fri, Aug 14, 2015 at 06:34:18PM -0300, Paulo Zanoni wrote:
> > > > The spec says we just can't use it.
> > > 
> > > But what about when we inherit a framebuffer at that address?
> > 
> > Indeed. I asked the same question several times during the past attempts
> > at this, and even tried to outline a potential solution at least once [1]
> 
> Paulo's only patching the insert_node_in_range allocator, for fbcon
> takeover we use reserve_node. I think this patch seems to do what you want
> it to do, no need for fancier schemes.

Hmm. Indeed. It's a somewhat surprising place to find such a check, but
it does solve the preallocated vs. normal issue. It does mean we may
end up with some corruption in the fbdev bo but I suppose that's short
lived enough that we can ignore it.

It could use a comment as to why we put the check there instead of
totally removing the first page from the stolen mm. Otherwise someone
might decide to clean it up and break things. Also lacking the w/a name
and some explanation why it's needed. And as I mentioned before, CHV
needs to be included. SKL/BXT only have it listed for early steppings,
so I think we can ignore those.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/16] drm/i915: make sure we're not changing the FBC CFB with FBC enabled

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:06PM -0300, Paulo Zanoni wrote:
> We used to have this bug in the past, but now that we properly track
> the size of the CFB, we don't have it anymore. Still, add the WARN
> just to make sure we don't go back to the bad state.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 1f97fb5..c97aba2 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -589,6 +589,8 @@ static int intel_fbc_alloc_cfb(struct drm_i915_private *dev_priv, int size,
>  
>  	dev_priv->fbc.threshold = ret;
>  
> +	WARN_ON(dev_priv->fbc.enabled);
> +

We should really check it already before we free the stolen memory (or
maybe sprinkle it to both places?), and we should make sure FBC really
got disabled, ie. at last one vblank must have occured since the enable
bit was cleared.

Since we lack the vblank workers, I think having this WARN before the
stolen is freed, with a FIXME about the disable vs. vblank issue would
be acceptable.

>  	if (INTEL_INFO(dev_priv)->gen >= 5)
>  		I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->fbc.compressed_fb.start);
>  	else if (IS_GM45(dev_priv)) {
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
> Always update the currrent crtc, fb and vertical offset after calling
> enable_fbc. We were forgetting to do so along the failure paths when
> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
> and update the state simultaneously.
> 
> v2: Improve commit message (Chris).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index c97aba2..fa9b004 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>  	return dev_priv->fbc.enabled;
>  }
>  
> +static void intel_fbc_enable(struct intel_crtc *crtc,
> +			     struct drm_framebuffer *fb)

fb could be const

> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +
> +	dev_priv->fbc.enable_fbc(crtc);
> +
> +	dev_priv->fbc.crtc = crtc;
> +	dev_priv->fbc.fb_id = fb->base.id;
> +	dev_priv->fbc.y = crtc->base.y;
> +}
> +
>  static void intel_fbc_work_fn(struct work_struct *__work)
>  {
>  	struct intel_fbc_work *work =
> @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  		/* Double check that we haven't switched fb without cancelling
>  		 * the prior work.
>  		 */
> -		if (crtc_fb == work->fb) {
> -			dev_priv->fbc.enable_fbc(work->crtc);
> -
> -			dev_priv->fbc.crtc = work->crtc;
> -			dev_priv->fbc.fb_id = crtc_fb->base.id;
> -			dev_priv->fbc.y = work->crtc->base.y;
> -		}
> +		if (crtc_fb == work->fb)
> +			intel_fbc_enable(work->crtc, work->fb);

The no locking or refcounts nature of this scares me, and should be
dealt with eventually.

But in the meantime it makes things nicer, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  		dev_priv->fbc.fbc_work = NULL;
>  	}
> @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
>  	dev_priv->fbc.fbc_work = NULL;
>  }
>  
> -static void intel_fbc_enable(struct intel_crtc *crtc)
> +static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
>  {
>  	struct intel_fbc_work *work;
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc)
>  	work = kzalloc(sizeof(*work), GFP_KERNEL);
>  	if (work == NULL) {
>  		DRM_ERROR("Failed to allocate FBC work structure\n");
> -		dev_priv->fbc.enable_fbc(crtc);
> +		intel_fbc_enable(crtc, crtc->base.primary->fb);
>  		return;
>  	}

BTW getting rid of this allocation would be nice. Would be one less
thing that can fail...

>  
> @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		__intel_fbc_disable(dev_priv);
>  	}
>  
> -	intel_fbc_enable(intel_crtc);
> +	intel_fbc_schedule_enable(intel_crtc);
>  	dev_priv->fbc.no_fbc_reason = FBC_OK;
>  	return;
>  
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/16] drm/i915: fix FBC for cases where crtc->base.y is non-zero

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:08PM -0300, Paulo Zanoni wrote:
> I only tested this on BDW, but since the register description is the
> same ever since gen4, let's assume that all gens take the same
> register format. If that's not true, then hopefully someone will
> bisect a bug to this patch and we'll fix it.
> 
> Notice that the wrong fence offset register just means that the
> hardware tracking will be wrong.
> 
> Testcases:
>  - igt/kms_frontbuffer_tracking/fbc-1p-primscrn-pri-shrfb-draw-mmap-gtt
>  - igt/kms_frontbuffer_tracking/fbc-2p-primscrn-pri-shrfb-draw-mmap-gtt
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index fa9b004..9ffa7dc 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -64,6 +64,20 @@ static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
>  	DRM_DEBUG_KMS("disabled FBC\n");
>  }
>  
> +static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	unsigned int tile_height;
> +
> +	tile_height = intel_tile_height(dev_priv->dev, fb->pixel_format,
> +					fb->modifier[0]);
> +
> +	/* The value we want is the line number of the first tile that contains
> +	 * the first vertical line of the CRTC. */
> +	return crtc->base.y - (crtc->base.y % tile_height);

As mentioned before, I believe this depends on an implementtion detail
of intel_gen4_compute_page_offset(). I think it would be nicer if the
callers of intel_gen4_compute_page_offset() would store the correct
fbc y offset already somewhere under intel_crtc, so that you could
just look it up here. That would safeguard this code from any changes in
intel_gen4_compute_page_offset().

> +}
> +
>  static void i8xx_fbc_enable(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> @@ -97,7 +111,7 @@ static void i8xx_fbc_enable(struct intel_crtc *crtc)
>  		fbc_ctl2 = FBC_CTL_FENCE_DBL | FBC_CTL_IDLE_IMM | FBC_CTL_CPU_FENCE;
>  		fbc_ctl2 |= FBC_CTL_PLANE(crtc->plane);
>  		I915_WRITE(FBC_CONTROL2, fbc_ctl2);
> -		I915_WRITE(FBC_FENCE_OFF, crtc->base.y);
> +		I915_WRITE(FBC_FENCE_OFF, get_crtc_fence_y_offset(crtc));
>  	}
>  
>  	/* enable it... */
> @@ -135,7 +149,7 @@ static void g4x_fbc_enable(struct intel_crtc *crtc)
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
>  	dpfc_ctl |= DPFC_CTL_FENCE_EN | obj->fence_reg;
>  
> -	I915_WRITE(DPFC_FENCE_YOFF, crtc->base.y);
> +	I915_WRITE(DPFC_FENCE_YOFF, get_crtc_fence_y_offset(crtc));
>  
>  	/* enable it... */
>  	I915_WRITE(DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> @@ -177,6 +191,7 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	u32 dpfc_ctl;
>  	int threshold = dev_priv->fbc.threshold;
> +	unsigned int y_offset;
>  
>  	dev_priv->fbc.enabled = true;
>  
> @@ -200,7 +215,8 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
>  	if (IS_GEN5(dev_priv))
>  		dpfc_ctl |= obj->fence_reg;
>  
> -	I915_WRITE(ILK_DPFC_FENCE_YOFF, crtc->base.y);
> +	y_offset = get_crtc_fence_y_offset(crtc);
> +	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
>  	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
>  	/* enable it... */
>  	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> @@ -208,7 +224,7 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
>  	if (IS_GEN6(dev_priv)) {
>  		I915_WRITE(SNB_DPFC_CTL_SA,
>  			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> -		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->base.y);
> +		I915_WRITE(DPFC_CPU_FENCE_OFFSET, y_offset);
>  	}
>  
>  	intel_fbc_nuke(dev_priv);
> @@ -288,7 +304,7 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
>  
>  	I915_WRITE(SNB_DPFC_CTL_SA,
>  		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> -	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->base.y);
> +	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
>  
>  	intel_fbc_nuke(dev_priv);
>  
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/16] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:09PM -0300, Paulo Zanoni wrote:
> The doc is pretty clear that this register should be set to 0 on SNB.
> We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines below.

Bspec says:
"Restriction : The CPU fence is always programmed to match the Display
 Buffer base, so this offset must be programmed to 0 to match. 	DevSNB"

We definitely don't program the fence to match DSPSURF, so it's not very
clear that this is really the right thing to do. I suppose it depends on
how the Y offset in the SA register interacts with this one. I never got
around to fixing the Y offset stuff in my FBC efforts, so I've not tried
it on real hardware and so I have no sure answer here.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9ffa7dc..f7be9ab8 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
>  		dpfc_ctl |= obj->fence_reg;
>  
>  	y_offset = get_crtc_fence_y_offset(crtc);
> -	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
> +
> +	if (IS_GEN5(dev_priv))
> +		I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
> +	else
> +		I915_WRITE(ILK_DPFC_FENCE_YOFF, 0);
> +
>  	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
>  	/* enable it... */
>  	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Paulo Zanoni]

2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
>> Always update the currrent crtc, fb and vertical offset after calling
>> enable_fbc. We were forgetting to do so along the failure paths when
>> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
>> and update the state simultaneously.
>>
>> v2: Improve commit message (Chris).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index c97aba2..fa9b004 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>>       return dev_priv->fbc.enabled;
>>  }
>>
>> +static void intel_fbc_enable(struct intel_crtc *crtc,
>> +                          struct drm_framebuffer *fb)
>
> fb could be const

I guess a lot of things could be constified, if we decide to do this.

>
>> +{
>> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +
>> +     dev_priv->fbc.enable_fbc(crtc);
>> +
>> +     dev_priv->fbc.crtc = crtc;
>> +     dev_priv->fbc.fb_id = fb->base.id;
>> +     dev_priv->fbc.y = crtc->base.y;
>> +}
>> +
>>  static void intel_fbc_work_fn(struct work_struct *__work)
>>  {
>>       struct intel_fbc_work *work =
>> @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>>               /* Double check that we haven't switched fb without cancelling
>>                * the prior work.
>>                */
>> -             if (crtc_fb == work->fb) {
>> -                     dev_priv->fbc.enable_fbc(work->crtc);
>> -
>> -                     dev_priv->fbc.crtc = work->crtc;
>> -                     dev_priv->fbc.fb_id = crtc_fb->base.id;
>> -                     dev_priv->fbc.y = work->crtc->base.y;
>> -             }
>> +             if (crtc_fb == work->fb)
>> +                     intel_fbc_enable(work->crtc, work->fb);
>
> The no locking or refcounts nature of this scares me, and should be
> dealt with eventually.
>
> But in the meantime it makes things nicer, so
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>>
>>               dev_priv->fbc.fbc_work = NULL;
>>       }
>> @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
>>       dev_priv->fbc.fbc_work = NULL;
>>  }
>>
>> -static void intel_fbc_enable(struct intel_crtc *crtc)
>> +static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
>>  {
>>       struct intel_fbc_work *work;
>>       struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc)
>>       work = kzalloc(sizeof(*work), GFP_KERNEL);
>>       if (work == NULL) {
>>               DRM_ERROR("Failed to allocate FBC work structure\n");
>> -             dev_priv->fbc.enable_fbc(crtc);
>> +             intel_fbc_enable(crtc, crtc->base.primary->fb);
>>               return;
>>       }
>
> BTW getting rid of this allocation would be nice. Would be one less
> thing that can fail...

Chris disagrees :)


>
>>
>> @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>>               __intel_fbc_disable(dev_priv);
>>       }
>>
>> -     intel_fbc_enable(intel_crtc);
>> +     intel_fbc_schedule_enable(intel_crtc);
>>       dev_priv->fbc.no_fbc_reason = FBC_OK;
>>       return;
>>
>> --
>> 2.4.6
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/16] drm/i915: check for the supported strides on HSW+ FBC

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:10PM -0300, Paulo Zanoni wrote:
> I could only find the restrictions for HSW+, but I think it's safe to
> assume that the older platforms also can't support the configurations
> HSW can't support. The older platforms probably have additional
> restrictions, so we need to figure out those and implement them later.
> Let's not block HSW+ FBC based on missing information for the older
> platforms.

gen2/3	4K or 8K
gen4	2K - 16K
g4x/ilk	not really documented, I'd just go with gen6+ limits since it's fbc2 already
gen5+	512 - 16K

Which means we don't really need another upper limit besides 16K since
all the older platforms are already limited at that or below for tiled
buffers. But on old platforms we do need to check the lower limits.

> 
> v2:
>   - Just WARN_ON() the strides that should have been caught earlier
>     (Daniel)
>   - Make it a new function since I expect this to grow more.
> v3:
>   - Document which IGT test is exercised by this.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-badstride
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e0f3f05..4fd7858 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -940,6 +940,7 @@ struct i915_fbc {
>  		FBC_CHIP_DEFAULT, /* disabled by default on this chip */
>  		FBC_ROTATION, /* rotation is not supported */
>  		FBC_IN_DBG_MASTER, /* kernel debugger is active */
> +		FBC_BAD_STRIDE, /* stride is not supported */
>  	} no_fbc_reason;
>  
>  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index f7be9ab8..73bd559 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -501,6 +501,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
>  		return "rotation unsupported";
>  	case FBC_IN_DBG_MASTER:
>  		return "Kernel debugger is active";
> +	case FBC_BAD_STRIDE:
> +		return "framebuffer stride not supported";
>  	default:
>  		MISSING_CASE(reason);
>  		return "unknown reason";
> @@ -694,6 +696,22 @@ static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
>  	return intel_fbc_alloc_cfb(dev_priv, size, fb_cpp);
>  }
>  
> +static bool stride_is_valid(unsigned int stride)
> +{
> +	/* TODO: we need to figure out what are the stride restrictions for the
> +	 * older platforms. */
> +
> +	/* These should have been caught earlier. */
> +	WARN_ON(stride < 512);
> +	WARN_ON((stride & (64 - 1)) != 0);

This is rather weird thing to check since we need an X-tiled buffer. I
do see it mentioned in the SNB+ PM guides though.

> +
> +	/* These are additional FBC restrictions. */
> +	if (stride > 16385)

Off by one?

> +		return false;
> +
> +	return true;
> +}
> +
>  /**
>   * __intel_fbc_update - enable/disable FBC as needed, unlocked
>   * @dev_priv: i915 device instance
> @@ -804,6 +822,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> +	if (!stride_is_valid(fb->pitches[0])) {
> +		set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE);
> +		goto out_disable;
> +	}
> +
>  	/* If the kernel debugger is active, always disable compression */
>  	if (in_dbg_master()) {
>  		set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Ville Syrjälä]

On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote:
> 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
> >> Always update the currrent crtc, fb and vertical offset after calling
> >> enable_fbc. We were forgetting to do so along the failure paths when
> >> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
> >> and update the state simultaneously.
> >>
> >> v2: Improve commit message (Chris).
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
> >>  1 file changed, 17 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index c97aba2..fa9b004 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> >>       return dev_priv->fbc.enabled;
> >>  }
> >>
> >> +static void intel_fbc_enable(struct intel_crtc *crtc,
> >> +                          struct drm_framebuffer *fb)
> >
> > fb could be const
> 
> I guess a lot of things could be constified, if we decide to do this.
> 
> >
> >> +{
> >> +     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> +
> >> +     dev_priv->fbc.enable_fbc(crtc);
> >> +
> >> +     dev_priv->fbc.crtc = crtc;
> >> +     dev_priv->fbc.fb_id = fb->base.id;
> >> +     dev_priv->fbc.y = crtc->base.y;
> >> +}
> >> +
> >>  static void intel_fbc_work_fn(struct work_struct *__work)
> >>  {
> >>       struct intel_fbc_work *work =
> >> @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> >>               /* Double check that we haven't switched fb without cancelling
> >>                * the prior work.
> >>                */
> >> -             if (crtc_fb == work->fb) {
> >> -                     dev_priv->fbc.enable_fbc(work->crtc);
> >> -
> >> -                     dev_priv->fbc.crtc = work->crtc;
> >> -                     dev_priv->fbc.fb_id = crtc_fb->base.id;
> >> -                     dev_priv->fbc.y = work->crtc->base.y;
> >> -             }
> >> +             if (crtc_fb == work->fb)
> >> +                     intel_fbc_enable(work->crtc, work->fb);
> >
> > The no locking or refcounts nature of this scares me, and should be
> > dealt with eventually.
> >
> > But in the meantime it makes things nicer, so
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >>
> >>               dev_priv->fbc.fbc_work = NULL;
> >>       }
> >> @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
> >>       dev_priv->fbc.fbc_work = NULL;
> >>  }
> >>
> >> -static void intel_fbc_enable(struct intel_crtc *crtc)
> >> +static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
> >>  {
> >>       struct intel_fbc_work *work;
> >>       struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc)
> >>       work = kzalloc(sizeof(*work), GFP_KERNEL);
> >>       if (work == NULL) {
> >>               DRM_ERROR("Failed to allocate FBC work structure\n");
> >> -             dev_priv->fbc.enable_fbc(crtc);
> >> +             intel_fbc_enable(crtc, crtc->base.primary->fb);
> >>               return;
> >>       }
> >
> > BTW getting rid of this allocation would be nice. Would be one less
> > thing that can fail...
> 
> Chris disagrees :)

He's wrong ;)

> 
> 
> >
> >>
> >> @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
> >>               __intel_fbc_disable(dev_priv);
> >>       }
> >>
> >> -     intel_fbc_enable(intel_crtc);
> >> +     intel_fbc_schedule_enable(intel_crtc);
> >>       dev_priv->fbc.no_fbc_reason = FBC_OK;
> >>       return;
> >>
> >> --
> >> 2.4.6
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/16] drm/i915: try a little harder to find an FBC CRTC

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:11PM -0300, Paulo Zanoni wrote:
> Keep searching in case the candidate has a NULL primary fb. This is
> only relevant for the platforms that don't have the pipe_a_only
> restriction.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 73bd559..a63d10a 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -532,16 +532,14 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
>  		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>  
>  		if (intel_crtc_active(tmp_crtc) &&
> -		    to_intel_plane_state(tmp_crtc->primary->state)->visible)
> +		    to_intel_plane_state(tmp_crtc->primary->state)->visible &&
> +		    tmp_crtc->primary->fb != NULL)

I'd hoped we'd gotten past these plane is supposedly visible but doesn't
have an fb bugs. Does this actually happen in real life still?

>  			crtc = tmp_crtc;
>  
>  		if (pipe_a_only)
>  			break;
>  	}
>  
> -	if (!crtc || crtc->primary->fb == NULL)
> -		return NULL;
> -
>  	return crtc;
>  }
>  
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/16] drm/i915: print the correct amount of bytes allocated for the CFB

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:14PM -0300, Paulo Zanoni wrote:
> And also print the threshold. I was surprised to see a log message
> claiming the CFB size was 32mb when there was less than 24mb available
> for it.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9fd7b74..dc84e67 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -655,8 +655,9 @@ static int intel_fbc_alloc_cfb(struct drm_i915_private *dev_priv, int size,
>  
>  	dev_priv->fbc.uncompressed_size = size;
>  
> -	DRM_DEBUG_KMS("reserved %d bytes of contiguous stolen space for FBC\n",
> -		      size);
> +	DRM_DEBUG_KMS("reserved %llu bytes of contiguous stolen space for FBC, threshold: %d\n",
> +		      dev_priv->fbc.compressed_fb.size,
> +		      dev_priv->fbc.threshold);

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	return 0;
>  
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/16] drm/i915: fix CFB size calculation

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:15PM -0300, Paulo Zanoni wrote:
> We were considering the whole framebuffer height, but the spec clearly
> says that we should only consider the active display height size.
> 
> On my current testing machine, this moves us from 124 successes and
> 502 skips to 209 successes and 417 skips on "kms_frontbuffer_tracking
> --fbc-only". The high amount of skips is due to the --fbc-only
> argument. We had those skips due to not enough stolen memory for the
> tests. We're now passing the maximum possible amount: 209.
> 
> Note: when this patch was written, the amount of tests we had for FBC
> was different than what we have now.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc-*
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index dc84e67..cfd4cba 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -695,9 +695,15 @@ void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->fbc.lock);
>  }
>  
> -static int intel_fbc_setup_cfb(struct drm_i915_private *dev_priv, int size,
> -			       int fb_cpp)
> +static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
>  {
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	int size, fb_cpp;
> +
> +	size = crtc->config->pipe_src_h * fb->pitches[0];
> +	fb_cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> +

From the looks of it SKL ought to be able to use FBC even with a
non-fullscreen plane, so this should look up the size from the plane
state. What I don't know is whether FBC would still work when plane scaling
is enabled. But I suppose it if can, it would deal with the data before
scaling, so using the clipped src size would be the right thing here I think.

>  	if (size <= dev_priv->fbc.uncompressed_size)
>  		return 0;
>  
> @@ -844,8 +850,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	if (intel_fbc_setup_cfb(dev_priv, obj->base.size,
> -				drm_format_plane_cpp(fb->pixel_format, 0))) {
> +	if (intel_fbc_setup_cfb(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
>  		goto out_disable;
>  	}
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/16] drm/i915/bdw: don't enable FBC when pixel rate exceeds 95%

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:16PM -0300, Paulo Zanoni wrote:
> BSpec says we shouldn't enable FBC on BDW when the pipe pixel rate
> exceeds 95% of the core display clock.

w/a db says HSW has the same issue, apparently it was found on HSW
originally. Actually it says BDW G0 would have it fixed. Bspec says
G1 in the w/a section. Not sure what to believe here. Better safe than
sorry probably and just do the w/a for BDW.

Also this has a name 'WaFbcExceedCdClockThreshold'

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 1 +
>  drivers/gpu/drm/i915/intel_fbc.c | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f44d101..f8f3ed3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -946,6 +946,7 @@ struct i915_fbc {
>  		FBC_ROTATION, /* rotation is not supported */
>  		FBC_IN_DBG_MASTER, /* kernel debugger is active */
>  		FBC_BAD_STRIDE, /* stride is not supported */
> +		FBC_PIXEL_RATE, /* pixel rate is too big */
>  	} no_fbc_reason;
>  
>  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index cfd4cba..9dee0b5 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -503,6 +503,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
>  		return "Kernel debugger is active";
>  	case FBC_BAD_STRIDE:
>  		return "framebuffer stride not supported";
> +	case FBC_PIXEL_RATE:
> +		return "pixel rate is too big";
>  	default:
>  		MISSING_CASE(reason);
>  		return "unknown reason";
> @@ -850,6 +852,12 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> +	if (IS_BROADWELL(dev_priv) && ilk_pipe_pixel_rate(intel_crtc->config) >=
> +	    dev_priv->max_cdclk_freq * 95 / 100) {

That should be looking at the current cdclk, not the max. We should
probablt change our cdclk computations to include some headroom for
FBC/IPS when there aren't other factors that would preclude their use.

> +		set_no_fbc_reason(dev_priv, FBC_PIXEL_RATE);
> +		goto out_disable;
> +	}
> +
>  	if (intel_fbc_setup_cfb(intel_crtc)) {
>  		set_no_fbc_reason(dev_priv, FBC_STOLEN_TOO_SMALL);
>  		goto out_disable;
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/16] drm/i915: apply WaFbcAsynchFlipDisableFbcQueue earlier

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:17PM -0300, Paulo Zanoni wrote:
> The spec says the register should have that value for the entire time
> that FBC is enabled, so apply the WA before we enable FBC.
> 
> Notice that we also have this WA for ILK/SNB, but it is implemented at
> init_clock_gating(). I could move the IVB/HSW/BDW WA code to
> init_clock_gating() too, but since we recently had some complaints
> about WAs not staying after being set, I'm going to play safe and keep
> this here for now.

Yeah I think we tried to optimize things by applying the w/a only when
absolutely needed, on the theory that it might save some power when FBC
is disabled. But I have no idea if that's true or not. Oh and we don't
actually undo the w/a after disalbing FBC which pretty mucch defeats the
purpose of the optimizaton.

Would be better to do it the same way for all platforms I think. Either
in init_clock_gating, or we should do and undo the w/a around FBC
enable/disable.

In the meantime this patch seems fine, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9dee0b5..b76c19f 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -293,8 +293,6 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
>  	if (dev_priv->fbc.false_color)
>  		dpfc_ctl |= FBC_CTL_FALSE_COLOR;
>  
> -	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> -
>  	if (IS_IVYBRIDGE(dev_priv)) {
>  		/* WaFbcAsynchFlipDisableFbcQueue:ivb */
>  		I915_WRITE(ILK_DISPLAY_CHICKEN1,
> @@ -307,6 +305,8 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
>  			   HSW_FBCQ_DIS);
>  	}
>  
> +	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> +
>  	I915_WRITE(SNB_DPFC_CTL_SA,
>  		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>  	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/16] drm/i915: don't apply WaFbcAsynchFlipDisableFbcQueue on SKL

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:19PM -0300, Paulo Zanoni wrote:
> This WA is only for HSW/BDW.

w/a db says it was needed for early SKL, but we can ignore it. w/a db
still has it for BXT. I guess we can hope BXT would be fixed too if SKL
is.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index b76c19f..5dfe460 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -298,7 +298,7 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
>  		I915_WRITE(ILK_DISPLAY_CHICKEN1,
>  			   I915_READ(ILK_DISPLAY_CHICKEN1) |
>  			   ILK_FBCQ_DIS);
> -	} else {
> +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		/* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */
>  		I915_WRITE(CHICKEN_PIPESL_1(crtc->pipe),
>  			   I915_READ(CHICKEN_PIPESL_1(crtc->pipe)) |
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 16/16] drm/i915: reject invalid formats for FBC

[Ville Syrjälä]

On Fri, Aug 14, 2015 at 06:34:21PM -0300, Paulo Zanoni wrote:
> This commit is essentially a rewrite of "drm/i915: Check pixel format
> for fbc" from Ville Syrjälä. The idea is the same, but the code is
> different due to all the changes that happened since his original
> patch. So any bugs are due to my bad rewrite.
> 
> Testcases: igt/kms_frontbuffer_tracking/*fbc*-${format_name}-draw-*
> Credits-to: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f8f3ed3..938f69f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -947,6 +947,7 @@ struct i915_fbc {
>  		FBC_IN_DBG_MASTER, /* kernel debugger is active */
>  		FBC_BAD_STRIDE, /* stride is not supported */
>  		FBC_PIXEL_RATE, /* pixel rate is too big */
> +		FBC_PIXEL_FORMAT /* pixel format is invalid */
>  	} no_fbc_reason;
>  
>  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 5dfe460..cd3ed90 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -505,6 +505,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
>  		return "framebuffer stride not supported";
>  	case FBC_PIXEL_RATE:
>  		return "pixel rate is too big";
> +	case FBC_PIXEL_FORMAT:
> +		return "pixel format is invalid";
>  	default:
>  		MISSING_CASE(reason);
>  		return "unknown reason";
> @@ -731,6 +733,34 @@ static bool stride_is_valid(unsigned int stride)
>  	return true;
>  }
>  
> +static bool pixel_format_is_valid(struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = fb->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* Primary planes don't support alpha, so the "A" formats and "X"
> +	 * formats are one and the same. */
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		return true;
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +	case DRM_FORMAT_RGB565:

I think we've since concluded that ARGB format implies alpha blending,
and removed the ARGB formats for most platforms from the list of
supported formats. However SKL includes ARGB in it's list of formats,
and I think it enables alpha blending when encountering it. Since FBC is
not compatible with alpha blending we should drop the alpha formats from
this list.

> +		/* 16bpp not supported on gen2 */
> +		if (IS_GEN2(dev))
> +			return false;
> +		/* WaFbcOnly1to1Ratio:ctg */
> +		if (IS_G4X(dev_priv))
> +			return false;
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /**
>   * __intel_fbc_update - enable/disable FBC as needed, unlocked
>   * @dev_priv: i915 device instance
> @@ -846,6 +876,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> +	if (!pixel_format_is_valid(fb)) {
> +		set_no_fbc_reason(dev_priv, FBC_PIXEL_FORMAT);
> +		goto out_disable;
> +	}
> +
>  	/* If the kernel debugger is active, always disable compression */
>  	if (in_dbg_master()) {
>  		set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Daniel Vetter]

On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote:
> 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
> >> Always update the currrent crtc, fb and vertical offset after calling
> >> enable_fbc. We were forgetting to do so along the failure paths when
> >> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
> >> and update the state simultaneously.
> >>
> >> v2: Improve commit message (Chris).
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
> >>  1 file changed, 17 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index c97aba2..fa9b004 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> >>       return dev_priv->fbc.enabled;
> >>  }
> >>
> >> +static void intel_fbc_enable(struct intel_crtc *crtc,
> >> +                          struct drm_framebuffer *fb)
> >
> > fb could be const
> 
> I guess a lot of things could be constified, if we decide to do this.

Personally I like const on .data (especially vfunc tables since those are
nice to create exploits if they're writable and you can get at them). And
for core functions/vfuncs where the const has a semantic meaning.
Otherwise I personally don't see to much value in sprinkling const all
over.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Ville Syrjälä]

On Tue, Sep 01, 2015 at 12:07:01PM +0200, Daniel Vetter wrote:
> On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote:
> > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
> > >> Always update the currrent crtc, fb and vertical offset after calling
> > >> enable_fbc. We were forgetting to do so along the failure paths when
> > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
> > >> and update the state simultaneously.
> > >>
> > >> v2: Improve commit message (Chris).
> > >>
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
> > >>  1 file changed, 17 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > >> index c97aba2..fa9b004 100644
> > >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> > >>       return dev_priv->fbc.enabled;
> > >>  }
> > >>
> > >> +static void intel_fbc_enable(struct intel_crtc *crtc,
> > >> +                          struct drm_framebuffer *fb)
> > >
> > > fb could be const
> > 
> > I guess a lot of things could be constified, if we decide to do this.
> 
> Personally I like const on .data (especially vfunc tables since those are
> nice to create exploits if they're writable and you can get at them). And
> for core functions/vfuncs where the const has a semantic meaning.
> Otherwise I personally don't see to much value in sprinkling const all
> over.

I especially like making display mode, state, etc. structs const to make
it clear which functions can change them and which can't. IMO
drm_framebuffer could use the same treatment.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/16] drm/i915: fix the FBC work allocation failure path

[Daniel Vetter]

On Tue, Sep 01, 2015 at 02:03:34PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 01, 2015 at 12:07:01PM +0200, Daniel Vetter wrote:
> > On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote:
> > > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote:
> > > >> Always update the currrent crtc, fb and vertical offset after calling
> > > >> enable_fbc. We were forgetting to do so along the failure paths when
> > > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc()
> > > >> and update the state simultaneously.
> > > >>
> > > >> v2: Improve commit message (Chris).
> > > >>
> > > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >> ---
> > > >>  drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++----------
> > > >>  1 file changed, 17 insertions(+), 10 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > >> index c97aba2..fa9b004 100644
> > > >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> > > >>       return dev_priv->fbc.enabled;
> > > >>  }
> > > >>
> > > >> +static void intel_fbc_enable(struct intel_crtc *crtc,
> > > >> +                          struct drm_framebuffer *fb)
> > > >
> > > > fb could be const
> > > 
> > > I guess a lot of things could be constified, if we decide to do this.
> > 
> > Personally I like const on .data (especially vfunc tables since those are
> > nice to create exploits if they're writable and you can get at them). And
> > for core functions/vfuncs where the const has a semantic meaning.
> > Otherwise I personally don't see to much value in sprinkling const all
> > over.
> 
> I especially like making display mode, state, etc. structs const to make
> it clear which functions can change them and which can't. IMO
> drm_framebuffer could use the same treatment.

Yeah I guess making a few things in the drm core static could be useful
indeed, for example plane_state->fb reall should be static, or
crtc_state->mode_blob (or any other fb or blob pointer in state
structures). Same for all the {plane, connector, crtc}->state pointers
(although that would need some casts in swap_states()). I'm not too sure
how much benefit there is if we do this just in i915, since if the
top-level entry points called from drm core aren't enforcing constness
trying to do it in i915 only will probably be an endless effort of fixing
things up all the time.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/16] drm/i915: avoid the last 8mb of stolen on BDW/SKL

[Paulo Zanoni]

2015-08-19 5:24 GMT-03:00 chris@chris-wilson.co.uk <chris@chris-wilson.co.uk>:
> On Tue, Aug 18, 2015 at 09:49:34PM +0000, Zanoni, Paulo R wrote:
>> Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu:
>> > On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote:
>> > > The FBC hardware for these platforms doesn't have access to the
>> > > bios_reserved range, so it always assumes the maximum (8mb) is
>> > > used.
>> > > So avoid this range while allocating.
>> > >
>> > > This solves a bunch of FIFO underruns that happen if you end up
>> > > putting the CFB in that memory range. On my machine, with 32mb of
>> > > stolen, I need a 2560x1440 mode for that.
>> >
>> > If the restriction applies only to FBC, apply it to only fbc.
>>
>> That's what the patch is doing. The part where we set the unusual
>> start/end is at intel_fbc.c:find_compression_threshold(). Everything
>> else should be behaving just as before.
>>
>> Maybe you're being confused by the addition of the stolen_usable_size
>> variable.
>
> Hmm, I expected to see the constaint being passed into the search
> routine.a It looked like you were adjusting the stolen_size probably the
> root of my confusion.
>
> Anyway we have a quandary. You want to reserve stolen space, and I want
> to make sure as much of it gets used as possible (especially for long
> lived objects).
>
> What I have in mind is adding a priority to the search and allow us to
> evict anything of lower priority. We can use a page replacement
> algorithm even for the pinned fbcon (since only the GGTT itself is
> pinned and we can swap the pages underneath). This of course requires a
> callback for low priority evictable objects. I have 3 priorities in mind
> plus the evictable flag: USER, KERNEL, POWER (with FBC taking the
> highest priority of POWER). That will allow us to do the BIOS takeover
> and only if we run out of space force the copy.

While I understand your idea, I just want to clarify one thing: it
does not apply to _this_ patch, right? I suppose we're discussing
about "[PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen
memory" here. The memory avoided by this patch (08/16) can still be
used by the other stolen memory users.

For the fbcon patch, can't we adopt a simpler "if FBC is enabled by
default on this platform, don't alloc fbcon from stolen"?.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/16] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Zanoni, Paulo R]

Em Sex, 2015-08-28 às 17:46 +0300, Ville Syrjälä escreveu:
> On Fri, Aug 14, 2015 at 06:34:09PM -0300, Paulo Zanoni wrote:
> > The doc is pretty clear that this register should be set to 0 on
> > SNB.
> > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines
> > below.
> 
> Bspec says:
> "Restriction : The CPU fence is always programmed to match the
> Display
>  Buffer base, so this offset must be programmed to 0 to match. 	
> DevSNB"
> 
> We definitely don't program the fence to match DSPSURF, so it's not
> very
> clear that this is really the right thing to do. I suppose it depends
> on
> how the Y offset in the SA register interacts with this one. I never
> got
> around to fixing the Y offset stuff in my FBC efforts, so I've not
> tried
> it on real hardware and so I have no sure answer here.

The BSpec page for DPFC Control on SNB says:

8<---------------------
Project :DEVSNB
iMPH will only send the host modify message when modifications are in
the fence selected in the DPFC_CONTROL_SA register CPUFNCNUM field. The
fence field in the FBC Host Modification message will always be 0 and
this field must be programmed to 0 to match.
8<---------------------

(and DPFC_CONTROL_SA is register 0x100100)

> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 9ffa7dc..f7be9ab8 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc
> > *crtc)
> >  		dpfc_ctl |= obj->fence_reg;
> >  
> >  	y_offset = get_crtc_fence_y_offset(crtc);
> > -	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
> > +
> > +	if (IS_GEN5(dev_priv))
> > +		I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
> > +	else
> > +		I915_WRITE(ILK_DPFC_FENCE_YOFF, 0);
> > +
> >  	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj)
> > | ILK_FBC_RT_VALID);
> >  	/* enable it... */
> >  	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> > -- 
> > 2.4.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/16] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 09:26:19PM +0000, Zanoni, Paulo R wrote:
> Em Sex, 2015-08-28 às 17:46 +0300, Ville Syrjälä escreveu:
> > On Fri, Aug 14, 2015 at 06:34:09PM -0300, Paulo Zanoni wrote:
> > > The doc is pretty clear that this register should be set to 0 on
> > > SNB.
> > > We already write y_offset to DPFC_CPU_FENCE_OFFSET a few lines
> > > below.
> > 
> > Bspec says:
> > "Restriction : The CPU fence is always programmed to match the
> > Display
> >  Buffer base, so this offset must be programmed to 0 to match. 	
> > DevSNB"
> > 
> > We definitely don't program the fence to match DSPSURF, so it's not
> > very
> > clear that this is really the right thing to do. I suppose it depends
> > on
> > how the Y offset in the SA register interacts with this one. I never
> > got
> > around to fixing the Y offset stuff in my FBC efforts, so I've not
> > tried
> > it on real hardware and so I have no sure answer here.
> 
> The BSpec page for DPFC Control on SNB says:
> 
> 8<---------------------
> Project :DEVSNB
> iMPH will only send the host modify message when modifications are in
> the fence selected in the DPFC_CONTROL_SA register CPUFNCNUM field. The
> fence field in the FBC Host Modification message will always be 0 and
> this field must be programmed to 0 to match.
> 8<---------------------

That's about the fence number, it doesn't say anything about the offset
IIRC.

I guess it could be tested by, say:
- set the SA fence offset to some high number and leave this one low
- do the opposite
and in each case see if GTT tracking still works.

And to make sure were testin the right thing, probably repeat with
both offsets set high and make sure the tracking really does not work.

> 
> (and DPFC_CONTROL_SA is register 0x100100)
> 
> > 
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 9ffa7dc..f7be9ab8 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -216,7 +216,12 @@ static void ilk_fbc_enable(struct intel_crtc
> > > *crtc)
> > >  		dpfc_ctl |= obj->fence_reg;
> > >  
> > >  	y_offset = get_crtc_fence_y_offset(crtc);
> > > -	I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
> > > +
> > > +	if (IS_GEN5(dev_priv))
> > > +		I915_WRITE(ILK_DPFC_FENCE_YOFF, y_offset);
> > > +	else
> > > +		I915_WRITE(ILK_DPFC_FENCE_YOFF, 0);
> > > +
> > >  	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj)
> > > | ILK_FBC_RT_VALID);
> > >  	/* enable it... */
> > >  	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> > > -- 
> > > 2.4.6
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx