[PATCH 00/13] Yet another FBC series, v3 part 1

[PATCH 00/13] Yet another FBC series, v3 part 1

[Paulo Zanoni]

Hi

So Ville pointed a problem on patch 02/26 of the previous series, and the nice
fix for that would make me rebase most of the subsequent patches. In order to
avoid blocking the other patches on the review of patch 02 and in order to avoid
having to rebase everything again and again during this I decided to change the
order of the patches on the series and try to get a smaller chunk of patches
merged before moving on to the others.

These first 13 patches are somewhat trivial and are mostly just moving code
around, with only one or two simple bug fixes. If you wonder why some of these
changes are needed, please go check the complete series (the short answer is:
most of the changes are needed for the new model with enable/disable +
activate/deactivate).

After all these patches are merged I'll resend the rest.

Thanks for all the reviews so far,
Paulo

Paulo Zanoni (13):
  drm/i915: rename intel_fbc_nuke to intel_fbc_recompress
  drm/i915: extract fbc_on_pipe_a_only()
  drm/i915: remove unnecessary check for crtc->primary->fb
  drm/i915: extract crtc_is_valid() on the FBC code
  drm/i915: use struct intel_crtc *crtc at __intel_fbc_update()
  drm/i915: fix the __intel_fbc_update() comments
  drm/i915: don't disable_fbc() if FBC is already disabled
  drm/i915: refactor FBC deactivation at init
  drm/i915: remove too-frequent FBC debug message
  drm/i915: fix the CFB size check
  drm/i915: clarify that checking the FB stride for CFB is intentional
  drm/i915: remove in_dbg_master check from intel_fbc.c
  drm/i915: remove newline from a no_fbc_reason message

 drivers/gpu/drm/i915/intel_display.c |   3 -
 drivers/gpu/drm/i915/intel_fbc.c     | 123 ++++++++++++++++-------------------
 2 files changed, 57 insertions(+), 69 deletions(-)

-- 
2.6.1

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

[PATCH 01/13] drm/i915: rename intel_fbc_nuke to intel_fbc_recompress

[Paulo Zanoni]

Although the term "nuke" is part of the FBC spec, it's not very
intuitive, so let's rename it to make it easier for people that are
not familiar with the spec.

Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index c245116..19a3e93 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -182,7 +182,8 @@ static bool g4x_fbc_enabled(struct drm_i915_private *dev_priv)
 	return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
-static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
+/* This funcion forces a CFB recompression through the nuke operation. */
+static void intel_fbc_recompress(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE(MSG_FBC_REND_STATE, FBC_REND_NUKE);
 	POSTING_READ(MSG_FBC_REND_STATE);
@@ -231,7 +232,7 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, y_offset);
 	}
 
-	intel_fbc_nuke(dev_priv);
+	intel_fbc_recompress(dev_priv);
 
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
 }
@@ -310,7 +311,7 @@ static void gen7_fbc_enable(struct intel_crtc *crtc)
 		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 	I915_WRITE(DPFC_CPU_FENCE_OFFSET, get_crtc_fence_y_offset(crtc));
 
-	intel_fbc_nuke(dev_priv);
+	intel_fbc_recompress(dev_priv);
 
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
 }
-- 
2.6.1

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

[PATCH 02/13] drm/i915: extract fbc_on_pipe_a_only()

[Paulo Zanoni]

Make the code easier to read.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 19a3e93..55bef12 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -46,6 +46,11 @@ static inline bool fbc_supported(struct drm_i915_private *dev_priv)
 	return dev_priv->fbc.enable_fbc != NULL;
 }
 
