Re: [Intel-gfx] [PATCH] drm/i915/fbc: Assume maximum 8mb of stolen is used for gen8+

[Paulo Zanoni]

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+

[David Weinehall]

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+

[Vivi, Rodrigo]

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+

[David Weinehall]

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+

[Rodrigo Vivi]

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 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..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+

[Rodrigo Vivi]

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.
 * 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