Re: [Intel-gfx] [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 :
> > > 
> > > 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 
> > > Cc: Daniel Vetter 
> > > Cc: "Zanoni, Paulo R" 
> > 
> > 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

Re: [Intel-gfx] [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  wrote:
> On Mon, Aug 22, 2016 at 09:39:17PM -0300, Paulo Zanoni wrote:
>> 2016-08-18 5:21 GMT-03:00 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 
>> > Cc: Daniel Vetter 
>> > Cc: "Zanoni, Paulo R" 
>>
>> 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: [Intel-gfx] [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 :
> > 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 
> > Cc: Daniel Vetter 
> > Cc: "Zanoni, Paulo R" 
> 
> 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: [Intel-gfx] [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 :
> > 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 
> > Cc: Daniel Vetter 
> > Cc: "Zanoni, Paulo R" 
> 
> 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: [Intel-gfx] [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 
> Cc: Daniel Vetter 
> Cc: "Zanoni, Paulo R" 

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: [Intel-gfx] [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 :
> 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 
> Cc: Daniel Vetter 
> Cc: "Zanoni, Paulo R" 

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: [Intel-gfx] [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 
> Cc: Daniel Vetter 
> Cc: "Zanoni, Paulo R" 

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 

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: [Intel-gfx] [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen

[ch...@chris-wilson.co.uk]

On Thu, Aug 18, 2016 at 01:56:56PM +, 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: [Intel-gfx] [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 
> Cc: Daniel Vetter 
> Cc: "Zanoni, Paulo R" 
> ---
>  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