[PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 <mjg@redhat.com>
---
 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

[PATCH 2/3] ACPI: Update the backlight state when we change brightness

[Matthew Garrett]

Trigger a status update when we change the brightness in the driver, thus
allowing userspace to present appropriate UI.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 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

[PATCH 3/3] eeepc-laptop: Update the backlight state when we change brightness

[Matthew Garrett]

Trigger a status update when the user hits a brightness key, allowing
userspace to present appropriate UI.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[ykzhao]

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 <mjg@redhat.com>
> ---
>  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)
>  

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Henrique de Moraes Holschuh]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Henrique de Moraes Holschuh]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Richard Purdie]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Zhang Rui]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Richard Purdie]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Zhang Rui]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Richard Purdie]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Zhang Rui]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Pavel Machek]

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Richard Purdie]

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

[PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

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 <mjg@redhat.com>
---

Updated to allow drivers to provide a reason for the change

 drivers/video/backlight/backlight.c |   41 +++++++++++++++++++++++++++++++++++
 include/linux/backlight.h           |    7 ++++++
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 157057c..01efb2d 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -73,6 +73,26 @@ static inline void backlight_unregister_fb(struct backlight_device *bd)
 }
 #endif /* CONFIG_FB */
 
+static void backlight_generate_event(struct backlight_device *bd,
+				     enum backlight_update_reason reason)
+{
+	char *envp[2];
+
+	switch (reason) {
+	case BACKLIGHT_UPDATE_SYSFS:
+		envp[0] = "SOURCE=sysfs";
+		break;
+	case BACKLIGHT_UPDATE_HOTKEY:
+		envp[0] = "SOURCE=hotkey";
+		break;
+	default:
+		envp[0] = "SORUCE=unknown";
+		break;
+	}
+	envp[1] = NULL;
+	kobject_uevent_env(&bd->dev.kobj, KOBJ_CHANGE, envp);
+}
+
 static ssize_t backlight_show_power(struct device *dev,
 		struct device_attribute *attr,char *buf)
 {
@@ -142,6 +162,8 @@ static ssize_t backlight_store_brightness(struct device *dev,
 	}
 	mutex_unlock(&bd->ops_lock);
 
+	backlight_generate_event(bd, BACKLIGHT_UPDATE_SYSFS);
+
 	return rc;
 }
 
@@ -214,6 +236,25 @@ 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,
+			    enum backlight_update_reason reason)
+{
+	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);
+	backlight_generate_event(bd, reason);
+}
+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..0f5f578 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -27,6 +27,11 @@
  * Any other use of the locks below is probably wrong.
  */
 
+enum backlight_update_reason {
+	BACKLIGHT_UPDATE_HOTKEY,
+	BACKLIGHT_UPDATE_SYSFS,
+};
+
 struct backlight_device;
 struct fb_info;
 
@@ -100,6 +105,8 @@ 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,
+				   enum backlight_update_reason reason);
 
 #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
 
-- 
1.6.2.5

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Michal Schmidt]

Dne Tue, 14 Jul 2009 17:06:02 +0100 Matthew Garrett napsal:
> +static void backlight_generate_event(struct backlight_device *bd,
> +				     enum backlight_update_reason
> reason) +{
> +	char *envp[2];
> +
> +	switch (reason) {
> +	case BACKLIGHT_UPDATE_SYSFS:
> +		envp[0] = "SOURCE=sysfs";
> +		break;
> +	case BACKLIGHT_UPDATE_HOTKEY:
> +		envp[0] = "SOURCE=hotkey";
> +		break;
> +	default:
> +		envp[0] = "SORUCE=unknown";

There's a typo here. s/SORUCE/SOURCE/

Michal

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Pavel Machek]

On Tue 2009-07-14 17:06:02, 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 adds new kernel API. Should it be documented somewhere?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Henrique de Moraes Holschuh]

On Tue, 14 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.

Ok.  I have it working (event-based AND poll-based) in thinkpad-acpi,
although it needs to be tested by dual-GPU, ACPI-based and OpRegion-based
owners.

Should the driver notify of *any* changes, even when the user is hitting the
"ceiling" or the "floor" and the backlight value didn't really change?

If the answer is no, shouldn't the backlight class filter these out itself,
to guarantee a consistent experience?

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

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Matthew Garrett]

On Sat, Sep 19, 2009 at 08:24:24PM -0300, Henrique de Moraes Holschuh wrote:

> Should the driver notify of *any* changes, even when the user is hitting the
> "ceiling" or the "floor" and the backlight value didn't really change?

I'd say yes - the user will expect the UI to appear whenever they hit 
the button.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/3] backlight: Allow drivers to update the core, and generate events on changes

[Henrique de Moraes Holschuh]

On Sun, 20 Sep 2009, Matthew Garrett wrote:
> On Sat, Sep 19, 2009 at 08:24:24PM -0300, Henrique de Moraes Holschuh wrote:
> > Should the driver notify of *any* changes, even when the user is hitting the
> > "ceiling" or the "floor" and the backlight value didn't really change?
> 
> I'd say yes - the user will expect the UI to appear whenever they hit 
> the button.

Ok.  The thinkpad-acpi changes depends on a large patchset that I should
send to Len today.  After that, I will send the patch that adds brightness
event support.

Are you backlight patches in any tree?  Are them expected to hit mainline
this merge window?

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