[PATCH 0/6] More small FBC fixes

[PATCH 0/6] More small FBC fixes

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Serve these while the others are still baking.

Paulo Zanoni (6):
  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

 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 76 ++++++++++++++++++++++++++++++----------
 2 files changed, 58 insertions(+), 19 deletions(-)

-- 
2.1.4

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

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 c271af7..1448746 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.1.4

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

[PATCH 2/6] drm/i915: fix the FBC work allocation failure path

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We were forgetting to set fbc.crtc, fbc.fb_id and fbc.y when the
kzalloc call failed. Luckily, this is not a very common case, I hope.

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 1448746..64f317a 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.1.4

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

Re: [PATCH 2/6] drm/i915: fix the FBC work allocation failure path

[Chris Wilson]

On Wed, Jul 08, 2015 at 05:58:55PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We were forgetting to set fbc.crtc, fbc.fb_id and fbc.y when the
> kzalloc call failed. Luckily, this is not a very common case, I hope.

That did not prepare me for the patch that lies ahead.

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

[PATCH 2/6] drm/i915: fix the FBC work allocation failure path

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 1448746..64f317a 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.1.4

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

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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-mfb-draw-mmap-gtt
 - igt/kms_frontbuffer_tracking/fbc-2p-primscrn-pri-mfb-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 64f317a..0373cbc 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.1.4

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

[PATCH 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 0373cbc..0a24e96 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.1.4

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

Re: [PATCH 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Daniel Vetter]

On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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>

Hm, do we have testcases where we have a sufficiently big y offset? We can
just allocate 128 lines more and use that as the offset, that should be
big enough everywhere. Actually make that 129 lines to check the tile-size
rounding ;-)

Ofc this means we need to have two sets of testcases for all the affected
tests (i.e. everything that tries to test the gtt hw tracking).

Another funny corner case (which we're getting wrong on skl even without
fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
bigger values and then it wraps).

I.e. I'd like this patch (and the others) to be augmented with a Testcase:
tag.

Cheers, Daniel

> ---
>  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 0373cbc..0a24e96 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.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Paulo Zanoni]

2015-07-09 14:10 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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>
>
> Hm, do we have testcases where we have a sufficiently big y offset? We can
> just allocate 128 lines more and use that as the offset, that should be
> big enough everywhere. Actually make that 129 lines to check the tile-size
> rounding ;-)

Yes, it's 500x500. See FBS_MULTI on kms_frontbuffer_tracking. Subtests
with "sfb" on their names create a Single frontbuffer for each
possible primary plane (primary, secondary, offscreen), while subtests
with "mfb" have Multiple pipes pointing to the same frontbuffer. See
the small drawing inside kms_frontbuffer_tracking, at the top of
create_big_fb(). By the way, it's on my TODO list to change that
arrangement a little bit in order to avoid super-huge strides.

>
> Ofc this means we need to have two sets of testcases for all the affected
> tests (i.e. everything that tries to test the gtt hw tracking).

We do.

>
> Another funny corner case (which we're getting wrong on skl even without
> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
> bigger values and then it wraps).

Can you please clarify the sentence above? For the dual-screen
subtests, if we use 2 screens of 1024 + the 500 pixel offset, we'll
already be bigger than 2048 pixels.

>
> I.e. I'd like this patch (and the others) to be augmented with a Testcase:

This one doesn't have a Testcase tag because I'm not testing SNB right
now, so I can't confirm anything here. Patch 3 has the useful
Testcases tags.

> tag.
>
> Cheers, Daniel
>
>> ---
>>  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 0373cbc..0a24e96 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.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

Re: [PATCH 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Paulo Zanoni]

2015-07-09 14:15 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> 2015-07-09 14:10 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> 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>
>>
>> Hm, do we have testcases where we have a sufficiently big y offset? We can
>> just allocate 128 lines more and use that as the offset, that should be
>> big enough everywhere. Actually make that 129 lines to check the tile-size
>> rounding ;-)
>
> Yes, it's 500x500. See FBS_MULTI on kms_frontbuffer_tracking. Subtests
> with "sfb" on their names create a Single frontbuffer for each
> possible primary plane (primary, secondary, offscreen), while subtests
> with "mfb" have Multiple pipes pointing to the same frontbuffer. See
> the small drawing inside kms_frontbuffer_tracking, at the top of
> create_big_fb(). By the way, it's on my TODO list to change that
> arrangement a little bit in order to avoid super-huge strides.