+static inline bool fbc_on_pipe_a_only(struct drm_i915_private *dev_priv)
+{
+	return IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8;
+}
+
 /*
  * In some platforms where the CRTC's x:0/y:0 coordinates doesn't match the
  * frontbuffer's x:0/y:0 coordinates we lie to the hardware about the plane's
@@ -486,10 +491,6 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 {
 	struct drm_crtc *crtc = NULL, *tmp_crtc;
 	enum pipe pipe;
-	bool pipe_a_only = false;
-
-	if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
-		pipe_a_only = true;
 
 	for_each_pipe(dev_priv, pipe) {
 		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
@@ -498,7 +499,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 		    to_intel_plane_state(tmp_crtc->primary->state)->visible)
 			crtc = tmp_crtc;
 
-		if (pipe_a_only)
+		if (fbc_on_pipe_a_only(dev_priv))
 			break;
 	}
 
@@ -1057,7 +1058,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 		dev_priv->fbc.possible_framebuffer_bits |=
 				INTEL_FRONTBUFFER_PRIMARY(pipe);
 
-		if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
+		if (fbc_on_pipe_a_only(dev_priv))
 			break;
 	}
 
-- 
2.6.1

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

[PATCH 03/13] drm/i915: remove unnecessary check for crtc->primary->fb

[Paulo Zanoni]

We already check if the CRTC is visible, and it shouldn't be possible
to have a visible CRTC without an FB.

This was noticed by both Chris and Ville on different ocasions.

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 55bef12..687dc65 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -503,7 +503,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 			break;
 	}
 
-	if (!crtc || crtc->primary->fb == NULL)
+	if (!crtc)
 		return NULL;
 
 	return crtc;
-- 
2.6.1

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

[PATCH 04/13] drm/i915: extract crtc_is_valid() on the FBC code

[Paulo Zanoni]

We're going to kill intel_fbc_find_crtc(), that's why a big part of
the logic moved from intel_fbc_find_crtc() to crtc_is_valid().

v2:
  - Rebase due to pipe_a_only change.
  - Split the multiline conditional (Chris).

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 687dc65..52db73c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -487,6 +487,22 @@ static void set_no_fbc_reason(struct drm_i915_private *dev_priv,
 	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
 }
 
+static bool crtc_is_valid(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+
+	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
+		return false;
+
+	if (!intel_crtc_active(&crtc->base))
+		return false;
+
+	if (!to_intel_plane_state(crtc->base.primary->state)->visible)
+		return false;
+
+	return true;
+}
+
 static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 {
 	struct drm_crtc *crtc = NULL, *tmp_crtc;
@@ -495,12 +511,8 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 	for_each_pipe(dev_priv, pipe) {
 		tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
-		if (intel_crtc_active(tmp_crtc) &&
-		    to_intel_plane_state(tmp_crtc->primary->state)->visible)
+		if (crtc_is_valid(to_intel_crtc(tmp_crtc)))
 			crtc = tmp_crtc;
-
-		if (fbc_on_pipe_a_only(dev_priv))
-			break;
 	}
 
 	if (!crtc)
-- 
2.6.1

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

[PATCH 05/13] drm/i915: use struct intel_crtc *crtc at __intel_fbc_update()

[Paulo Zanoni]

This change was part of the commit that makes intel_fbc_update()
receive an intel_crtc as argument instead of dev_priv, but since it
was polluting the diff with too many chunks I decided to move it to
its own commit.

It seems that our developers are favoring having this instead of the
old combination drm_crtc *crtc + intel_crtc *intel_crtc, and on the
mentioned commit we'll get rid of the drm_crtc variable, so let's do
an intermediate commit with the rename, so on the next commit we'll
have just struct intel_crtc *crtc.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 52db73c..7721122 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -823,8 +823,8 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
  */
 static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 {
-	struct drm_crtc *crtc = NULL;
-	struct intel_crtc *intel_crtc;
+	struct drm_crtc *drm_crtc = NULL;
+	struct intel_crtc *crtc;
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	const struct drm_display_mode *adjusted_mode;
@@ -854,8 +854,8 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 	 *   - new fb is too large to fit in compressed buffer
 	 *   - going to an unsupported config (interlace, pixel multiply, etc.)
 	 */
-	crtc = intel_fbc_find_crtc(dev_priv);
-	if (!crtc) {
+	drm_crtc = intel_fbc_find_crtc(dev_priv);
+	if (!drm_crtc) {
 		set_no_fbc_reason(dev_priv, "no output");
 		goto out_disable;
 	}
@@ -865,10 +865,10 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	intel_crtc = to_intel_crtc(crtc);
-	fb = crtc->primary->fb;
+	crtc = to_intel_crtc(drm_crtc);
+	fb = crtc->base.primary->fb;
 	obj = intel_fb_obj(fb);
-	adjusted_mode = &intel_crtc->config->base.adjusted_mode;
+	adjusted_mode = &crtc->config->base.adjusted_mode;
 
 	if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
 	    (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
@@ -876,13 +876,13 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	if (!intel_fbc_hw_tracking_covers_screen(intel_crtc)) {
+	if (!intel_fbc_hw_tracking_covers_screen(crtc)) {
 		set_no_fbc_reason(dev_priv, "mode too large for compression");
 		goto out_disable;
 	}
 
 	if ((INTEL_INFO(dev_priv)->gen < 4 || HAS_DDI(dev_priv)) &&
-	    intel_crtc->plane != PLANE_A) {
+	    crtc->plane != PLANE_A) {
 		set_no_fbc_reason(dev_priv, "FBC unsupported on plane");
 		goto out_disable;
 	}
@@ -896,7 +896,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
-	    crtc->primary->state->rotation != BIT(DRM_ROTATE_0)) {
+	    crtc->base.primary->state->rotation != BIT(DRM_ROTATE_0)) {
 		set_no_fbc_reason(dev_priv, "rotation unsupported");
 		goto out_disable;
 	}
@@ -919,13 +919,13 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 
 	/* WaFbcExceedCdClockThreshold:hsw,bdw */
 	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
-	    ilk_pipe_pixel_rate(intel_crtc->config) >=
+	    ilk_pipe_pixel_rate(crtc->config) >=
 	    dev_priv->cdclk_freq * 95 / 100) {
 		set_no_fbc_reason(dev_priv, "pixel rate is too big");
 		goto out_disable;
 	}
 
-	if (intel_fbc_setup_cfb(intel_crtc)) {
+	if (intel_fbc_setup_cfb(crtc)) {
 		set_no_fbc_reason(dev_priv, "not enough stolen memory");
 		goto out_disable;
 	}
@@ -935,9 +935,9 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 	 * cannot be unpinned (and have its GTT offset and fence revoked)
 	 * without first being decoupled from the scanout and FBC disabled.
 	 */
-	if (dev_priv->fbc.crtc == intel_crtc &&
+	if (dev_priv->fbc.crtc == crtc &&
 	    dev_priv->fbc.fb_id == fb->base.id &&
-	    dev_priv->fbc.y == crtc->y)
+	    dev_priv->fbc.y == crtc->base.y)
 		return;
 
 	if (intel_fbc_enabled(dev_priv)) {
@@ -968,7 +968,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		__intel_fbc_disable(dev_priv);
 	}
 
-	intel_fbc_schedule_enable(intel_crtc);
+	intel_fbc_schedule_enable(crtc);
 	dev_priv->fbc.no_fbc_reason = "FBC enabled (not necessarily active)\n";
 	return;
 
-- 
2.6.1

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

[PATCH 06/13] drm/i915: fix the __intel_fbc_update() comments

[Paulo Zanoni]

Don't try to list in comments the cases where we should enable or
disable FBC: it varies a lot with the hardware generations and the
code should be the documentation. Also notice that there's already a
huge gap between the comments and what's in the code.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 7721122..dfb8657 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -806,20 +806,8 @@ static bool intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
  * __intel_fbc_update - enable/disable FBC as needed, unlocked
  * @dev_priv: i915 device instance
  *
- * Set up the framebuffer compression hardware at mode set time.  We
- * enable it if possible:
- *   - plane A only (on pre-965)
- *   - no pixel mulitply/line duplication
- *   - no alpha buffer discard
- *   - no dual wide
- *   - framebuffer <= max_hdisplay in width, max_vdisplay in height
- *
- * We can't assume that any compression will take place (worst case),
- * so the compressed buffer has to be the same size as the uncompressed
- * one.  It also must reside (along with the line length buffer) in
- * stolen memory.
- *
- * We need to enable/disable FBC on a global basis.
+ * This function completely reevaluates the status of FBC, then enables,
+ * disables or maintains it on the same state.
  */
 static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 {
@@ -831,7 +819,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
-	/* disable framebuffer compression in vGPU */
 	if (intel_vgpu_active(dev_priv->dev))
 		i915.enable_fbc = 0;
 
@@ -845,15 +832,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	/*
-	 * If FBC is already on, we just have to verify that we can
-	 * keep it that way...
-	 * Need to disable if:
-	 *   - more than one pipe is active
-	 *   - changing FBC params (stride, fence, mode)
-	 *   - new fb is too large to fit in compressed buffer
-	 *   - going to an unsupported config (interlace, pixel multiply, etc.)
-	 */
 	drm_crtc = intel_fbc_find_crtc(dev_priv);
 	if (!drm_crtc) {
 		set_no_fbc_reason(dev_priv, "no output");
-- 
2.6.1

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

[PATCH 07/13] drm/i915: don't disable_fbc() if FBC is already disabled

[Paulo Zanoni]

If FBC is disabled we will still call intel_fbc_invalidate(), and as a
result we may call intel_fbc_deactivate(), which will try to touch
registers.

I'm pretty sure I saw this happen on a runtime suspended device, and
I'm almost sure I was running igt/pm_rpm. It produced the "you touched
registers while the device is suspended" WARNs. But this was some time
ago and I can't remember exactly which conditions were necessary to
reproduce the problem.

v2: Rebase to new series order.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index dfb8657..5a853c6 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -438,7 +438,8 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
 
 	intel_fbc_cancel_work(dev_priv);
 
-	dev_priv->fbc.disable_fbc(dev_priv);
+	if (dev_priv->fbc.enabled)
+		dev_priv->fbc.disable_fbc(dev_priv);
 	dev_priv->fbc.crtc = NULL;
 }
 
-- 
2.6.1

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

[PATCH 08/13] drm/i915: refactor FBC deactivation at init

[Paulo Zanoni]

Make sure we deactivate FBC at intel_fbc_init(), so we can remove the
call from intel_display.c. Currently we only have the "enabled"
software state, but later we'll have both "enabled" and "active", and
we'll add assertions to them, so just calling intel_fbc_disable() from
intel_modeset_init() won't work. It's better to make sure
intel_fbc_init() already puts the hardware in the expected state, so
we can put nice assertions in the other functions.

v2: Keep/improve the comment (Chris).
v3: Improve the commit message a little bit.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b3cfb6..5a7f012 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15030,9 +15030,6 @@ void intel_modeset_init(struct drm_device *dev)
 	i915_disable_vga(dev);
 	intel_setup_outputs(dev);
 
-	/* Just in case the BIOS is doing something questionable. */
-	intel_fbc_disable(dev_priv);
-
 	drm_modeset_lock_all(dev);
 	intel_modeset_setup_hw_state(dev);
 	drm_modeset_unlock_all(dev);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 5a853c6..8abf70e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1038,9 +1038,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	enum pipe pipe;
 
 	mutex_init(&dev_priv->fbc.lock);
+	dev_priv->fbc.enabled = false;
 
 	if (!HAS_FBC(dev_priv)) {
-		dev_priv->fbc.enabled = false;
 		dev_priv->fbc.no_fbc_reason = "unsupported by this chipset";
 		return;
 	}
@@ -1074,5 +1074,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
 	}
 
-	dev_priv->fbc.enabled = dev_priv->fbc.fbc_enabled(dev_priv);
+	/* We still don't have any sort of hardware state readout for FBC, so
+	 * disable it in case the BIOS enabled it to make sure software matches
+	 * the hardware state. */
+	if (dev_priv->fbc.fbc_enabled(dev_priv))
+		dev_priv->fbc.disable_fbc(dev_priv);
 }
-- 
2.6.1

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

[PATCH 09/13] drm/i915: remove too-frequent FBC debug message

[Paulo Zanoni]

If we run igt/kms_frontbuffer_tracking, this message will appear
thousands of times, eating a significant part of our dmesg buffer.
It's part of the expected FBC behavior, so let's just silence it.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 8abf70e..dee99c9 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -376,8 +376,6 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
 	if (dev_priv->fbc.fbc_work == NULL)
 		return;
 
-	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
-
 	/* Synchronisation is provided by struct_mutex and checking of
 	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
 	 * entirely asynchronously.
-- 
2.6.1

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

[PATCH 10/13] drm/i915: fix the CFB size check

[Paulo Zanoni]

In function find_compression_threshold() we try to over-allocate CFB
space in order to reudce reallocations and fragmentation, and we're
not considering that at the CFB size check. Consider it.

There is also a longer-term plan to kill
dev_priv->fbc.uncompressed_size, but this will come later.

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

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index dee99c9..e99aacc 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
 	size = intel_fbc_calculate_cfb_size(crtc);
 	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 
-	if (size <= dev_priv->fbc.uncompressed_size)
+	if (dev_priv->fbc.compressed_fb.allocated &&
+	    size <= dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold)
 		return 0;
 
 	/* Release any current block */
-- 
2.6.1

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

[PATCH 11/13] drm/i915: clarify that checking the FB stride for CFB is intentional

[Paulo Zanoni]

Daniel was looking at this code and asked about whether fb->pitches[0]
is correct, then he suggested we should a comment to make sure it is
actually intentional.

For more information on the CFB size calculation, please see the
commit message of:

commit c4ffd40908c30a33291227920e921f6b45b9e8f7
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Thu Oct 1 19:55:57 2015 -0300
    drm/i915: fix CFB size calculation

Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index e99aacc..8e806be 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -707,6 +707,7 @@ static int intel_fbc_calculate_cfb_size(struct intel_crtc *crtc)
 	if (INTEL_INFO(dev_priv)->gen >= 7)
 		lines = min(lines, 2048);
 
+	/* Hardware needs the full buffer stride, not just the active area. */
 	return lines * fb->pitches[0];
 }
 
-- 
2.6.1

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

[PATCH 12/13] drm/i915: remove in_dbg_master check from intel_fbc.c

[Paulo Zanoni]

From our maintainer Daniel Vetter a few days ago:
  "Oh dear this is dead code. kdbg uses the fbcon, which always uses
  untiled, which means fbc will never be enabled. Also we have 0 users
  and 0 test coverage for kdbg on top of i915 (Jesse implemented it
  for fun years back). Imo just remove all this code."

Adding to what Daniel said: for kgdboc's KMS support,
intel_pipe_set_base_atomic() already manually disables FBC, so we
won't do the in_dbg_master() check there. This is essentially a revert
of:

commit c924b934d0cd14a4559611da91f28f59acebe32a
Author: Jason Wessel <jason.wessel@windriver.com>
Date:   Thu Aug 5 09:22:32 2010 -0500
    i915: when kgdb is active display compression should be off

Besides, it is not clear what is the exact problem caused by FBC, and
why other features such as PSR, DRRS, IPS and RPM are not also
checking for in_dbg_master(). IMHO we should either remove the code as
suggested by Daniel or we add some nice comments explaining why is FBC
so special.

v2: Rebase due to new patch order.

Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 8e806be..e496cb0 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -890,12 +890,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
-	/* If the kernel debugger is active, always disable compression */
-	if (in_dbg_master()) {
-		set_no_fbc_reason(dev_priv, "Kernel debugger is active");
-		goto out_disable;
-	}
-
 	/* WaFbcExceedCdClockThreshold:hsw,bdw */
 	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
 	    ilk_pipe_pixel_rate(crtc->config) >=
-- 
2.6.1

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

[PATCH 13/13] drm/i915: remove newline from a no_fbc_reason message

[Paulo Zanoni]

Newlines are not needed and they're not used by the other messages. I
added the newline by mistake.

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 e496cb0..4275375 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -942,7 +942,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 	}
 
 	intel_fbc_schedule_enable(crtc);
