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.
This causes all _BQC calls to return bogus values causing weird behavior
from the user's perspective. For example: xbacklight -set 10; xbacklight
-set 20; would flash to 90% and then slowly down to the desired level
(20).
The solution is simple; test anything other than the first level (e.g.
1).
Signed-off-by: Felipe Contreras <[email protected]>
---
On top of this we might want to test yet another value, because br->levels[3]
might be the current value (although very unlikely).
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.4.rc1
On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
> 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.
>
> This causes all _BQC calls to return bogus values causing weird behavior
> from the user's perspective. For example: xbacklight -set 10; xbacklight
> -set 20; would flash to 90% and then slowly down to the desired level
> (20).
>
> The solution is simple; test anything other than the first level (e.g.
> 1).
>
> Signed-off-by: Felipe Contreras <[email protected]>
Looks reasonable.
Aaron, what do you think?
Rafael
> ---
>
> On top of this we might want to test yet another value, because br->levels[3]
> might be the current value (although very unlikely).
>
> 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)
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
>> 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.
>>
>> This causes all _BQC calls to return bogus values causing weird behavior
>> from the user's perspective. For example: xbacklight -set 10; xbacklight
>> -set 20; would flash to 90% and then slowly down to the desired level
>> (20).
>>
>> The solution is simple; test anything other than the first level (e.g.
>> 1).
>>
>> Signed-off-by: Felipe Contreras <[email protected]>
>
> Looks reasonable.
>
> Aaron, what do you think?
Aaron has a similar patch does many more checks. I think we should add
more checks, but I think those should go into a separate patch.
This patch alone fixes a real problem, which is rather urgent to fix,
and I did it this way so it's trivial to review and merge.
--
Felipe Contreras
On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
> >> 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.
> >>
> >> This causes all _BQC calls to return bogus values causing weird behavior
> >> from the user's perspective. For example: xbacklight -set 10; xbacklight
> >> -set 20; would flash to 90% and then slowly down to the desired level
> >> (20).
> >>
> >> The solution is simple; test anything other than the first level (e.g.
> >> 1).
> >>
> >> Signed-off-by: Felipe Contreras <[email protected]>
> >
> > Looks reasonable.
> >
> > Aaron, what do you think?
>
> Aaron has a similar patch does many more checks. I think we should add
> more checks, but I think those should go into a separate patch.
>
> This patch alone fixes a real problem, which is rather urgent to fix,
> and I did it this way so it's trivial to review and merge.
And I still would like to know the Aaron's opinion, what's wrong with that?
On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
>> >> 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.
>> >>
>> >> This causes all _BQC calls to return bogus values causing weird behavior
>> >> from the user's perspective. For example: xbacklight -set 10; xbacklight
>> >> -set 20; would flash to 90% and then slowly down to the desired level
>> >> (20).
>> >>
>> >> The solution is simple; test anything other than the first level (e.g.
>> >> 1).
>> >>
>> >> Signed-off-by: Felipe Contreras <[email protected]>
>> >
>> > Looks reasonable.
>> >
>> > Aaron, what do you think?
>>
>> Aaron has a similar patch does many more checks. I think we should add
>> more checks, but I think those should go into a separate patch.
>>
>> This patch alone fixes a real problem, which is rather urgent to fix,
>> and I did it this way so it's trivial to review and merge.
>
> And I still would like to know the Aaron's opinion, what's wrong with that?
Nothing. What's wrong with my clarification?
--
Felipe Contreras
On Friday, August 02, 2013 08:07:37 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote:
> >> On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
> >> >> 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.
> >> >>
> >> >> This causes all _BQC calls to return bogus values causing weird behavior
> >> >> from the user's perspective. For example: xbacklight -set 10; xbacklight
> >> >> -set 20; would flash to 90% and then slowly down to the desired level
> >> >> (20).
> >> >>
> >> >> The solution is simple; test anything other than the first level (e.g.
> >> >> 1).
> >> >>
> >> >> Signed-off-by: Felipe Contreras <[email protected]>
> >> >
> >> > Looks reasonable.
> >> >
> >> > Aaron, what do you think?
> >>
> >> Aaron has a similar patch does many more checks. I think we should add
> >> more checks, but I think those should go into a separate patch.
> >>
> >> This patch alone fixes a real problem, which is rather urgent to fix,
> >> and I did it this way so it's trivial to review and merge.
> >
> > And I still would like to know the Aaron's opinion, what's wrong with that?
>
> Nothing. What's wrong with my clarification?
You're not Aaron. :-)
On Fri, Aug 2, 2013 at 8:19 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Friday, August 02, 2013 08:07:37 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote:
>> >> On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <[email protected]> wrote:
>> >> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
>> >> >> 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.
>> >> >>
>> >> >> This causes all _BQC calls to return bogus values causing weird behavior
>> >> >> from the user's perspective. For example: xbacklight -set 10; xbacklight
>> >> >> -set 20; would flash to 90% and then slowly down to the desired level
>> >> >> (20).
>> >> >>
>> >> >> The solution is simple; test anything other than the first level (e.g.
>> >> >> 1).
>> >> >>
>> >> >> Signed-off-by: Felipe Contreras <[email protected]>
>> >> >
>> >> > Looks reasonable.
>> >> >
>> >> > Aaron, what do you think?
>> >>
>> >> Aaron has a similar patch does many more checks. I think we should add
>> >> more checks, but I think those should go into a separate patch.
>> >>
>> >> This patch alone fixes a real problem, which is rather urgent to fix,
>> >> and I did it this way so it's trivial to review and merge.
>> >
>> > And I still would like to know the Aaron's opinion, what's wrong with that?
>>
>> Nothing. What's wrong with my clarification?
>
> You're not Aaron. :-)
I can clarify and comment without your permission. All you can do is
disregard my comments, but others might find them useful, including
Aaron.
--
Felipe Contreras
On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote:
> On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
>> 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.
>>
>> This causes all _BQC calls to return bogus values causing weird behavior
>> from the user's perspective. For example: xbacklight -set 10; xbacklight
>> -set 20; would flash to 90% and then slowly down to the desired level
>> (20).
>>
>> The solution is simple; test anything other than the first level (e.g.
>> 1).
>>
>> Signed-off-by: Felipe Contreras <[email protected]>
>
> Looks reasonable.
>
> Aaron, what do you think?
Yes, the patch is correct, but I still prefer my own version :-)
https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
In case you want to take mine and mine needs refresh, please let me know
and I can do the re-base, thanks.
-Aaron
>
> Rafael
>
>
>> ---
>>
>> On top of this we might want to test yet another value, because br->levels[3]
>> might be the current value (although very unlikely).
>>
>> 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)
>>
On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
> On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote:
> > On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
> >> 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.
> >>
> >> This causes all _BQC calls to return bogus values causing weird behavior
> >> from the user's perspective. For example: xbacklight -set 10; xbacklight
> >> -set 20; would flash to 90% and then slowly down to the desired level
> >> (20).
> >>
> >> The solution is simple; test anything other than the first level (e.g.
> >> 1).
> >>
> >> Signed-off-by: Felipe Contreras <[email protected]>
> >
> > Looks reasonable.
> >
> > Aaron, what do you think?
>
> Yes, the patch is correct, but I still prefer my own version :-)
> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>
> In case you want to take mine and mine needs refresh, please let me know
> and I can do the re-base, thanks.
Well, I prefer simpler, unless there's a good reason to use more complicated.
Why exactly do you think your version is better?
Rafael
On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <[email protected]> wrote:
> On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>> Yes, the patch is correct, but I still prefer my own version :-)
>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>
>> In case you want to take mine and mine needs refresh, please let me know
>> and I can do the re-base, thanks.
>
> Well, I prefer simpler, unless there's a good reason to use more complicated.
Note that these are not exclusionary; his patch can be applied on top
of mine. I don't think his patch is needed though.
--
Felipe Contreras
On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote:
> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <[email protected]> wrote:
> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>
> >> Yes, the patch is correct, but I still prefer my own version :-)
> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
> >>
> >> In case you want to take mine and mine needs refresh, please let me know
> >> and I can do the re-base, thanks.
> >
> > Well, I prefer simpler, unless there's a good reason to use more complicated.
>
> Note that these are not exclusionary; his patch can be applied on top
> of mine. I don't think his patch is needed though.
OK
Do we still need to revert commit efaa14c if this patch is applied?
Rafael
On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote:
>> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>>
>> >> Yes, the patch is correct, but I still prefer my own version :-)
>> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>> >>
>> >> In case you want to take mine and mine needs refresh, please let me know
>> >> and I can do the re-base, thanks.
>> >
>> > Well, I prefer simpler, unless there's a good reason to use more complicated.
>>
>> Note that these are not exclusionary; his patch can be applied on top
>> of mine. I don't think his patch is needed though.
>
> OK
>
> Do we still need to revert commit efaa14c if this patch is applied?
I guess not. At least in this machine changing the backlight works
correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10
didn't work at all. I cannot see how it would affect negatively other
machines.
That being said, the blacklisting is still needed, because 1. the
level is not preserved between boots, and 2. level 0 turns off the
screen, which I personally consider a regression.
At least it boots to level 100 instead of 0.
--
Felipe Contreras
On Saturday, August 03, 2013 05:20:33 PM Felipe Contreras wrote:
> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote:
> >> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <[email protected]> wrote:
> >> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
> >>
> >> >> Yes, the patch is correct, but I still prefer my own version :-)
> >> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
> >> >>
> >> >> In case you want to take mine and mine needs refresh, please let me know
> >> >> and I can do the re-base, thanks.
> >> >
> >> > Well, I prefer simpler, unless there's a good reason to use more complicated.
> >>
> >> Note that these are not exclusionary; his patch can be applied on top
> >> of mine. I don't think his patch is needed though.
> >
> > OK
> >
> > Do we still need to revert commit efaa14c if this patch is applied?
>
> I guess not. At least in this machine changing the backlight works
> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10
> didn't work at all. I cannot see how it would affect negatively other
> machines.
>
> That being said, the blacklisting is still needed, because 1. the
> level is not preserved between boots, and 2. level 0 turns off the
> screen, which I personally consider a regression.
>
> At least it boots to level 100 instead of 0.
OK
I'll push this patch to Linus for -rc5 then without the revert of commit
commit efaa14c. That's all I'm going to do for 3.11 in the ACPI video
area at this point.
As far as the blacklisting is concerned, I still have the blacklist of
your Asus machine queued up for 3.12. Since you're claiming that it
doesn't have any side effects on that machine, I think I can apply it.
However, for other machines to be added to that blacklist I need to see
requests from their users with confirmation that there are no visible side
effects there.
I think this is fair enough.
Thanks,
Rafael
On Sat, Aug 3, 2013 at 5:38 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Saturday, August 03, 2013 05:20:33 PM Felipe Contreras wrote:
>> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote:
>> >> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <[email protected]> wrote:
>> >> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>> >>
>> >> >> Yes, the patch is correct, but I still prefer my own version :-)
>> >> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>> >> >>
>> >> >> In case you want to take mine and mine needs refresh, please let me know
>> >> >> and I can do the re-base, thanks.
>> >> >
>> >> > Well, I prefer simpler, unless there's a good reason to use more complicated.
>> >>
>> >> Note that these are not exclusionary; his patch can be applied on top
>> >> of mine. I don't think his patch is needed though.
>> >
>> > OK
>> >
>> > Do we still need to revert commit efaa14c if this patch is applied?
>>
>> I guess not. At least in this machine changing the backlight works
>> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10
>> didn't work at all. I cannot see how it would affect negatively other
>> machines.
>>
>> That being said, the blacklisting is still needed, because 1. the
>> level is not preserved between boots, and 2. level 0 turns off the
>> screen, which I personally consider a regression.
>>
>> At least it boots to level 100 instead of 0.
>
> OK
>
> I'll push this patch to Linus for -rc5 then without the revert of commit
> commit efaa14c. That's all I'm going to do for 3.11 in the ACPI video
> area at this point.
That seems fair.
> As far as the blacklisting is concerned, I still have the blacklist of
> your Asus machine queued up for 3.12. Since you're claiming that it
> doesn't have any side effects on that machine, I think I can apply it.
>
> However, for other machines to be added to that blacklist I need to see
> requests from their users with confirmation that there are no visible side
> effects there.
Good, that's the purpose of bug 60682.
https://bugzilla.kernel.org/show_bug.cgi?id=60682
--
Felipe Contreras
On 08/03/2013 07:34 PM, Rafael J. Wysocki wrote:
> On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>> On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote:
>>> On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
>>>> 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.
>>>>
>>>> This causes all _BQC calls to return bogus values causing weird behavior
>>>> from the user's perspective. For example: xbacklight -set 10; xbacklight
>>>> -set 20; would flash to 90% and then slowly down to the desired level
>>>> (20).
>>>>
>>>> The solution is simple; test anything other than the first level (e.g.
>>>> 1).
>>>>
>>>> Signed-off-by: Felipe Contreras <[email protected]>
>>>
>>> Looks reasonable.
>>>
>>> Aaron, what do you think?
>>
>> Yes, the patch is correct, but I still prefer my own version :-)
>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>
>> In case you want to take mine and mine needs refresh, please let me know
>> and I can do the re-base, thanks.
>
> Well, I prefer simpler, unless there's a good reason to use more complicated.
>
> Why exactly do you think your version is better?
As explained here:
https://lkml.org/lkml/2013/8/2/81
https://lkml.org/lkml/2013/8/2/112
And for the demo broken _BQC, mine patch will disable _BQC while still
make the backlight work, and this patch here is testing the max
brightness level and may fail.
-Aaron
On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras
<[email protected]> wrote:
> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <[email protected]> wrote:
>> On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote:
>>> On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <[email protected]> wrote:
>>> > On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>>>
>>> >> Yes, the patch is correct, but I still prefer my own version :-)
>>> >> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>> >>
>>> >> In case you want to take mine and mine needs refresh, please let me know
>>> >> and I can do the re-base, thanks.
>>> >
>>> > Well, I prefer simpler, unless there's a good reason to use more complicated.
>>>
>>> Note that these are not exclusionary; his patch can be applied on top
>>> of mine. I don't think his patch is needed though.
>>
>> OK
>>
>> Do we still need to revert commit efaa14c if this patch is applied?
>
> I guess not. At least in this machine changing the backlight works
> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10
> didn't work at all. I cannot see how it would affect negatively other
> machines.
That commit makes hotkey emit notifications, and it's not the
problem of "booting into a black screen", that problem is due to
broken _BQC.
BTW, the efaa14c will also make screen off at level 0 according
to Felipe, who consider this is a bug. But since it is required to
let firmware emit notifications on hotkey press, I think user will
want it.
-Aaron
>
> That being said, the blacklisting is still needed, because 1. the
> level is not preserved between boots, and 2. level 0 turns off the
> screen, which I personally consider a regression.
>
> At least it boots to level 100 instead of 0.
>
> --
> Felipe Contreras
On Sat, Aug 3, 2013 at 8:18 PM, Aaron Lu <[email protected]> wrote:
> On 08/03/2013 07:34 PM, Rafael J. Wysocki wrote:
>> On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
>>> On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote:
>>>> On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
>>>>> 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.
>>>>>
>>>>> This causes all _BQC calls to return bogus values causing weird behavior
>>>>> from the user's perspective. For example: xbacklight -set 10; xbacklight
>>>>> -set 20; would flash to 90% and then slowly down to the desired level
>>>>> (20).
>>>>>
>>>>> The solution is simple; test anything other than the first level (e.g.
>>>>> 1).
>>>>>
>>>>> Signed-off-by: Felipe Contreras <[email protected]>
>>>>
>>>> Looks reasonable.
>>>>
>>>> Aaron, what do you think?
>>>
>>> Yes, the patch is correct, but I still prefer my own version :-)
>>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
>>>
>>> In case you want to take mine and mine needs refresh, please let me know
>>> and I can do the re-base, thanks.
>>
>> Well, I prefer simpler, unless there's a good reason to use more complicated.
>>
>> Why exactly do you think your version is better?
>
> As explained here:
> https://lkml.org/lkml/2013/8/2/81
> https://lkml.org/lkml/2013/8/2/112
>
> And for the demo broken _BQC, mine patch will disable _BQC while still
> make the backlight work, and this patch here is testing the max
> brightness level and may fail.
Yes, but that problem can *only* happen in such a simplified _BCL,
which is very very unlikely. Still, it would make sense to fix the
code for that case.
However, we can fix the problem first for the real known cases with my
simple one-liner patch that can even be merged for v3.11, and *later*
fix the issue for the synthetic unlikely case.
Personally I think there are better ways to fix the code for the
synthetic case than what you patch does, which will also make _BQC
work. That can be discussed later though, the one-liner is simple, and
it works.
--
Felipe Contreras
On Sat, Aug 3, 2013 at 8:47 PM, Aaron Lu <[email protected]> wrote:
> On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras
> <[email protected]> wrote:
>> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <[email protected]> wrote:
>>> Do we still need to revert commit efaa14c if this patch is applied?
>>
>> I guess not. At least in this machine changing the backlight works
>> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10
>> didn't work at all. I cannot see how it would affect negatively other
>> machines.
>
> That commit makes hotkey emit notifications, and it's not the
> problem of "booting into a black screen", that problem is due to
> broken _BQC.
The broken _BQC has been there for quite some time, hasn't it?
Either way, without efaa14c, changing the backlight doesn't work at
all either way, so there's no black screen, because there cannot be.
> BTW, the efaa14c will also make screen off at level 0 according
> to Felipe, who consider this is a bug. But since it is required to
> let firmware emit notifications on hotkey press, I think user will
> want it.
With or without efaa14c, level 0 makes the screen off, or at least it
would, if the control worked at all. So efaa14c is one step forward,
but two back, my patch removes the two steps back, but we are still
not at the level of what my blacklisting patch does, for that we would
need to fix two issues:
1. Fix the retrieval of the last level at boot
2. Fix level 0 (yes, I consider that a regression)
But we cannot achieve either of those for v3.11, the only
possibilities seem to be either a) revert efaa14c, or b) keep it and
apply my patch. Anything else doesn't seem to be a possible or
sensible option, and I vote for b).
--
Felipe Contreras
On Sunday, August 04, 2013 01:54:21 AM Felipe Contreras wrote:
> On Sat, Aug 3, 2013 at 8:47 PM, Aaron Lu <[email protected]> wrote:
> > On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras
> > <[email protected]> wrote:
> >> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <[email protected]> wrote:
>
> >>> Do we still need to revert commit efaa14c if this patch is applied?
> >>
> >> I guess not. At least in this machine changing the backlight works
> >> correctly, whereas in v3.11-rc3 it was all weird, and v3.7-v3.10
> >> didn't work at all. I cannot see how it would affect negatively other
> >> machines.
> >
> > That commit makes hotkey emit notifications, and it's not the
> > problem of "booting into a black screen", that problem is due to
> > broken _BQC.
>
> The broken _BQC has been there for quite some time, hasn't it?
>
> Either way, without efaa14c, changing the backlight doesn't work at
> all either way, so there's no black screen, because there cannot be.
>
> > BTW, the efaa14c will also make screen off at level 0 according
> > to Felipe, who consider this is a bug. But since it is required to
> > let firmware emit notifications on hotkey press, I think user will
> > want it.
>
> With or without efaa14c, level 0 makes the screen off, or at least it
> would, if the control worked at all. So efaa14c is one step forward,
> but two back, my patch removes the two steps back, but we are still
> not at the level of what my blacklisting patch does, for that we would
> need to fix two issues:
>
> 1. Fix the retrieval of the last level at boot
> 2. Fix level 0 (yes, I consider that a regression)
>
> But we cannot achieve either of those for v3.11, the only
> possibilities seem to be either a) revert efaa14c, or b) keep it and
> apply my patch. Anything else doesn't seem to be a possible or
> sensible option, and I vote for b).
I've already said I'm going to do that for 3.11.
Thanks,
Rafael
On Sun, Aug 4, 2013 at 9:14 AM, Rafael J. Wysocki <[email protected]> wrote:
> On Sunday, August 04, 2013 01:54:21 AM Felipe Contreras wrote:
>> But we cannot achieve either of those for v3.11, the only
>> possibilities seem to be either a) revert efaa14c, or b) keep it and
>> apply my patch. Anything else doesn't seem to be a possible or
>> sensible option, and I vote for b).
>
> I've already said I'm going to do that for 3.11.
I was just explaining why reverting efaa14c was an alternative, but it
appears I'm wasting my breath.
--
Felipe Contreras
On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote:
> On Sat, Aug 3, 2013 at 8:18 PM, Aaron Lu <[email protected]> wrote:
> > On 08/03/2013 07:34 PM, Rafael J. Wysocki wrote:
> >> On Saturday, August 03, 2013 04:14:04 PM Aaron Lu wrote:
> >>> On 08/03/2013 07:47 AM, Rafael J. Wysocki wrote:
> >>>> On Friday, August 02, 2013 02:37:09 PM Felipe Contreras wrote:
> >>>>> 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.
> >>>>>
> >>>>> This causes all _BQC calls to return bogus values causing weird behavior
> >>>>> from the user's perspective. For example: xbacklight -set 10; xbacklight
> >>>>> -set 20; would flash to 90% and then slowly down to the desired level
> >>>>> (20).
> >>>>>
> >>>>> The solution is simple; test anything other than the first level (e.g.
> >>>>> 1).
> >>>>>
> >>>>> Signed-off-by: Felipe Contreras <[email protected]>
> >>>>
> >>>> Looks reasonable.
> >>>>
> >>>> Aaron, what do you think?
> >>>
> >>> Yes, the patch is correct, but I still prefer my own version :-)
> >>> https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
> >>>
> >>> In case you want to take mine and mine needs refresh, please let me know
> >>> and I can do the re-base, thanks.
> >>
> >> Well, I prefer simpler, unless there's a good reason to use more complicated.
> >>
> >> Why exactly do you think your version is better?
> >
> > As explained here:
> > https://lkml.org/lkml/2013/8/2/81
> > https://lkml.org/lkml/2013/8/2/112
> >
> > And for the demo broken _BQC, mine patch will disable _BQC while still
> > make the backlight work, and this patch here is testing the max
> > brightness level and may fail.
>
> Yes, but that problem can *only* happen in such a simplified _BCL,
> which is very very unlikely. Still, it would make sense to fix the
> code for that case.
Yes, it would.
> However, we can fix the problem first for the real known cases with my
> simple one-liner patch that can even be merged for v3.11, and *later*
> fix the issue for the synthetic unlikely case.
If we're going to fix it in 3.12, it's good to discuss it now, which is why
I'm askig about that.
> Personally I think there are better ways to fix the code for the
> synthetic case than what you patch does, which will also make _BQC
> work. That can be discussed later though, the one-liner is simple, and
> it works.
So, let's assume that the one-liner goes into 3.11 and work further with that
assumption.
How would you address the sythetic case (on top of the one-liner)?
Rafael
On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki <[email protected]> wrote:
> On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote:
>> Personally I think there are better ways to fix the code for the
>> synthetic case than what you patch does, which will also make _BQC
>> work. That can be discussed later though, the one-liner is simple, and
>> it works.
>
> So, let's assume that the one-liner goes into 3.11 and work further with that
> assumption.
>
> How would you address the sythetic case (on top of the one-liner)?
I would write and read two values instead of one. The code is trying
to check if _BQC is always returning the maximum, and if you try with
two values you can be absolutely certain if that's happening or not;
it doesn't even matter which values you choose. Even in the synthetic
case that only has two values the check would work correctly and
detect that _BQC works correctly (or not).
In my machine I think the issue is slightly different, I think _BCM is
failing, at least until enabling the _DOS thing, but at the end of the
day it's the same thing for the check; _BQC is always returning the
same value, and the code above will find that out, regardless of which
values are tested.
For my particular machine though, I think it's more interesting to
find out why _BCM is failing before _DOS, and why efaa14c made it
work. If that is actually the case.
--
Felipe Contreras
On Sunday, August 04, 2013 09:19:56 AM Felipe Contreras wrote:
> On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki <[email protected]> wrote:
> > On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote:
>
> >> Personally I think there are better ways to fix the code for the
> >> synthetic case than what you patch does, which will also make _BQC
> >> work. That can be discussed later though, the one-liner is simple, and
> >> it works.
> >
> > So, let's assume that the one-liner goes into 3.11 and work further with that
> > assumption.
> >
> > How would you address the sythetic case (on top of the one-liner)?
>
> I would write and read two values instead of one. The code is trying
> to check if _BQC is always returning the maximum, and if you try with
> two values you can be absolutely certain if that's happening or not;
> it doesn't even matter which values you choose. Even in the synthetic
> case that only has two values the check would work correctly and
> detect that _BQC works correctly (or not).
I like that.
> In my machine I think the issue is slightly different, I think _BCM is
> failing, at least until enabling the _DOS thing, but at the end of the
> day it's the same thing for the check; _BQC is always returning the
> same value, and the code above will find that out, regardless of which
> values are tested.
>
> For my particular machine though, I think it's more interesting to
> find out why _BCM is failing before _DOS, and why efaa14c made it
> work. If that is actually the case.
That depends on how the BIOS+platform is designed and that may change from
one system to another quite a bit.
The only common denominator is what Windows expects (and that unfortunately
depends on the version of Windows too), because that's the functionality
which is likely to have been tested. Anything else is likely to be untested
and therefore most probably buggy.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Mon, Aug 5, 2013 at 9:04 AM, Rafael J. Wysocki <[email protected]> wrote:
>> In my machine I think the issue is slightly different, I think _BCM is
>> failing, at least until enabling the _DOS thing, but at the end of the
>> day it's the same thing for the check; _BQC is always returning the
>> same value, and the code above will find that out, regardless of which
>> values are tested.
>>
>> For my particular machine though, I think it's more interesting to
>> find out why _BCM is failing before _DOS, and why efaa14c made it
>> work. If that is actually the case.
>
> That depends on how the BIOS+platform is designed and that may change from
> one system to another quite a bit.
Maybe, but it seems there's at least another ASUS machine with the
same behavior, and it seems a lot of ASUS machines behave very
similarly.
> The only common denominator is what Windows expects (and that unfortunately
> depends on the version of Windows too), because that's the functionality
> which is likely to have been tested. Anything else is likely to be untested
> and therefore most probably buggy.
Maybe, or maybe we are doing something wrong.
It's worth to try to find that out.
--
Felipe Contreras
On 08/04/2013 10:19 PM, Felipe Contreras wrote:
> On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki <[email protected]> wrote:
>> On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote:
>
>>> Personally I think there are better ways to fix the code for the
>>> synthetic case than what you patch does, which will also make _BQC
>>> work. That can be discussed later though, the one-liner is simple, and
>>> it works.
>>
>> So, let's assume that the one-liner goes into 3.11 and work further with that
>> assumption.
>>
>> How would you address the sythetic case (on top of the one-liner)?
>
> I would write and read two values instead of one. The code is trying
> to check if _BQC is always returning the maximum, and if you try with
The code is introduced by commit a50188dae3089dcd15a6ae793528c157680891f1
where the broken system will always return a constant value for _BQC,
either 0 or 100. So the commit at that time tries to not test a maximum
value for the quirk.
Then we have the ASUS NV56Z problem and its problem is explained in:
https://github.com/aaronlu/linux/commit/0a3d2c5b59caf80ae5bb1ca1fda0f7bf448b38c9
And due to its reverse order of _BCL, testing the minimum value is not
good either.
So if the two values test is going to be adopted, I would suggest avoid
testing edge values. But then I'm not sure if it is still worth to test
two values instead of one.
> two values you can be absolutely certain if that's happening or not;
> it doesn't even matter which values you choose. Even in the synthetic
> case that only has two values the check would work correctly and
> detect that _BQC works correctly (or not).
>
> In my machine I think the issue is slightly different, I think _BCM is
> failing, at least until enabling the _DOS thing, but at the end of the
> day it's the same thing for the check; _BQC is always returning the
> same value, and the code above will find that out, regardless of which
> values are tested.
If you think _BCM fails before _DOS and that makes acpi_video_bqc_quirk
not correct, I think you can call acpi_video_bus_start_devices before the
acpi_video_bus_get_devices in acpi_video_bus_add to make _BCM work before
we do the quirk test and then add some debug prints in acpi_video_bqc_quirk
and add some test levels to check it out.
-Aaron
>
> For my particular machine though, I think it's more interesting to
> find out why _BCM is failing before _DOS, and why efaa14c made it
> work. If that is actually the case.