[PATCH 1/2] drm/i915/fbc: Don't set an illegal fence if unfenced

[PATCH 1/2] drm/i915/fbc: Don't set an illegal fence if unfenced

[Chris Wilson]

If the frontbuffer doesn't have an associated fence, it will have a
fence reg of -1. If we attempt to OR in this register into the FBC
control register we end up setting all control bits, oops!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 37415f96f906..57e1ca624d73 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -190,9 +190,11 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
 		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
 	else
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
-	dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
 
-	I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
+	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
+		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
+		I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
+	}
 
 	/* enable it... */
 	I915_WRITE(DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
@@ -244,21 +246,24 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
 		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
 		break;
 	}
-	dpfc_ctl |= DPFC_CTL_FENCE_EN;
-	if (IS_GEN5(dev_priv))
-		dpfc_ctl |= params->fb.fence_reg;
+
+	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
+		dpfc_ctl |= DPFC_CTL_FENCE_EN;
+		if (IS_GEN5(dev_priv))
+			dpfc_ctl |= params->fb.fence_reg;
+		if (IS_GEN6(dev_priv)) {
+			I915_WRITE(SNB_DPFC_CTL_SA,
+				   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
+			I915_WRITE(DPFC_CPU_FENCE_OFFSET,
+				   params->crtc.fence_y_offset);
+		}
+	}
 
 	I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
 	I915_WRITE(ILK_FBC_RT_BASE, params->fb.ggtt_offset | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
-	if (IS_GEN6(dev_priv)) {
-		I915_WRITE(SNB_DPFC_CTL_SA,
-			   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
-		I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
-	}
-
 	intel_fbc_recompress(dev_priv);
 }
 
@@ -305,7 +310,12 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
 		break;
 	}
 
-	dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
+	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
+		dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
+		I915_WRITE(SNB_DPFC_CTL_SA,
+			   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
+	}
 
 	if (dev_priv->fbc.false_color)
 		dpfc_ctl |= FBC_CTL_FALSE_COLOR;
@@ -324,10 +334,6 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
 
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
-	I915_WRITE(SNB_DPFC_CTL_SA,
-		   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
-	I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
-
 	intel_fbc_recompress(dev_priv);
 }
 
-- 
2.9.3

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

[PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Chris Wilson]

Only fbc1 is tied to using a fence. Later iterations of fbc are more
flexible and allow operation on unfenced frontbuffers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 57e1ca624d73..9534f90c6551 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -789,8 +789,10 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 	 */
 	if (cache->fb.tiling_mode != I915_TILING_X ||
 	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
-		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
-		return false;
+		if (INTEL_GEN(dev_priv) < 5) {
+			fbc->no_fbc_reason = "framebuffer not tiled or fenced";
+			return false;
+		}
 	}
 	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
 	    cache->plane.rotation != DRM_ROTATE_0) {
-- 
2.9.3

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

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915/fbc: Don't set an illegal fence if unfenced

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/fbc: Don't set an illegal fence if unfenced
URL   : https://patchwork.freedesktop.org/series/11256/
State : failure

== Summary ==

Series 11256v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/11256/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (ro-skl3-i5-6260u)
                fail       -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i7-5557U)
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)

fi-kbl-qkkr      total:244  pass:186  dwarn:28  dfail:0   fail:3   skip:27 
fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:219  dwarn:4   dfail:0   fail:0   skip:17 
ro-bdw-i7-5557U  total:240  pass:219  dwarn:2   dfail:0   fail:0   skip:19 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1918/

e45fdef drm-intel-nightly: 2016y-08m-17d-13h-26m-04s UTC integration manifest
6053b4f drm/i915/fbc: Allow on unfenced surfaces, for recent gen
ad3c29c drm/i915/fbc: Don't set an illegal fence if unfenced

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

Re: [PATCH 1/2] drm/i915/fbc: Don't set an illegal fence if unfenced

[Joonas Lahtinen]

On to, 2016-08-18 at 09:21 +0100, Chris Wilson wrote:
> @@ -190,9 +190,11 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
>  		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
>  	else
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
> -	dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
>  
> -	I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
> +	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
> +		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->fb.fence_reg;
> +		I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
> +	}

Here the fence_y_offset is conditional on the fence existing.

> @@ -244,21 +246,24 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
>  		break;
>  	}
> -	dpfc_ctl |= DPFC_CTL_FENCE_EN;
> -	if (IS_GEN5(dev_priv))
> -		dpfc_ctl |= params->fb.fence_reg;
> +
> +	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
> +		dpfc_ctl |= DPFC_CTL_FENCE_EN;
> +		if (IS_GEN5(dev_priv))
> +			dpfc_ctl |= params->fb.fence_reg;
> +		if (IS_GEN6(dev_priv)) {
> +			I915_WRITE(SNB_DPFC_CTL_SA,
> +				   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
> +			I915_WRITE(DPFC_CPU_FENCE_OFFSET,
> +				   params->crtc.fence_y_offset);
> +		}
> +	}
>  
>  	I915_WRITE(ILK_DPFC_FENCE_YOFF, params->crtc.fence_y_offset);

Here it is not?

> @@ -305,7 +310,12 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>  
> -	dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
> +	if (params->fb.fence_reg != I915_FENCE_REG_NONE) {
> +		dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
> +		I915_WRITE(SNB_DPFC_CTL_SA,
> +			   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
> +		I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);
> +	}
>  
>  	if (dev_priv->fbc.false_color)
>  		dpfc_ctl |= FBC_CTL_FALSE_COLOR;
> @@ -324,10 +334,6 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
>  
>  	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
>  
> -	I915_WRITE(SNB_DPFC_CTL_SA,
> -		   SNB_CPU_FENCE_ENABLE | params->fb.fence_reg);
> -	I915_WRITE(DPFC_CPU_FENCE_OFFSET, params->crtc.fence_y_offset);

Here it is again moved conditionally. So I suggest you unify that.

This changes the order of the register writes on older platforms so
Tested-bys would be good.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Zanoni, Paulo R]

Em Qui, 2016-08-18 às 09:21 +0100, Chris Wilson escreveu:
> Only fbc1 is tied to using a fence. Later iterations of fbc are more
> flexible and allow operation on unfenced frontbuffers.

But then we'll lose GTT tracking - which we currently rely on - and I'm
87.5% sure we'll need to implement some workarounds we skipped.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 57e1ca624d73..9534f90c6551 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -789,8 +789,10 @@ static bool intel_fbc_can_activate(struct
> intel_crtc *crtc)
>  	 */
>  	if (cache->fb.tiling_mode != I915_TILING_X ||
>  	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
> -		fbc->no_fbc_reason = "framebuffer not tiled or
> fenced";
> -		return false;
> +		if (INTEL_GEN(dev_priv) < 5) {
> +			fbc->no_fbc_reason = "framebuffer not tiled
> or fenced";
> +			return false;
> +		}
>  	}
>  	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
>  	    cache->plane.rotation != DRM_ROTATE_0) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[chris]

On Thu, Aug 18, 2016 at 01:56:56PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2016-08-18 às 09:21 +0100, Chris Wilson escreveu:
> > Only fbc1 is tied to using a fence. Later iterations of fbc are more
> > flexible and allow operation on unfenced frontbuffers.
> 
> But then we'll lose GTT tracking - which we currently rely on - and I'm
> 87.5% sure we'll need to implement some workarounds we skipped.

You've missed the patches that point out that the GTT tracking is
broken? :)

We don't rely on GTT tracking at all because we don't use the tracked
GTT address for pwrite or GTT mmap access (and not for WC or CPU mmap
access). And CS uses a completely different address space (caveat those
older gen in which it looks like the GGTT).
-Chris

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

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Joonas Lahtinen]

On to, 2016-08-18 at 09:21 +0100, Chris Wilson wrote:
> Only fbc1 is tied to using a fence. Later iterations of fbc are more
> flexible and allow operation on unfenced frontbuffers.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>

I could only find that unfenced compressed framebuffer does not work on
Gen4 but will work Gen6.

So somebody should test with Gen5. Maybe we should document this as a
comment?

Code itself makes sense;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 57e1ca624d73..9534f90c6551 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -789,8 +789,10 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  	 */
>  	if (cache->fb.tiling_mode != I915_TILING_X ||
>  	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
> -		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> -		return false;
> +		if (INTEL_GEN(dev_priv) < 5) {
> +			fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> +			return false;
> +		}
>  	}
>  	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
>  	    cache->plane.rotation != DRM_ROTATE_0) {
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Zanoni, Paulo R]

Em Qui, 2016-08-18 às 09:21 +0100, Chris Wilson escreveu:
> Only fbc1 is tied to using a fence. Later iterations of fbc are more
> flexible and allow operation on unfenced frontbuffers.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>

Hi

I see this patch was applied. Now, on my Skylake machine, if I boot
with i915.enable_fbc=1 I'll get FIFO underruns under fbcon. Just
booting already gives me a FIFO underrun message, and then if I "sudo
systemctl stop lightdm" I'll get a constantly-blinking screen.

Of course, applying the patch that disables FBC after a FIFO underrun
will help stopping the ever-blinking fbcon, but I think we should try
to avoid the underruns in the places we know we can, and leave the
"disable FBC on FIFO underruns" just for the cases we're not expecting.

Also, please remember that I mentioned there are FBC workarounds for
untiled that we still don't implement. IMHO this argument alone should
have prevented this patch from being merged...

Based on that, can we please revert this patch? I'm afraid some people
would consider these underruns as blockers to enabling FBC, so it's
probably better to enable FBC only on X tiled for now, and leave this
for when we know how to prevent the underrun.

Thanks,
Paulo

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 57e1ca624d73..9534f90c6551 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -789,8 +789,10 @@ static bool intel_fbc_can_activate(struct
> intel_crtc *crtc)
>  	 */
>  	if (cache->fb.tiling_mode != I915_TILING_X ||
>  	    cache->fb.fence_reg == I915_FENCE_REG_NONE) {
> -		fbc->no_fbc_reason = "framebuffer not tiled or
> fenced";
> -		return false;
> +		if (INTEL_GEN(dev_priv) < 5) {
> +			fbc->no_fbc_reason = "framebuffer not tiled
> or fenced";
> +			return false;
> +		}
>  	}
>  	if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
>  	    cache->plane.rotation != DRM_ROTATE_0) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Paulo Zanoni]

2016-08-18 5:21 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> Only fbc1 is tied to using a fence. Later iterations of fbc are more
> flexible and allow operation on unfenced frontbuffers.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>

Hi

I see this patch was applied. Now, on my Skylake machine, if I boot
with i915.enable_fbc=1 I'll get FIFO underruns under fbcon. Just
booting already gives me a FIFO underrun message, and then if I "sudo
systemctl stop lightdm" I'll get a constantly-blinking screen.

Of course, applying the patch that disables FBC after a FIFO underrun
will help stopping the ever-blinking fbcon, but I think we should try
to avoid the underruns in the places we know we can, and leave the
"disable FBC on FIFO underruns" just for the cases we're not expecting.

Also, please remember that I mentioned there are FBC workarounds for
untiled that we still don't implement (although when I re-read my
email it may sound like I suggested the workarounds are for non-GTT
tracking). IMHO this argument alone should
have prevented this patch from being merged...

Based on that, can we please revert this patch? I'm afraid some people
would consider these underruns as blockers to enabling FBC, so it's
probably better to enable FBC only on X tiled for now, and leave this
for when we know how to prevent the underrun (possibly by implementing
the missing WAs).


(I'm sorry if you got this message twice, but the mail servers are a
little crazy these days and I didn't receive my copy, so I'm sending
it again).

Thanks,
Paulo


> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 57e1ca624d73..9534f90c6551 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -789,8 +789,10 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>          */
>         if (cache->fb.tiling_mode != I915_TILING_X ||
>             cache->fb.fence_reg == I915_FENCE_REG_NONE) {
> -               fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> -               return false;
> +               if (INTEL_GEN(dev_priv) < 5) {
> +                       fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> +                       return false;
> +               }
>         }
>         if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
>             cache->plane.rotation != DRM_ROTATE_0) {
> --
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Daniel Vetter]

