[PATCH 0/3] ACPI video: Fix brightness control initialization sequence

[PATCH 0/3] ACPI video: Fix brightness control initialization sequence

[Danny Baumann]

This patch set fixes a some logic mistakes in the brightness control
initialization sequence of the ACPI video driver. This should make
the initialization (and as a consequence in-kernel brightness control)
work for a number of laptops. One example that was broken before is the
Dell Inspiron 15R SE (7520).

Regards,

Danny

Danny Baumann (3):
  ACPI video: Fix brightness control initialization for some laptops.
  ACPI video: Make logic a little easier to understand.
  ACPI video: Fix applying indexed initial brightness value.

 drivers/acpi/video.c | 59 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 23 deletions(-)

-- 
1.8.1.4

[PATCH 1/3] ACPI video: Fix brightness control initialization for some laptops.

[Danny Baumann]

In particular, this fixes brightness control initialization for all
devices that return index values from _BQC and don't happen to have the
initial index set by the BIOS in their _BCL table. One example for that
is the Dell Inspiron 15R SE (model number 7520).

What happened for those devices is that acpi_init_brightness queried the
initial brightness by calling acpi_video_device_lcd_get_level_current.
This called _BQC, which returned e.g. 13. As _BQC_use_index isn't
determined at this point (and thus has its initial value of 0), the
index isn't converted into the actual level. As '13' isn't present in
the _BCL list, *level is later overwritten with brightness->curr, which
was initialized to max_level (100) before. Later in
acpi_init_brightness, level_old (with the value 100) is used as an index
into the _BCL table, which causes a value outside of the actual table to
be used as input into acpi_video_device_lcd_set_level(). Depending on
the (undefined) value of that location, this call will fail, causing the
brightness control for the device in question not to be enabled.

Fix that by returning the raw value returned by the _BQC call in the
initialization case.
---
 drivers/acpi/video.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 313f959..ef85574 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -463,6 +463,15 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 		status = acpi_evaluate_integer(device->dev->handle, buf,
 						NULL, level);
 		if (ACPI_SUCCESS(status)) {
+			if (init) {
+				/*
+				 * At init time we don't yet know whether the
+				 * value is indexed or not. Don't mess with it
+				 * until we have determined how to handle it.
+				 */
+				return 0;
+			}
+
 			if (device->brightness->flags._BQC_use_index) {
 				if (device->brightness->flags._BCL_reversed)
 					*level = device->brightness->count
@@ -476,16 +485,14 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 					device->brightness->curr = *level;
 					return 0;
 			}
-			if (!init) {
-				/*
-				 * BQC returned an invalid level.
-				 * Stop using it.
-				 */
-				ACPI_WARNING((AE_INFO,
-					      "%s returned an invalid level",
-					      buf));
-				device->cap._BQC = device->cap._BCQ = 0;
-			}
+			/*
+			 * BQC returned an invalid level.
+			 * Stop using it.
+			 */
+			ACPI_WARNING((AE_INFO,
+				      "%s returned an invalid level",
+				      buf));
+			device->cap._BQC = device->cap._BCQ = 0;
 		} else {
 			/* Fixme:
 			 * should we return an error or ignore this failure?
@@ -714,7 +721,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	if (result)
 		goto out_free_levels;
 
-	result = acpi_video_device_lcd_get_level_current(device, &level, 0);
+	result = acpi_video_device_lcd_get_level_current(device, &level, 1);
 	if (result)
 		goto out_free_levels;
 
-- 
1.8.1.4

Re: [PATCH 1/3] ACPI video: Fix brightness control initialization for some laptops.

[Aaron Lu]

On 03/14/2013 06:34 PM, Danny Baumann wrote:
> In particular, this fixes brightness control initialization for all
> devices that return index values from _BQC and don't happen to have the
> initial index set by the BIOS in their _BCL table. One example for that
> is the Dell Inspiron 15R SE (model number 7520).
> 
> What happened for those devices is that acpi_init_brightness queried the
> initial brightness by calling acpi_video_device_lcd_get_level_current.
> This called _BQC, which returned e.g. 13. As _BQC_use_index isn't
> determined at this point (and thus has its initial value of 0), the
> index isn't converted into the actual level. As '13' isn't present in
> the _BCL list, *level is later overwritten with brightness->curr, which
> was initialized to max_level (100) before. Later in
> acpi_init_brightness, level_old (with the value 100) is used as an index
> into the _BCL table, which causes a value outside of the actual table to
> be used as input into acpi_video_device_lcd_set_level(). Depending on
> the (undefined) value of that location, this call will fail, causing the
> brightness control for the device in question not to be enabled.
> 
> Fix that by returning the raw value returned by the _BQC call in the
> initialization case.

You missed Signed-off-by tag here.
Other than that, Reviewed-by: Aaron Lu <aaron.lu@intel.com>.

Thanks,
Aaron

> ---
>  drivers/acpi/video.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 313f959..ef85574 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -463,6 +463,15 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>  		status = acpi_evaluate_integer(device->dev->handle, buf,
>  						NULL, level);
>  		if (ACPI_SUCCESS(status)) {
> +			if (init) {
> +				/*
> +				 * At init time we don't yet know whether the
> +				 * value is indexed or not. Don't mess with it
> +				 * until we have determined how to handle it.
> +				 */
> +				return 0;
> +			}
> +
>  			if (device->brightness->flags._BQC_use_index) {
>  				if (device->brightness->flags._BCL_reversed)
>  					*level = device->brightness->count
> @@ -476,16 +485,14 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>  					device->brightness->curr = *level;
>  					return 0;
>  			}
> -			if (!init) {
> -				/*
> -				 * BQC returned an invalid level.
> -				 * Stop using it.
> -				 */
> -				ACPI_WARNING((AE_INFO,
> -					      "%s returned an invalid level",
> -					      buf));
> -				device->cap._BQC = device->cap._BCQ = 0;
> -			}
> +			/*
> +			 * BQC returned an invalid level.
> +			 * Stop using it.
> +			 */
> +			ACPI_WARNING((AE_INFO,
> +				      "%s returned an invalid level",
> +				      buf));
> +			device->cap._BQC = device->cap._BCQ = 0;
>  		} else {
>  			/* Fixme:
>  			 * should we return an error or ignore this failure?
> @@ -714,7 +721,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>  	if (result)
>  		goto out_free_levels;
>  
> -	result = acpi_video_device_lcd_get_level_current(device, &level, 0);
> +	result = acpi_video_device_lcd_get_level_current(device, &level, 1);
>  	if (result)
>  		goto out_free_levels;
>  
> 

[PATCH 2/3] ACPI video: Make logic a little easier to understand.

[Danny Baumann]

Make code paths a little easier to follow, and don't needlessly continue
list iteration.
---
 drivers/acpi/video.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index ef85574..d0fade7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -736,16 +736,17 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 		 */
 		if (use_bios_initial_backlight) {
 			for (i = 2; i < br->count; i++)
-				if (level_old == br->levels[i])
+				if (level_old == br->levels[i]) {
 					level = level_old;
+					break;
+				}
 		}
-		goto set_level;
+	} else {
+		if (br->flags._BCL_reversed)
+			level_old = (br->count - 1) - level_old;
+		level = br->levels[level_old];
 	}
 
-	if (br->flags._BCL_reversed)
-		level_old = (br->count - 1) - level_old;
-	level = br->levels[level_old];
-
 set_level:
 	result = acpi_video_device_lcd_set_level(device, level);
 	if (result)
-- 
1.8.1.4

Re: [PATCH 2/3] ACPI video: Make logic a little easier to understand.

[Aaron Lu]

On 03/14/2013 06:34 PM, Danny Baumann wrote:
> Make code paths a little easier to follow, and don't needlessly continue
> list iteration.

Same here, please add Signed-off-by tag.
Reviewed-by: Aaron Lu <aaron.lu@intel.com>

Thanks,
Aaron

> ---
>  drivers/acpi/video.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index ef85574..d0fade7 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -736,16 +736,17 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>  		 */
>  		if (use_bios_initial_backlight) {
>  			for (i = 2; i < br->count; i++)
> -				if (level_old == br->levels[i])
> +				if (level_old == br->levels[i]) {
>  					level = level_old;
> +					break;
> +				}
>  		}
> -		goto set_level;
> +	} else {
> +		if (br->flags._BCL_reversed)
> +			level_old = (br->count - 1) - level_old;
> +		level = br->levels[level_old];
>  	}
>  
> -	if (br->flags._BCL_reversed)
> -		level_old = (br->count - 1) - level_old;
> -	level = br->levels[level_old];
> -
>  set_level:
>  	result = acpi_video_device_lcd_set_level(device, level);
>  	if (result)
> 

[PATCH 3/3] ACPI video: Fix applying indexed initial brightness value.

[Danny Baumann]

The value initially read via _BQC also needs to be offset by 2 to
compensate for the first 2 special items in _BCL. Introduce a helper
function to do the conversion in order to not needlessly duplicate code.
---
 drivers/acpi/video.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index d0fade7..562ccc3 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -450,6 +450,16 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
 	{}
 };
 
+static unsigned long long
+acpi_video_index_to_level(struct acpi_video_device *device,
+			  unsigned long long index)
+{
+	if (device->brightness->flags._BCL_reversed)
+		index = device->brightness->count - 3 - index;
+
+	return device->brightness->levels[index + 2];
+}
+
 static int
 acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 					unsigned long long *level, int init)
@@ -472,13 +482,10 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 				return 0;
 			}
 
-			if (device->brightness->flags._BQC_use_index) {
-				if (device->brightness->flags._BCL_reversed)
-					*level = device->brightness->count
-								 - 3 - (*level);
-				*level = device->brightness->levels[*level + 2];
+			if (device->brightness->flags._BQC_use_index)
+				*level = acpi_video_index_to_level(device,
+								   *level);
 
-			}
 			*level += bqc_offset_aml_bug_workaround;
 			for (i = 2; i < device->brightness->count; i++)
 				if (device->brightness->levels[i] == *level) {
@@ -742,9 +749,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 				}
 		}
 	} else {
-		if (br->flags._BCL_reversed)
-			level_old = (br->count - 1) - level_old;
-		level = br->levels[level_old];
+		level = acpi_video_index_to_level(device, level_old);
 	}
 
 set_level:
-- 
1.8.1.4

Re: [PATCH 3/3] ACPI video: Fix applying indexed initial brightness value.

[Aaron Lu]

On 03/14/2013 06:34 PM, Danny Baumann wrote:
> The value initially read via _BQC also needs to be offset by 2 to
> compensate for the first 2 special items in _BCL. Introduce a helper
> function to do the conversion in order to not needlessly duplicate code.
> ---
>  drivers/acpi/video.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index d0fade7..562ccc3 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -450,6 +450,16 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>  	{}
>  };
>  
> +static unsigned long long
> +acpi_video_index_to_level(struct acpi_video_device *device,
> +			  unsigned long long index)
> +{
> +	if (device->brightness->flags._BCL_reversed)
> +		index = device->brightness->count - 3 - index;
> +
> +	return device->brightness->levels[index + 2];
> +}

What about making this function also take care of the
bqc_offset_aml_bug_workaround? so that this function serves more like a
conversion from raw value to fixed value, the function name can perhaps
be named as: acpi_video_fix_bqc_value, or whatever you think is more
appropriate.

> +
>  static int
>  acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>  					unsigned long long *level, int init)
> @@ -472,13 +482,10 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>  				return 0;
>  			}
>  
> -			if (device->brightness->flags._BQC_use_index) {
> -				if (device->brightness->flags._BCL_reversed)
> -					*level = device->brightness->count
> -								 - 3 - (*level);
> -				*level = device->brightness->levels[*level + 2];
> +			if (device->brightness->flags._BQC_use_index)
> +				*level = acpi_video_index_to_level(device,
> +								   *level);
>  
> -			}
>  			*level += bqc_offset_aml_bug_workaround;
>  			for (i = 2; i < device->brightness->count; i++)
>  				if (device->brightness->levels[i] == *level) {
> @@ -742,9 +749,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>  				}
>  		}
>  	} else {
> -		if (br->flags._BCL_reversed)
> -			level_old = (br->count - 1) - level_old;
> -		level = br->levels[level_old];
> +		level = acpi_video_index_to_level(device, level_old);

And here, that new function should be used, which also takes care of the
offset_aml_bug problem(though in theory, the two problems may not happen
on the same BIOS table).

And the acpi_video_device_lcd_get_level_current's param init can
probably be renamed as raw, meaning if raw value is desired or fixed
value, but it's not a big deal.

Thanks,
Aaron

>  	}
>  
>  set_level:
> 

