CPUFreq addon in HAL

[Holger Macht]

On Wed 12. Jul - 11:47:26, David Zeuthen wrote:
> On Tue, 2006-07-11 at 15:29 +0200, Holger Macht wrote:
> > > Yea, probably we want a D-BUS interface on each device object
> > > representing an actual CPU which exports
> > 
> > I thought about this possibility, too, but came to the conclusion that it
> > doesn't make sense. There main reasons are:
> 
> <snip good reasons>
> 
> Yea, you're right.
> 
> > There's absolutely no need for setting a specific frequency for a CPU. You
> > just need fixed low, fixed high and a tuneable dynamic frequency scaling
> > mechanism.
> 
> Right. I do however expect a lot of users wants to set the frequency
> manually because they got that feature already from e.g. the CPU speed
> applet in GNOME. But I can live without this, it doesn't really make
> sense to me to set the frequency manually anyway.... What do you think?

Well, usually I'm the one arguing for having a possibility to export all
possible features to the user on the desktop ;-) But...

This doesn't really fit into the concept of this addon. It would be a
method that can only exclusively used if the userspace governor is
set. And that contradics the concept to use common interfaces for all
governors. And to be honest, we all hope that the userspace governor
wouldn't be needed anymore someday. However, it would be possible. It
would need another method SetCPUFreqFrequency and another property
'cpufreq_available_frequencies'.  But I really like to avoid that. This
would also be something which we could add afterwards if it turns out be
be _really_ needed.

> 
> <snip explanation of performance value from 1-100>
> 
> > I hope you now understand what it does. And I think the other mail
> > explains that the GUI can map this 1 to 100 range to whatever it likes.
> 
> Yeah. This makes a lot of sense. The policy daemon (for example
> gnome-power-manager) could also export an option to control the
> automatically, e.g. it could do do SetCPUFreqPerformance(1) if on
> battery and you have selected "prefer power savings over performance". 
> 
> Or it could do SetCPUFreqPerformance(batteryLeft) to scale down
> performance as your battery degrades. Or something. The UI and policy
> daemon details probably needs some thinking.

Yes, of course.

> 
> I like this interface, it makes a lot of sense to me.
> 
> > Yes, the addon just sits there and does nothing until someone sets a
> > governor through the SetCPUFreqGovernor interface. So they can coexist,
> > but but my goal actually is that cpuspeed, powernowd, don't forget
> > powersaved ;-) (at least the CPUFreq part of it), shouldn't be needed
> > anymore.
> 
> Sounds good, that was the answer I was looking for!
> 
> > > Maybe throw an exception on error? Exceptions are more flexible since
> > > the user won't have to remember what the integer means and you can grow
> > > this as needed (it's somewhat like an enum in C). The only exception I
> > > can think of here is org.freedesktop.Hal.Device.SystemPowerManagement.
> > > UnknownGovernor.
> > 
> > Yes, I will look into this.
> 
> Sounds good!

One question though. What do you exactly mean with "throw an
exception". Throw it over DBus? Something like

Return Value:
       int32 : boolean
       string: The exception

And the exception is something like
org.freedesktop.Hal.Device.SystemPowerManagement.UnknownGovernor or maybe even
org.freedesktop.Hal.Device.SystemPowerManagement.Success on success?

> 
> > Yes, we definitely need a possibility to forbid the use of this interface
> > for the usual user or in general. But I have to look into PolicyKit in the
> > first instance.
> > 
> > Same applies to SetCPUFreqPerformance and SetCPUFreqConsider Nice.
> 
> Cool.
> 
> > 
> > [...]
> > 
> > > > Get the current active governor
> > > > -------------------------------
> > > >   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
> > > >   Member   : "GetCPUFreqGovernor"
> > > >   Type     : Method call with return
> > > > 
> > > >   Return type:
> > > >     string : The current active governor
> > > 
> > > Perhaps this should be a hal property instead? Maybe not if it can
> > > change underneath us.
> > 
> > No, this can't change underneath. 
> 
> What if someone writes
> into  /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor ? Maybe our
> answer is just "don't do that", that would be somewhat fine with me...

When thinking about this from this standpoint, yes, it can change. I
always think that if you have a process that controls CPUFreq, noone
should change the settings manually, it's his own fault. I thought a
little bit more about this and a few problems came to my mind although I
at first thought we should have HAL properties. Especially when thinking
about the case that addon-cpufreq is not used to advantage another
daemon. However, one importance with HAL should be that the date
abstracted has to be correct and consistent IMHO. And this can't be
ensured in this case IMO. See below...

> 
> 
> > But again, I don't know which device
> > object to use for this. Would
> > org.freedesktop.Hal.Device.SystemPowerManagement be ok?
> 
> I think we already use power_management.* namespace for properties. We
> probably should have used system_power_management to match the D-BUS
> interface name. I suggest to use
> 
>  power_management.current_cpufreq_governor      (string)
>  power_management.current_cpufreq_performance   (uint64)
>  power_management.current_cpufreq_consider_nice (bool)

Same as above applies here. You don't always have these properties, for
instance when the powersave governor is set or again, if another CPUFreq
system is used. So I would have to dynamically remove and add properties
depending on the current governor or if the cpufreq addon is used at
all. I think that's overkill.

> 
> > > How about a function to get a list of available governors?
> > 
> > Kevin already proposed it and I can add it. But if the other settings are
> > exported as hal property, wouldn't it make sense to do the same for the
> > available governors?
> 
> Good point, so we'd have
> 
>  power_management.available_cpufreq_governors      (strlist)

When writing this mail, at first I thought that this property has the only
real good reasons to exist because this shouldn't change underneath us,
except if modules are loaded or removed. But there's another critical
point about this. Every distribution has to squeamishly pay attention to
that the cpufreq modules are loaded before HAL starts up. And if they're
loaded afterwards, boom, you don't have the correct information exported
by HAL.

As a conclusion, I think this all can be done as properties, but it's far
more clean and easier to have DBus method to get all this information.

> 
> (and of course feel free to choose better names than what I suggest
> above, I don't pretend they're any good :-)
> 
> > > Personally I think it makes sense to add. I'm unsure how this is handled
> > > on FreeBSD and Solaris? It looks to me as the interface is sufficiently
> > > abstract.
> > 
> > I hope so.
> 
> Cool. Perhaps Artem and Joe could let us know if they're fine with it?
> 
> > > > +		dbus_add_method(halctx, "SetCPUFreqGovernor", "s");
> > > > +		dbus_add_method(halctx, "SetCPUFreqPerformance", "i");
> > > > +		dbus_add_method(halctx, "SetCPUFreqConsiderNice", "b");
> > > > +		dbus_add_method(halctx, "GetCPUFreqGovernor", "");
> > > > +		dbus_add_method(halctx, "GetCPUFreqPerformance", "");
> > > > +		dbus_add_method(halctx, "GetCPUFreqConsiderNice", "");
> > > 
> > > This shouldn't be necessary.
> > 
> > But it is. As soon as I remove these calls, I don't see the methods in
> > lshal output and they also can't be used. But maybe I'm using
> > libhal_claim_interface in a wrong...
> 
> You're not supposed to see them in lshal output for using
> libhal_claim_interface. It appears to work for me using the Macbook Pro
> backlight addon... but this code hasn't seen much testing so it might be
> a bug...
> 

I will look at this addon again and will test again when doing the next,
hopefully final patch.

Regards,
	Holger