-	dev_priv->fbc.no_fbc_reason = "FBC enabled (not necessarily active)\n";
+	dev_priv->fbc.no_fbc_reason = "FBC enabled (not necessarily active)";
 	return;
 
 out_disable:
-- 
2.6.1

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

Re: [PATCH 12/13] drm/i915: remove in_dbg_master check from intel_fbc.c

[Jesse Barnes]

On 11/04/2015 11:10 AM, Paulo Zanoni wrote:
> From our maintainer Daniel Vetter a few days ago:
>   "Oh dear this is dead code. kdbg uses the fbcon, which always uses
>   untiled, which means fbc will never be enabled. Also we have 0 users
>   and 0 test coverage for kdbg on top of i915 (Jesse implemented it
>   for fun years back). Imo just remove all this code."
> 
> Adding to what Daniel said: for kgdboc's KMS support,
> intel_pipe_set_base_atomic() already manually disables FBC, so we
> won't do the in_dbg_master() check there. This is essentially a revert
> of:
> 
> commit c924b934d0cd14a4559611da91f28f59acebe32a
> Author: Jason Wessel <jason.wessel@windriver.com>
> Date:   Thu Aug 5 09:22:32 2010 -0500
>     i915: when kgdb is active display compression should be off
> 
> Besides, it is not clear what is the exact problem caused by FBC, and
> why other features such as PSR, DRRS, IPS and RPM are not also
> checking for in_dbg_master(). IMHO we should either remove the code as
> suggested by Daniel or we add some nice comments explaining why is FBC
> so special.
> 
> v2: Rebase due to new patch order.
> 
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 8e806be..e496cb0 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> -	/* If the kernel debugger is active, always disable compression */
> -	if (in_dbg_master()) {
> -		set_no_fbc_reason(dev_priv, "Kernel debugger is active");
> -		goto out_disable;
> -	}
> -
>  	/* WaFbcExceedCdClockThreshold:hsw,bdw */
>  	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
>  	    ilk_pipe_pixel_rate(crtc->config) >=
> 

