[PATCH] drm/i915: Enable FBC on GEN7 by default

[PATCH] drm/i915: Enable FBC on GEN7 by default

[Ben Widawsky]

I am not clear why we've never enabled it by default for GEN7. Looking
at the git hostiry, it seems Rodrigo disabled it by default, and it's
never been turned on. Quite a few fixes have gone in over the past year,
and I think many of us are running this successfully.

If there is some reason we know of why we don't enable this by default
on GEN7, then please ignore the patch, and forgive my laziness.

Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a7da962..a9890df 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev)
 	adjusted_mode = &intel_crtc->config.adjusted_mode;
 
 	if (i915.enable_fbc < 0 &&
-	    INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev)) {
+	    INTEL_INFO(dev)->gen <= 6) {
 		if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
 			DRM_DEBUG_KMS("disabled per chip default\n");
 		goto out_disable;
-- 
1.9.1

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Stéphane Marchesin]

On Mon, Mar 24, 2014 at 6:21 PM, Ben Widawsky
<benjamin.widawsky@linux.intel.com> wrote:
> I am not clear why we've never enabled it by default for GEN7. Looking
> at the git hostiry, it seems Rodrigo disabled it by default, and it's
> never been turned on. Quite a few fixes have gone in over the past year,
> and I think many of us are running this successfully.

Did you try with a high resolution panel? I could never get IVB FBC to
be completely stable with a 2560x1700 panel...

Stéphane

>
> If there is some reason we know of why we don't enable this by default
> on GEN7, then please ignore the patch, and forgive my laziness.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a7da962..a9890df 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev)
>         adjusted_mode = &intel_crtc->config.adjusted_mode;
>
>         if (i915.enable_fbc < 0 &&
> -           INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev)) {
> +           INTEL_INFO(dev)->gen <= 6) {
>                 if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
>                         DRM_DEBUG_KMS("disabled per chip default\n");
>                 goto out_disable;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Ben Widawsky]

On Mon, Mar 24, 2014 at 06:41:47PM -0700, Stéphane Marchesin wrote:
> On Mon, Mar 24, 2014 at 6:21 PM, Ben Widawsky
> <benjamin.widawsky@linux.intel.com> wrote:
> > I am not clear why we've never enabled it by default for GEN7. Looking
> > at the git hostiry, it seems Rodrigo disabled it by default, and it's
> > never been turned on. Quite a few fixes have gone in over the past year,
> > and I think many of us are running this successfully.
> 
> Did you try with a high resolution panel? I could never get IVB FBC to
> be completely stable with a 2560x1700 panel...
> 
> Stéphane

1600x900 is working fine. At my current laziness, that's the best I can
test. I can try on a higher resolution tomorrow.  Perhaps the solution
would be to disable if the resolution goes above a certain point.

Honestly, I assumed there was a reason it was disabled, I just couldn't
find it.

> 
> >
> > If there is some reason we know of why we don't enable this by default
> > on GEN7, then please ignore the patch, and forgive my laziness.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a7da962..a9890df 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev)
> >         adjusted_mode = &intel_crtc->config.adjusted_mode;
> >
> >         if (i915.enable_fbc < 0 &&
> > -           INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev)) {
> > +           INTEL_INFO(dev)->gen <= 6) {
> >                 if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
> >                         DRM_DEBUG_KMS("disabled per chip default\n");
> >                 goto out_disable;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Stéphane Marchesin]

On Mon, Mar 24, 2014 at 6:46 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, Mar 24, 2014 at 06:41:47PM -0700, Stéphane Marchesin wrote:
>> On Mon, Mar 24, 2014 at 6:21 PM, Ben Widawsky
>> <benjamin.widawsky@linux.intel.com> wrote:
>> > I am not clear why we've never enabled it by default for GEN7. Looking
>> > at the git hostiry, it seems Rodrigo disabled it by default, and it's
>> > never been turned on. Quite a few fixes have gone in over the past year,
>> > and I think many of us are running this successfully.
>>
>> Did you try with a high resolution panel? I could never get IVB FBC to
>> be completely stable with a 2560x1700 panel...
>>
>> Stéphane
>
> 1600x900 is working fine. At my current laziness, that's the best I can
> test. I can try on a higher resolution tomorrow.  Perhaps the solution
> would be to disable if the resolution goes above a certain point.
>
> Honestly, I assumed there was a reason it was disabled, I just couldn't
> find it.

For what it's worth, it's fine for me up to 2k wide. Things seem to go
bad above that.

Stéphane

>
>>
>> >
>> > If there is some reason we know of why we don't enable this by default
>> > on GEN7, then please ignore the patch, and forgive my laziness.
>> >
>> > Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> > ---
>> >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > index a7da962..a9890df 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev)
>> >         adjusted_mode = &intel_crtc->config.adjusted_mode;
>> >
>> >         if (i915.enable_fbc < 0 &&
>> > -           INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev)) {
>> > +           INTEL_INFO(dev)->gen <= 6) {
>> >                 if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
>> >                         DRM_DEBUG_KMS("disabled per chip default\n");
>> >                 goto out_disable;
>> > --
>> > 1.9.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] [v2] drm/i915: Enable FBC on GEN7 by default