Also notice that I tested values other than 500x500 while developing
patch 3. 500x500 seems like a nice value since it's not aligned with
any tiles and never part of the first tile.

>
>>
>> Ofc this means we need to have two sets of testcases for all the affected
>> tests (i.e. everything that tries to test the gtt hw tracking).
>
> We do.
>
>>
>> Another funny corner case (which we're getting wrong on skl even without
>> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
>> bigger values and then it wraps).
>
> Can you please clarify the sentence above? For the dual-screen
> subtests, if we use 2 screens of 1024 + the 500 pixel offset, we'll
> already be bigger than 2048 pixels.
>
>>
>> I.e. I'd like this patch (and the others) to be augmented with a Testcase:
>
> This one doesn't have a Testcase tag because I'm not testing SNB right
> now, so I can't confirm anything here. Patch 3 has the useful
> Testcases tags.
>
>> tag.
>>
>> Cheers, Daniel
>>
>>> ---
>>>  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 0373cbc..0a24e96 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.1.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
>
>
> --
> Paulo Zanoni



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

Re: [PATCH 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Ville Syrjälä]

On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote:
> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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>
> 
> Hm, do we have testcases where we have a sufficiently big y offset? We can
> just allocate 128 lines more and use that as the offset, that should be
> big enough everywhere. Actually make that 129 lines to check the tile-size
> rounding ;-)
> 
> Ofc this means we need to have two sets of testcases for all the affected
> tests (i.e. everything that tries to test the gtt hw tracking).
> 
> Another funny corner case (which we're getting wrong on skl even without
> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
> bigger values and then it wraps).
> 
> I.e. I'd like this patch (and the others) to be augmented with a Testcase:
> tag.

I think the entire Y offset thing is currently being misprogrammed.
IIRC the offset is from the display base address but we program in
the offset from the start of the FB.

> 
> Cheers, Daniel
> 
> > ---
> >  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 0373cbc..0a24e96 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.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> 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

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

Re: [PATCH 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Paulo Zanoni]

2015-07-09 14:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote:
>> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > 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>
>>
>> Hm, do we have testcases where we have a sufficiently big y offset? We can
>> just allocate 128 lines more and use that as the offset, that should be
>> big enough everywhere. Actually make that 129 lines to check the tile-size
>> rounding ;-)
>>
>> Ofc this means we need to have two sets of testcases for all the affected
>> tests (i.e. everything that tries to test the gtt hw tracking).
>>
>> Another funny corner case (which we're getting wrong on skl even without
>> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
>> bigger values and then it wraps).
>>
>> I.e. I'd like this patch (and the others) to be augmented with a Testcase:
>> tag.
>
> I think the entire Y offset thing is currently being misprogrammed.
> IIRC the offset is from the display base address but we program in
> the offset from the start of the FB.

After patch 3, all the current tests pass on BDW. Can you suggest a
different test that won't pass?

>
>>
>> Cheers, Daniel
>>
>> > ---
>> >  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 0373cbc..0a24e96 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.1.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> 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
>
> --
> 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 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Ville Syrjälä]

On Thu, Jul 09, 2015 at 02:31:15PM -0300, Paulo Zanoni wrote:
> 2015-07-09 14:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote:
> >> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > 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>
> >>
> >> Hm, do we have testcases where we have a sufficiently big y offset? We can
> >> just allocate 128 lines more and use that as the offset, that should be
> >> big enough everywhere. Actually make that 129 lines to check the tile-size
> >> rounding ;-)
> >>
> >> Ofc this means we need to have two sets of testcases for all the affected
> >> tests (i.e. everything that tries to test the gtt hw tracking).
> >>
> >> Another funny corner case (which we're getting wrong on skl even without
> >> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
> >> bigger values and then it wraps).
> >>
> >> I.e. I'd like this patch (and the others) to be augmented with a Testcase:
> >> tag.
> >
> > I think the entire Y offset thing is currently being misprogrammed.
> > IIRC the offset is from the display base address but we program in
> > the offset from the start of the FB.
> 
> After patch 3, all the current tests pass on BDW. Can you suggest a
> different test that won't pass?

Ah patch 3 tries to fix it. It's not entirely accurate though since it
simply relies on an implementation detail of intel_gen4_compute_page_offset().
Well, assuming my recollection of the hardware details is correct.

Also IIRC intel_gen4_compute_page_offset() isn't even used on SKL/BXT
currently, so it should fail on those platforms.

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

Re: [PATCH 4/6] drm/i915: set ILK_DPFC_FENCE_YOFF to 0 on SNB

[Paulo Zanoni]

2015-07-09 14:39 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Jul 09, 2015 at 02:31:15PM -0300, Paulo Zanoni wrote:
>> 2015-07-09 14:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>> > On Thu, Jul 09, 2015 at 07:10:04PM +0200, Daniel Vetter wrote:
>> >> On Wed, Jul 08, 2015 at 05:58:57PM -0300, Paulo Zanoni wrote:
>> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> >
>> >> > 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>
>> >>
>> >> Hm, do we have testcases where we have a sufficiently big y offset? We can
>> >> just allocate 128 lines more and use that as the offset, that should be
>> >> big enough everywhere. Actually make that 129 lines to check the tile-size
>> >> rounding ;-)
>> >>
>> >> Ofc this means we need to have two sets of testcases for all the affected
>> >> tests (i.e. everything that tries to test the gtt hw tracking).
>> >>
>> >> Another funny corner case (which we're getting wrong on skl even without
>> >> fbc) is x offsets > 2048 pixels (since x/y offset registers don't hold
>> >> bigger values and then it wraps).
>> >>
>> >> I.e. I'd like this patch (and the others) to be augmented with a Testcase:
>> >> tag.
>> >
>> > I think the entire Y offset thing is currently being misprogrammed.
>> > IIRC the offset is from the display base address but we program in
>> > the offset from the start of the FB.
>>
>> After patch 3, all the current tests pass on BDW. Can you suggest a
>> different test that won't pass?
>
> Ah patch 3 tries to fix it. It's not entirely accurate though since it
> simply relies on an implementation detail of intel_gen4_compute_page_offset().
> Well, assuming my recollection of the hardware details is correct.
>
> Also IIRC intel_gen4_compute_page_offset() isn't even used on SKL/BXT
> currently, so it should fail on those platforms.

Daniel clarified the problem to me, so I implemented the following test case:
http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/?id=04d1311fc3d2127d609b5c5e670bf9887652cb17

I hope this exercises the problem you're mentioning. So far I only
tested BDW and it passes.

>
> --
> Ville Syrjälä
> Intel OTC



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

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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.

This may fix kms_frontbuffer_tracking failures depending on your
monitor configuration.

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 | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 52d07fb..4a4b2a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -936,6 +936,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 0a24e96..7b65b00 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";
@@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
 		goto out_disable;
 	}
 
+	/* TODO: we need to figure out what are the stride restrictions for the
+	 * older platforms. */
+	if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 ||
+	    (fb->pitches[0] & (64 - 1)) != 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.1.4

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

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

[Daniel Vetter]

On Wed, Jul 08, 2015 at 05:58:58PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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.
> 
> This may fix kms_frontbuffer_tracking failures depending on your
> monitor configuration.
> 
> 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 | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 52d07fb..4a4b2a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -936,6 +936,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 0a24e96..7b65b00 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";
> @@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>  		goto out_disable;
>  	}
>  
> +	/* TODO: we need to figure out what are the stride restrictions for the
> +	 * older platforms. */
> +	if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 ||

< 512 is already take care of by the X-tiling restriction (requires a
multiple of 512 bytes stride). > 16kb might be the general stride limit,
we should probably reject that in intel_framebuffer_init. 64b alignment is
strange since X-tiled is stricter, and we that already in
intel_fb_stride_alignment.