Yeah looks fine.  I haven't had any bug reports from the kdboc work, so
I guess that means no one is using it. :)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/13] drm/i915: remove in_dbg_master check from intel_fbc.c

[Jason Wessel]

On 11/04/2015 02:13 PM, Jesse Barnes wrote:
> On 11/04/2015 11:10 AM, Paulo Zanoni wrote:
>>  From our maintainer Daniel Vetter a few days ago:
>>    "Oh dear this is dead code. kdbg uses the fbcon, which always uses
>>    untiled, which means fbc will never be enabled. Also we have 0 users
>>    and 0 test coverage for kdbg on top of i915 (Jesse implemented it
>>    for fun years back). Imo just remove all this code."
>>
>> Adding to what Daniel said: for kgdboc's KMS support,
>> intel_pipe_set_base_atomic() already manually disables FBC, so we
>> won't do the in_dbg_master() check there. This is essentially a revert
>> of:
>>
>> commit c924b934d0cd14a4559611da91f28f59acebe32a
>> Author: Jason Wessel <jason.wessel@windriver.com>
>> Date:   Thu Aug 5 09:22:32 2010 -0500
>>      i915: when kgdb is active display compression should be off
>>
>> Besides, it is not clear what is the exact problem caused by FBC, and
>> why other features such as PSR, DRRS, IPS and RPM are not also
>> checking for in_dbg_master(). IMHO we should either remove the code as
>> suggested by Daniel or we add some nice comments explaining why is FBC
>> so special.
>>
>> v2: Rebase due to new patch order.
>>
>> Cc: Jason Wessel <jason.wessel@windriver.com>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_fbc.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 8e806be..e496cb0 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>>   		goto out_disable;
>>   	}
>>   
>> -	/* If the kernel debugger is active, always disable compression */
>> -	if (in_dbg_master()) {
>> -		set_no_fbc_reason(dev_priv, "Kernel debugger is active");
>> -		goto out_disable;
>> -	}
>> -
>>   	/* WaFbcExceedCdClockThreshold:hsw,bdw */
>>   	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
>>   	    ilk_pipe_pixel_rate(crtc->config) >=
>>
> Yeah looks fine.  I haven't had any bug reports from the kdboc work, so
> I guess that means no one is using it. :)
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>


It was previously the case that the code here only got hit when you had a oops or a panic while running with the graphics console up. We would end up with fuzzy lines instead of a readable text console when we activated the atomic mode set.

The kgdboc probably doesn't get used a whole lot but I have debugged a few strange WiFi and PM problems with it on a laptop and it worked well.  :-)

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

Re: [PATCH 12/13] drm/i915: remove in_dbg_master check from intel_fbc.c

[Zanoni, Paulo R]

Em Qua, 2015-11-04 às 14:19 -0600, Jason Wessel escreveu:
> On 11/04/2015 02:13 PM, Jesse Barnes wrote:
> > On 11/04/2015 11:10 AM, Paulo Zanoni wrote:
> > >  From our maintainer Daniel Vetter a few days ago:
> > >    "Oh dear this is dead code. kdbg uses the fbcon, which always
> > > uses
> > >    untiled, which means fbc will never be enabled. Also we have 0
> > > users
> > >    and 0 test coverage for kdbg on top of i915 (Jesse implemented
> > > it
> > >    for fun years back). Imo just remove all this code."
> > > 
> > > Adding to what Daniel said: for kgdboc's KMS support,
> > > intel_pipe_set_base_atomic() already manually disables FBC, so we
> > > won't do the in_dbg_master() check there. This is essentially a
> > > revert
> > > of:
> > > 
> > > commit c924b934d0cd14a4559611da91f28f59acebe32a
> > > Author: Jason Wessel <jason.wessel@windriver.com>
> > > Date:   Thu Aug 5 09:22:32 2010 -0500
> > >      i915: when kgdb is active display compression should be off
> > > 
> > > Besides, it is not clear what is the exact problem caused by FBC,
> > > and
> > > why other features such as PSR, DRRS, IPS and RPM are not also
> > > checking for in_dbg_master(). IMHO we should either remove the
> > > code as
> > > suggested by Daniel or we add some nice comments explaining why
> > > is FBC
> > > so special.
> > > 
> > > v2: Rebase due to new patch order.
> > > 
> > > Cc: Jason Wessel <jason.wessel@windriver.com>
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_fbc.c | 6 ------
> > >   1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 8e806be..e496cb0 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct
> > > drm_i915_private *dev_priv)
> > >   		goto out_disable;
> > >   	}
> > >   
> > > -	/* If the kernel debugger is active, always disable
> > > compression */
> > > -	if (in_dbg_master()) {
> > > -		set_no_fbc_reason(dev_priv, "Kernel debugger is
> > > active");
> > > -		goto out_disable;
> > > -	}
> > > -
> > >   	/* WaFbcExceedCdClockThreshold:hsw,bdw */
> > >   	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
> > >   	    ilk_pipe_pixel_rate(crtc->config) >=
> > > 
> > Yeah looks fine.  I haven't had any bug reports from the kdboc
> > work, so
> > I guess that means no one is using it. :)
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> 
> It was previously the case that the code here only got hit when you
> had a oops or a panic while running with the graphics console up. We
> would end up with fuzzy lines instead of a readable text console when
> we activated the atomic mode set.

But on this case we'll call pipe_set_base_atomic(), which will disable
FBC before doing the modeset. Maybe this was not the case in the
past..?

> 
> The kgdboc probably doesn't get used a whole lot but I have debugged
> a few strange WiFi and PM problems with it on a laptop and it worked
> well.  :-)


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

Re: [PATCH 12/13] drm/i915: remove in_dbg_master check from intel_fbc.c

[Jesse Barnes]

On 11/04/2015 12:26 PM, Zanoni, Paulo R wrote:
> Em Qua, 2015-11-04 às 14:19 -0600, Jason Wessel escreveu:
>> On 11/04/2015 02:13 PM, Jesse Barnes wrote:
>>> On 11/04/2015 11:10 AM, Paulo Zanoni wrote:
>>>>  From our maintainer Daniel Vetter a few days ago:
>>>>    "Oh dear this is dead code. kdbg uses the fbcon, which always
>>>> uses
>>>>    untiled, which means fbc will never be enabled. Also we have 0
>>>> users
>>>>    and 0 test coverage for kdbg on top of i915 (Jesse implemented
>>>> it
>>>>    for fun years back). Imo just remove all this code."
>>>>
>>>> Adding to what Daniel said: for kgdboc's KMS support,
>>>> intel_pipe_set_base_atomic() already manually disables FBC, so we
>>>> won't do the in_dbg_master() check there. This is essentially a
>>>> revert
>>>> of:
>>>>
>>>> commit c924b934d0cd14a4559611da91f28f59acebe32a
>>>> Author: Jason Wessel <jason.wessel@windriver.com>
>>>> Date:   Thu Aug 5 09:22:32 2010 -0500
>>>>      i915: when kgdb is active display compression should be off
>>>>
>>>> Besides, it is not clear what is the exact problem caused by FBC,
>>>> and
>>>> why other features such as PSR, DRRS, IPS and RPM are not also
>>>> checking for in_dbg_master(). IMHO we should either remove the
>>>> code as
>>>> suggested by Daniel or we add some nice comments explaining why
>>>> is FBC
>>>> so special.
>>>>
>>>> v2: Rebase due to new patch order.
>>>>
>>>> Cc: Jason Wessel <jason.wessel@windriver.com>
>>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_fbc.c | 6 ------
>>>>   1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
>>>> b/drivers/gpu/drm/i915/intel_fbc.c
>>>> index 8e806be..e496cb0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>>>> @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct
>>>> drm_i915_private *dev_priv)
>>>>   		goto out_disable;
>>>>   	}
>>>>   
>>>> -	/* If the kernel debugger is active, always disable
>>>> compression */
>>>> -	if (in_dbg_master()) {
>>>> -		set_no_fbc_reason(dev_priv, "Kernel debugger is
>>>> active");
>>>> -		goto out_disable;
>>>> -	}
>>>> -
>>>>   	/* WaFbcExceedCdClockThreshold:hsw,bdw */
>>>>   	if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) &&
>>>>   	    ilk_pipe_pixel_rate(crtc->config) >=
>>>>
>>> Yeah looks fine.  I haven't had any bug reports from the kdboc
>>> work, so
>>> I guess that means no one is using it. :)
>>>
>>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>>
>> It was previously the case that the code here only got hit when you
>> had a oops or a panic while running with the graphics console up. We
>> would end up with fuzzy lines instead of a readable text console when
>> we activated the atomic mode set.
> 
> But on this case we'll call pipe_set_base_atomic(), which will disable
> FBC before doing the modeset. Maybe this was not the case in the
> past..?

Or some subsequent call re-enabled it somehow.  You could replace it
with a WARN and keep my r-b if you want, or just push as-is.

But my guess is that kgdboc hasn't survived all the other modeset work
we've done the past couple of years...  I guess we need some igt tests
and a bunch of fixups to keep it going.  We could address this problem
if/when that happens.

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

Re: [PATCH 07/13] drm/i915: don't disable_fbc() if FBC is already disabled

[Ville Syrjälä]

On Wed, Nov 04, 2015 at 05:10:51PM -0200, Paulo Zanoni wrote:
> If FBC is disabled we will still call intel_fbc_invalidate(), and as a
> result we may call intel_fbc_deactivate(), which will try to touch
> registers.
> 
> I'm pretty sure I saw this happen on a runtime suspended device, and

This is one of the BAT failures we have. I don't really understand why
we track the frontbuffer bits with FBC totally disabled, but this fix
seems simple enough on its own. Not sure if it applies without the
others, but if it doesn't I suggest trying to reorder the patchset so
we can put this in ASAP.

> I'm almost sure I was running igt/pm_rpm. It produced the "you touched
> registers while the device is suspended" WARNs. But this was some time
> ago and I can't remember exactly which conditions were necessary to
> reproduce the problem.
> 
> v2: Rebase to new series order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index dfb8657..5a853c6 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -438,7 +438,8 @@ static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
>  
>  	intel_fbc_cancel_work(dev_priv);
>  
> -	dev_priv->fbc.disable_fbc(dev_priv);
> +	if (dev_priv->fbc.enabled)
> +		dev_priv->fbc.disable_fbc(dev_priv);
>  	dev_priv->fbc.crtc = NULL;
>  }
>  
> -- 
> 2.6.1
> 
> _______________________________________________
> 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/13] drm/i915: don't disable_fbc() if FBC is already disabled

[Maarten Lankhorst]

Hey,

Op 05-11-15 om 12:29 schreef Ville Syrjälä:
> On Wed, Nov 04, 2015 at 05:10:51PM -0200, Paulo Zanoni wrote:
>> If FBC is disabled we will still call intel_fbc_invalidate(), and as a
>> result we may call intel_fbc_deactivate(), which will try to touch
>> registers.
>>
>> I'm pretty sure I saw this happen on a runtime suspended device, and
> This is one of the BAT failures we have. I don't really understand why
> we track the frontbuffer bits with FBC totally disabled, but this fix
> seems simple enough on its own. Not sure if it applies without the
> others, but if it doesn't I suggest trying to reorder the patchset so
> we can put this in ASAP.
>
For the commit message what BAT is affected?

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

Re: [PATCH 07/13] drm/i915: don't disable_fbc() if FBC is already disabled

[Ville Syrjälä]

On Thu, Nov 05, 2015 at 03:52:31PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 05-11-15 om 12:29 schreef Ville Syrjälä:
> > On Wed, Nov 04, 2015 at 05:10:51PM -0200, Paulo Zanoni wrote:
> >> If FBC is disabled we will still call intel_fbc_invalidate(), and as a
> >> result we may call intel_fbc_deactivate(), which will try to touch
> >> registers.
> >>
> >> I'm pretty sure I saw this happen on a runtime suspended device, and
> > This is one of the BAT failures we have. I don't really understand why
> > we track the frontbuffer bits with FBC totally disabled, but this fix
> > seems simple enough on its own. Not sure if it applies without the
> > others, but if it doesn't I suggest trying to reorder the patchset so
> > we can put this in ASAP.
> >
> For the commit message what BAT is affected?