[Ben Widawsky]

I am not clear why we've never enabled it by default for GEN7. Looking
at the git hostiry, it seems Rodrigo disabled it by default, and it's
never been turned on. Quite a few fixes have gone in over the past year,
and I think many of us are running this successfully.

If there is some reason we know of why we don't enable this by default
on GEN7, then please ignore the patch, and forgive my laziness.

v2: Disable FBC if width is greater than 2k, and user doesn't override.  This
is due to issues seen by Stéphane, and possibly others. I wasn't sure what to
do with max_height.  v2 was only compile tested.

Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a7da962..2529cb8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev)
 	adjusted_mode = &intel_crtc->config.adjusted_mode;
 
 	if (i915.enable_fbc < 0 &&
-	    INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev)) {
+	    INTEL_INFO(dev)->gen <= 6) {
 		if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
 			DRM_DEBUG_KMS("disabled per chip default\n");
 		goto out_disable;
@@ -530,7 +530,14 @@ void intel_update_fbc(struct drm_device *dev)
 		goto out_disable;
 	}
 
-	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
+	/* If the user hasn't overridden the defaults, try to get them working
+	 * FBC on GEN7. If they have overridden the defaults, then assume they
+	 * know what they're doing and allow foot shooting.
+	 */
+	if (i915.enable_fbc < 0 && IS_GEN7(dev) && !IS_HASWELL(dev)) {
+		max_width = 2048;
+		max_height = -1;
+	} else if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
 		max_width = 4096;
 		max_height = 2048;
 	} else {
-- 
1.9.1

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

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Chris Wilson]

On Mon, Mar 24, 2014 at 06:21:22PM -0700, Ben Widawsky wrote:
> I am not clear why we've never enabled it by default for GEN7. Looking
> at the git hostiry, it seems Rodrigo disabled it by default, and it's
> never been turned on. Quite a few fixes have gone in over the past year,
> and I think many of us are running this successfully.
> 
> If there is some reason we know of why we don't enable this by default
> on GEN7, then please ignore the patch, and forgive my laziness.

Other than the major performance degredation due to our implementation,
and that there is a known deadlock (when unplugging/plugging in external
displays) due to the broken locking... It should not have been enabled.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Daniel Vetter]

On Tue, Mar 25, 2014 at 8:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Mar 24, 2014 at 06:21:22PM -0700, Ben Widawsky wrote:
>> I am not clear why we've never enabled it by default for GEN7. Looking
>> at the git hostiry, it seems Rodrigo disabled it by default, and it's
>> never been turned on. Quite a few fixes have gone in over the past year,
>> and I think many of us are running this successfully.
>>
>> If there is some reason we know of why we don't enable this by default
>> on GEN7, then please ignore the patch, and forgive my laziness.
>
> Other than the major performance degredation due to our implementation,
> and that there is a known deadlock (when unplugging/plugging in external
> displays) due to the broken locking... It should not have been enabled.

Also, have you run the full kms_fbc_crc testsuite to make sure it's
actually functionally correct? Iirc we even fail at that stage still
on some platforms ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Stéphane Marchesin]

On Tue, Mar 25, 2014 at 12:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Mar 24, 2014 at 06:21:22PM -0700, Ben Widawsky wrote:
>> I am not clear why we've never enabled it by default for GEN7. Looking
>> at the git hostiry, it seems Rodrigo disabled it by default, and it's
>> never been turned on. Quite a few fixes have gone in over the past year,
>> and I think many of us are running this successfully.
>>
>> If there is some reason we know of why we don't enable this by default
>> on GEN7, then please ignore the patch, and forgive my laziness.
>
> Other than the major performance degredation due to our implementation,

That sounds interesting, can you elaborate on the performance
degradation? I haven't noticed any, but of course I don't know how
it's supposed to behave...

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

Re: [PATCH] drm/i915: Enable FBC on GEN7 by default

[Chris Wilson]

On Tue, Mar 25, 2014 at 07:15:42PM -0700, Stéphane Marchesin wrote:
> On Tue, Mar 25, 2014 at 12:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Mar 24, 2014 at 06:21:22PM -0700, Ben Widawsky wrote:
> >> I am not clear why we've never enabled it by default for GEN7. Looking
> >> at the git hostiry, it seems Rodrigo disabled it by default, and it's
> >> never been turned on. Quite a few fixes have gone in over the past year,
> >> and I think many of us are running this successfully.
> >>
> >> If there is some reason we know of why we don't enable this by default
> >> on GEN7, then please ignore the patch, and forgive my laziness.
> >
> > Other than the major performance degredation due to our implementation,
> 
> That sounds interesting, can you elaborate on the performance
> degradation? I haven't noticed any, but of course I don't know how
> it's supposed to behave...

The way we setup the FBC is that it causes it to be invalidated after
every operation (not just batch, or upon flushing the framebuffer). The
impact of this is that lightweight 3D rendering operations such as firefox
are about 60% slower, game impact though is less than 10% (more often in the
noise), and all but the most GPU bound of BLT operations are orders of
magnitude slower. All of this is mitigable by disabling FBC invalidations
except when we write to the scanout.

https://bugs.freedesktop.org/show_bug.cgi?id=72023
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre