[PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Rodrigo Vivi]

If it wasn't never enabled by kernel parameter or platform default
we can avoid reading registers so many times in vain

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a988862..afcc284 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -339,6 +339,12 @@ bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* If it wasn't never enabled by kernel parameter or platform default
+	 * we can avoid reading registers so many times in vain
+	 */
+	if (!i915.enable_fbc)
+		return false;
+
 	if (!dev_priv->display.fbc_enabled)
 		return false;
 
-- 
1.9.3

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Chris Wilson]

On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
> If it wasn't never enabled by kernel parameter or platform default
> we can avoid reading registers so many times in vain

Nak.
 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a988862..afcc284 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -339,6 +339,12 @@ bool intel_fbc_enabled(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	/* If it wasn't never enabled by kernel parameter or platform default
> +	 * we can avoid reading registers so many times in vain
> +	 */
> +	if (!i915.enable_fbc)
> +		return false;
> +
>  	if (!dev_priv->display.fbc_enabled)
>  		return false;

The correct state to check here is whether we have enabled fbc, not the
module parameter which just rules whether we should turn it on.
Look at making dev_priv->fbc.no_fbc_reason always correct instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Daniel Vetter]

On Thu, Sep 18, 2014 at 07:28:48AM +0100, Chris Wilson wrote:
> On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
> > If it wasn't never enabled by kernel parameter or platform default
> > we can avoid reading registers so many times in vain
> 
> Nak.

Well I've merged this for now to reduce fbc impact.

> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a988862..afcc284 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -339,6 +339,12 @@ bool intel_fbc_enabled(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	/* If it wasn't never enabled by kernel parameter or platform default
> > +	 * we can avoid reading registers so many times in vain
> > +	 */
> > +	if (!i915.enable_fbc)
> > +		return false;
> > +
> >  	if (!dev_priv->display.fbc_enabled)
> >  		return false;
> 
> The correct state to check here is whether we have enabled fbc, not the
> module parameter which just rules whether we should turn it on.
> Look at making dev_priv->fbc.no_fbc_reason always correct instead.

Well we need to fix this all up anyway, since pretty much everything on
the software side of fbc is busted (locking, tracking, piles of rechecks
and other hilarious stuff).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2741 bytes --]

On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Sep 18, 2014 at 07:28:48AM +0100, Chris Wilson wrote:
> > On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
> > > If it wasn't never enabled by kernel parameter or platform default
> > > we can avoid reading registers so many times in vain
> >
> > Nak.
>
> Well I've merged this for now to reduce fbc impact.
>

Uhm, unfortunatelly I'm afraid Chris was right.
Paulo also nacked it. Because it just helps when it was explicitly disabled
by setting i915.enable_fbc=0 while the default is -1.

I though about returning on <= 0, but Paulo is afraid that when enabling
back for some platform people would forget to fix this part here and I
agree.


>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> > > index a988862..afcc284 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -339,6 +339,12 @@ bool intel_fbc_enabled(struct drm_device *dev)
> > >  {
> > >     struct drm_i915_private *dev_priv = dev->dev_private;
> > >
> > > +   /* If it wasn't never enabled by kernel parameter or platform
> default
> > > +    * we can avoid reading registers so many times in vain
> > > +    */
> > > +   if (!i915.enable_fbc)
> > > +           return false;
> > > +
> > >     if (!dev_priv->display.fbc_enabled)
> > >             return false;
> >
> > The correct state to check here is whether we have enabled fbc, not the
> > module parameter which just rules whether we should turn it on.
> > Look at making dev_priv->fbc.no_fbc_reason always correct instead.
>
> Well we need to fix this all up anyway, since pretty much everything on
> the software side of fbc is busted (locking, tracking, piles of rechecks
> and other hilarious stuff).
>

I absolutely agree with you Daniel. We need a full fbc rework and
simplification.

But for now we need a quick stuff to protect the current code.

Maybe an extra variable dev_priv.fbc_ever_enabled... This is ugly enough
that someone wokirng to get fbc enabled by default would never forget as he
can forget about i915.enable_fbc <= 0 at some random point of the code.




> -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
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 4452 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Paulo Zanoni]

2014-09-18 15:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
>
>
> On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, Sep 18, 2014 at 07:28:48AM +0100, Chris Wilson wrote:
>> > On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
>> > > If it wasn't never enabled by kernel parameter or platform default
>> > > we can avoid reading registers so many times in vain
>> >
>> > Nak.
>>
>> Well I've merged this for now to reduce fbc impact.
>
>
> Uhm, unfortunatelly I'm afraid Chris was right.
> Paulo also nacked it. Because it just helps when it was explicitly disabled
> by setting i915.enable_fbc=0 while the default is -1.
>
> I though about returning on <= 0, but Paulo is afraid that when enabling
> back for some platform people would forget to fix this part here and I
> agree.
>
>>
>>
>> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>> > >  1 file changed, 6 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> > > b/drivers/gpu/drm/i915/intel_pm.c
>> > > index a988862..afcc284 100644
>> > > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > > @@ -339,6 +339,12 @@ bool intel_fbc_enabled(struct drm_device *dev)
>> > >  {
>> > >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > >
>> > > +   /* If it wasn't never enabled by kernel parameter or platform
>> > > default
>> > > +    * we can avoid reading registers so many times in vain
>> > > +    */
>> > > +   if (!i915.enable_fbc)
>> > > +           return false;
>> > > +
>> > >     if (!dev_priv->display.fbc_enabled)
>> > >             return false;
>> >
>> > The correct state to check here is whether we have enabled fbc, not the
>> > module parameter which just rules whether we should turn it on.
>> > Look at making dev_priv->fbc.no_fbc_reason always correct instead.
>>
>> Well we need to fix this all up anyway, since pretty much everything on
>> the software side of fbc is busted (locking, tracking, piles of rechecks
>> and other hilarious stuff).
>
>
> I absolutely agree with you Daniel. We need a full fbc rework and
> simplification.
>
> But for now we need a quick stuff to protect the current code.
>
> Maybe an extra variable dev_priv.fbc_ever_enabled... This is ugly enough
> that someone wokirng to get fbc enabled by default would never forget as he
> can forget about i915.enable_fbc <= 0 at some random point of the code.

I just spotted another problem with this patch. If i915.enable_fbc is
zero but FBC is actually enabled in HW, we won't end up disabling it.
This case can be true if the user is playing with
/sys/module/i915/parameters/enable_fbc.

That said, if we change i915.enable_fbc so that it _can't_ be changed
at runtime, we'd probably be able to simplify a few things.

>
>
>
>>
>> -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
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Daniel Vetter]

On Thu, Sep 18, 2014 at 11:42:44AM -0700, Rodrigo Vivi wrote:
> On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Thu, Sep 18, 2014 at 07:28:48AM +0100, Chris Wilson wrote:
> > > On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
> > > > If it wasn't never enabled by kernel parameter or platform default
> > > > we can avoid reading registers so many times in vain
> > >
> > > Nak.
> >
> > Well I've merged this for now to reduce fbc impact.
> >
> 
> Uhm, unfortunatelly I'm afraid Chris was right.
> Paulo also nacked it. Because it just helps when it was explicitly disabled
> by setting i915.enable_fbc=0 while the default is -1.
> 
> I though about returning on <= 0, but Paulo is afraid that when enabling
> back for some platform people would forget to fix this part here and I
> agree.

Well I guess I should have read mails before pushing out a new -next ;-)
So this is now baked in. Should I revert or can we just fix up on top?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 1639 bytes --]

On Fri, Sep 19, 2014 at 8:40 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Sep 18, 2014 at 11:42:44AM -0700, Rodrigo Vivi wrote:
> > On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > > On Thu, Sep 18, 2014 at 07:28:48AM +0100, Chris Wilson wrote:
> > > > On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
> > > > > If it wasn't never enabled by kernel parameter or platform default
> > > > > we can avoid reading registers so many times in vain
> > > >
> > > > Nak.
> > >
> > > Well I've merged this for now to reduce fbc impact.
> > >
> >
> > Uhm, unfortunatelly I'm afraid Chris was right.
> > Paulo also nacked it. Because it just helps when it was explicitly
> disabled
> > by setting i915.enable_fbc=0 while the default is -1.
> >
> > I though about returning on <= 0, but Paulo is afraid that when enabling
> > back for some platform people would forget to fix this part here and I
> > agree.
>
> Well I guess I should have read mails before pushing out a new -next ;-)
> So this is now baked in. Should I revert or can we just fix up on top?
>

No problem, I can fix this on top.

But what do you prefer:

1. <=0 return and changing parameter permission from 600 to 400
2. dev_priv->fbc_enabled
3. or check if fbc_no_reason was ever set? Although I don't like the
fbc_no_reason and would like to clean it up in the future. But anyway, any
option here is temporary until a proper rework for cleanup and real fix.


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 2725 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Avoid reading fbc registers in vain when fbc was never enabled.

[Paulo Zanoni]

2014-09-19 15:24 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
>
>
> On Fri, Sep 19, 2014 at 8:40 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Thu, Sep 18, 2014 at 11:42:44AM -0700, Rodrigo Vivi wrote:
>> > On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >
>> > > On Thu, Sep 18, 2014 at 07:28:48AM +0100, Chris Wilson wrote:
>> > > > On Wed, Sep 17, 2014 at 04:59:20PM -0400, Rodrigo Vivi wrote:
>> > > > > If it wasn't never enabled by kernel parameter or platform default
>> > > > > we can avoid reading registers so many times in vain
>> > > >
>> > > > Nak.
>> > >
>> > > Well I've merged this for now to reduce fbc impact.
>> > >
>> >
>> > Uhm, unfortunatelly I'm afraid Chris was right.
>> > Paulo also nacked it. Because it just helps when it was explicitly
>> > disabled
>> > by setting i915.enable_fbc=0 while the default is -1.
>> >
>> > I though about returning on <= 0, but Paulo is afraid that when enabling
>> > back for some platform people would forget to fix this part here and I
>> > agree.
>>
>> Well I guess I should have read mails before pushing out a new -next ;-)
>> So this is now baked in. Should I revert or can we just fix up on top?
>
>
> No problem, I can fix this on top.
>
> But what do you prefer:
>
> 1. <=0 return and changing parameter permission from 600 to 400

If i915.enable_fbc is -1, you don't know whether it's enabled or not.

> 2. dev_priv->fbc_enabled

I was just quickly tried to write this patch. It fixes the runtime PM
regression I was seeing...

> 3. or check if fbc_no_reason was ever set? Although I don't like the
> fbc_no_reason and would like to clean it up in the future. But anyway, any
> option here is temporary until a proper rework for cleanup and real fix.

I also tried the fbc_no_reason path but gave up. When no_fbc_reason is
FBC_OK, there's no guarantee that FBC is actually enabled: it just
mean that we may be enabling/disabling it as part of the normal FBC
operation. So to do what we want, we'd have to twist the meaning of
no_fbc_reason and add values like FBC_OK_AND_ENABLED and
FBC_OK_AND_DISABLED, and I don't think that's a cool solution.

I'll send the patch for item 2 in a few minutes.

>
>>
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Paulo Zanoni