/me looks

pm_rpm@basic-rte at least.

-- 
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/13] drm/i915: fix the CFB size check

[Maarten Lankhorst]

Op 04-11-15 om 20:10 schreef Paulo Zanoni:
> In function find_compression_threshold() we try to over-allocate CFB
> space in order to reudce reallocations and fragmentation, and we're
> not considering that at the CFB size check. Consider it.
>
> There is also a longer-term plan to kill
> dev_priv->fbc.uncompressed_size, but this will come later.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index dee99c9..e99aacc 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct intel_crtc *crtc)
>  	size = intel_fbc_calculate_cfb_size(crtc);
>  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>  
> -	if (size <= dev_priv->fbc.uncompressed_size)
> +	if (dev_priv->fbc.compressed_fb.allocated &&
> +	    size <= dev_priv->fbc.compressed_fb.size * dev_priv->fbc.threshold)
>  		return 0;
>  
>  	/* Release any current block */
Should i8xx_fbc_enable be changed too then?

Rest of the patches look ok, applied those.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/13] drm/i915: fix the CFB size check

[Zanoni, Paulo R]

Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
> Op 04-11-15 om 20:10 schreef Paulo Zanoni:
> > In function find_compression_threshold() we try to over-allocate
> > CFB
> > space in order to reudce reallocations and fragmentation, and we're
> > not considering that at the CFB size check. Consider it.
> > 
> > There is also a longer-term plan to kill
> > dev_priv->fbc.uncompressed_size, but this will come later.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index dee99c9..e99aacc 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
> > intel_crtc *crtc)
> >  	size = intel_fbc_calculate_cfb_size(crtc);
> >  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  
> > -	if (size <= dev_priv->fbc.uncompressed_size)
> > +	if (dev_priv->fbc.compressed_fb.allocated &&
> > +	    size <= dev_priv->fbc.compressed_fb.size * dev_priv-
> > >fbc.threshold)
> >  		return 0;
> >  
> >  	/* Release any current block */
> Should i8xx_fbc_enable be changed too then?

As far as I understand, no. We're just reserving a bigger buffer in
case we need it later, but the size used by the hardware is still the
same. But I'm not 100% sure the i8xx code is actually correct since I
didn't dig deep into the ancient scrolls. By not touching i8xx we're
also avoiding a possible new regression.

> 
> Rest of the patches look ok, applied those.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/13] drm/i915: fix the CFB size check

[Maarten Lankhorst]

Op 10-11-15 om 13:20 schreef Zanoni, Paulo R:
> Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
>> Op 04-11-15 om 20:10 schreef Paulo Zanoni:
>>> In function find_compression_threshold() we try to over-allocate
>>> CFB
>>> space in order to reudce reallocations and fragmentation, and we're
>>> not considering that at the CFB size check. Consider it.
>>>
>>> There is also a longer-term plan to kill
>>> dev_priv->fbc.uncompressed_size, but this will come later.
>>>
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
>>> b/drivers/gpu/drm/i915/intel_fbc.c
>>> index dee99c9..e99aacc 100644
>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>>> @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
>>> intel_crtc *crtc)
>>>  	size = intel_fbc_calculate_cfb_size(crtc);
>>>  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>>>  
>>> -	if (size <= dev_priv->fbc.uncompressed_size)
>>> +	if (dev_priv->fbc.compressed_fb.allocated &&
>>> +	    size <= dev_priv->fbc.compressed_fb.size * dev_priv-
>>>> fbc.threshold)
>>>  		return 0;
>>>  
>>>  	/* Release any current block */
>> Should i8xx_fbc_enable be changed too then?
> As far as I understand, no. We're just reserving a bigger buffer in
> case we need it later, but the size used by the hardware is still the
> same. But I'm not 100% sure the i8xx code is actually correct since I
> didn't dig deep into the ancient scrolls. By not touching i8xx we're
> also avoiding a possible new regression.
>
The original check was for size <= uncompressed_size,
new is threshold * compressed.

I think i8xx_fbc_enable might have to do the same when calculating cfb_pitch
for this patch to work as intended.

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

Re: [PATCH 10/13] drm/i915: fix the CFB size check

[Zanoni, Paulo R]

Em Ter, 2015-11-10 às 14:04 +0100, Maarten Lankhorst escreveu:
> Op 10-11-15 om 13:20 schreef Zanoni, Paulo R:
> > Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
> > > Op 04-11-15 om 20:10 schreef Paulo Zanoni:
> > > > In function find_compression_threshold() we try to over-
> > > > allocate
> > > > CFB
> > > > space in order to reudce reallocations and fragmentation, and
> > > > we're
> > > > not considering that at the CFB size check. Consider it.
> > > > 
> > > > There is also a longer-term plan to kill
> > > > dev_priv->fbc.uncompressed_size, but this will come later.
> > > > 
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > index dee99c9..e99aacc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
> > > > intel_crtc *crtc)
> > > >  	size = intel_fbc_calculate_cfb_size(crtc);
> > > >  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > >  
> > > > -	if (size <= dev_priv->fbc.uncompressed_size)
> > > > +	if (dev_priv->fbc.compressed_fb.allocated &&
> > > > +	    size <= dev_priv->fbc.compressed_fb.size *
> > > > dev_priv-
> > > > > fbc.threshold)
> > > >  		return 0;
> > > >  
> > > >  	/* Release any current block */
> > > Should i8xx_fbc_enable be changed too then?
> > As far as I understand, no. We're just reserving a bigger buffer in
> > case we need it later, but the size used by the hardware is still
> > the
> > same. But I'm not 100% sure the i8xx code is actually correct since
> > I
> > didn't dig deep into the ancient scrolls. By not touching i8xx
> > we're
> > also avoiding a possible new regression.
> > 
> The original check was for size <= uncompressed_size,
> new is threshold * compressed.

But threshold is always 1 for i8xx, so the difference of
uncompressed_size vs compressed_fb.size is really just the
overallocation, and the hardware doesn't even need to know we're
overallocating.

> 
> I think i8xx_fbc_enable might have to do the same when calculating
> cfb_pitch
> for this patch to work as intended.

Why?

Please notice I'm trying to keep the i8xx behavior the same instead of
risking introducing a new bug I can't test.

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

Re: [PATCH 10/13] drm/i915: fix the CFB size check

[Maarten Lankhorst]