On Mon, Aug 22, 2016 at 09:39:17PM -0300, Paulo Zanoni wrote:
> 2016-08-18 5:21 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > Only fbc1 is tied to using a fence. Later iterations of fbc are more
> > flexible and allow operation on unfenced frontbuffers.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> I see this patch was applied. Now, on my Skylake machine, if I boot
> with i915.enable_fbc=1 I'll get FIFO underruns under fbcon. Just
> booting already gives me a FIFO underrun message, and then if I "sudo
> systemctl stop lightdm" I'll get a constantly-blinking screen.
> 
> Of course, applying the patch that disables FBC after a FIFO underrun
> will help stopping the ever-blinking fbcon, but I think we should try
> to avoid the underruns in the places we know we can, and leave the
> "disable FBC on FIFO underruns" just for the cases we're not expecting.
> 
> Also, please remember that I mentioned there are FBC workarounds for
> untiled that we still don't implement (although when I re-read my
> email it may sound like I suggested the workarounds are for non-GTT
> tracking). IMHO this argument alone should
> have prevented this patch from being merged...
> 
> Based on that, can we please revert this patch? I'm afraid some people
> would consider these underruns as blockers to enabling FBC, so it's
> probably better to enable FBC only on X tiled for now, and leave this
> for when we know how to prevent the underrun (possibly by implementing
> the missing WAs).
> 
> 
> (I'm sorry if you got this message twice, but the mail servers are a
> little crazy these days and I didn't receive my copy, so I'm sending
> it again).

Yeah, mailman was on vacation a bit the last few days due to a ddos
probably. +1 from me for just reverting if this is causing troubles.
Also, patch doesn't seem to have a Testcase: line, was the
kms_frontbuffer_tracking test not extended to cover this new use-case? In
that case definitely revert, since failed to pass testing requirements.
-Daniel

> 
> Thanks,
> Paulo
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 57e1ca624d73..9534f90c6551 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -789,8 +789,10 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> >          */
> >         if (cache->fb.tiling_mode != I915_TILING_X ||
> >             cache->fb.fence_reg == I915_FENCE_REG_NONE) {
> > -               fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> > -               return false;
> > +               if (INTEL_GEN(dev_priv) < 5) {
> > +                       fbc->no_fbc_reason = "framebuffer not tiled or fenced";
> > +                       return false;
> > +               }
> >         }
> >         if (INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) &&
> >             cache->plane.rotation != DRM_ROTATE_0) {
> > --
> > 2.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Chris Wilson]

On Mon, Aug 22, 2016 at 09:39:17PM -0300, Paulo Zanoni wrote:
> 2016-08-18 5:21 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > Only fbc1 is tied to using a fence. Later iterations of fbc are more
> > flexible and allow operation on unfenced frontbuffers.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> I see this patch was applied. Now, on my Skylake machine, if I boot
> with i915.enable_fbc=1 I'll get FIFO underruns under fbcon. Just
> booting already gives me a FIFO underrun message, and then if I "sudo
> systemctl stop lightdm" I'll get a constantly-blinking screen.
> 
> Of course, applying the patch that disables FBC after a FIFO underrun
> will help stopping the ever-blinking fbcon, but I think we should try
> to avoid the underruns in the places we know we can, and leave the
> "disable FBC on FIFO underruns" just for the cases we're not expecting.
> 
> Also, please remember that I mentioned there are FBC workarounds for
> untiled that we still don't implement (although when I re-read my
> email it may sound like I suggested the workarounds are for non-GTT
> tracking). IMHO this argument alone should
> have prevented this patch from being merged...
> 
> Based on that, can we please revert this patch? I'm afraid some people
> would consider these underruns as blockers to enabling FBC, so it's
> probably better to enable FBC only on X tiled for now, and leave this
> for when we know how to prevent the underrun (possibly by implementing
> the missing WAs).

Sure you can disable FBC - just note that typically framebuffers will be
unfenced.
-Chris

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

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Daniel Vetter]

