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

[Rodrigo Vivi]

On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel at 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 at intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140918/0f270b0e/attachment.html>