* [PATCH] drm/i915: Enable FBC on GEN7 by default @ 2014-03-25 1:21 Ben Widawsky 2014-03-25 1:41 ` Stéphane Marchesin 2014-03-25 7:27 ` [PATCH] " Chris Wilson 0 siblings, 2 replies; 9+ messages in thread From: Ben Widawsky @ 2014-03-25 1:21 UTC (permalink / raw) To: Intel GFX 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-25 1:21 [PATCH] drm/i915: Enable FBC on GEN7 by default Ben Widawsky @ 2014-03-25 1:41 ` Stéphane Marchesin 2014-03-25 1:46 ` Ben Widawsky 2014-03-25 7:27 ` [PATCH] " Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: Stéphane Marchesin @ 2014-03-25 1:41 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-25 1:41 ` Stéphane Marchesin @ 2014-03-25 1:46 ` Ben Widawsky 2014-03-25 1:50 ` Stéphane Marchesin 0 siblings, 1 reply; 9+ messages in thread From: Ben Widawsky @ 2014-03-25 1:46 UTC (permalink / raw) To: Stéphane Marchesin; +Cc: Ben Widawsky, Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-25 1:46 ` Ben Widawsky @ 2014-03-25 1:50 ` Stéphane Marchesin 2014-03-25 2:13 ` [PATCH] [v2] " Ben Widawsky 0 siblings, 1 reply; 9+ messages in thread From: Stéphane Marchesin @ 2014-03-25 1:50 UTC (permalink / raw) To: Ben Widawsky; +Cc: Ben Widawsky, Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] [v2] drm/i915: Enable FBC on GEN7 by default 2014-03-25 1:50 ` Stéphane Marchesin @ 2014-03-25 2:13 ` Ben Widawsky 0 siblings, 0 replies; 9+ messages in thread From: Ben Widawsky @ 2014-03-25 2:13 UTC (permalink / raw) To: Intel GFX 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-25 1:21 [PATCH] drm/i915: Enable FBC on GEN7 by default Ben Widawsky 2014-03-25 1:41 ` Stéphane Marchesin @ 2014-03-25 7:27 ` Chris Wilson 2014-03-25 8:08 ` Daniel Vetter 2014-03-26 2:15 ` Stéphane Marchesin 1 sibling, 2 replies; 9+ messages in thread From: Chris Wilson @ 2014-03-25 7:27 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-25 7:27 ` [PATCH] " Chris Wilson @ 2014-03-25 8:08 ` Daniel Vetter 2014-03-26 2:15 ` Stéphane Marchesin 1 sibling, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2014-03-25 8:08 UTC (permalink / raw) To: Chris Wilson, Ben Widawsky, Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-25 7:27 ` [PATCH] " Chris Wilson 2014-03-25 8:08 ` Daniel Vetter @ 2014-03-26 2:15 ` Stéphane Marchesin 2014-03-26 7:28 ` Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: Stéphane Marchesin @ 2014-03-26 2:15 UTC (permalink / raw) To: Chris Wilson, Ben Widawsky, Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Enable FBC on GEN7 by default 2014-03-26 2:15 ` Stéphane Marchesin @ 2014-03-26 7:28 ` Chris Wilson 0 siblings, 0 replies; 9+ messages in thread From: Chris Wilson @ 2014-03-26 7:28 UTC (permalink / raw) To: Stéphane Marchesin; +Cc: Ben Widawsky, Intel GFX 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-03-26 7:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-25 1:21 [PATCH] drm/i915: Enable FBC on GEN7 by default Ben Widawsky 2014-03-25 1:41 ` Stéphane Marchesin 2014-03-25 1:46 ` Ben Widawsky 2014-03-25 1:50 ` Stéphane Marchesin 2014-03-25 2:13 ` [PATCH] [v2] " Ben Widawsky 2014-03-25 7:27 ` [PATCH] " Chris Wilson 2014-03-25 8:08 ` Daniel Vetter 2014-03-26 2:15 ` Stéphane Marchesin 2014-03-26 7:28 ` Chris Wilson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox