Certain hardware will send us events when the backlight brightness
changes. Add a function to update the value in the core, and
additionally send a uevent so that userspace can pop up appropriate
UI. The uevents are flagged depending on whether the update originated
in the kernel or from userspace, making it easier to only display UI
at the appropriate time.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/video/backlight/backlight.c | 23 +++++++++++++++++++++++
include/linux/backlight.h | 1 +
2 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 157057c..98ab76c 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -120,6 +120,7 @@ static ssize_t backlight_store_brightness(struct device *dev,
{
int rc;
struct backlight_device *bd = to_backlight_device(dev);
+ char *envp[] = { "SOURCE=userspace", NULL };
unsigned long brightness;
rc = strict_strtoul(buf, 0, &brightness);
@@ -142,6 +143,8 @@ static ssize_t backlight_store_brightness(struct device *dev,
}
mutex_unlock(&bd->ops_lock);
+ kobject_uevent_env(&bd->dev.kobj, KOBJ_CHANGE, envp);
+
return rc;
}
@@ -214,6 +217,26 @@ static struct device_attribute bl_device_attributes[] = {
};
/**
+ * backlight_force_update - tell the backlight subsystem that hardware state
+ * has changed
+ * @bd: the backlight device to update
+ *
+ * Updates the internal state of the backlight in response to a hardware event,
+ * and generate a uevent to notify userspace
+ */
+void backlight_force_update(struct backlight_device *bd)
+{
+ char *envp[] = { "SOURCE=kernel", NULL };
+
+ mutex_lock(&bd->ops_lock);
+ if (bd->ops && bd->ops->get_brightness)
+ bd->props.brightness = bd->ops->get_brightness(bd);
+ mutex_unlock(&bd->ops_lock);
+ kobject_uevent_env(&bd->dev.kobj, KOBJ_CHANGE, envp);
+}
+EXPORT_SYMBOL(backlight_force_update);
+
+/**
* backlight_device_register - create and register a new object of
* backlight_device class.
* @name: the name of the new object(must be the same as the name of the
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 79ca2da..8298c43 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -100,6 +100,7 @@ static inline void backlight_update_status(struct backlight_device *bd)
extern struct backlight_device *backlight_device_register(const char *name,
struct device *dev, void *devdata, struct backlight_ops *ops);
extern void backlight_device_unregister(struct backlight_device *bd);
+extern void backlight_force_update(struct backlight_device *bd);
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
--
1.6.2.5
Trigger a status update when the user hits a brightness key, allowing
userspace to present appropriate UI.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/platform/x86/eeepc-laptop.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index ec560f1..832db81 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -641,7 +641,7 @@ static int notify_brn(void)
struct backlight_device *bd = eeepc_backlight_device;
if (bd) {
int old = bd->props.brightness;
- bd->props.brightness = read_brightness(bd);
+ backlight_force_update(bd);
return old;
}
return -1;
--
1.6.2.5
Trigger a status update when we change the brightness in the driver, thus
allowing userspace to present appropriate UI.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/acpi/video.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 8851315..d7219f6 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1960,6 +1960,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
result = acpi_video_device_lcd_set_level(device, level_next);
+ backlight_force_update(device->backlight);
+
out:
if (result)
printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
--
1.6.2.5
On Tue, 2009-07-14 at 04:41 +0800, Matthew Garrett wrote:
> Certain hardware will send us events when the backlight brightness
> changes. Add a function to update the value in the core, and
> additionally send a uevent so that userspace can pop up appropriate
> UI. The uevents are flagged depending on whether the update originated
> in the kernel or from userspace, making it easier to only display UI
> at the appropriate time.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/video/backlight/backlight.c | 23 +++++++++++++++++++++++
> include/linux/backlight.h | 1 +
> 2 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 157057c..98ab76c 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -120,6 +120,7 @@ static ssize_t backlight_store_brightness(struct device *dev,
> {
> int rc;
> struct backlight_device *bd = to_backlight_device(dev);
> + char *envp[] = { "SOURCE=userspace", NULL };
> unsigned long brightness;
>
> rc = strict_strtoul(buf, 0, &brightness);
> @@ -142,6 +143,8 @@ static ssize_t backlight_store_brightness(struct device *dev,
> }
> mutex_unlock(&bd->ops_lock);
>
> + kobject_uevent_env(&bd->dev.kobj, KOBJ_CHANGE, envp);
> +
It will be better to send the event to user space only when the
brightness is updated successfully.
> return rc;
> }
>
> @@ -214,6 +217,26 @@ static struct device_attribute bl_device_attributes[] = {
> };
>
> /**
> + * backlight_force_update - tell the backlight subsystem that hardware state
> + * has changed
> + * @bd: the backlight device to update
> + *
> + * Updates the internal state of the backlight in response to a hardware event,
> + * and generate a uevent to notify userspace
> + */
> +void backlight_force_update(struct backlight_device *bd)
> +{
> + char *envp[] = { "SOURCE=kernel", NULL };
> +
> + mutex_lock(&bd->ops_lock);
> + if (bd->ops && bd->ops->get_brightness)
> + bd->props.brightness = bd->ops->get_brightness(bd);
> + mutex_unlock(&bd->ops_lock);
> + kobject_uevent_env(&bd->dev.kobj, KOBJ_CHANGE, envp);
> +}
> +EXPORT_SYMBOL(backlight_force_update);
It seems that the acpi video driver will send the ACPI event to user
space when updating the brightness.
Is it necessary to send the udev-event again?
Thanks.
> +
> +/**
> * backlight_device_register - create and register a new object of
> * backlight_device class.
> * @name: the name of the new object(must be the same as the name of the
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 79ca2da..8298c43 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -100,6 +100,7 @@ static inline void backlight_update_status(struct backlight_device *bd)
> extern struct backlight_device *backlight_device_register(const char *name,
> struct device *dev, void *devdata, struct backlight_ops *ops);
> extern void backlight_device_unregister(struct backlight_device *bd);
> +extern void backlight_force_update(struct backlight_device *bd);
>
> #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
>
On Tue, Jul 14, 2009 at 08:58:48AM +0800, ykzhao wrote:
> It will be better to send the event to user space only when the
> brightness is updated successfully.
That guarantees that we have to read back the brightness, which may be
expensive on some hardware.
> It seems that the acpi video driver will send the ACPI event to user
> space when updating the brightness.
> Is it necessary to send the udev-event again?
Yes. There's no reason to special-case the ACPI driver.
--
Matthew Garrett | [email protected]
On Mon, 13 Jul 2009, Matthew Garrett wrote:
> Certain hardware will send us events when the backlight brightness
> changes. Add a function to update the value in the core, and
> additionally send a uevent so that userspace can pop up appropriate
> UI. The uevents are flagged depending on whether the update originated
> in the kernel or from userspace, making it easier to only display UI
> at the appropriate time.
Any reasons to not do it using poll() support (since sysfs has it)? Or at
least, do both poll and uevents?
Other than that, I like the idea a lot. thinkpad-acpi will use this event
support.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Mon, 2009-07-13 at 21:41 +0100, Matthew Garrett wrote:
> Certain hardware will send us events when the backlight brightness
> changes. Add a function to update the value in the core, and
> additionally send a uevent so that userspace can pop up appropriate
> UI. The uevents are flagged depending on whether the update originated
> in the kernel or from userspace, making it easier to only display UI
> at the appropriate time.
This looks good and I like the idea.
My main concern is that we don't start getting bug reports of 'missing'
events and have clearly defined expectations of when we see what kind of
events. For example, should an event be emitted when low battery causes
the backlight to be limited? How about console blanking events turning
off the backlight? Are there any other occasions we should be emitting
change events and do we need to audit other drivers?
I did look to see if we could integrate this more into the backlight
core but that doesn't look to be possible unfortunately, at least not
without changing the drivers which these patches start.
Also, are "userspace" and "kernel" as meaningful as they could be? Would
"sysfs" and "hwkeys" make more sense and allow for other future hardware
differences? Perhaps someone will tie the backlight to an ambient light
sensor for example...
Cheers,
Richard
--
Richard Purdie
Intel Open Source Technology Centre
On Tue, Jul 14, 2009 at 01:29:33PM +0100, Richard Purdie wrote:
> My main concern is that we don't start getting bug reports of 'missing'
> events and have clearly defined expectations of when we see what kind of
> events. For example, should an event be emitted when low battery causes
> the backlight to be limited? How about console blanking events turning
> off the backlight? Are there any other occasions we should be emitting
> change events and do we need to audit other drivers?
These are all good questions. I'm working through the drivers now, and
there's definitely a few more that need doing.
> Also, are "userspace" and "kernel" as meaningful as they could be? Would
> "sysfs" and "hwkeys" make more sense and allow for other future hardware
> differences? Perhaps someone will tie the backlight to an ambient light
> sensor for example...
Yes, though we'd need to extend the interface slightly in that case to
allow the information to be provided. The ALS case is a good one - if
that ends up happening in-kernel then there probably shouldn't be a
UI update.
--
Matthew Garrett | [email protected]
On Tue, Jul 14, 2009 at 07:30:07AM -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 13 Jul 2009, Matthew Garrett wrote:
> > Certain hardware will send us events when the backlight brightness
> > changes. Add a function to update the value in the core, and
> > additionally send a uevent so that userspace can pop up appropriate
> > UI. The uevents are flagged depending on whether the update originated
> > in the kernel or from userspace, making it easier to only display UI
> > at the appropriate time.
>
> Any reasons to not do it using poll() support (since sysfs has it)? Or at
> least, do both poll and uevents?
More code? It doesn't really seem necessary.
> Other than that, I like the idea a lot. thinkpad-acpi will use this event
> support.
Good, that's one of the use-cases I wanted to deal with - but the hotkey
mask stuff is complicated enough that I hadn't got round to touching
that yet. The combination of this and the ALSA mixer code should get us
full notification.
--
Matthew Garrett | [email protected]
On Tue, Jul 14, 2009 at 02:53:26AM +0100, Matthew Garrett wrote:
> On Tue, Jul 14, 2009 at 08:58:48AM +0800, ykzhao wrote:
>
> > It will be better to send the event to user space only when the
> > brightness is updated successfully.
>
> That guarantees that we have to read back the brightness, which may be
> expensive on some hardware.
Oh, sorry, I see what you mean. I'll update that.
--
Matthew Garrett | [email protected]
On Tue, 14 Jul 2009, Matthew Garrett wrote:
> On Tue, Jul 14, 2009 at 07:30:07AM -0300, Henrique de Moraes Holschuh wrote:
> > On Mon, 13 Jul 2009, Matthew Garrett wrote:
> > > Certain hardware will send us events when the backlight brightness
> > > changes. Add a function to update the value in the core, and
> > > additionally send a uevent so that userspace can pop up appropriate
> > > UI. The uevents are flagged depending on whether the update originated
> > > in the kernel or from userspace, making it easier to only display UI
> > > at the appropriate time.
> >
> > Any reasons to not do it using poll() support (since sysfs has it)? Or at
> > least, do both poll and uevents?
>
> More code? It doesn't really seem necessary.
Well, we all know just how marvelously engineered and high-bandwidth the
uevent+udev+everything else channel is, don't we?
Adding the poll() notification is not expensive, one line to do it, plus a
few more to find out the exact sysfs node that needs to receive the
notification (hideous API, that).
> > Other than that, I like the idea a lot. thinkpad-acpi will use this event
> > support.
>
> Good, that's one of the use-cases I wanted to deal with - but the hotkey
> mask stuff is complicated enough that I hadn't got round to touching
> that yet. The combination of this and the ALSA mixer code should get us
> full notification.
Leave it to me. Even if it arrives a bit later than the main patch, it
won't cause problems for the merging of the main work anyway, and as you
said, it *IS* hairy code to do it right and it plugs right in the middle of
the more confusing parts of thinkpad-acpi. I will do it in a way that makes
the whole thing also usable for the mixer notifications.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, 2009-07-14 at 20:29 +0800, Richard Purdie wrote:
> On Mon, 2009-07-13 at 21:41 +0100, Matthew Garrett wrote:
> > Certain hardware will send us events when the backlight brightness
> > changes. Add a function to update the value in the core, and
> > additionally send a uevent so that userspace can pop up appropriate
> > UI. The uevents are flagged depending on whether the update originated
> > in the kernel or from userspace, making it easier to only display UI
> > at the appropriate time.
>
> This looks good and I like the idea.
>
> My main concern is that we don't start getting bug reports of 'missing'
> events and have clearly defined expectations of when we see what kind of
> events. For example, should an event be emitted when low battery causes
> the backlight to be limited? How about console blanking events turning
> off the backlight? Are there any other occasions we should be emitting
> change events and do we need to audit other drivers?
>
> I did look to see if we could integrate this more into the backlight
> core but that doesn't look to be possible unfortunately, at least not
> without changing the drivers which these patches start.
>
> Also, are "userspace" and "kernel" as meaningful as they could be? Would
> "sysfs" and "hwkeys" make more sense and allow for other future hardware
> differences? Perhaps someone will tie the backlight to an ambient light
> sensor for example...
>
Hah, I just finished a patch to introduce the ACPI als driver.
I'm not quite familiar with the status of ALS support in Linux kernel.
and here is my questions,
I don't think we have a generic sysfs driver for ALS, i.e. ALS class
device, right?
do you guys think it's reasonable to have one?
thanks,
rui
On Wed, 2009-07-15 at 15:55 +0800, Zhang Rui wrote:
> > Also, are "userspace" and "kernel" as meaningful as they could be?
> Would
> > "sysfs" and "hwkeys" make more sense and allow for other future hardware
> > differences? Perhaps someone will tie the backlight to an ambient light
> > sensor for example...
> >
>
> Hah, I just finished a patch to introduce the ACPI als driver.
>
> I'm not quite familiar with the status of ALS support in Linux kernel.
> and here is my questions,
> I don't think we have a generic sysfs driver for ALS, i.e. ALS class
> device, right?
> do you guys think it's reasonable to have one?
We don't have one that I'm aware of.
The first question is what information and configuration options are we
likely to have with ALS sensors? If its just a light level reading, the
input subsystem is ideally suited for sharing that. Is there
configuration we need to expose?
Cheers,
Richard
--
Richard Purdie
Intel Open Source Technology Centre
On Wed, 2009-07-15 at 16:22 +0800, Richard Purdie wrote:
> On Wed, 2009-07-15 at 15:55 +0800, Zhang Rui wrote:
> > > Also, are "userspace" and "kernel" as meaningful as they could be?
> > Would
> > > "sysfs" and "hwkeys" make more sense and allow for other future hardware
> > > differences? Perhaps someone will tie the backlight to an ambient light
> > > sensor for example...
> > >
> >
> > Hah, I just finished a patch to introduce the ACPI als driver.
> >
> > I'm not quite familiar with the status of ALS support in Linux kernel.
> > and here is my questions,
> > I don't think we have a generic sysfs driver for ALS, i.e. ALS class
> > device, right?
> > do you guys think it's reasonable to have one?
>
> We don't have one that I'm aware of.
>
> The first question is what information and configuration options are we
> likely to have with ALS sensors? If its just a light level reading, the
> input subsystem is ideally suited for sharing that. Is there
> configuration we need to expose?
>
For an ACPI ALS device, only these two are mandatory
1. the current ambient light illuminance.
2. A list of luminance mappings, including the "display luminance
adjustment" and "ambient light illuminance", which can be used by OS to
determine the ambient light policy.
IMO, the ALS driver should just export these info to user space, and we
can do the display backlight adjustment in user space,
via /sys/class/backlight.
thanks,
rui
On Wed, 2009-07-15 at 16:38 +0800, Zhang Rui wrote:
> On Wed, 2009-07-15 at 16:22 +0800, Richard Purdie wrote:
> > We don't have one that I'm aware of.
> >
> > The first question is what information and configuration options are we
> > likely to have with ALS sensors? If its just a light level reading, the
> > input subsystem is ideally suited for sharing that. Is there
> > configuration we need to expose?
> >
> For an ACPI ALS device, only these two are mandatory
> 1. the current ambient light illuminance.
> 2. A list of luminance mappings, including the "display luminance
> adjustment" and "ambient light illuminance", which can be used by OS to
> determine the ambient light policy.
So there are basically two values we're ever interested in at a given
point in time. The current ambient light reading and the corresponding
display luminance adjustment. Both of those could just be exposed as an
input device really?
> IMO, the ALS driver should just export these info to user space, and we
> can do the display backlight adjustment in user space,
> via /sys/class/backlight.
Agreed, userspace should be where policy is decided. Having a standalone
module which connected the two together with a "default" policy might be
acceptable too though? If userspace then wants to handle things it just
makes sure the module is not loaded.
Cheers,
Richard
--
Richard Purdie
Intel Open Source Technology Centre
On Wed, Jul 15, 2009 at 10:11:29AM +0100, Richard Purdie wrote:
> So there are basically two values we're ever interested in at a given
> point in time. The current ambient light reading and the corresponding
> display luminance adjustment. Both of those could just be exposed as an
> input device really?
The alternative would be hwmon, I guess.
> Agreed, userspace should be where policy is decided. Having a standalone
> module which connected the two together with a "default" policy might be
> acceptable too though? If userspace then wants to handle things it just
> makes sure the module is not loaded.
You want smoothing even in a default policy, and doing that well might
be a bit much for the kernel?
--
Matthew Garrett | [email protected]
On Wed, 2009-07-15 at 21:58 +0800, Matthew Garrett wrote:
> On Wed, Jul 15, 2009 at 10:11:29AM +0100, Richard Purdie wrote:
>
> > So there are basically two values we're ever interested in at a given
> > point in time. The current ambient light reading and the corresponding
> > display luminance adjustment. Both of those could just be exposed as an
> > input device really?
>
> The alternative would be hwmon, I guess.
you mean convert an ACPI ALS device to a hwmon device?
>
> > Agreed, userspace should be where policy is decided. Having a standalone
> > module which connected the two together with a "default" policy might be
> > acceptable too though? If userspace then wants to handle things it just
> > makes sure the module is not loaded.
>
> You want smoothing even in a default policy, and doing that well might
> be a bit much for the kernel?
>
sorry, I don't understand.
do you mean that there should be a default policy in the kernel?
thanks,
rui
On Thu, Jul 16, 2009 at 10:39:34AM +0800, Zhang Rui wrote:
> On Wed, 2009-07-15 at 21:58 +0800, Matthew Garrett wrote:
> > On Wed, Jul 15, 2009 at 10:11:29AM +0100, Richard Purdie wrote:
> >
> > > So there are basically two values we're ever interested in at a given
> > > point in time. The current ambient light reading and the corresponding
> > > display luminance adjustment. Both of those could just be exposed as an
> > > input device really?
> >
> > The alternative would be hwmon, I guess.
>
> you mean convert an ACPI ALS device to a hwmon device?
Yes.
> > You want smoothing even in a default policy, and doing that well might
> > be a bit much for the kernel?
> >
> sorry, I don't understand.
> do you mean that there should be a default policy in the kernel?
No, I mean that the only default policy you could reasonably have in the
kernel would be tying the backlight directly to the ALS. And that sucks.
You need some degree of smoothing, and that's a job better left to
userspace.
--
Matthew Garrett | [email protected]
> > > You want smoothing even in a default policy, and doing that well might
> > > be a bit much for the kernel?
> > >
> > sorry, I don't understand.
> > do you mean that there should be a default policy in the kernel?
>
> No, I mean that the only default policy you could reasonably have in the
> kernel would be tying the backlight directly to the ALS. And that sucks.
> You need some degree of smoothing, and that's a job better left to
> userspace.
How would that be done? Wake up userspace 100times a second so it can
read an integer and ardd it to running average?
Is ALS device stupid sensor, or can it do some smoothing/watermarks
itself?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Jul 29, 2009 at 05:05:24PM +0200, Pavel Machek wrote:
> > No, I mean that the only default policy you could reasonably have in the
> > kernel would be tying the backlight directly to the ALS. And that sucks.
> > You need some degree of smoothing, and that's a job better left to
> > userspace.
>
> How would that be done? Wake up userspace 100times a second so it can
> read an integer and ardd it to running average?
I don't think you'd need more than once a second, possibly modified by
the size of the change since the last measurement. Response doesn't need
to be immediate.
> Is ALS device stupid sensor, or can it do some smoothing/watermarks
> itself?
Generally stupid.
--
Matthew Garrett | [email protected]
On Wed, 2009-07-29 at 16:20 +0100, Matthew Garrett wrote:
> On Wed, Jul 29, 2009 at 05:05:24PM +0200, Pavel Machek wrote:
> > > No, I mean that the only default policy you could reasonably have in the
> > > kernel would be tying the backlight directly to the ALS. And that sucks.
> > > You need some degree of smoothing, and that's a job better left to
> > > userspace.
> >
> > How would that be done? Wake up userspace 100times a second so it can
> > read an integer and ardd it to running average?
>
> I don't think you'd need more than once a second, possibly modified by
> the size of the change since the last measurement. Response doesn't need
> to be immediate.
It depends how fancy your algorithm is really. If its not that complex
and not floating point etc. then you probably could have some basic
kernel implementation in a few lines of code which used a timer callback
to smooth things out. I agree that userspace should be able to run the
show if it wants to or its some horrible complex calculation though.
Assuming no complaints I'll queue the patches from this series in the
backlight tree in the next could of days. I'll probably include the ACPI
bits since they're more backlight than APCI unless Len has an objection.
Cheers,
Richard
--
Richard Purdie
Intel Open Source Technology Centre