[PATCH 1/2] video: always update the brightness when poking "brightness"

[PATCH 1/2] video: always update the brightness when poking "brightness"

[Zhang Rui]

bd->props.brightness is a cached value.
the current code has some potential problems when poking the
backlight I/F.

For example,
1. set the brightness level to 5 via sysfs I/F
2. set the backlight to another level via the procfs,
3. setting it back to 5 doesn't work because bd->props.brightness
is still 5.

http://bugzilla.kernel.org/show_bug.cgi?id=12249

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/video/backlight/backlight.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/video/backlight/backlight.c
===================================================================
--- linux-2.6.orig/drivers/video/backlight/backlight.c
+++ linux-2.6/drivers/video/backlight/backlight.c
@@ -134,10 +134,8 @@ static ssize_t backlight_store_brightnes
 		else {
 			pr_debug("backlight: set brightness to %d\n",
 				 brightness);
-			if (bd->props.brightness != brightness) {
-				bd->props.brightness = brightness;
-				backlight_update_status(bd);
-			}
+			bd->props.brightness = brightness;
+			backlight_update_status(bd);
 			rc = count;
 		}
 	}

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Richard Purdie]

On Wed, 2008-12-24 at 11:07 +0800, Zhang Rui wrote:
> bd->props.brightness is a cached value.
> the current code has some potential problems when poking the
> backlight I/F.
> 
> For example,
> 1. set the brightness level to 5 via sysfs I/F
> 2. set the backlight to another level via the procfs,
> 3. setting it back to 5 doesn't work because bd->props.brightness
> is still 5.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=12249

The root of the problem here is that there are two interfaces
controlling the same piece of hardware. Personally, I'd like to see the
proc interfaces to backlights depreacated but we do need to make them
work I guess. The real bug looks like the proc interface should update
props.brightness for that hardware...

Cheers,

Richard

-- 
Richard Purdie
Intel Open Source Technology Centre

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Matthew Garrett]

On Wed, Dec 31, 2008 at 09:22:20AM +0800, Zhao Yakui wrote:

> Sometimes the brightness will be changed by BIOS and there is no
> notification event. In such case the props.brightness can't be updated.
> If we want to update the brightness, the request will have no effect.

In the common case (ie, ACPI) clients should read from actual_brightness 
and set via brightness. The distinction exists for precisely this kind 
of eventuality.

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

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Zhao Yakui]

On Tue, 2008-12-30 at 18:34 +0800, Richard Purdie wrote:
> On Wed, 2008-12-24 at 11:07 +0800, Zhang Rui wrote:
> > bd->props.brightness is a cached value.
> > the current code has some potential problems when poking the
> > backlight I/F.
> > 
> > For example,
> > 1. set the brightness level to 5 via sysfs I/F
> > 2. set the backlight to another level via the procfs,
> > 3. setting it back to 5 doesn't work because bd->props.brightness
> > is still 5.
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=12249
> 
> The root of the problem here is that there are two interfaces
> controlling the same piece of hardware. Personally, I'd like to see the
> proc interfaces to backlights depreacated but we do need to make them
> work I guess. The real bug looks like the proc interface should update
> props.brightness for that hardware...
It is also ok that the props.brightness is also updated when the proc
interface is called. But in such case the "brightness" won't display the
user requested value. 

Sometimes the brightness will be changed by BIOS and there is no
notification event. In such case the props.brightness can't be updated.
If we want to update the brightness, the request will have no effect.

So IMO it will be more appropriate that brightness is always updated
when poking "brightness".

  
Thanks.
   Yakui    
> 
> Cheers,
> 
> Richard
> 

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Zhang Rui]

On Tue, 2008-12-30 at 18:34 +0800, Richard Purdie wrote:
> On Wed, 2008-12-24 at 11:07 +0800, Zhang Rui wrote:
> > bd->props.brightness is a cached value.
> > the current code has some potential problems when poking the
> > backlight I/F.
> > 
> > For example,
> > 1. set the brightness level to 5 via sysfs I/F
> > 2. set the backlight to another level via the procfs,
> > 3. setting it back to 5 doesn't work because bd->props.brightness
> > is still 5.
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=12249
> 
> The root of the problem here is that there are two interfaces
> controlling the same piece of hardware.
then how about this case:
1. set the brightness level to 5 via sysfs I/F
2. set the backlight to another level via HOTKEYS.
3. setting it back to 5 doesn't work because bd->props.brightness
is still 5.

> Personally, I'd like to see the
> proc interfaces to backlights depreacated but we do need to make them
> work I guess. The real bug looks like the proc interface should update
> props.brightness for that hardware...
IMO, the real problem is that bd->props.brightness doesn't reflect the
actual brightness.
If we can make sure that bd->props.brightness is always updated when
the brightness is change, we won't have this problem any more.
But I'm afraid we can't, because backlight can be changed in other ways,
either via procfs or hotkeys.
Of course we can fix these two problems in the ACPI video driver, but I
think this patch is safer. because we don't know if there is any other
case besides procfs and hotkey that is not covered.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Matthew Garrett]

On Wed, Dec 31, 2008 at 09:43:01AM +0800, Zhang Rui wrote:

> then how about this case:
> 1. set the brightness level to 5 via sysfs I/F
> 2. set the backlight to another level via HOTKEYS.
> 3. setting it back to 5 doesn't work because bd->props.brightness
> is still 5.

If they're using the ACPI interface then video.c should update 
props.brightness itself.

> IMO, the real problem is that bd->props.brightness doesn't reflect the
> actual brightness.

If you want the actual brightness, why aren't you reading 
actual_brightness?

-- 
Matthew Garrett | mjg59@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Zhang Rui]

On Wed, 2008-12-31 at 10:01 +0800, Matthew Garrett wrote:
> On Wed, Dec 31, 2008 at 09:43:01AM +0800, Zhang Rui wrote:
> 
> > then how about this case:
> > 1. set the brightness level to 5 via sysfs I/F
> > 2. set the backlight to another level via HOTKEYS.
> > 3. setting it back to 5 doesn't work because bd->props.brightness
> > is still 5.
> 
> If they're using the ACPI interface then video.c should update 
> props.brightness itself.
> 
> > IMO, the real problem is that bd->props.brightness doesn't reflect the
> > actual brightness.
> 
> If you want the actual brightness, why aren't you reading 
> actual_brightness?
> 
right, that's the problem.
because IMO, we don't need the actual_brightness any more. :)
And if you agree that bd->props.brightness doesn't reflect the actual
brightness, we can see that
                        if (bd->props.brightness != brightness) {
                                bd->props.brightness = brightness;
                                backlight_update_status(bd);
                        }
doesn't make sense because "bd->props.brightness != brightness" doesn't
mean anything.

But if bd->props.brightness does reflect the actual brightness, then do
we need "actual_brightness" any more?


> If they're using the ACPI interface then video.c should update 
> props.brightness itself.
that also makes sense, patch attached.
But anyway, we still need some changes in the video backlight class.
right?

From: Zhang Rui <rui.zhang@intel.com>
Subject: always update props.brightness when brightness is changed

always update pros.brightness no matter the backlight is changed
via procfs, hotkeys or sysfs.

Sighed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -481,6 +481,7 @@ acpi_video_device_lcd_set_level(struct a
 	int status = AE_OK;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
+	int state;
 
 
 	arg0.integer.value = level;
@@ -489,6 +490,10 @@ acpi_video_device_lcd_set_level(struct a
 		status = acpi_evaluate_object(device->dev->handle, "_BCM",
 					      &args, NULL);
 	device->brightness->curr = level;
+	for (state = 2; state < device->brightness->count; state++)
+		if (level == device->brightness->levels[state])
+			device->backlight->props.brightness = state - 2;
+
 	return status;
 }
 


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Zhang Rui]

On Wed, 2008-12-31 at 10:58 +0800, Zhang Rui wrote:
> On Wed, 2008-12-31 at 10:01 +0800, Matthew Garrett wrote:
> > On Wed, Dec 31, 2008 at 09:43:01AM +0800, Zhang Rui wrote:
> > 
> > > then how about this case:
> > > 1. set the brightness level to 5 via sysfs I/F
> > > 2. set the backlight to another level via HOTKEYS.
> > > 3. setting it back to 5 doesn't work because bd->props.brightness
> > > is still 5.
> > 
> > If they're using the ACPI interface then video.c should update 
> > props.brightness itself.
> > 
> > > IMO, the real problem is that bd->props.brightness doesn't reflect the
> > > actual brightness.
> > 
> > If you want the actual brightness, why aren't you reading 
> > actual_brightness?
> > 
> right, that's the problem.
> because IMO, we don't need the actual_brightness any more. :)
> And if you agree that bd->props.brightness doesn't reflect the actual
> brightness, we can see that
>                         if (bd->props.brightness != brightness) {
>                                 bd->props.brightness = brightness;
>                                 backlight_update_status(bd);
>                         }
> doesn't make sense because "bd->props.brightness != brightness" doesn't
> mean anything.
> 
> But if bd->props.brightness does reflect the actual brightness, then do
> we need "actual_brightness" any more?
> 
> 
> > If they're using the ACPI interface then video.c should update 
> > props.brightness itself.
> that also makes sense, patch attached.
> But anyway, we still need some changes in the video backlight class.
> right?
> 
> From: Zhang Rui <rui.zhang@intel.com>
> Subject: always update props.brightness when brightness is changed
> 
> always update pros.brightness no matter the backlight is changed
> via procfs, hotkeys or sysfs.
by the way, with this patch applied, we really don't need
"actual_brightness" any more, at least for ACPI backlight class I/F.
is there any chance that "actual_brightness" returns a different value
from "brightness"?