Re: [PATCH 3/3] ACPI video: Fix applying indexed initial brightness value.

[Danny Baumann]

Hi,

 >> +static unsigned long long
>> +acpi_video_index_to_level(struct acpi_video_device *device,
>> +			  unsigned long long index)
>> +{
>> +	if (device->brightness->flags._BCL_reversed)
>> +		index = device->brightness->count - 3 - index;
>> +
>> +	return device->brightness->levels[index + 2];
>> +}
>
> What about making this function also take care of the
> bqc_offset_aml_bug_workaround? so that this function serves more like a
> conversion from raw value to fixed value, the function name can perhaps
> be named as: acpi_video_fix_bqc_value, or whatever you think is more
> appropriate.

Makes sense to me. How about acpi_video_bqc_to_level? I'd suggest 
acpi_video_bqc_value_to_level, but that makes it hard to keep the 80 
char limit in acpi_video_device_lcd_get_level_current.

>> @@ -742,9 +749,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>>   				}
>>   		}
>>   	} else {
>> -		if (br->flags._BCL_reversed)
>> -			level_old = (br->count - 1) - level_old;
>> -		level = br->levels[level_old];
>> +		level = acpi_video_index_to_level(device, level_old);
>
> And here, that new function should be used, which also takes care of the
> offset_aml_bug problem(though in theory, the two problems may not happen
> on the same BIOS table).

But that new function (whatever it's named) is already used here?

BTW, shouldn't the use_bios_initial_backlight also be respected for the 
BQC-returns-index case? Currently it's only used for the 
BQC-returns-level case.

> And the acpi_video_device_lcd_get_level_current's param init can
> probably be renamed as raw, meaning if raw value is desired or fixed
> value, but it's not a big deal.

Agreed. I'll send a new patch set (this time with signed-off-by) once I 
get opinion on the question above.

Thanks,

Danny

Re: [PATCH 3/3] ACPI video: Fix applying indexed initial brightness value.

[Aaron Lu]

On 03/15/2013 04:55 PM, Danny Baumann wrote:
> Hi,
> 
>  >> +static unsigned long long
>>> +acpi_video_index_to_level(struct acpi_video_device *device,
>>> +			  unsigned long long index)
>>> +{
>>> +	if (device->brightness->flags._BCL_reversed)
>>> +		index = device->brightness->count - 3 - index;
>>> +
>>> +	return device->brightness->levels[index + 2];
>>> +}
>>
>> What about making this function also take care of the
>> bqc_offset_aml_bug_workaround? so that this function serves more like a
>> conversion from raw value to fixed value, the function name can perhaps
>> be named as: acpi_video_fix_bqc_value, or whatever you think is more
>> appropriate.
> 
> Makes sense to me. How about acpi_video_bqc_to_level? I'd suggest 
> acpi_video_bqc_value_to_level, but that makes it hard to keep the 80 
> char limit in acpi_video_device_lcd_get_level_current.

Right, so let's go with acpi_video_bqc_to_level.

> 
>>> @@ -742,9 +749,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>>>   				}
>>>   		}
>>>   	} else {
>>> -		if (br->flags._BCL_reversed)
>>> -			level_old = (br->count - 1) - level_old;
>>> -		level = br->levels[level_old];
>>> +		level = acpi_video_index_to_level(device, level_old);
>>
>> And here, that new function should be used, which also takes care of the
>> offset_aml_bug problem(though in theory, the two problems may not happen
>> on the same BIOS table).
> 
> But that new function (whatever it's named) is already used here?

Yes, I mean the new function that also takes care of offset_aml_bug :-)

> 
> BTW, shouldn't the use_bios_initial_backlight also be respected for the 
> BQC-returns-index case? Currently it's only used for the 
> BQC-returns-level case.

Definitely, we should care that.

> 
>> And the acpi_video_device_lcd_get_level_current's param init can
>> probably be renamed as raw, meaning if raw value is desired or fixed
>> value, but it's not a big deal.
> 
> Agreed. I'll send a new patch set (this time with signed-off-by) once I 
> get opinion on the question above.

Cool, thanks.

-Aaron