On Wed, Aug 24, 2016 at 7:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Aug 22, 2016 at 09:39:17PM -0300, Paulo Zanoni wrote:
>> 2016-08-18 5:21 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> > Only fbc1 is tied to using a fence. Later iterations of fbc are more
>> > flexible and allow operation on unfenced frontbuffers.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Daniel Vetter <daniel.vetter@intel.com>
>> > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
>>
>> Hi
>>
>> I see this patch was applied. Now, on my Skylake machine, if I boot
>> with i915.enable_fbc=1 I'll get FIFO underruns under fbcon. Just
>> booting already gives me a FIFO underrun message, and then if I "sudo
>> systemctl stop lightdm" I'll get a constantly-blinking screen.
>>
>> Of course, applying the patch that disables FBC after a FIFO underrun
>> will help stopping the ever-blinking fbcon, but I think we should try
>> to avoid the underruns in the places we know we can, and leave the
>> "disable FBC on FIFO underruns" just for the cases we're not expecting.
>>
>> Also, please remember that I mentioned there are FBC workarounds for
>> untiled that we still don't implement (although when I re-read my
>> email it may sound like I suggested the workarounds are for non-GTT
>> tracking). IMHO this argument alone should
>> have prevented this patch from being merged...
>>
>> Based on that, can we please revert this patch? I'm afraid some people
>> would consider these underruns as blockers to enabling FBC, so it's
>> probably better to enable FBC only on X tiled for now, and leave this
>> for when we know how to prevent the underrun (possibly by implementing
>> the missing WAs).
>>
>>
>> (I'm sorry if you got this message twice, but the mail servers are a
>> little crazy these days and I didn't receive my copy, so I'm sending
>> it again).
>
> Yeah, mailman was on vacation a bit the last few days due to a ddos
> probably. +1 from me for just reverting if this is causing troubles.
> Also, patch doesn't seem to have a Testcase: line, was the
> kms_frontbuffer_tracking test not extended to cover this new use-case? In
> that case definitely revert, since failed to pass testing requirements.

Original patch also wasn't acked by Paulo, which it pretty much has to
be as an fbc patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[Zanoni, Paulo R]

Em Qua, 2016-08-24 às 07:43 +0100, Chris Wilson escreveu:
> On Mon, Aug 22, 2016 at 09:39:17PM -0300, Paulo Zanoni wrote:
> > 
> > 2016-08-18 5:21 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > > 
> > > Only fbc1 is tied to using a fence. Later iterations of fbc are
> > > more
> > > flexible and allow operation on unfenced frontbuffers.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> > 
> > Hi
> > 
> > I see this patch was applied. Now, on my Skylake machine, if I boot
> > with i915.enable_fbc=1 I'll get FIFO underruns under fbcon. Just
> > booting already gives me a FIFO underrun message, and then if I
> > "sudo
> > systemctl stop lightdm" I'll get a constantly-blinking screen.
> > 
> > Of course, applying the patch that disables FBC after a FIFO
> > underrun
> > will help stopping the ever-blinking fbcon, but I think we should
> > try
> > to avoid the underruns in the places we know we can, and leave the
> > "disable FBC on FIFO underruns" just for the cases we're not
> > expecting.
> > 
> > Also, please remember that I mentioned there are FBC workarounds
> > for
> > untiled that we still don't implement (although when I re-read my
> > email it may sound like I suggested the workarounds are for non-GTT
> > tracking). IMHO this argument alone should
> > have prevented this patch from being merged...
> > 
> > Based on that, can we please revert this patch? I'm afraid some
> > people
> > would consider these underruns as blockers to enabling FBC, so it's
> > probably better to enable FBC only on X tiled for now, and leave
> > this
> > for when we know how to prevent the underrun (possibly by
> > implementing
> > the missing WAs).
> 
> Sure you can disable FBC - just note that typically framebuffers will
> be
> unfenced.

So we need to investigate why the underruns are happening and implement
the missing workarounds. But IMHO while that's still not happening, tem
porarily reverting the patch seems to be the way to keep the codebase
sane.

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