* [PATCH] acpi: video: improve quirk check @ 2013-08-02 19:37 Felipe Contreras 2013-08-02 23:47 ` Rafael J. Wysocki 0 siblings, 1 reply; 25+ messages in thread From: Felipe Contreras @ 2013-08-02 19:37 UTC (permalink / raw) To: linux-kernel, linux-acpi Cc: Rafael J. Wysocki, Len Brown, Zhang Rui, Aaron Lu, Felipe Contreras 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 <felipe.contreras@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-02 19:37 [PATCH] acpi: video: improve quirk check Felipe Contreras @ 2013-08-02 23:47 ` Rafael J. Wysocki 2013-08-03 1:04 ` Felipe Contreras 2013-08-03 8:14 ` Aaron Lu 0 siblings, 2 replies; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-02 23:47 UTC (permalink / raw) To: Felipe Contreras, Aaron Lu; +Cc: linux-kernel, linux-acpi, Len Brown, Zhang Rui 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 <felipe.contreras@gmail.com> 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-02 23:47 ` Rafael J. Wysocki @ 2013-08-03 1:04 ` Felipe Contreras 2013-08-03 1:16 ` Rafael J. Wysocki 2013-08-03 8:14 ` Aaron Lu 1 sibling, 1 reply; 25+ messages in thread From: Felipe Contreras @ 2013-08-03 1:04 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <felipe.contreras@gmail.com> > > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 1:04 ` Felipe Contreras @ 2013-08-03 1:16 ` Rafael J. Wysocki 2013-08-03 1:07 ` Felipe Contreras 0 siblings, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 1:16 UTC (permalink / raw) To: Felipe Contreras; +Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Friday, August 02, 2013 08:04:52 PM Felipe Contreras wrote: > On Fri, Aug 2, 2013 at 6:47 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <felipe.contreras@gmail.com> > > > > 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? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 1:16 ` Rafael J. Wysocki @ 2013-08-03 1:07 ` Felipe Contreras 2013-08-03 1:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 25+ messages in thread From: Felipe Contreras @ 2013-08-03 1:07 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 <felipe.contreras@gmail.com> >> > >> > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 1:07 ` Felipe Contreras @ 2013-08-03 1:19 ` Rafael J. Wysocki 2013-08-03 1:30 ` Felipe Contreras 0 siblings, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 1:19 UTC (permalink / raw) To: Felipe Contreras; +Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Friday, August 02, 2013 08:07:37 PM Felipe Contreras wrote: > On Fri, Aug 2, 2013 at 8:16 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 <felipe.contreras@gmail.com> > >> > > >> > 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. :-) ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 1:19 ` Rafael J. Wysocki @ 2013-08-03 1:30 ` Felipe Contreras 0 siblings, 0 replies; 25+ messages in thread From: Felipe Contreras @ 2013-08-03 1:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Fri, Aug 2, 2013 at 8:19 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 <felipe.contreras@gmail.com> >> >> > >> >> > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-02 23:47 ` Rafael J. Wysocki 2013-08-03 1:04 ` Felipe Contreras @ 2013-08-03 8:14 ` Aaron Lu 2013-08-03 11:34 ` Rafael J. Wysocki 1 sibling, 1 reply; 25+ messages in thread From: Aaron Lu @ 2013-08-03 8:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Felipe Contreras, linux-kernel, linux-acpi, Len Brown, Zhang Rui 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 <felipe.contreras@gmail.com> > > 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) >> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 8:14 ` Aaron Lu @ 2013-08-03 11:34 ` Rafael J. Wysocki 2013-08-03 20:24 ` Felipe Contreras 2013-08-04 1:18 ` Aaron Lu 0 siblings, 2 replies; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 11:34 UTC (permalink / raw) To: Aaron Lu; +Cc: Felipe Contreras, linux-kernel, linux-acpi, Len Brown, Zhang Rui 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 <felipe.contreras@gmail.com> > > > > 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 11:34 ` Rafael J. Wysocki @ 2013-08-03 20:24 ` Felipe Contreras 2013-08-03 21:40 ` Rafael J. Wysocki 2013-08-04 1:18 ` Aaron Lu 1 sibling, 1 reply; 25+ messages in thread From: Felipe Contreras @ 2013-08-03 20:24 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 20:24 ` Felipe Contreras @ 2013-08-03 21:40 ` Rafael J. Wysocki 2013-08-03 22:20 ` Felipe Contreras 0 siblings, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 21:40 UTC (permalink / raw) To: Felipe Contreras; +Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Saturday, August 03, 2013 03:24:16 PM Felipe Contreras wrote: > On Sat, Aug 3, 2013 at 6:34 AM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 21:40 ` Rafael J. Wysocki @ 2013-08-03 22:20 ` Felipe Contreras 2013-08-03 22:38 ` Rafael J. Wysocki 2013-08-04 1:47 ` Aaron Lu 0 siblings, 2 replies; 25+ messages in thread From: Felipe Contreras @ 2013-08-03 22:20 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 22:20 ` Felipe Contreras @ 2013-08-03 22:38 ` Rafael J. Wysocki 2013-08-03 22:37 ` Felipe Contreras 2013-08-04 1:47 ` Aaron Lu 1 sibling, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 22:38 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Linus Torvalds, Matthew Garrett, Igor Gnatenko On Saturday, August 03, 2013 05:20:33 PM Felipe Contreras wrote: > On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 22:38 ` Rafael J. Wysocki @ 2013-08-03 22:37 ` Felipe Contreras 0 siblings, 0 replies; 25+ messages in thread From: Felipe Contreras @ 2013-08-03 22:37 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Linus Torvalds, Matthew Garrett, Igor Gnatenko On Sat, Aug 3, 2013 at 5:38 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 22:20 ` Felipe Contreras 2013-08-03 22:38 ` Rafael J. Wysocki @ 2013-08-04 1:47 ` Aaron Lu 2013-08-04 6:54 ` Felipe Contreras 1 sibling, 1 reply; 25+ messages in thread From: Aaron Lu @ 2013-08-04 1:47 UTC (permalink / raw) To: Felipe Contreras Cc: Rafael J. Wysocki, Kernel development list, linux-acpi, Len Brown, Zhang Rui On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote: > On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 1:47 ` Aaron Lu @ 2013-08-04 6:54 ` Felipe Contreras 2013-08-04 14:14 ` Rafael J. Wysocki 0 siblings, 1 reply; 25+ messages in thread From: Felipe Contreras @ 2013-08-04 6:54 UTC (permalink / raw) To: Aaron Lu Cc: Rafael J. Wysocki, Kernel development list, linux-acpi, Len Brown, Zhang Rui On Sat, Aug 3, 2013 at 8:47 PM, Aaron Lu <aaron.lwe@gmail.com> wrote: > On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras > <felipe.contreras@gmail.com> wrote: >> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 6:54 ` Felipe Contreras @ 2013-08-04 14:14 ` Rafael J. Wysocki 2013-08-04 14:08 ` Felipe Contreras 0 siblings, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-04 14:14 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, Kernel development list, linux-acpi, Len Brown, Zhang Rui On Sunday, August 04, 2013 01:54:21 AM Felipe Contreras wrote: > On Sat, Aug 3, 2013 at 8:47 PM, Aaron Lu <aaron.lwe@gmail.com> wrote: > > On Sun, Aug 4, 2013 at 6:20 AM, Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > >> On Sat, Aug 3, 2013 at 4:40 PM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 14:14 ` Rafael J. Wysocki @ 2013-08-04 14:08 ` Felipe Contreras 0 siblings, 0 replies; 25+ messages in thread From: Felipe Contreras @ 2013-08-04 14:08 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, Kernel development list, linux-acpi, Len Brown, Zhang Rui On Sun, Aug 4, 2013 at 9:14 AM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-03 11:34 ` Rafael J. Wysocki 2013-08-03 20:24 ` Felipe Contreras @ 2013-08-04 1:18 ` Aaron Lu 2013-08-04 6:42 ` Felipe Contreras 1 sibling, 1 reply; 25+ messages in thread From: Aaron Lu @ 2013-08-04 1:18 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Felipe Contreras, linux-kernel, linux-acpi, Len Brown, Zhang Rui 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 <felipe.contreras@gmail.com> >>> >>> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 1:18 ` Aaron Lu @ 2013-08-04 6:42 ` Felipe Contreras 2013-08-04 14:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 25+ messages in thread From: Felipe Contreras @ 2013-08-04 6:42 UTC (permalink / raw) To: Aaron Lu Cc: Rafael J. Wysocki, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Sat, Aug 3, 2013 at 8:18 PM, Aaron Lu <aaron.lwe@gmail.com> 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 <felipe.contreras@gmail.com> >>>> >>>> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 6:42 ` Felipe Contreras @ 2013-08-04 14:19 ` Rafael J. Wysocki 2013-08-04 14:19 ` Felipe Contreras 0 siblings, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-04 14:19 UTC (permalink / raw) To: Felipe Contreras; +Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Sunday, August 04, 2013 01:42:49 AM Felipe Contreras wrote: > On Sat, Aug 3, 2013 at 8:18 PM, Aaron Lu <aaron.lwe@gmail.com> 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 <felipe.contreras@gmail.com> > >>>> > >>>> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 14:19 ` Rafael J. Wysocki @ 2013-08-04 14:19 ` Felipe Contreras 2013-08-05 14:04 ` Rafael J. Wysocki 2013-08-07 4:35 ` Aaron Lu 0 siblings, 2 replies; 25+ messages in thread From: Felipe Contreras @ 2013-08-04 14:19 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 14:19 ` Felipe Contreras @ 2013-08-05 14:04 ` Rafael J. Wysocki 2013-08-05 14:41 ` Felipe Contreras 2013-08-07 4:35 ` Aaron Lu 1 sibling, 1 reply; 25+ messages in thread From: Rafael J. Wysocki @ 2013-08-05 14:04 UTC (permalink / raw) To: Felipe Contreras; +Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Sunday, August 04, 2013 09:19:56 AM Felipe Contreras wrote: > On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki <rjw@sisk.pl> 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-05 14:04 ` Rafael J. Wysocki @ 2013-08-05 14:41 ` Felipe Contreras 0 siblings, 0 replies; 25+ messages in thread From: Felipe Contreras @ 2013-08-05 14:41 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui On Mon, Aug 5, 2013 at 9:04 AM, Rafael J. Wysocki <rjw@sisk.pl> 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] acpi: video: improve quirk check 2013-08-04 14:19 ` Felipe Contreras 2013-08-05 14:04 ` Rafael J. Wysocki @ 2013-08-07 4:35 ` Aaron Lu 1 sibling, 0 replies; 25+ messages in thread From: Aaron Lu @ 2013-08-07 4:35 UTC (permalink / raw) To: Felipe Contreras Cc: Rafael J. Wysocki, linux-kernel, linux-acpi, Len Brown, Zhang Rui On 08/04/2013 10:19 PM, Felipe Contreras wrote: > On Sun, Aug 4, 2013 at 9:19 AM, Rafael J. Wysocki <rjw@sisk.pl> 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-08-07 4:34 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-02 19:37 [PATCH] acpi: video: improve quirk check Felipe Contreras 2013-08-02 23:47 ` Rafael J. Wysocki 2013-08-03 1:04 ` Felipe Contreras 2013-08-03 1:16 ` Rafael J. Wysocki 2013-08-03 1:07 ` Felipe Contreras 2013-08-03 1:19 ` Rafael J. Wysocki 2013-08-03 1:30 ` Felipe Contreras 2013-08-03 8:14 ` Aaron Lu 2013-08-03 11:34 ` Rafael J. Wysocki 2013-08-03 20:24 ` Felipe Contreras 2013-08-03 21:40 ` Rafael J. Wysocki 2013-08-03 22:20 ` Felipe Contreras 2013-08-03 22:38 ` Rafael J. Wysocki 2013-08-03 22:37 ` Felipe Contreras 2013-08-04 1:47 ` Aaron Lu 2013-08-04 6:54 ` Felipe Contreras 2013-08-04 14:14 ` Rafael J. Wysocki 2013-08-04 14:08 ` Felipe Contreras 2013-08-04 1:18 ` Aaron Lu 2013-08-04 6:42 ` Felipe Contreras 2013-08-04 14:19 ` Rafael J. Wysocki 2013-08-04 14:19 ` Felipe Contreras 2013-08-05 14:04 ` Rafael J. Wysocki 2013-08-05 14:41 ` Felipe Contreras 2013-08-07 4:35 ` Aaron Lu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.