At present, the backlight class presents two attributes to sysfs,
brightness and power. I'm a little confused as to whether these
attributes are currently doing the right things.
Taking brightness, at any one time we have several different brightness
values:
* User requested brightness (echo y > /sys/class/backlight/xxx/brightness)
* Driver determined brightness which accounts for things like FB
blanking, low battery backlight limiting (an example from corgi_bl),
user requested power state, device suspend/resume.
The solution might be to have brightness always return the user
requested value y and have a new attribute returning the brightness as
determined by the driver once it accounts for all the factors it needs
to consider. Naming of such an attribute is tricky - "driver_brightness"
perthaps?
The same problem applies to the power attribute. This could easily
confused with the other forms of power attribute in sysfs but ignoring
that, should this report:
* The last user requested power state
* The current power state accounting for FB blanking.
* The current power state for device suspend/resume
Should the FB blanking override the user requested values? At the moment
it only does unless a user changes an attributes whilst the display is
blanked in which case the user's change overrides. This could be classed
as a bug but solving it as straight forward as it sounds using only the
existing backlight class functions.
Also, at the moment this attribute reports VESA power states which can
be confusing (0 = on, [1-3] = off).
Again, the solution would appear to be to return the last user requested
power state. The driver_brightness attribute would tell you if the
display was blanked for any other reason (although not why).
I have various patches that implement these changes but before I finish
them, does anyone have an views on what the correct behaviour should be?
Richard
On Sun, 05 Mar 2006 15:08:53 +0000
Richard Purdie <[email protected]> wrote:
> The solution might be to have brightness always return the user
> requested value y and have a new attribute returning the brightness as
> determined by the driver once it accounts for all the factors it needs
> to consider. Naming of such an attribute is tricky -
> "driver_brightness" perthaps?
Maybe by analogy to sound card there can be a 'master' control, and one
'user' control. But it's not clear how many of these strings you will
need for every 'real' attribute. Why two and not three? Why three and
not ten?
But I don't think that's a kernel-side problem. I think it would be
better to have some daemon controlling these attributes (and keeping
track of as many factors as needed) and programming the final, 'true'
kernel value. Then a set of user-mode tools could be provided to alter
them, and a simple API.
> The same problem applies to the power attribute. This could easily
> confused with the other forms of power attribute in sysfs but ignoring
> that, should this report:
>
> * The last user requested power state
> * The current power state accounting for FB blanking.
> * The current power state for device suspend/resume
Well with power it's simple: LCD is either powered or it's not. After
resuming the LCD should be always powered on, and the program can then
turn it back off if it's desired. FB blanking isn't a issue with X11/
Qtopia, as far as I understand? And finally, the 'user' requested power
state has to be tracked by the program that does the blanking (say an
audio player or such).
> Should the FB blanking override the user requested values? At the
> moment it only does unless a user changes an attributes whilst the
> display is blanked in which case the user's change overrides. This
> could be classed as a bug but solving it as straight forward as it
> sounds using only the existing backlight class functions.
Before a program changes the attribute it must check the attribute. If
it is not 0, it means the display is blanked for some reason, so the
program should not change it, unless it really wants to power up the
LCD.
> Also, at the moment this attribute reports VESA power states which can
> be confusing (0 = on, [1-3] = off).
Well it's documented in a lot of places, and besides, end-user will
never have to program lcd power attribute directly.
> Again, the solution would appear to be to return the last user
> requested power state. The driver_brightness attribute would tell you
> if the display was blanked for any other reason (although not why).
I don't think all this bookkeeping has to be done in kernel.
--
Greetings,
Andrew
P.S. A wild idea: Every requester could tag it's request with its
own alphanumeric tag. That is, if GPE wants to set brightness to 50 and
turn the power on, it writes 'GPE:50' and 'GPE:1' to 'brightness' and
'power' attributes respectively. Then user' scripts would put
'JOHN:100' into brightness, and some player would put
'OpieMediaPlayer:0' into 'power'. This way the driver will always know
who wants what and could integrate all these values into one.
But I personally don't think it's worth the trouble.
On Mon, 2006-03-06 at 01:09 +0300, Andrew Zabolotny wrote:
> On Sun, 05 Mar 2006 15:08:53 +0000
> Richard Purdie <[email protected]> wrote:
>
> > The solution might be to have brightness always return the user
> > requested value y and have a new attribute returning the brightness as
> > determined by the driver once it accounts for all the factors it needs
> > to consider. Naming of such an attribute is tricky -
> > "driver_brightness" perthaps?
> Maybe by analogy to sound card there can be a 'master' control, and one
> 'user' control. But it's not clear how many of these strings you will
> need for every 'real' attribute. Why two and not three? Why three and
> not ten?
I'm just wondering about the name of the single attribute that reflects
the status of the current actual brightness value that the driver has
chosen.
> But I don't think that's a kernel-side problem. I think it would be
> better to have some daemon controlling these attributes (and keeping
> track of as many factors as needed) and programming the final, 'true'
> kernel value. Then a set of user-mode tools could be provided to alter
> them, and a simple API.
If all the events that could influence the backlight were available in
userspace, I'd agree but they're not. There might be a need for a daemon
in userspace for various reasons depending on the application but I'm
not concerned about that.
> Well with power it's simple: LCD is either powered or it's not. After
> resuming the LCD should be always powered on, and the program can then
> turn it back off if it's desired. FB blanking isn't a issue with X11/
> Qtopia, as far as I understand? And finally, the 'user' requested power
> state has to be tracked by the program that does the blanking (say an
> audio player or such).
So the user powers down the LCD, the FB blanking then blanks and
unblanks. What should the current LCD power status be? The LCD should
still be off as far as I can see yet the LCD/backlight class doesn't do
this at present.
I'm beginning to favour a system where backlight drivers only provide
two functions:
int (*set_status)(struct backlight_device *, int brightness, int power, int fb_blank);
int (*get_status)(struct backlight_device *);
set_status passes the user requested brightness and power values along
with the current fb_blanking status to the driver. The driver can then
set the hardware up as appropriate.
get_status returns the brightness for the current configuration.
The backlight core itself keeps track of the requested power and
brightness values rather than having every backlight driver including
the logic. This has the advantage of keeping behaviour the same and
avoiding subtle logic bugs of which there are several at the moment.
This also means that "echo 31 > brightness; cat brightness" will always
give the expected answer of whatever brightness the user is requesting
and the actual current driver brightness choice is available through
"cat driver_brightness".
Does this seem reasonable?
Richard
Richard Purdie wrote:
> At present, the backlight class presents two attributes to sysfs,
> brightness and power. I'm a little confused as to whether these
> attributes are currently doing the right things.
>
> Taking brightness, at any one time we have several different brightness
> values:
>
> * User requested brightness (echo y > /sys/class/backlight/xxx/brightness)
> * Driver determined brightness which accounts for things like FB
> blanking, low battery backlight limiting (an example from corgi_bl),
> user requested power state, device suspend/resume.
>
> The solution might be to have brightness always return the user
> requested value y and have a new attribute returning the brightness as
> determined by the driver once it accounts for all the factors it needs
> to consider. Naming of such an attribute is tricky - "driver_brightness"
> perthaps?
Why not just agree on a normal range of values (ie, 0-255), and let the
driver "denormalize" them? Thus, a driver that has only 2 levels of
brightness, will treat 0-127 as 0 and 128-255 as 1, and will return only
two possible values 0 and 255.
If a user requests max brightness (255), and the driver is capable of setting
it, then it returns 255. But if for some reason it can't (ie low power state),
it returns the current max brightness, normalized, (ie 128 instead of 255 and
because that the scale is 0-255, a value of 128 tells the user that only
half of max brightness was set).
And instead of a new "driver_brightness" attribute, why not just a new
attribute that returns the number of brightness levels?
It's similar for power. We can agree on a range, 0-255. The range might be
overkill but is consistent with brightness. The callback converts the VESA
constants to 0 - 0, 1 - 64, 2 - 128, 3 - 255 and sends the value to the driver.
The driver, denormalizes them to, most probably, on and off (0-127 - on,
128-255 - off).
Tony
On Mon, 2006-03-06 at 09:00 +0800, Antonino A. Daplas wrote:
> Why not just agree on a normal range of values (ie, 0-255), and let the
> driver "denormalize" them? Thus, a driver that has only 2 levels of
> brightness, will treat 0-127 as 0 and 128-255 as 1, and will return only
> two possible values 0 and 255.
>
> If a user requests max brightness (255), and the driver is capable of setting
> it, then it returns 255. But if for some reason it can't (ie low power state),
> it returns the current max brightness, normalized, (ie 128 instead of 255 and
> because that the scale is 0-255, a value of 128 tells the user that only
> half of max brightness was set).
We already have a max_brightness attribute and brightness can vary
between 0 and max_brightness (hence avoiding scaling issues which we
leave to userspace).
> And instead of a new "driver_brightness" attribute, why not just a new
> attribute that returns the number of brightness levels?
The idea of the driver_brightness attribute is to separate the user
requested brightness value from the actual brightness value decided by
the driver. The driver determines the real brightness by combining:
* the user supplied power sysfs attribute
* the user supplied brightness sysfs attribute
* the current FB blanking state
* any other driver specific factors
At present, the user specified values through the sysfs attributes can
easily get lost as other factors overwrite it (FB blanking overwrites
user specified power for example). This leads to a class interface with
unpredictable behaviour.
I'm therefore suggesting separation of the values so each attribute
reads and writes in a predicable manner and the actual state determined
by the driver becomes a new readonly sysfs attribute
("driver_brightness" although I'm open to suggestions of a better name).
Richard
On Po 06-03-06 09:00:27, Antonino A. Daplas wrote:
> Richard Purdie wrote:
> > At present, the backlight class presents two attributes to sysfs,
> > brightness and power. I'm a little confused as to whether these
> > attributes are currently doing the right things.
> >
> > Taking brightness, at any one time we have several different brightness
> > values:
> >
> > * User requested brightness (echo y > /sys/class/backlight/xxx/brightness)
> > * Driver determined brightness which accounts for things like FB
> > blanking, low battery backlight limiting (an example from corgi_bl),
> > user requested power state, device suspend/resume.
> >
> > The solution might be to have brightness always return the user
> > requested value y and have a new attribute returning the brightness as
> > determined by the driver once it accounts for all the factors it needs
> > to consider. Naming of such an attribute is tricky - "driver_brightness"
> > perthaps?
>
> Why not just agree on a normal range of values (ie, 0-255), and let the
> driver "denormalize" them? Thus, a driver that has only 2 levels of
> brightness, will treat 0-127 as 0 and 128-255 as 1, and will return only
> two possible values 0 and 255.
Does not work... how do you set minimum brightness with backlight on
(so that text is visible)?
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...
Hi!
> > Well with power it's simple: LCD is either powered or it's not. After
> > resuming the LCD should be always powered on, and the program can then
> > turn it back off if it's desired. FB blanking isn't a issue with X11/
> > Qtopia, as far as I understand? And finally, the 'user' requested power
> > state has to be tracked by the program that does the blanking (say an
> > audio player or such).
>
> So the user powers down the LCD, the FB blanking then blanks and
> unblanks. What should the current LCD power status be? The LCD should
> still be off as far as I can see yet the LCD/backlight class doesn't do
> this at present.
>
> I'm beginning to favour a system where backlight drivers only provide
> two functions:
>
> int (*set_status)(struct backlight_device *, int brightness, int power, int fb_blank);
> int (*get_status)(struct backlight_device *);
>
> set_status passes the user requested brightness and power values along
> with the current fb_blanking status to the driver. The driver can then
> set the hardware up as appropriate.
>
> get_status returns the brightness for the current configuration.
>
> The backlight core itself keeps track of the requested power and
> brightness values rather than having every backlight driver including
> the logic. This has the advantage of keeping behaviour the same and
> avoiding subtle logic bugs of which there are several at the moment.
>
> This also means that "echo 31 > brightness; cat brightness" will always
> give the expected answer of whatever brightness the user is requesting
> and the actual current driver brightness choice is available through
> "cat driver_brightness".
>
> Does this seem reasonable?
Yep. Keeping hardware drivers as simple as possible is good.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...
On Mon, 06 Mar 2006 08:45:28 +0000
Richard Purdie <[email protected]> wrote:
> * the user supplied power sysfs attribute
> * the user supplied brightness sysfs attribute
> * the current FB blanking state
> * any other driver specific factors
As far I see the only real concern here is the console blanking. So why
not make it just another device state flag, which doesn't influence the
'power' attribute and which isn't visible from user space (what for?).
And the 'real' power state will be computed as "blank && power"
attributes. The entire logic could be hidden in backlight.c so that no
driver will have to be modified for this. Maybe a 'hw_power' or such
would be needed to see the 'real' hardware state (and possibly
modify, if you really really want to).
Is there any need for a broader-range solution here?
--
Greetings,
Andrew
On Tue, 2006-03-07 at 00:07 +0300, Andrew Zabolotny wrote:
> On Mon, 06 Mar 2006 08:45:28 +0000
> Richard Purdie <[email protected]> wrote:
>
> > * the user supplied power sysfs attribute
> > * the user supplied brightness sysfs attribute
> > * the current FB blanking state
> > * any other driver specific factors
> As far I see the only real concern here is the console blanking. So why
> not make it just another device state flag, which doesn't influence the
> 'power' attribute and which isn't visible from user space (what for?).
> And the 'real' power state will be computed as "blank && power"
> attributes. The entire logic could be hidden in backlight.c so that no
> driver will have to be modified for this. Maybe a 'hw_power' or such
> would be needed to see the 'real' hardware state (and possibly
> modify, if you really really want to).
>
> Is there any need for a broader-range solution here?
You've ignored the "driver specific factors" and one of the drivers
already has such a thing (borgi_bl's low battery backlight limiting).
My driver_brightness is really a more generic version of your "hw_power"
which allows other factors to be taken into consideration as well. We
may as well have the broader-range solution as it costs us nothing.
(I don't expect each factor to be visible to userspace, just the end
result).
Richard
On Mon, 06 Mar 2006 23:05:07 +0000
Richard Purdie <[email protected]> wrote:
> My driver_brightness is really a more generic version of your
> "hw_power" which allows other factors to be taken into consideration
> as well. We may as well have the broader-range solution as it costs
> us nothing.
Ok, your solution seems fine to me. In fact, with the old backend we
don't have guarantees that current hardware state can be read in
general; it just happened to be true for used hardware.
I just would propose to change the prototype of
int (*set_status)(struct backlight_device *, int brightness, int power,
int fb_blank);
to
int (*set_status)(struct backlight_device *, struct backlight_status *);
This will allow adding new factors without changing the prototype
(thus without breaking old drivers which will simply ignore new added
factors).
I'm also not sure what should return
int (*get_status)(struct backlight_device *);
the brightness or the power status? Maybe it should be:
int (*get_status)(struct backlight_device *, struct backlight_status *);
--
Greetings,
Andrew