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.
Signed-off-by: Danny Baumann <[email protected]>
---
drivers/acpi/video.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 313f959..9c33871 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
int level);
static int acpi_video_device_lcd_get_level_current(
struct acpi_video_device *device,
- unsigned long long *level, int init);
+ unsigned long long *level, int raw);
static int acpi_video_get_next_level(struct acpi_video_device *device,
u32 level_current, u32 event);
static int acpi_video_switch_brightness(struct acpi_video_device *device,
@@ -452,7 +452,7 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
static int
acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
- unsigned long long *level, int init)
+ unsigned long long *level, int raw)
{
acpi_status status = AE_OK;
int i;
@@ -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 (raw) {
+ /*
+ * Caller has indicated he wants the raw
+ * value returned by _BQC, so don't furtherly
+ * mess with the value.
+ */
+ 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
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 that does the BQC-value-to-level conversion in order to not
needlessly duplicate code.
Signed-off-by: Danny Baumann <[email protected]>
---
drivers/acpi/video.c | 52 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 33 insertions(+), 19 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 5ef329a..8faea35 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -450,6 +450,26 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
{}
};
+static unsigned long long
+acpi_video_bqc_value_to_level(struct acpi_video_device *device,
+ unsigned long long bqc_value)
+{
+ unsigned long long level;
+
+ if (device->brightness->flags._BQC_use_index) {
+ if (device->brightness->flags._BCL_reversed)
+ bqc_value = device->brightness->count - 3 - bqc_value;
+
+ level = device->brightness->levels[bqc_value + 2];
+ } else {
+ level = bqc_value;
+ }
+
+ level += bqc_offset_aml_bug_workaround;
+
+ return level;
+}
+
static int
acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
unsigned long long *level, int raw)
@@ -472,14 +492,8 @@ 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];
+ *level = acpi_video_bqc_value_to_level(device, *level);
- }
- *level += bqc_offset_aml_bug_workaround;
for (i = 2; i < device->brightness->count; i++)
if (device->brightness->levels[i] == *level) {
device->brightness->curr = *level;
@@ -727,24 +741,24 @@ acpi_video_init_brightness(struct acpi_video_device *device)
br->flags._BQC_use_index = (level == max_level ? 0 : 1);
- if (!br->flags._BQC_use_index) {
- /*
- * Set the backlight to the initial state.
- * On some buggy laptops, _BQC returns an uninitialized value
- * when invoked for the first time, i.e. level_old is invalid.
- * set the backlight to max_level in this case
- */
- if (use_bios_initial_backlight) {
+ if (use_bios_initial_backlight) {
+ if (!br->flags._BQC_use_index) {
+ /*
+ * Set the backlight to the initial state.
+ * On some buggy laptops, _BQC returns an uninitialized
+ * value when invoked for the first time, i.e.
+ * level_old is invalid. Set the backlight to max_level
+ * in this case.
+ */
for (i = 2; i < br->count; i++)
if (level_old == br->levels[i]) {
level = level_old;
break;
}
+ } else {
+ level = acpi_video_bqc_value_to_level(device,
+ level_old);
}
- } else {
- if (br->flags._BCL_reversed)
- level_old = (br->count - 1) - level_old;
- level = br->levels[level_old];
}
set_level:
--
1.8.1.4
Make code paths a little easier to follow, and don't needlessly continue
list iteration.
Signed-off-by: Danny Baumann <[email protected]>
---
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 9c33871..5ef329a 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
On 03/15/2013 06:10 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.
>
> Signed-off-by: Danny Baumann <[email protected]>
> ---
> drivers/acpi/video.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 313f959..9c33871 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
> int level);
> static int acpi_video_device_lcd_get_level_current(
> struct acpi_video_device *device,
> - unsigned long long *level, int init);
> + unsigned long long *level, int raw);
Why not bool?
Thanks,
Aaron
> static int acpi_video_get_next_level(struct acpi_video_device *device,
> u32 level_current, u32 event);
> static int acpi_video_switch_brightness(struct acpi_video_device *device,
> @@ -452,7 +452,7 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>
> static int
> acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> - unsigned long long *level, int init)
> + unsigned long long *level, int raw)
> {
> acpi_status status = AE_OK;
> int i;
> @@ -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 (raw) {
> + /*
> + * Caller has indicated he wants the raw
> + * value returned by _BQC, so don't furtherly
> + * mess with the value.
> + */
> + 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;
>
>
On 03/15/2013 06:10 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 that does the BQC-value-to-level conversion in order to not
> needlessly duplicate code.
>
> Signed-off-by: Danny Baumann <[email protected]>
> ---
> drivers/acpi/video.c | 52 +++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 5ef329a..8faea35 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -450,6 +450,26 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
> {}
> };
>
> +static unsigned long long
> +acpi_video_bqc_value_to_level(struct acpi_video_device *device,
> + unsigned long long bqc_value)
> +{
> + unsigned long long level;
> +
> + if (device->brightness->flags._BQC_use_index) {
> + if (device->brightness->flags._BCL_reversed)
> + bqc_value = device->brightness->count - 3 - bqc_value;
> +
> + level = device->brightness->levels[bqc_value + 2];
I don't understand this, what does the +2 have to do here?
_BQC returned us an index, and then we should just convert it to level,
why +2?
The only explanation would be, for BIOS tables that return _BQC as
index, they are indexing from the 3rd entry. Is it the case? If so, I
think we need to put a comment here.
> + } else {
> + level = bqc_value;
> + }
> +
> + level += bqc_offset_aml_bug_workaround;
> +
> + return level;
> +}
> +
> static int
> acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> unsigned long long *level, int raw)
> @@ -472,14 +492,8 @@ 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];
> + *level = acpi_video_bqc_value_to_level(device, *level);
>
> - }
> - *level += bqc_offset_aml_bug_workaround;
> for (i = 2; i < device->brightness->count; i++)
> if (device->brightness->levels[i] == *level) {
> device->brightness->curr = *level;
> @@ -727,24 +741,24 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>
> br->flags._BQC_use_index = (level == max_level ? 0 : 1);
>
> - if (!br->flags._BQC_use_index) {
> - /*
> - * Set the backlight to the initial state.
> - * On some buggy laptops, _BQC returns an uninitialized value
> - * when invoked for the first time, i.e. level_old is invalid.
> - * set the backlight to max_level in this case
> - */
> - if (use_bios_initial_backlight) {
-------------------------------------------------------------------
> + if (use_bios_initial_backlight) {
> + if (!br->flags._BQC_use_index) {
> + /*
> + * Set the backlight to the initial state.
> + * On some buggy laptops, _BQC returns an uninitialized
> + * value when invoked for the first time, i.e.
> + * level_old is invalid. Set the backlight to max_level
> + * in this case.
> + */
> for (i = 2; i < br->count; i++)
> if (level_old == br->levels[i]) {
> level = level_old;
> break;
> }
> + } else {
> + level = acpi_video_bqc_value_to_level(device,
> + level_old);
What about we convert the value to level first?
if (use_bios_initial_backlight) {
level = acpi_video_bqc_value_to_level(device, level_old);
/*
* Set the backlight to the initial state.
* On some buggy laptops, _BQC returns an uninitialized
* value when invoked for the first time, i.e.
* level_old is invalid(no matter it is a level, or an
* index.) Set the backlight to max_level in this case.
*/
for (i = 2; i < br->count; i++)
if (level == br->levels[i])
break;
if (i == br->count)
level = max_level;
}
Thanks,
Aaron
> }
> - } else {
> - if (br->flags._BCL_reversed)
> - level_old = (br->count - 1) - level_old;
> - level = br->levels[level_old];
> }
>
> set_level:
>
Hi,
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index 313f959..9c33871 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
>> int level);
>> static int acpi_video_device_lcd_get_level_current(
>> struct acpi_video_device *device,
>> - unsigned long long *level, int init);
>> + unsigned long long *level, int raw);
>
> Why not bool?
For two reasons:
- It previous was int ;-)
- It allowed the invocations of this function to fit within one 80-char
line.
Neither reason is terribly strong, so if you prefer, I can convert this
argument to bool.
Regards,
Danny
On 03/18/2013 04:22 PM, Danny Baumann wrote:
> Hi,
>
>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>>> index 313f959..9c33871 100644
>>> --- a/drivers/acpi/video.c
>>> +++ b/drivers/acpi/video.c
>>> @@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
>>> int level);
>>> static int acpi_video_device_lcd_get_level_current(
>>> struct acpi_video_device *device,
>>> - unsigned long long *level, int init);
>>> + unsigned long long *level, int raw);
>>
>> Why not bool?
>
> For two reasons:
> - It previous was int ;-)
Right, but since we are doing something now, we can probably change it
for better :-)
> - It allowed the invocations of this function to fit within one 80-char
> line.
the unsigned long long for level made me curious, do we really need that?
int would suffice I think.
>
> Neither reason is terribly strong, so if you prefer, I can convert this
> argument to bool.
It's OK, I can do the cleaup some time later.
Thanks,
Aaron
On 03/18/2013 04:26 PM, Danny Baumann wrote:
> Hi,
>
> >> +static unsigned long long
>>> +acpi_video_bqc_value_to_level(struct acpi_video_device *device,
>>> + unsigned long long bqc_value)
>>> +{
>>> + unsigned long long level;
>>> +
>>> + if (device->brightness->flags._BQC_use_index) {
>>> + if (device->brightness->flags._BCL_reversed)
>>> + bqc_value = device->brightness->count - 3 - bqc_value;
>>> +
>>> + level = device->brightness->levels[bqc_value + 2];
>>
>> I don't understand this, what does the +2 have to do here?
>> _BQC returned us an index, and then we should just convert it to level,
>> why +2?
>>
>> The only explanation would be, for BIOS tables that return _BQC as
>> index, they are indexing from the 3rd entry. Is it the case? If so, I
>> think we need to put a comment here.
>
> Yes, that's the case. The old code did the same thing:
Yes, I see that, but I didn't really think about it until reviewing your
patch :-)
>
>>> - 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];
>>> + *level = acpi_video_bqc_value_to_level(device, *level);
>
> That's also the reason for the -3 instead of -1 in the BCL_reversed
> case. I can add a comment, though.
OK, thanks.
That would make people reading the code feel less confused, hopefully.
-Aaron
>
>>> + if (use_bios_initial_backlight) {
>>> + if (!br->flags._BQC_use_index) {
>>> + /*
>>> + * Set the backlight to the initial state.
>>> + * On some buggy laptops, _BQC returns an uninitialized
>>> + * value when invoked for the first time, i.e.
>>> + * level_old is invalid. Set the backlight to max_level
>>> + * in this case.
>>> + */
>>> for (i = 2; i < br->count; i++)
>>> if (level_old == br->levels[i]) {
>>> level = level_old;
>>> break;
>>> }
>>> + } else {
>>> + level = acpi_video_bqc_value_to_level(device,
>>> + level_old);
>>
>> What about we convert the value to level first?
>>
>> if (use_bios_initial_backlight) {
>> level = acpi_video_bqc_value_to_level(device, level_old);
>> /*
>> * Set the backlight to the initial state.
>> * On some buggy laptops, _BQC returns an uninitialized
>> * value when invoked for the first time, i.e.
>> * level_old is invalid(no matter it is a level, or an
>> * index.) Set the backlight to max_level in this case.
>> */
>> for (i = 2; i < br->count; i++)
>> if (level == br->levels[i])
>> break;
>> if (i == br->count)
>> level = max_level;
>> }
>
> That works as well, and looks cleaner to me as well. I'll change that.
>
> Regards,
>
> Danny
>
Hi,
>> +static unsigned long long
>> +acpi_video_bqc_value_to_level(struct acpi_video_device *device,
>> + unsigned long long bqc_value)
>> +{
>> + unsigned long long level;
>> +
>> + if (device->brightness->flags._BQC_use_index) {
>> + if (device->brightness->flags._BCL_reversed)
>> + bqc_value = device->brightness->count - 3 - bqc_value;
>> +
>> + level = device->brightness->levels[bqc_value + 2];
>
> I don't understand this, what does the +2 have to do here?
> _BQC returned us an index, and then we should just convert it to level,
> why +2?
>
> The only explanation would be, for BIOS tables that return _BQC as
> index, they are indexing from the 3rd entry. Is it the case? If so, I
> think we need to put a comment here.
Yes, that's the case. The old code did the same thing:
>> - 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];
>> + *level = acpi_video_bqc_value_to_level(device, *level);
That's also the reason for the -3 instead of -1 in the BCL_reversed
case. I can add a comment, though.
>> + if (use_bios_initial_backlight) {
>> + if (!br->flags._BQC_use_index) {
>> + /*
>> + * Set the backlight to the initial state.
>> + * On some buggy laptops, _BQC returns an uninitialized
>> + * value when invoked for the first time, i.e.
>> + * level_old is invalid. Set the backlight to max_level
>> + * in this case.
>> + */
>> for (i = 2; i < br->count; i++)
>> if (level_old == br->levels[i]) {
>> level = level_old;
>> break;
>> }
>> + } else {
>> + level = acpi_video_bqc_value_to_level(device,
>> + level_old);
>
> What about we convert the value to level first?
>
> if (use_bios_initial_backlight) {
> level = acpi_video_bqc_value_to_level(device, level_old);
> /*
> * Set the backlight to the initial state.
> * On some buggy laptops, _BQC returns an uninitialized
> * value when invoked for the first time, i.e.
> * level_old is invalid(no matter it is a level, or an
> * index.) Set the backlight to max_level in this case.
> */
> for (i = 2; i < br->count; i++)
> if (level == br->levels[i])
> break;
> if (i == br->count)
> level = max_level;
> }
That works as well, and looks cleaner to me as well. I'll change that.
Regards,
Danny