Re: [Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+
Em Qua, 2016-10-26 às 11:22 -0700, Rodrigo Vivi escreveu: > Since Broxton has same FBC block as BDW+ let's assume it also > don't have access to the stolen usable range. > > FBC is currently not saving power on Broxton and I believe > the compression threshold is limited to 1x. NAK. What is said above has nothing to do with what the code below does. The amount of stolen memory is decided by i915_gem_init_stolen() and its friends, not by the FBC code. What this patch does is that it potentially reduces the amount of usable stolen memory available to FBC by up to 8mb. That's totally not going to help making FBC save more power. It can only do the opposite. Also, the 1x compression threshold is the best thing. The (unpatched) code below implements the restriction documented in FBC_CFB_BASE, and it's not applicable to BXT (the spec explicitly says that). Also, we have a history of people who try to measure how much power FBC saves, but they forget to do things such as properly setting the SATA link power management or other stuff, leaving the system stuck in PC3 state or worse, and then they toggle FBC and conclude that FBC does not save power. Power testing should really be done by the power management team. Due to the past situations we had, I'd like to see data on the measurements done before I can believe that the problem is really FBC. > > Cc: Paulo Zanoni> Cc: Marc Herbert > Signed-off-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/intel_fbc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > b/drivers/gpu/drm/i915/intel_fbc.c > index cbe2ebd..640db67 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -530,12 +530,11 @@ static int find_compression_threshold(struct > drm_i915_private *dev_priv, > int ret; > u64 end; > > - /* The FBC hardware for BDW/SKL doesn't have access to the > stolen > + /* The FBC hardware for gen8+ doesn't have access to the > stolen > * reserved range size, so it always assumes the maximum > (8mb) is used. > * If we enable FBC using a CFB on that memory range we'll > get FIFO > * underruns, even if that range is not reserved by the > BIOS. */ > - if (IS_BROADWELL(dev_priv) || > - IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) > + if (INTEL_INFO(dev_priv)->gen <= 8) > end = ggtt->stolen_size - 8 * 1024 * 1024; > else > end = ggtt->stolen_usable_size; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+
On Wed, Oct 26, 2016 at 09:18:59PM +, Vivi, Rodrigo wrote: > On Wed, 2016-10-26 at 22:45 +0200, David Weinehall wrote: > > On Wed, Oct 26, 2016 at 11:22:00AM -0700, Rodrigo Vivi wrote: > > > Since Broxton has same FBC block as BDW+ let's assume it also > > > don't have access to the stolen usable range. > > > > > > FBC is currently not saving power on Broxton and I believe > > > the compression threshold is limited to 1x. > > > > > > Cc: Paulo Zanoni> > > Cc: Marc Herbert > > > Signed-off-by: Rodrigo Vivi > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index cbe2ebd..640db67 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -530,12 +530,11 @@ static int find_compression_threshold(struct > > > drm_i915_private *dev_priv, > > > int ret; > > > u64 end; > > > > > > - /* The FBC hardware for BDW/SKL doesn't have access to the stolen > > > + /* The FBC hardware for gen8+ doesn't have access to the stolen > > >* reserved range size, so it always assumes the maximum (8mb) is used. > > > > Might I suggest correcting the comment to say megabyte instead of > > millibit while at it? > > you mean s/8mb/8MB right? I can change it in a v3 for sure. Indeed. > > > > >* If we enable FBC using a CFB on that memory range we'll get FIFO > > >* underruns, even if that range is not reserved by the BIOS. */ > > > - if (IS_BROADWELL(dev_priv) || > > > - IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) > > > + if (INTEL_INFO(dev_priv)->gen <= 8) > > > > INTEL_GEN(dev_priv) > > yeap, good point. will change that later . > > Although for Marc the old style has more chance to get applied on his > tree for now. Fair enough. > Let's wait for his confirmation before the v3 anyway. > > > > > Also, shouldn't this be >= 8? Also, > > duh! sorry! > Marc already pointed me this mistake... v2 was sent already. Yeah, I noticed just after I pressed send :) > > remember that gen8 also includes > > CherryView -- is the behaviour the same there? > > chv doesn't have fbc so I hope it never reaches this path... Goodie. Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+
On Wed, 2016-10-26 at 22:45 +0200, David Weinehall wrote: > On Wed, Oct 26, 2016 at 11:22:00AM -0700, Rodrigo Vivi wrote: > > Since Broxton has same FBC block as BDW+ let's assume it also > > don't have access to the stolen usable range. > > > > FBC is currently not saving power on Broxton and I believe > > the compression threshold is limited to 1x. > > > > Cc: Paulo Zanoni> > Cc: Marc Herbert > > Signed-off-by: Rodrigo Vivi > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > b/drivers/gpu/drm/i915/intel_fbc.c > > index cbe2ebd..640db67 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -530,12 +530,11 @@ static int find_compression_threshold(struct > > drm_i915_private *dev_priv, > > int ret; > > u64 end; > > > > - /* The FBC hardware for BDW/SKL doesn't have access to the stolen > > + /* The FBC hardware for gen8+ doesn't have access to the stolen > > * reserved range size, so it always assumes the maximum (8mb) is used. > > Might I suggest correcting the comment to say megabyte instead of > millibit while at it? you mean s/8mb/8MB right? I can change it in a v3 for sure. > > > * If we enable FBC using a CFB on that memory range we'll get FIFO > > * underruns, even if that range is not reserved by the BIOS. */ > > - if (IS_BROADWELL(dev_priv) || > > - IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) > > + if (INTEL_INFO(dev_priv)->gen <= 8) > > INTEL_GEN(dev_priv) yeap, good point. will change that later . Although for Marc the old style has more chance to get applied on his tree for now. Let's wait for his confirmation before the v3 anyway. > > Also, shouldn't this be >= 8? Also, duh! sorry! Marc already pointed me this mistake... v2 was sent already. > remember that gen8 also includes > CherryView -- is the behaviour the same there? chv doesn't have fbc so I hope it never reaches this path... > > > end = ggtt->stolen_size - 8 * 1024 * 1024; > > else > > end = ggtt->stolen_usable_size; > > -- > > 1.9.1 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+
On Wed, Oct 26, 2016 at 11:22:00AM -0700, Rodrigo Vivi wrote: > Since Broxton has same FBC block as BDW+ let's assume it also > don't have access to the stolen usable range. > > FBC is currently not saving power on Broxton and I believe > the compression threshold is limited to 1x. > > Cc: Paulo Zanoni> Cc: Marc Herbert > Signed-off-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/intel_fbc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > b/drivers/gpu/drm/i915/intel_fbc.c > index cbe2ebd..640db67 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -530,12 +530,11 @@ static int find_compression_threshold(struct > drm_i915_private *dev_priv, > int ret; > u64 end; > > - /* The FBC hardware for BDW/SKL doesn't have access to the stolen > + /* The FBC hardware for gen8+ doesn't have access to the stolen >* reserved range size, so it always assumes the maximum (8mb) is used. Might I suggest correcting the comment to say megabyte instead of millibit while at it? >* If we enable FBC using a CFB on that memory range we'll get FIFO >* underruns, even if that range is not reserved by the BIOS. */ > - if (IS_BROADWELL(dev_priv) || > - IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) > + if (INTEL_INFO(dev_priv)->gen <= 8) INTEL_GEN(dev_priv) Also, shouldn't this be >= 8? Also, remember that gen8 also includes CherryView -- is the behaviour the same there? > end = ggtt->stolen_size - 8 * 1024 * 1024; > else > end = ggtt->stolen_usable_size; > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+
Since Broxton has same FBC block as BDW+ let's assume it also don't have access to the stolen usable range. FBC is currently not saving power on Broxton and I believe the compression threshold is limited to 1x. v2: duh! inverted check. (Thanks Marc) Cc: Paulo ZanoniCc: Marc Herbert Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_fbc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index cbe2ebd..214b2de 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -530,12 +530,11 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv, int ret; u64 end; - /* The FBC hardware for BDW/SKL doesn't have access to the stolen + /* The FBC hardware for gen8+ doesn't have access to the stolen * reserved range size, so it always assumes the maximum (8mb) is used. * If we enable FBC using a CFB on that memory range we'll get FIFO * underruns, even if that range is not reserved by the BIOS. */ - if (IS_BROADWELL(dev_priv) || - IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) + if (INTEL_INFO(dev_priv)->gen >= 8) end = ggtt->stolen_size - 8 * 1024 * 1024; else end = ggtt->stolen_usable_size; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+
Since Broxton has same FBC block as BDW+ let's assume it also don't have access to the stolen usable range. FBC is currently not saving power on Broxton and I believe the compression threshold is limited to 1x. Cc: Paulo ZanoniCc: Marc Herbert Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_fbc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index cbe2ebd..640db67 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -530,12 +530,11 @@ static int find_compression_threshold(struct drm_i915_private *dev_priv, int ret; u64 end; - /* The FBC hardware for BDW/SKL doesn't have access to the stolen + /* The FBC hardware for gen8+ doesn't have access to the stolen * reserved range size, so it always assumes the maximum (8mb) is used. * If we enable FBC using a CFB on that memory range we'll get FIFO * underruns, even if that range is not reserved by the BIOS. */ - if (IS_BROADWELL(dev_priv) || - IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) + if (INTEL_INFO(dev_priv)->gen <= 8) end = ggtt->stolen_size - 8 * 1024 * 1024; else end = ggtt->stolen_usable_size; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx