2014-09-18 15:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>:
>
>
> On Thu, Sep 18, 2014 at 4:39 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx>
>> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> > > ---
>> > > 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