Op 11-11-15 om 14:39 schreef Zanoni, Paulo R:
> Em Ter, 2015-11-10 às 14:04 +0100, Maarten Lankhorst escreveu:
>> Op 10-11-15 om 13:20 schreef Zanoni, Paulo R:
>>> Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
>>>> Op 04-11-15 om 20:10 schreef Paulo Zanoni:
>>>>> In function find_compression_threshold() we try to over-
>>>>> allocate
>>>>> CFB
>>>>> space in order to reudce reallocations and fragmentation, and
>>>>> we're
>>>>> not considering that at the CFB size check. Consider it.
>>>>>
>>>>> There is also a longer-term plan to kill
>>>>> dev_priv->fbc.uncompressed_size, but this will come later.
>>>>>
>>>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
>>>>> b/drivers/gpu/drm/i915/intel_fbc.c
>>>>> index dee99c9..e99aacc 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>>>>> @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
>>>>> intel_crtc *crtc)
>>>>>  	size = intel_fbc_calculate_cfb_size(crtc);
>>>>>  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>>>>>  
>>>>> -	if (size <= dev_priv->fbc.uncompressed_size)
>>>>> +	if (dev_priv->fbc.compressed_fb.allocated &&
>>>>> +	    size <= dev_priv->fbc.compressed_fb.size *
>>>>> dev_priv-
>>>>>> fbc.threshold)
>>>>>  		return 0;
>>>>>  
>>>>>  	/* Release any current block */
>>>> Should i8xx_fbc_enable be changed too then?
>>> As far as I understand, no. We're just reserving a bigger buffer in
>>> case we need it later, but the size used by the hardware is still
>>> the
>>> same. But I'm not 100% sure the i8xx code is actually correct since
>>> I
>>> didn't dig deep into the ancient scrolls. By not touching i8xx
>>> we're
>>> also avoiding a possible new regression.
>>>
>> The original check was for size <= uncompressed_size,
>> new is threshold * compressed.
> But threshold is always 1 for i8xx, so the difference of
> uncompressed_size vs compressed_fb.size is really just the
> overallocation, and the hardware doesn't even need to know we're
> overallocating.
Ah ok, wasn't aware of threshold being 1. :) In that case I guess nothing changes so patch should be safe to apply.

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

Re: [PATCH 10/13] drm/i915: fix the CFB size check

[Ville Syrjälä]

On Tue, Nov 10, 2015 at 02:04:48PM +0100, Maarten Lankhorst wrote:
> Op 10-11-15 om 13:20 schreef Zanoni, Paulo R:
> > Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
> >> Op 04-11-15 om 20:10 schreef Paulo Zanoni:
> >>> In function find_compression_threshold() we try to over-allocate
> >>> CFB
> >>> space in order to reudce reallocations and fragmentation, and we're
> >>> not considering that at the CFB size check. Consider it.
> >>>
> >>> There is also a longer-term plan to kill
> >>> dev_priv->fbc.uncompressed_size, but this will come later.
> >>>
> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> >>> b/drivers/gpu/drm/i915/intel_fbc.c
> >>> index dee99c9..e99aacc 100644
> >>> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >>> @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
> >>> intel_crtc *crtc)
> >>>  	size = intel_fbc_calculate_cfb_size(crtc);
> >>>  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >>>  
> >>> -	if (size <= dev_priv->fbc.uncompressed_size)
> >>> +	if (dev_priv->fbc.compressed_fb.allocated &&
> >>> +	    size <= dev_priv->fbc.compressed_fb.size * dev_priv-
> >>>> fbc.threshold)
> >>>  		return 0;
> >>>  
> >>>  	/* Release any current block */
> >> Should i8xx_fbc_enable be changed too then?
> > As far as I understand, no. We're just reserving a bigger buffer in
> > case we need it later, but the size used by the hardware is still the
> > same. But I'm not 100% sure the i8xx code is actually correct since I
> > didn't dig deep into the ancient scrolls. By not touching i8xx we're
> > also avoiding a possible new regression.
> >
> The original check was for size <= uncompressed_size,
> new is threshold * compressed.
> 
> I think i8xx_fbc_enable might have to do the same when calculating cfb_pitch
> for this patch to work as intended.

I think the FBC1 code is fairly confused already.

It does the right thing by capping CFB_PITCH to the fb pitch. The way
the hardware works is that any line that compresses to >CFB_PITCH is
discarded and marked as uncompressed. So we definitely don't want to 
allow CFB_PITCH to exceed the original pitch as that would just increase
the amount of data getting scanned out. The nasty thing is that it'll
try to recompress any line tagged as uncompressed every time it tries
to recompress anything, so we should be rather careful not to set
CFB_PITCH too low for FBC1, and if we have poorly compressing data we
might just want to disable FBC entirely to avoid trying to recompress
everything all the time.

What doesn't really make sense to me is the
'cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE' part. 
That basically assumes we always have a maximum height plane (1536
lines) in use. I'm not entirely sure if the hardware fully skips the
unused lines, or if we would have to allocate some pixel run sets
(swords) even for the unused lines (maybe 1 per line). But I wouldn't
think that we would need to allocate them to cover the entire worst
case length.

What makes things more confusing I think is the naming of the variables
and functions. uncompressed_size is actually the size we've allocated
for the cfb, and intel_fbc_calculate_cfb_size() sort of returns the
uncompressed fb size. Well since it has the 2k line limit applied, I
suppose you can actually think of it as the max cfb size we would want
to have for the particular plane configuration.

-- 
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/13] drm/i915: fix the CFB size check

[Zanoni, Paulo R]

