| Message ID | 20170306093350.GB1372@ozzy.nask.waw.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
> > Hi Michael > > > > Some quick feedback. > > > > On Mon, Mar 06, 2017 at 03:31:04PM +1030, Jonathan Woithe wrote: > > > > > I can add that immediately after loading the driver the value returned by a > > > > > read of bl_power is 0. As noted above, setting to 1 makes no difference to > > > > > the backlight, neither does returning it to 0. > > > > > > > > Have you tried setting bl_power to 4? Because that is the value of > > > > FB_BLANK_POWERDOWN, which is the value the patch is supposed to handle. > > > > > > Oh no, I didn't try 4. I should have. I will try to squeeze in a test of > > > this tonight (time is short but the test won't take a lot of time). > > > > With an unpatched 4.5 kernel, writing 4 (as opposed to 1, which I stupidly > > tried earlier) to bl_power caused the backlight to turn off. Writing 0 > > turned it back on again. > > > > With patches 1-4/4 applied, writing 4 to bl_power did *NOT* turn the > > backlight off. > > > > With patch 2 reverted, writing 4 to bl_power turned the backlight off. > > Writing 0 to bl_power turned it back on again. > > > > This means that patch 2/4 seems to prevent bl_power from operating as > > expected on the S7020 hardware. Without this patch (but with all the others > > in place) bl_power works. > > > > I am unlikely to have any more time to investigate this further tonight. > > > > In light of the above findings, what would you like to do? > > Thanks for testing, good that we caught this before the patch series was > applied. I think it is reasonable to skip applying this version of the > series as at least patch 2/4 is faulty and breaks a working feature. > > Moving on, though, as I do not have access to Fujitsu hardware on which > this feature works, I was hoping you could help me verify whether my > assumptions were reasonable in the first place. > > I attached a crude patch to this message. I would like to understand > how the underlying ACPI variables behave when the FEXT interface is > used, so please apply this patch on top of dvhart/testing (i.e. without > this series applied). After compiling, please load the module with > debugging enabled, then test backlight control once again by writing 4 > and then 0 to bl_power (this should work). Then please send me all the > messages spit out by the driver into dmesg. This should shed some light > on the matter. Actually, scratch that. I just ordered a banged up S7020 for €15 to avoid pestering you with experimental patches and hopefully make the whole driver cleanup process a bit smoother. Darren, Andy, please ignore this whole series for now. I will post v3 once I figure out how to clean things up without breaking working features.
Hi Michael On Mon, Mar 06, 2017 at 07:47:23PM +0100, Micha?? K??pie?? wrote: > > > In light of the above findings, what would you like to do? > > > > Thanks for testing, good that we caught this before the patch series was > > applied. I think it is reasonable to skip applying this version of the > > series as at least patch 2/4 is faulty and breaks a working feature. > > > > Moving on, though, as I do not have access to Fujitsu hardware on which > > this feature works, I was hoping you could help me verify whether my > > assumptions were reasonable in the first place. > > > > I attached a crude patch to this message. I would like to understand > > how the underlying ACPI variables behave when the FEXT interface is > > used, so please apply this patch on top of dvhart/testing (i.e. without > > this series applied). After compiling, please load the module with > > debugging enabled, then test backlight control once again by writing 4 > > and then 0 to bl_power (this should work). Then please send me all the > > messages spit out by the driver into dmesg. This should shed some light > > on the matter. I have done this. Writing 4 to bl_power did indeed turn the backlight off, and 0 restored it. Annotated output from dmesg is at the end of this message. > Actually, scratch that. I just ordered a banged up S7020 for ???15 to > avoid pestering you with experimental patches and hopefully make the > whole driver cleanup process a bit smoother. Ok, no problem. Obviously I'm still happy to test as required. > Darren, Andy, please ignore this whole series for now. I will post v3 > once I figure out how to clean things up without breaking working > features. To clarify, I see no reason why the earlier 2-patch cleanup series can't go in at this stage. It's only this 4-part patch which needs revision in light of recent findings. Regards jonathan Module loading [ 3292.204366] input: Fujitsu FUJ02B1 as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:05/FUJ02B1:00/input/input19 [ 3292.208512] fujitsu_laptop: ACPI: Fujitsu FUJ02B1 [FJEX] (on) [ 3292.209653] fujitsu_laptop: acpi_fujitsu_bl_add: auto-detected usealt as 0 [ 3292.209654] fujitsu_laptop: acpi_fujitsu_bl_add: config: [alt interface: 0], [adjust disable: 0] [ 3292.209655] fujitsu_laptop: get_max_brightness: get max lcd level via RBLL [ 3292.209701] fujitsu_laptop: get_lcd_level: get lcd level via GBLL [ 3292.212086] input: Fujitsu FUJ02E3 as /devices/LNXSYSTM:00/LNXSYBUS:00/FUJ02E3:00/input/input20 [ 3292.213483] fujitsu_laptop: ACPI: Fujitsu FUJ02E3 [FEXT] (on) [ 3292.214627] fujitsu_laptop: acpi_fujitsu_laptop_add: Invoking _INI [ 3292.214740] fujitsu_laptop: call_fext_func: FUNC 0x1002 (args 0x1, 0x0, 0x0) returned 0x0 [ 3292.214741] fujitsu_laptop: acpi_fujitsu_laptop_add: Discarded 0 ringbuffer entries [ 3292.214770] fujitsu_laptop: call_fext_func: FUNC 0x1000 (args 0x0, 0x0, 0x0) returned 0x80000000 [ 3292.214819] fujitsu_laptop: call_fext_func: FUNC 0x1002 (args 0x0, 0x0, 0x0) returned 0xf0001 [ 3292.214820] fujitsu_laptop: BTNI: [0xf0001] [ 3292.216523] fujitsu_laptop: call_fext_func: FUNC 0x1001 (args 0x0, 0x0, 0x0) returned 0x0 [ 3292.216567] fujitsu_laptop: call_fext_func: FUNC 0x1001 (args 0x0, 0x0, 0x0) returned 0x0 [ 3292.216616] fujitsu_laptop: call_fext_func: FUNC 0x1002 (args 0x0, 0x0, 0x0) returned 0xf0001 [ 3292.216658] fujitsu_laptop: call_fext_func: FUNC 0x1001 (args 0x0, 0x0, 0x0) returned 0x0 [ 3292.216964] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x2, 0x4, 0x0) returned 0x0 [ 3292.216965] fujitsu_laptop: driver 0.6.0 successfully loaded echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power [ 3320.168775] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x3) returned 0x0 [ 3320.168779] fujitsu_laptop: bl_update_status: Backlight power set to 4 [ 3320.168793] fujitsu_laptop: bl_update_status: BLCT = 1 [ 3320.168800] fujitsu_laptop: bl_update_status: NGTM = 3 [ 3320.168805] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC [ 3320.168808] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7] echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power [ 3322.774773] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x0) returned 0x0 [ 3322.774776] fujitsu_laptop: bl_update_status: Backlight power set to 0 [ 3322.774790] fujitsu_laptop: bl_update_status: BLCT = 0 [ 3322.774798] fujitsu_laptop: bl_update_status: NGTM = 0 [ 3322.774802] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC [ 3322.774804] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7]
> Hi Michael > > On Mon, Mar 06, 2017 at 07:47:23PM +0100, Micha?? K??pie?? wrote: > > > > In light of the above findings, what would you like to do? > > > > > > Thanks for testing, good that we caught this before the patch series was > > > applied. I think it is reasonable to skip applying this version of the > > > series as at least patch 2/4 is faulty and breaks a working feature. > > > > > > Moving on, though, as I do not have access to Fujitsu hardware on which > > > this feature works, I was hoping you could help me verify whether my > > > assumptions were reasonable in the first place. > > > > > > I attached a crude patch to this message. I would like to understand > > > how the underlying ACPI variables behave when the FEXT interface is > > > used, so please apply this patch on top of dvhart/testing (i.e. without > > > this series applied). After compiling, please load the module with > > > debugging enabled, then test backlight control once again by writing 4 > > > and then 0 to bl_power (this should work). Then please send me all the > > > messages spit out by the driver into dmesg. This should shed some light > > > on the matter. > > I have done this. Writing 4 to bl_power did indeed turn the backlight off, > and 0 restored it. Annotated output from dmesg is at the end of this > message. > > > Actually, scratch that. I just ordered a banged up S7020 for ???15 to > > avoid pestering you with experimental patches and hopefully make the > > whole driver cleanup process a bit smoother. > > Ok, no problem. Obviously I'm still happy to test as required. > > > Darren, Andy, please ignore this whole series for now. I will post v3 > > once I figure out how to clean things up without breaking working > > features. > > To clarify, I see no reason why the earlier 2-patch cleanup series can't go > in at this stage. It's only this 4-part patch which needs revision in light > of recent findings. Yes, this is what I meant. As I mentioned earlier in this thread [1], v2 of the 2-patch series for acpi_fujitsu_bl_notify() is independent from this one. [] > > echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power > > [ 3320.168775] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x3) returned 0x0 > [ 3320.168779] fujitsu_laptop: bl_update_status: Backlight power set to 4 > [ 3320.168793] fujitsu_laptop: bl_update_status: BLCT = 1 > [ 3320.168800] fujitsu_laptop: bl_update_status: NGTM = 3 > [ 3320.168805] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC > [ 3320.168808] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7] > > echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power > > [ 3322.774773] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x0) returned 0x0 > [ 3322.774776] fujitsu_laptop: bl_update_status: Backlight power set to 0 > [ 3322.774790] fujitsu_laptop: bl_update_status: BLCT = 0 > [ 3322.774798] fujitsu_laptop: bl_update_status: NGTM = 0 > [ 3322.774802] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC > [ 3322.774804] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7] Okay, this is exactly what I feared. I correctly inferred how BLCT behaves from the DSDT dump, but it looks like it works in tandem with NGTM, which can only be set using the FUNC interface. This means that, to avoid code duplication, we will need to use call_fext_func() from within the backlight driver. If we decide to split backlight-related code and the rest into two separate modules, this means that the backlight module will depend on the laptop module. This is not really bad in and of itself, but I was hoping we could do better than that. That being said, a part of me is also happy that we will stick to the "official" interface instead of doing custom low-level tricks. Anyway, I will prepare a v3 that does not break backlight control. Driver untangling will have to wait. [1] https://www.spinics.net/lists/platform-driver-x86/msg10776.html
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index e12cc3504d48..34bf97d4d463 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -463,7 +463,11 @@ static int bl_get_brightness(struct backlight_device *b) static int bl_update_status(struct backlight_device *b) { + unsigned long long blank; + acpi_status status; + acpi_handle handle; int ret; + if (b->props.power == FB_BLANK_POWERDOWN) ret = call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3); else @@ -473,6 +477,26 @@ static int bl_update_status(struct backlight_device *b) "Unable to adjust backlight power, error code %i\n", ret); + vdbg_printk(FUJLAPTOP_DBG_INFO, "Backlight power set to %d\n", b->props.power); + + status = acpi_evaluate_integer(NULL, "\\BLCT", NULL, &blank); + if (ACPI_SUCCESS(status)) + vdbg_printk(FUJLAPTOP_DBG_INFO, "BLCT = %llx\n", blank); + else + vdbg_printk(FUJLAPTOP_DBG_ERROR, "Cannot evaluate BLCT\n"); + + status = acpi_evaluate_integer(NULL, "\\NGTM", NULL, &blank); + if (ACPI_SUCCESS(status)) + vdbg_printk(FUJLAPTOP_DBG_INFO, "NGTM = %llx\n", blank); + else + vdbg_printk(FUJLAPTOP_DBG_ERROR, "Cannot evaluate NGTM\n"); + + status = acpi_get_handle(NULL, "\\_SB.FEXT.SBLC", &handle); + if (ACPI_SUCCESS(status)) + vdbg_printk(FUJLAPTOP_DBG_INFO, "Got ACPI handle for SBLC\n"); + else + vdbg_printk(FUJLAPTOP_DBG_ERROR, "Cannot get ACPI handle for SBLC\n"); + if (use_alt_lcd_levels) ret = set_lcd_level_alt(b->props.brightness); else -- 2.12.0