So summary: Sounds like we need to add per-platform stride limit checks to
fb create code. Plus igt testcases to make sure we check for this (since
right now it seems like we don't). Additional checks here in the fbc code
don't seem required. But if you want to I guess you could convert them to
WARN_ON (without bailing out).
-Daniel

> +	    (fb->pitches[0] & (64 - 1)) != 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.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 5/6] drm/i915: check for the supported strides on HSW+ FBC

[Paulo Zanoni]

2015-07-09 14:15 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Jul 08, 2015 at 05:58:58PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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.
>>
>> This may fix kms_frontbuffer_tracking failures depending on your
>> monitor configuration.
>>
>> 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 | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 52d07fb..4a4b2a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -936,6 +936,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 0a24e96..7b65b00 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";
>> @@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv)
>>               goto out_disable;
>>       }
>>
>> +     /* TODO: we need to figure out what are the stride restrictions for the
>> +      * older platforms. */
>> +     if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 ||
>
> < 512 is already take care of by the X-tiling restriction (requires a
> multiple of 512 bytes stride)
. > 16kb might be the general stride limit,
> we should probably reject that in intel_framebuffer_init. 64b alignment is
> strange since X-tiled is stricter, and we that already in
> intel_fb_stride_alignment.

16384 bytes is just  4096 32bit pixels wide. If you have 2 modern
monitors configured in wide mode you already beat that (or maybe 3
not-so-fancy monitors). So if you want FBC you'll have to "xrandr
--output DP1 --above eDP1 --auto", insead of "--left-of" (also, pray
that the guys developing your desktop environment actually tested this
case). So this is certainly a check that we want in the FBC code but
not in other places. I can remove the other checks if you want.

I also did find that gen3 requires specifically 4k or 8k strides:
nothing except for that. But I'll only write this patch much later
(gen3 is not a priority for now, as you know).

>
> So summary: Sounds like we need to add per-platform stride limit checks to
> fb create code.

As mentioned above: no. FBC can be more strict.

> Plus igt testcases to make sure we check for this (since
> right now it seems like we don't).

It's on the TODO list but it's not a priority since the Kernel checks
are very straightforward. One of the problems is that different
platforms have different requirements, so I'll have to hardcode those
requirements on IGT too. And I'll have to stop using igt_create_fb for
that case since it only use powers of 2 for stride.

So yeah, one day we may have the test, but not a priority right now
since it's very easy to look at the Kernel code and make sure it's
correct.

> Additional checks here in the fbc code
> don't seem required. But if you want to I guess you could convert them to
> WARN_ON (without bailing out).
> -Daniel
>
>> +         (fb->pitches[0] & (64 - 1)) != 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.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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.

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 768d1db..6cde34f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -937,6 +937,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 0a24e96..f221c0e 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.1.4

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

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

[Paulo Zanoni]

2015-07-09 14:28 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> 2015-07-09 14:15 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
>> Plus igt testcases to make sure we check for this (since
>> right now it seems like we don't).
>
> It's on the TODO list but it's not a priority since the Kernel checks
> are very straightforward. One of the problems is that different
> platforms have different requirements, so I'll have to hardcode those
> requirements on IGT too. And I'll have to stop using igt_create_fb for
> that case since it only use powers of 2 for stride.
>
> So yeah, one day we may have the test, but not a priority right now
> since it's very easy to look at the Kernel code and make sure it's
> correct.

Ok, so I implemented fbc-badstride with the restrictions mentioned in
this patch:

http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/?id=cb3861a9e3f1bc12765160345bb0dd1d543f5086

If we add more restrictions we'll have to update the test.

>
>> Additional checks here in the fbc code
>> don't seem required. But if you want to I guess you could convert them to
>> WARN_ON (without bailing out).
>> -Daniel
>>
>>> +         (fb->pitches[0] & (64 - 1)) != 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.1.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
>
>
> --
> Paulo Zanoni



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

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 7b65b00..594b62d 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.1.4

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