Em Qua, 2015-11-11 às 16:27 +0200, Ville Syrjälä escreveu:
> On Tue, Nov 10, 2015 at 02:04:48PM +0100, Maarten Lankhorst wrote:
> > Op 10-11-15 om 13:20 schreef Zanoni, Paulo R:
> > > Em Ter, 2015-11-10 às 11:22 +0100, Maarten Lankhorst escreveu:
> > > > Op 04-11-15 om 20:10 schreef Paulo Zanoni:
> > > > > In function find_compression_threshold() we try to over-
> > > > > allocate
> > > > > CFB
> > > > > space in order to reudce reallocations and fragmentation, and
> > > > > we're
> > > > > not considering that at the CFB size check. Consider it.
> > > > > 
> > > > > There is also a longer-term plan to kill
> > > > > dev_priv->fbc.uncompressed_size, but this will come later.
> > > > > 
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > index dee99c9..e99aacc 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > @@ -719,7 +719,8 @@ static int intel_fbc_setup_cfb(struct
> > > > > intel_crtc *crtc)
> > > > >  	size = intel_fbc_calculate_cfb_size(crtc);
> > > > >  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> > > > >  
> > > > > -	if (size <= dev_priv->fbc.uncompressed_size)
> > > > > +	if (dev_priv->fbc.compressed_fb.allocated &&
> > > > > +	    size <= dev_priv->fbc.compressed_fb.size *
> > > > > dev_priv-
> > > > > > fbc.threshold)
> > > > >  		return 0;
> > > > >  
> > > > >  	/* Release any current block */
> > > > Should i8xx_fbc_enable be changed too then?
> > > As far as I understand, no. We're just reserving a bigger buffer
> > > in
> > > case we need it later, but the size used by the hardware is still
> > > the
> > > same. But I'm not 100% sure the i8xx code is actually correct
> > > since I
> > > didn't dig deep into the ancient scrolls. By not touching i8xx
> > > we're
> > > also avoiding a possible new regression.
> > > 
> > The original check was for size <= uncompressed_size,
> > new is threshold * compressed.
> > 
> > I think i8xx_fbc_enable might have to do the same when calculating
> > cfb_pitch
> > for this patch to work as intended.
> 
> I think the FBC1 code is fairly confused already.
> 
> It does the right thing by capping CFB_PITCH to the fb pitch. The way
> the hardware works is that any line that compresses to >CFB_PITCH is
> discarded and marked as uncompressed. So we definitely don't want to 
> allow CFB_PITCH to exceed the original pitch as that would just
> increase
> the amount of data getting scanned out. The nasty thing is that it'll
> try to recompress any line tagged as uncompressed every time it tries
> to recompress anything, so we should be rather careful not to set
> CFB_PITCH too low for FBC1, and if we have poorly compressing data we
> might just want to disable FBC entirely to avoid trying to recompress
> everything all the time.
> 
> What doesn't really make sense to me is the
> 'cfb_pitch = dev_priv->fbc.uncompressed_size / FBC_LL_SIZE' part. 
> That basically assumes we always have a maximum height plane (1536
> lines) in use. I'm not entirely sure if the hardware fully skips the
> unused lines, or if we would have to allocate some pixel run sets
> (swords) even for the unused lines (maybe 1 per line). But I wouldn't
> think that we would need to allocate them to cover the entire worst
> case length.

I really didn't stop to audit the i8xx code, and I was not planning to.
I'm just trying to not add any new regressions. Maybe we should open a
bug report or Jira task somewhere for your observation.



> What makes things more confusing I think is the naming of the
> variables
> and functions. uncompressed_size is actually the size we've allocated
> for the cfb

Except for the cases where threshold != 1, and for the cases where we
over-allocate the CFB.


> , and intel_fbc_calculate_cfb_size() sort of returns the
> uncompressed fb size

Yes, it's redundant. See below.


> . Well since it has the 2k line limit applied, I
> suppose you can actually think of it as the max cfb size we would
> want
> to have for the particular plane configuration.

Depends on how you see it. It's the maximum CFB size since we can
increase the threshold. It's also the minimum viable CFB size for
threshold=1. Since it's the base number used for the hardware
calculations, I decided to call it just "the cfb size". But I'm always
open to suggestions for better names.

By the way, I have a patch that completely kills uncompressed_size and
makes the i8xx code just call intel_fbc_calculate_cfb_size() directly.
IMHO, having less variables to track makes things simpler. The patch
will be on the list soon.

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

Re: [PATCH 00/13] Yet another FBC series, v3 part 1

[Daniel Stone]

Hi Paulo,

On 4 November 2015 at 19:10, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> So Ville pointed a problem on patch 02/26 of the previous series, and the nice
> fix for that would make me rebase most of the subsequent patches. In order to
> avoid blocking the other patches on the review of patch 02 and in order to avoid
> having to rebase everything again and again during this I decided to change the
> order of the patches on the series and try to get a smaller chunk of patches
> merged before moving on to the others.
>
> These first 13 patches are somewhat trivial and are mostly just moving code
> around, with only one or two simple bug fixes. If you wonder why some of these
> changes are needed, please go check the complete series (the short answer is:
> most of the changes are needed for the new model with enable/disable +
> activate/deactivate).
>
> After all these patches are merged I'll resend the rest.

This series is missing 01/26 of your previous series (make
no_fbc_reason a string).

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

Re: [PATCH 00/13] Yet another FBC series, v3 part 1

[Zanoni, Paulo R]

Em Sex, 2015-11-13 às 15:49 +0000, Daniel Stone escreveu:
> Hi Paulo,
> 
> On 4 November 2015 at 19:10, Paulo Zanoni <paulo.r.zanoni@intel.com>
> wrote:
> > So Ville pointed a problem on patch 02/26 of the previous series,
> > and the nice
> > fix for that would make me rebase most of the subsequent patches.
> > In order to
> > avoid blocking the other patches on the review of patch 02 and in
> > order to avoid
> > having to rebase everything again and again during this I decided
> > to change the
> > order of the patches on the series and try to get a smaller chunk
> > of patches
> > merged before moving on to the others.
> > 
> > These first 13 patches are somewhat trivial and are mostly just
> > moving code
> > around, with only one or two simple bug fixes. If you wonder why
> > some of these
> > changes are needed, please go check the complete series (the short
> > answer is:
> > most of the changes are needed for the new model with
> > enable/disable +
> > activate/deactivate).
> > 
> > After all these patches are merged I'll resend the rest.
> 
> This series is missing 01/26 of your previous series (make
> no_fbc_reason a string).

It was already merged when I resent:

http://cgit.freedesktop.org/drm-intel/commit/?id=bf6189c6f062b5cd6ca0d4
6754e4c3064643f48c


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

Re: [PATCH 00/13] Yet another FBC series, v3 part 1

[Daniel Stone]

Hey,

On 13 November 2015 at 16:36, Zanoni, Paulo R <paulo.r.zanoni@intel.com> wrote:
> Em Sex, 2015-11-13 às 15:49 +0000, Daniel Stone escreveu:
>> On 4 November 2015 at 19:10, Paulo Zanoni <paulo.r.zanoni@intel.com>
>> wrote:
>> > So Ville pointed a problem on patch 02/26 of the previous series,
>> > and the nice
>> > fix for that would make me rebase most of the subsequent patches.
>> > In order to
>> > avoid blocking the other patches on the review of patch 02 and in
>> > order to avoid
>> > having to rebase everything again and again during this I decided
>> > to change the
>> > order of the patches on the series and try to get a smaller chunk
>> > of patches
>> > merged before moving on to the others.
>> >
>> > These first 13 patches are somewhat trivial and are mostly just
>> > moving code
>> > around, with only one or two simple bug fixes. If you wonder why
>> > some of these
>> > changes are needed, please go check the complete series (the short
>> > answer is:
>> > most of the changes are needed for the new model with
>> > enable/disable +
>> > activate/deactivate).
>> >
>> > After all these patches are merged I'll resend the rest.
>>
>> This series is missing 01/26 of your previous series (make
>> no_fbc_reason a string).
>
> It was already merged when I resent:
>
> http://cgit.freedesktop.org/drm-intel/commit/?id=bf6189c6f062b5cd6ca0d4
> 6754e4c3064643f48c

Sorry, missed that somehow!

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