[Intel-gfx] [PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

On Wed, Jul 17, 2013 at 6:00 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Wed, Jul 17, 2013 at 10:13 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> On Wed, Jul 17, 2013 at 02:45:29PM -0300, Rodrigo Vivi wrote:
>>> So if I understood correctly you suggest that if we have
>>> i915_powersave=0 and i915_enable_rc6=1 we should enable rc6?
>>> This is not how it is implemented nowadays on fbc right now... And
>>> this lead me to use pwoesave as an umbrella for force disable/enable
>>> ignoring individual parameters. But I'm open to do the other way
>>> around as long as we have a standardized behavior.

While you were writing my git send-email on v2 was failling because of
dam proxy...
But It's never late. And I think I agree with you... I just started
this discussion exactly because I got confused about this flag and
didn't like the way it was at fbc.

The only advantage that I see on this two levels is an easy way to
disable all powersaving features at once, mainly now it is increasing
the numbers...
instead of boot a kernel and manually put at grub:

i915.powersave=0 i915.i915_enable_fbc=0 i915.i915_enable_rc6=0
i915.enable_ips=0 i915.enable_psr=0 i915.enable_slice_shutdown=0

you just have to write
i915.powersave=0

>>
>> Just depends on whether powersave=0 is default and powersave=-1 is
>> force off, with powersave=1 as force on.
>>
>>> So, we can either respect individual parameters when they are set or
>>> completely ignore. In the way it is now it doesn't respect individual
>>> fbc parameter for powersave=0.

>>
>> My opinion is that we respect the specific module parameters, and if
>> they are left to default values, then apply the global powersave
>> parameter. If that too is default, then we apply the module default.
>
> Jumping in a bit late, but: I've honestly never understood why we have
> two levels of module options. Imo having individual knobs for each
> delicate feature makes more sense, strange dependencies in module
> option will only confuse dim-witted developers like me when looking at
> a bug ;-)
>
> So could we just reduce powersave to the few things that we haven't
> touched yet (iirc only DRRS)?

That is fine for me too... either add all features under this umbrella
or make it be only one feature like drrs that doesn't have its own
parameter...
the only cons I see in this case is the name of parameter that is too
generic...

But honestly I don't have a stronger position I just wanted to start
the discussion because I don't like the way it is today... So it is up
to you... I can either send v2 or a new simple patch that removes fbc
from this i915_powersave. Just let me know what is better...


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