[PATCH] acpi: video: fix reversed indexed BQC

[PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
index values) assumed that bl->levels were not reverted, but at this
point they already are, so there's no need to revert them yet again.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/video.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..b27c049 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -499,19 +499,10 @@ acpi_video_bqc_value_to_level(struct acpi_video_device *device,
 {
 	unsigned long long level;
 
-	if (device->brightness->flags._BQC_use_index) {
-		/*
-		 * _BQC returns an index that doesn't account for
-		 * the first 2 items with special meaning, so we need
-		 * to compensate for that by offsetting ourselves
-		 */
-		if (device->brightness->flags._BCL_reversed)
-			bqc_value = device->brightness->count - 3 - bqc_value;
-
+	if (device->brightness->flags._BQC_use_index)
 		level = device->brightness->levels[bqc_value + 2];
-	} else {
+	else
 		level = bqc_value;
-	}
 
 	level += bqc_offset_aml_bug_workaround;
 
-- 
1.8.3.4

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Aaron Lu]

On 08/02/2013 07:34 AM, Felipe Contreras wrote:
> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
> index values) assumed that bl->levels were not reverted, but at this
> point they already are, so there's no need to revert them yet again.

When acpi_video_bqc_value_to_level is called, bl->levels is not
reverted. Can you please point me the code path when it is called and
bl->levels were still reverted? I think we will need to fix that then.

-Aaron

> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  drivers/acpi/video.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 0ec434d..b27c049 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -499,19 +499,10 @@ acpi_video_bqc_value_to_level(struct acpi_video_device *device,
>  {
>  	unsigned long long level;
>  
> -	if (device->brightness->flags._BQC_use_index) {
> -		/*
> -		 * _BQC returns an index that doesn't account for
> -		 * the first 2 items with special meaning, so we need
> -		 * to compensate for that by offsetting ourselves
> -		 */
> -		if (device->brightness->flags._BCL_reversed)
> -			bqc_value = device->brightness->count - 3 - bqc_value;
> -
> +	if (device->brightness->flags._BQC_use_index)
>  		level = device->brightness->levels[bqc_value + 2];
> -	} else {
> +	else
>  		level = bqc_value;
> -	}
>  
>  	level += bqc_offset_aml_bug_workaround;
>  
> 

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> On 08/02/2013 07:34 AM, Felipe Contreras wrote:
>> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
>> index values) assumed that bl->levels were not reverted, but at this
>> point they already are, so there's no need to revert them yet again.
>
> When acpi_video_bqc_value_to_level is called, bl->levels is not
> reverted.

This is the code that turns br->flags._BCL_reversed on:

	if (max_level == br->levels[2]) {
		br->flags._BCL_reversed = 1;
		sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
			acpi_video_cmp_level, NULL);
	}

Now tell me how br->flags._BCL_reversed can be on, and the br->levels
*not* reverted.

-- 
Felipe Contreras

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Aaron Lu]

On 08/02/2013 12:11 PM, Felipe Contreras wrote:
> On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> On 08/02/2013 07:34 AM, Felipe Contreras wrote:
>>> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
>>> index values) assumed that bl->levels were not reverted, but at this
>>> point they already are, so there's no need to revert them yet again.
>>
>> When acpi_video_bqc_value_to_level is called, bl->levels is not
>> reverted.
> 
> This is the code that turns br->flags._BCL_reversed on:
> 
> 	if (max_level == br->levels[2]) {
> 		br->flags._BCL_reversed = 1;
> 		sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
> 			acpi_video_cmp_level, NULL);
> 	}
> 
> Now tell me how br->flags._BCL_reversed can be on, and the br->levels
> *not* reverted.

Oh yes, it is reverted to be in increase order, so it is not in reverse
order. I'm a little confused by these words.

Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
level on a reversed _BCL, so we will need to revert level here too.

-Aaron

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/02/2013 12:11 PM, Felipe Contreras wrote:
>> On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>>> On 08/02/2013 07:34 AM, Felipe Contreras wrote:
>>>> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
>>>> index values) assumed that bl->levels were not reverted, but at this
>>>> point they already are, so there's no need to revert them yet again.
>>>
>>> When acpi_video_bqc_value_to_level is called, bl->levels is not
>>> reverted.
>>
>> This is the code that turns br->flags._BCL_reversed on:
>>
>>       if (max_level == br->levels[2]) {
>>               br->flags._BCL_reversed = 1;
>>               sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
>>                       acpi_video_cmp_level, NULL);
>>       }
>>
>> Now tell me how br->flags._BCL_reversed can be on, and the br->levels
>> *not* reverted.
>
> Oh yes, it is reverted to be in increase order, so it is not in reverse
> order. I'm a little confused by these words.

br->levels is always ascending, but that doesn't mean _BCL is.

> Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
> level on a reversed _BCL, so we will need to revert level here too.

I cannot parse that sentence, but nothing needs to change there; to
find out if _BQC is using an index, we need to see if the returned
value is the index of the level we are looking for, and to find that
out we need the original list of levels, which can be found by
reverting the already reverted list. If this wasn't the case there
would not be any need for the _BCL_reversed flag.

-- 
Felipe Contreras

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Aaron Lu]

On 08/02/2013 12:50 PM, Felipe Contreras wrote:
> On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu <aaron.lu@intel.com> wrote:
>> On 08/02/2013 12:11 PM, Felipe Contreras wrote:
>>> On Thu, Aug 1, 2013 at 9:03 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>>>> On 08/02/2013 07:34 AM, Felipe Contreras wrote:
>>>>> Commit 1a7c618 (ACPI video: support _BQC/_BCL/_BCM methods that use
>>>>> index values) assumed that bl->levels were not reverted, but at this
>>>>> point they already are, so there's no need to revert them yet again.
>>>>
>>>> When acpi_video_bqc_value_to_level is called, bl->levels is not
>>>> reverted.
>>>
>>> This is the code that turns br->flags._BCL_reversed on:
>>>
>>>       if (max_level == br->levels[2]) {
>>>               br->flags._BCL_reversed = 1;
>>>               sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
>>>                       acpi_video_cmp_level, NULL);
>>>       }
>>>
>>> Now tell me how br->flags._BCL_reversed can be on, and the br->levels
>>> *not* reverted.
>>
>> Oh yes, it is reverted to be in increase order, so it is not in reverse
>> order. I'm a little confused by these words.
> 
> br->levels is always ascending, but that doesn't mean _BCL is.

Right.

> 
>> Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
>> level on a reversed _BCL, so we will need to revert level here too.
> 
> I cannot parse that sentence, but nothing needs to change there; to
> find out if _BQC is using an index, we need to see if the returned
> value is the index of the level we are looking for, and to find that
> out we need the original list of levels, which can be found by
> reverting the already reverted list. If this wasn't the case there

Yes, but instead of reverting the already reverted list, we revert the
returned value to get the correct index in the reverted list. But your
patch removes the revert, is it correct?

> would not be any need for the _BCL_reversed flag.
> 
-Aaron

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

On Thu, Aug 1, 2013 at 11:59 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/02/2013 12:50 PM, Felipe Contreras wrote:
>> On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu <aaron.lu@intel.com> wrote:

>>> Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
>>> level on a reversed _BCL, so we will need to revert level here too.
>>
>> I cannot parse that sentence, but nothing needs to change there; to
>> find out if _BQC is using an index, we need to see if the returned
>> value is the index of the level we are looking for, and to find that
>> out we need the original list of levels, which can be found by
>> reverting the already reverted list. If this wasn't the case there
>
> Yes, but instead of reverting the already reverted list, we revert the
> returned value to get the correct index in the reverted list. But your
> patch removes the revert, is it correct?

It removes the revert in acpi_video_bqc_value_to_level(), not
acpi_video_bqc_quirk(). If I remove both reverts it doesn't work on
this machine, however, I think it works by accident.

The initial _BCM commands don't work, so the level remains at 100%.
Since the level is max_level, acpi_video_bqc_quirk() tries with the
first level, which is 0, and 0 happens to be the index of 100.

So _BQC is returning 100, which is not the index of 0 (what we tested
for), but actually 100.

I think the current code is correct, but acpi_video_bqc_quirk() should
be testing br->levels[3], or anything other than 0/100 which can be
easily confused.

If so, the code would find that _BQC doesn't work on this machine (in
win8 mode)... at least initially. My guess is that it only starts to
work after acpi_video_bus_start_devices() is called.

Forcing br->flags._BQC_use_index = 0 seems to work.

-- 
Felipe Contreras

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Aaron Lu]

On 08/02/2013 02:44 PM, Felipe Contreras wrote:
> On Thu, Aug 1, 2013 at 11:59 PM, Aaron Lu <aaron.lu@intel.com> wrote:
>> On 08/02/2013 12:50 PM, Felipe Contreras wrote:
>>> On Thu, Aug 1, 2013 at 11:30 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> 
>>>> Please see acpi_video_bqc_quirk, we set _BQC_use_index by revert the
>>>> level on a reversed _BCL, so we will need to revert level here too.
>>>
>>> I cannot parse that sentence, but nothing needs to change there; to
>>> find out if _BQC is using an index, we need to see if the returned
>>> value is the index of the level we are looking for, and to find that
>>> out we need the original list of levels, which can be found by
>>> reverting the already reverted list. If this wasn't the case there
>>
>> Yes, but instead of reverting the already reverted list, we revert the
>> returned value to get the correct index in the reverted list. But your
>> patch removes the revert, is it correct?
> 
> It removes the revert in acpi_video_bqc_value_to_level(), not
> acpi_video_bqc_quirk(). If I remove both reverts it doesn't work on

Of course I mean you remove the revert in acpi_video_bqc_value_to_level
as is shown in your patch.

> this machine, however, I think it works by accident.
> 
> The initial _BCM commands don't work, so the level remains at 100%.
> Since the level is max_level, acpi_video_bqc_quirk() tries with the
> first level, which is 0, and 0 happens to be the index of 100.
> 
> So _BQC is returning 100, which is not the index of 0 (what we tested
> for), but actually 100.
> 
> I think the current code is correct, but acpi_video_bqc_quirk() should
> be testing br->levels[3], or anything other than 0/100 which can be
> easily confused.
> 
> If so, the code would find that _BQC doesn't work on this machine (in
> win8 mode)... at least initially. My guess is that it only starts to
> work after acpi_video_bus_start_devices() is called.
> 
> Forcing br->flags._BQC_use_index = 0 seems to work.

Seems ASUS machines tend to have this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=52951
https://bugzilla.kernel.org/show_bug.cgi?id=56711

I have a patch to enhance the quirk some time ago:
https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
But at that time we have decided to remove acpi video interface if this
is a win8 system(all of these affected systems are win8), so I didn't
submit it. Can you please give it a test? Perhaps I can send it now.

-Aaron

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/02/2013 02:44 PM, Felipe Contreras wrote:

>> The initial _BCM commands don't work, so the level remains at 100%.
>> Since the level is max_level, acpi_video_bqc_quirk() tries with the
>> first level, which is 0, and 0 happens to be the index of 100.
>>
>> So _BQC is returning 100, which is not the index of 0 (what we tested
>> for), but actually 100.
>>
>> I think the current code is correct, but acpi_video_bqc_quirk() should
>> be testing br->levels[3], or anything other than 0/100 which can be
>> easily confused.
>>
>> If so, the code would find that _BQC doesn't work on this machine (in
>> win8 mode)... at least initially. My guess is that it only starts to
>> work after acpi_video_bus_start_devices() is called.
>>
>> Forcing br->flags._BQC_use_index = 0 seems to work.
>
> Seems ASUS machines tend to have this issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=52951
> https://bugzilla.kernel.org/show_bug.cgi?id=56711

I don't see any real solution for the ACPI driver.

> I have a patch to enhance the quirk some time ago:
> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9

I think this is unnecessarily complicated; the comment makes it clear
that the purpose is to find out if _BQC is working, and this does the
trick:

>From 2bfa401b0a50fcde292ac0eb60cb6f857caf2fc6 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Fri, 2 Aug 2013 02:27:44 -0500
Subject: [PATCH] acpi: video: improve quirk check

If the _BCL package is descending, the first level (br->levels[2]) will
be 0, and if the number of levels matches the number of steps, we might
confuse a returned level to mean the index.

For example:

  current_level = max_level = 100
  test_level = 0
  returned level = 100

In this case 100 means the level, not the index, and _BCM failed. But if
the _BCL package is descending, the index of level 0 is also 100, so we
assume _BQC is indexed, when it's not.

The solution is simple; test anything other than the first level (e.g.
1).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..e1284b8 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -689,7 +689,7 @@ static int acpi_video_bqc_quirk(struct
acpi_video_device *device,
 	 * Some systems always report current brightness level as maximum
 	 * through _BQC, we need to test another value for them.
 	 */
-	test_level = current_level == max_level ? br->levels[2] : max_level;
+	test_level = current_level == max_level ? br->levels[3] : max_level;

 	result = acpi_video_device_lcd_set_level(device, test_level);
 	if (result)
-- 
1.8.3.4

-- 
Felipe Contreras

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Aaron Lu]

On 08/02/2013 03:59 PM, Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu <aaron.lu@intel.com> wrote:
>> On 08/02/2013 02:44 PM, Felipe Contreras wrote:
> 
>>> The initial _BCM commands don't work, so the level remains at 100%.
>>> Since the level is max_level, acpi_video_bqc_quirk() tries with the
>>> first level, which is 0, and 0 happens to be the index of 100.
>>>
>>> So _BQC is returning 100, which is not the index of 0 (what we tested
>>> for), but actually 100.
>>>
>>> I think the current code is correct, but acpi_video_bqc_quirk() should
>>> be testing br->levels[3], or anything other than 0/100 which can be
>>> easily confused.
>>>
>>> If so, the code would find that _BQC doesn't work on this machine (in
>>> win8 mode)... at least initially. My guess is that it only starts to
>>> work after acpi_video_bus_start_devices() is called.
>>>
>>> Forcing br->flags._BQC_use_index = 0 seems to work.
>>
>> Seems ASUS machines tend to have this issue:
>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>> https://bugzilla.kernel.org/show_bug.cgi?id=56711
> 
> I don't see any real solution for the ACPI driver.
> 
>> I have a patch to enhance the quirk some time ago:
>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
> 
> I think this is unnecessarily complicated; the comment makes it clear

For your system, yes it is unnecessarily complicated. But since this is
a quirk, it better solves as many potential problems as possible, or we
would simply use a DMI entry to do the quirk.

-Aaron

> that the purpose is to find out if _BQC is working, and this does the
> trick:
> 
> From 2bfa401b0a50fcde292ac0eb60cb6f857caf2fc6 Mon Sep 17 00:00:00 2001
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Date: Fri, 2 Aug 2013 02:27:44 -0500
> Subject: [PATCH] acpi: video: improve quirk check
> 
> If the _BCL package is descending, the first level (br->levels[2]) will
> be 0, and if the number of levels matches the number of steps, we might
> confuse a returned level to mean the index.
> 
> For example:
> 
>   current_level = max_level = 100
>   test_level = 0
>   returned level = 100
> 
> In this case 100 means the level, not the index, and _BCM failed. But if
> the _BCL package is descending, the index of level 0 is also 100, so we
> assume _BQC is indexed, when it's not.
> 
> The solution is simple; test anything other than the first level (e.g.
> 1).
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  drivers/acpi/video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 0ec434d..e1284b8 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -689,7 +689,7 @@ static int acpi_video_bqc_quirk(struct
> acpi_video_device *device,
>  	 * Some systems always report current brightness level as maximum
>  	 * through _BQC, we need to test another value for them.
>  	 */
> -	test_level = current_level == max_level ? br->levels[2] : max_level;
> +	test_level = current_level == max_level ? br->levels[3] : max_level;
> 
>  	result = acpi_video_device_lcd_set_level(device, test_level);
>  	if (result)
> 

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/02/2013 03:59 PM, Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu <aaron.lu@intel.com> wrote:
>>> On 08/02/2013 02:44 PM, Felipe Contreras wrote:
>>
>>>> The initial _BCM commands don't work, so the level remains at 100%.
>>>> Since the level is max_level, acpi_video_bqc_quirk() tries with the
>>>> first level, which is 0, and 0 happens to be the index of 100.
>>>>
>>>> So _BQC is returning 100, which is not the index of 0 (what we tested
>>>> for), but actually 100.
>>>>
>>>> I think the current code is correct, but acpi_video_bqc_quirk() should
>>>> be testing br->levels[3], or anything other than 0/100 which can be
>>>> easily confused.
>>>>
>>>> If so, the code would find that _BQC doesn't work on this machine (in
>>>> win8 mode)... at least initially. My guess is that it only starts to
>>>> work after acpi_video_bus_start_devices() is called.
>>>>
>>>> Forcing br->flags._BQC_use_index = 0 seems to work.
>>>
>>> Seems ASUS machines tend to have this issue:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>> https://bugzilla.kernel.org/show_bug.cgi?id=56711
>>
>> I don't see any real solution for the ACPI driver.
>>
>>> I have a patch to enhance the quirk some time ago:
>>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>
>> I think this is unnecessarily complicated; the comment makes it clear
>
> For your system, yes it is unnecessarily complicated. But since this is
> a quirk, it better solves as many potential problems as possible, or we
> would simply use a DMI entry to do the quirk.

The only difference between my patch and yours is that your patch
checks that br->level[i] is not the current level, but that check is
not necessary. If _BQC always returns the max level, all we need to do
is pick another value, any other value, and br->level[3] works just
fine.

-- 
Felipe Contreras

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Aaron Lu]

On 08/02/2013 04:14 PM, Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu <aaron.lu@intel.com> wrote:
>> On 08/02/2013 03:59 PM, Felipe Contreras wrote:
>>> On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu <aaron.lu@intel.com> wrote:
>>>> On 08/02/2013 02:44 PM, Felipe Contreras wrote:
>>>
>>>>> The initial _BCM commands don't work, so the level remains at 100%.
>>>>> Since the level is max_level, acpi_video_bqc_quirk() tries with the
>>>>> first level, which is 0, and 0 happens to be the index of 100.
>>>>>
>>>>> So _BQC is returning 100, which is not the index of 0 (what we tested
>>>>> for), but actually 100.
>>>>>
>>>>> I think the current code is correct, but acpi_video_bqc_quirk() should
>>>>> be testing br->levels[3], or anything other than 0/100 which can be
>>>>> easily confused.
>>>>>
>>>>> If so, the code would find that _BQC doesn't work on this machine (in
>>>>> win8 mode)... at least initially. My guess is that it only starts to
>>>>> work after acpi_video_bus_start_devices() is called.
>>>>>
>>>>> Forcing br->flags._BQC_use_index = 0 seems to work.
>>>>
>>>> Seems ASUS machines tend to have this issue:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=56711
>>>
>>> I don't see any real solution for the ACPI driver.
>>>
>>>> I have a patch to enhance the quirk some time ago:
>>>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>>
>>> I think this is unnecessarily complicated; the comment makes it clear
>>
>> For your system, yes it is unnecessarily complicated. But since this is
>> a quirk, it better solves as many potential problems as possible, or we
>> would simply use a DMI entry to do the quirk.
> 
> The only difference between my patch and yours is that your patch
> checks that br->level[i] is not the current level, but that check is
> not necessary. If _BQC always returns the max level, all we need to do

_BQC does not always returns the max level.

> is pick another value, any other value, and br->level[3] works just
> fine.

For a _BCL only having 4 elements { 100, 40, 40, 100 }, the br->levels[3]
will be the max level. The example here may be too crazy to be true, but
since we are dealing with firmware, I tend to believe anything could
happen.

-Aaron

Re: [PATCH] acpi: video: fix reversed indexed BQC

[Felipe Contreras]

On Fri, Aug 2, 2013 at 3:25 AM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 08/02/2013 04:14 PM, Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 3:06 AM, Aaron Lu <aaron.lu@intel.com> wrote:
>>> On 08/02/2013 03:59 PM, Felipe Contreras wrote:
>>>> On Fri, Aug 2, 2013 at 1:56 AM, Aaron Lu <aaron.lu@intel.com> wrote:
>>>>> On 08/02/2013 02:44 PM, Felipe Contreras wrote:
>>>>
>>>>>> The initial _BCM commands don't work, so the level remains at 100%.
>>>>>> Since the level is max_level, acpi_video_bqc_quirk() tries with the
>>>>>> first level, which is 0, and 0 happens to be the index of 100.
>>>>>>
>>>>>> So _BQC is returning 100, which is not the index of 0 (what we tested
>>>>>> for), but actually 100.
>>>>>>
>>>>>> I think the current code is correct, but acpi_video_bqc_quirk() should
>>>>>> be testing br->levels[3], or anything other than 0/100 which can be
>>>>>> easily confused.
>>>>>>
>>>>>> If so, the code would find that _BQC doesn't work on this machine (in
>>>>>> win8 mode)... at least initially. My guess is that it only starts to
>>>>>> work after acpi_video_bus_start_devices() is called.
>>>>>>
>>>>>> Forcing br->flags._BQC_use_index = 0 seems to work.
>>>>>
>>>>> Seems ASUS machines tend to have this issue:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=56711
>>>>
>>>> I don't see any real solution for the ACPI driver.
>>>>
>>>>> I have a patch to enhance the quirk some time ago:
>>>>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>>>
>>>> I think this is unnecessarily complicated; the comment makes it clear
>>>
>>> For your system, yes it is unnecessarily complicated. But since this is
>>> a quirk, it better solves as many potential problems as possible, or we
>>> would simply use a DMI entry to do the quirk.
>>
>> The only difference between my patch and yours is that your patch
>> checks that br->level[i] is not the current level, but that check is
>> not necessary. If _BQC always returns the max level, all we need to do
>
> _BQC does not always returns the max level.
>
>> is pick another value, any other value, and br->level[3] works just
>> fine.
>
> For a _BCL only having 4 elements { 100, 40, 40, 100 }, the br->levels[3]
> will be the max level. The example here may be too crazy to be true, but
> since we are dealing with firmware, I tend to believe anything could
> happen.

That can be fixed easily by checking test_level == current_level and
do the same your patch does, but I actually don't think we should do
that. It might make sense to test two values instead of only one, that
way we can we properly test _BQC, while your patch simply assumes it
doesn't work, even though it might.

-- 
Felipe Contreras