thanks,
rui

> 
> Sighed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/video.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> Index: linux-2.6/drivers/acpi/video.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/video.c
> +++ linux-2.6/drivers/acpi/video.c
> @@ -481,6 +481,7 @@ acpi_video_device_lcd_set_level(struct a
>  	int status = AE_OK;
>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>  	struct acpi_object_list args = { 1, &arg0 };
> +	int state;
>  
> 
>  	arg0.integer.value = level;
> @@ -489,6 +490,10 @@ acpi_video_device_lcd_set_level(struct a
>  		status = acpi_evaluate_object(device->dev->handle, "_BCM",
>  					      &args, NULL);
>  	device->brightness->curr = level;
> +	for (state = 2; state < device->brightness->count; state++)
> +		if (level == device->brightness->levels[state])
> +			device->backlight->props.brightness = state - 2;
> +
>  	return status;
>  }
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Matthew Garrett]

On Wed, Dec 31, 2008 at 10:58:48AM +0800, Zhang Rui wrote:
> On Wed, 2008-12-31 at 10:01 +0800, Matthew Garrett wrote:
> > If you want the actual brightness, why aren't you reading 
> > actual_brightness?
> > 
> right, that's the problem.
> because IMO, we don't need the actual_brightness any more. :)
> And if you agree that bd->props.brightness doesn't reflect the actual
> brightness, we can see that

Brightness and actual_brightness have different semantics, and 
maintaining that difference is worthwhile.

>                         if (bd->props.brightness != brightness) {
>                                 bd->props.brightness = brightness;
>                                 backlight_update_status(bd);
>                         }
> doesn't make sense because "bd->props.brightness != brightness" doesn't
> mean anything.

I agree that this doesn't seem like a meaningful check. For setups where 
brightness change is an expensive operation, this could be done in the 
driver rather than the core?

> But if bd->props.brightness does reflect the actual brightness, then do
> we need "actual_brightness" any more?

I think maintaining "brightness" as "user requested brightness" is 
sensible, for situations like Richard described. 

Patch looks good to me.
-- 
Matthew Garrett | mjg59@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Matthew Garrett]

On Wed, Dec 31, 2008 at 11:11:59AM +0800, Zhang Rui wrote:
> On Wed, 2008-12-31 at 10:58 +0800, Zhang Rui wrote:
> > always update pros.brightness no matter the backlight is changed
> > via procfs, hotkeys or sysfs.
> by the way, with this patch applied, we really don't need
> "actual_brightness" any more, at least for ACPI backlight class I/F.
> is there any chance that "actual_brightness" returns a different value
> from "brightness"?

Depending on _DOS, AC events can switch the backlight value without 
sending a notification. I don't /think/ it's worth worrying about that 
case, but it's still worth keeping the two attributes so that there's a 
consistent interface for userspace.

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

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Matthew Garrett]

Richard, how about something like this? If the hardware changes the 
state of the backlight behind us, the current code may make it 
impossible to set it back to the previous state without resynchronising 
the value first. Is there any currently supported hardware where this 
would be a sufficiently expensive or disruptive operation to warrant the 
check?

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index fab0bc8..064b428 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -134,10 +134,8 @@ static ssize_t backlight_store_brightness(struct device *dev,
 		else {
 			pr_debug("backlight: set brightness to %d\n",
 				 brightness);
-			if (bd->props.brightness != brightness) {
-				bd->props.brightness = brightness;
-				backlight_update_status(bd);
-			}
+			bd->props.brightness = brightness;
+			backlight_update_status(bd);
 			rc = count;
 		}
 	}

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

Re: [PATCH 1/2] video: always update the brightness when poking "brightness"

[Richard Purdie]

On Wed, 2008-12-31 at 13:41 +0000, Matthew Garrett wrote:
> Richard, how about something like this? If the hardware changes the 
> state of the backlight behind us, the current code may make it 
> impossible to set it back to the previous state without resynchronising 
> the value first. Is there any currently supported hardware where this 
> would be a sufficiently expensive or disruptive operation to warrant the 
> check?

I'll accept that argument. It is an expensive operation on some hardware
but that path already has sysfs writes etc. so it doesn't really matter.
If it really bothers some hardware they can make the check in the driver
I guess.

I'll be sorting out the backlight tree in the next day or so and will
queue something like this for merging.

Cheers,

Richard


> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index fab0bc8..064b428 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -134,10 +134,8 @@ static ssize_t backlight_store_brightness(struct device *dev,
>  		else {
>  			pr_debug("backlight: set brightness to %d\n",
>  				 brightness);
> -			if (bd->props.brightness != brightness) {
> -				bd->props.brightness = brightness;
> -				backlight_update_status(bd);
> -			}
> +			bd->props.brightness = brightness;
> +			backlight_update_status(bd);
>  			rc = count;
>  		}
>  	}
> 
-- 
Richard Purdie
Intel Open Source Technology Centre