| Message ID | 20240919171952.403745-4-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) | expand |
On 9/19/2024 12:19, Antheas Kapenekakis wrote: > Move the screen on and off calls into dedicated callbacks that gate > the ACPI mutex, so they can be called outside of the suspend path. > This fixes issues on certain devices that expect kernel drivers to be > fully active during the calls, and allows for the flexibility of calling > them as part of a more elaborate userspace suspend sequence (such as > with "Screen Off" in Windows Modern Standby). > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> This patch is inspired by my patch https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/commit/?h=superm1/dsm-screen-on-off&id=e1a274ee7634f0f8fb877990df6d884189065228 So in a future revision I would appreciate a Suggested-by: tag. > --- > drivers/acpi/x86/s2idle.c | 72 +++++++++++++++++++++++++++++++-------- > 1 file changed, 57 insertions(+), 15 deletions(-) > > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c > index dd0b40b9bbe8..aa448ed58077 100644 > --- a/drivers/acpi/x86/s2idle.c > +++ b/drivers/acpi/x86/s2idle.c > @@ -60,6 +60,7 @@ static int lps0_dsm_func_mask; > static guid_t lps0_dsm_guid_microsoft; > static int lps0_dsm_func_mask_microsoft; > static int lps0_dsm_state; > +static bool lsp0_dsm_in_screen_off; It seems that this new variable is mostly for debugging purpose. If the variable stays I think that you should fail the screen_off/screen_on calls with it. More details below. > > /* Device constraint entry structure */ > struct lpi_device_info { > @@ -539,15 +540,19 @@ static struct acpi_scan_handler lps0_handler = { > .attach = lps0_device_attach, > }; > > -int acpi_s2idle_prepare_late(void) > +static int acpi_s2idle_screen_off(void) > { > - struct acpi_s2idle_dev_ops *handler; > - > if (!lps0_device_handle || sleep_no_lps0) > return 0; > > - if (pm_debug_messages_on) > - lpi_check_constraints(); > + if (lsp0_dsm_in_screen_off) { > + acpi_handle_info(lps0_device_handle, > + "called screen off call twice before calling screen on.\n"); > + // fallthrough for debugging, calling twice should be gated by the caller It seems like it would mostly be a programmer error if it was called twice. How about something like this: if (unlikely(WARN_ON(lsp0_dsm_in_screen_off))) return -EINVAL; You could do something similar with the inverse in the other call site too then. > + } > + > + lsp0_dsm_in_screen_off = true; > + acpi_scan_lock_acquire(); > > /* Screen off */ > if (lps0_dsm_func_mask > 0) > @@ -560,6 +565,50 @@ int acpi_s2idle_prepare_late(void) > acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF, > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > > + acpi_scan_lock_release(); > + > + return 0; > +} > + > +static int acpi_s2idle_screen_on(void) > +{ > + if (!lps0_device_handle || sleep_no_lps0) > + return 0; > + > + if (!lsp0_dsm_in_screen_off) { > + acpi_handle_info(lps0_device_handle, > + "called screen on before calling screen off.\n"); > + // fallthrough for debugging, calling twice should be gated by the caller > + } > + > + lsp0_dsm_in_screen_off = false; > + acpi_scan_lock_acquire(); > + > + /* Screen on */ > + if (lps0_dsm_func_mask_microsoft > 0) > + acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON, > + lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > + if (lps0_dsm_func_mask > 0) > + acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ? > + ACPI_LPS0_SCREEN_ON_AMD : > + ACPI_LPS0_SCREEN_ON, > + lps0_dsm_func_mask, lps0_dsm_guid); > + > + acpi_scan_lock_release(); > + > + return 0; > +} > + > +int acpi_s2idle_prepare_late(void) > +{ > + struct acpi_s2idle_dev_ops *handler; > + > + if (!lps0_device_handle || sleep_no_lps0) > + return 0; > + > + if (pm_debug_messages_on) > + lpi_check_constraints(); > + > /* LPS0 entry */ > if (lps0_dsm_func_mask > 0 && acpi_s2idle_vendor_amd()) > acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY_AMD, > @@ -623,19 +672,10 @@ void acpi_s2idle_restore_early(void) > acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT, > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > } > - > - /* Screen on */ > - if (lps0_dsm_func_mask_microsoft > 0) > - acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON, > - lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); > - if (lps0_dsm_func_mask > 0) > - acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ? > - ACPI_LPS0_SCREEN_ON_AMD : > - ACPI_LPS0_SCREEN_ON, > - lps0_dsm_func_mask, lps0_dsm_guid); > } > > static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { > + .screen_off = acpi_s2idle_screen_off, > .begin = acpi_s2idle_begin, > .prepare = acpi_s2idle_prepare, > .prepare_late = acpi_s2idle_prepare_late, > @@ -644,10 +684,12 @@ static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { > .restore_early = acpi_s2idle_restore_early, > .restore = acpi_s2idle_restore, > .end = acpi_s2idle_end, > + .screen_on = acpi_s2idle_screen_on, > }; > > void __init acpi_s2idle_setup(void) > { > + lsp0_dsm_in_screen_off = false; Doesn't it initialize to false already? Is this really needed? > acpi_scan_add_handler(&lps0_handler); > s2idle_set_ops(&acpi_s2idle_ops_lps0); > }
> It seems like it would mostly be a programmer error if it was called twice. > > How about something like this: > > if (unlikely(WARN_ON(lsp0_dsm_in_screen_off))) > return -EINVAL; > > You could do something similar with the inverse in the other call site > too then. Indeed, under normal operation calling it twice is an error. 2 concerns here: if I make this change just here, the code on patch 2 will make the suspend sequence bail. Is this a desired outcome? Shouldn't we just keep going? And if we should keep going, does that include calling Display Off twice? I am inclined to say the desired outcome is to not call Display Off twice and to keep the suspend sequence going. In which case, I will merge your suggestion and remove the bail sequence from patch 2. 2nd concern is that I suspect it is very likely firmware vendors will want to call this sequence as part of developing devices, so I am inclined to allow calling the functions back to back. However, that could be achieved differently, e.g., with a module flag. > > + lsp0_dsm_in_screen_off = false; > > Doesn't it initialize to false already? Is this really needed? More for my peace of mind. I will remove it. Antheas
On 9/19/2024 15:45, Antheas Kapenekakis wrote: >> It seems like it would mostly be a programmer error if it was called twice. >> >> How about something like this: >> >> if (unlikely(WARN_ON(lsp0_dsm_in_screen_off))) >> return -EINVAL; >> >> You could do something similar with the inverse in the other call site >> too then. > > Indeed, under normal operation calling it twice is an error. > > 2 concerns here: if I make this change just here, the code on patch 2 > will make the suspend sequence bail. Is this a desired outcome? > > Shouldn't we just keep going? And if we should keep going, does that > include calling Display Off twice? > > I am inclined to say the desired outcome is to not call Display Off > twice and to keep the suspend sequence going. In which case, I will > merge your suggestion and remove the bail sequence from patch 2. OK. We'll have to see Rafael's thoughts on your next version. FYI, he's at LPC, so expect delayed response. > > 2nd concern is that I suspect it is very likely firmware vendors will > want to call this sequence as part of developing devices, so I am > inclined to allow calling the functions back to back. However, that > could be achieved differently, e.g., with a module flag. I think vendors developing devices could rebuild the kernel too. I'm not really sure this would be a common enough need to justify a module parameter. > >>> + lsp0_dsm_in_screen_off = false; >> >> Doesn't it initialize to false already? Is this really needed? > > More for my peace of mind. I will remove it. > > Antheas
I will try to merge all the suggestions and make a V2 on Saturday then. Antheas
On 19/09/24 22:54, Antheas Kapenekakis wrote: > I will try to merge all the suggestions and make a V2 on Saturday then. > > Antheas Hello everybody, I own an RC71L and I want to see the issue solved, therefore I carefully tested proposed changes from you as well from Luke. Testing on yours has been performed firstly with a preview patch that you shared on github and then I dedicated a few hours to test the submitted work. On the preview I highlighted regressions I have found with your work, and given I have taken more time to test what you are submitting here I have found that not only reported regressions have not been addressed in any way, but I was able to identify some more: in particular entering s2idle appears to abort sometimes and I was able to reproduce failures in mcu_powersave=1. I have filmed a few samples of problems as they manifests, as well as gathering logs in form of dmesg and amd_s2idle.py. Visually all appears the same in mcu_powersave 0 and 1, therefore I filmed only mcu_powersave=0, but I will include logs of mcu_powersave=1 as well. I want to express concerns about two main points: - these patches are written with two devices in mind, but change the s2idle implementation of all devices: considering one of those two devices is showing serious regressions in every scenario I doubt the effectiveness of removing those csee call. - despite being fully aware of issues this patch series introduces, you sent them with very little modification, against my best judgment and against interests of owners of this device. I feel it's imperative you understand the root cause of these regressions and ensure they are adequately addressed before submitting a v2 of this patch as they renders the device completely unreliable (might wakeup in a bag, for example). I am available to provide testing before you submit to LKML. A test MCU fw was made available to Luke by ASUS after they worked together on the issue. This fixes the root cause of issues in Linux. Testing by others (including ASUS) has indicated that this patch series is not required if the fixed firmware is used - as such there is communication progressing with ASUS to bring them onboard to use fwupd to update firmware in Linux. The old quirk will be updated to account for the new FW. My own testing of this firmware is delayed while I test your patches. Best regards, Denis Benato mcu_powersave=0 AC spurious => https://youtu.be/GvM2LvdKGI4 mcu_powersave=0 battery wake by face buttons => https://youtu.be/xJQbzOawnsc mcu_powersave=0 battery spurious => https://youtu.be/RV_ununSVxA mcu_powersave=0 AC abort => https://youtu.be/H0lv9-CAEHQ This is an example on what is written on terminal when such spurious wakeups happens: Debugging script for s2idle on AMD systems
Hi Denis, thank you for taking the time to test the patch. First I want to give some context. I have tested variants of this patch on 6.11rc5 and linux-pm/bleeding-edge on both an Ally X and a Legion go. I experienced 0 sleep failures on both of them throughout my whole series of testing, on all variants of the patch. I did not test 6.11 final. So I hope you did A/B test, as it is not clear from your email. We have had a lot of issues with new kernels and suspend, so please make sure to test 6.11 stock. The ACPI error in the s2idle report for the _DSM is normal, Asus left a lid sensor call that goes nowhere below the CSEE call that turns off the MCU. I am not that well versed in s2idle traces, but from what I am seeing is that your controller suspends beautifully, but some suspends fail due to a PCI error? Then you have "[Errno 16] Device or resource busy" but no accounting of which device that is. All this patch does is move CSSE to happen outside the suspend sequence, whereas before it was done *twice* within the suspend sequence, @Mario what are your thoughts? The controller is expected to disconnect and reconnect to the device, we are not trying to avoid that. We plan to do a lot more thorough testing next week on Bazzite (on all devices), however our kernel maintainer is feeling under the weather so it will take a couple more days for that to start. Also, a small comment on this new firmware. This patch has merit regardless of what Asus does with the ROG Ally, it is both a feature and bug fix contribution. I would like to see suspend in Linux become a lot more modern, as it is a feature we are often asked about. Users want to be able to download games suspended for example. The Switch can do it, the Playstation 5 can do it, but uh Linux cannot? It will not cause regressions regardless of what Asus does, as this is making Linux mimic Windows more. Then, to move on to your other concern. Hopefully, you conducted these tests with a stock firmware. I think you did. Secondly, to me, requiring a firmware update + kernel quirk + software quirk (as extreme mode will be MCU version dependent) is something that I do not find to be a very satisfactory solution. In any case, we are happy to hold this patch out-of-tree for our users with older firmware and then tack on whatever else is required for the newer firmware. I will post an updated version of the patch later today, although it is functionally identical. Antheas
On 21/09/24 08:22, Antheas Kapenekakis wrote: > Hi Denis, > thank you for taking the time to test the patch. > Hello Antheas, > First I want to give some context. I have tested variants of this > patch on 6.11rc5 and linux-pm/bleeding-edge on both an Ally X and a > Legion go. I experienced 0 sleep failures on both of them throughout > my whole series of testing, on all variants of the patch. I did not > test 6.11 final. So I hope you did A/B test, as it is not clear from > your email. We have had a lot of issues with new kernels and suspend, > so please make sure to test 6.11 stock. > My tests were conducted on top of the released 6.11.0, as suggested to me by Mario. > The ACPI error in the s2idle report for the _DSM is normal, Asus left > a lid sensor call that goes nowhere below the CSEE call that turns off > the MCU. > Yes, there is a leftover call on lid in the ACPI, however it has been there since the very first release and it has never caused any issue. > I am not that well versed in s2idle traces, but from what I am seeing > is that your controller suspends beautifully, but some suspends fail > due to a PCI error? Then you have "[Errno 16] Device or resource busy" > but no accounting of which device that is. All this patch does is move > CSSE to happen outside the suspend sequence, whereas before it was > done *twice* within the suspend sequence, > Two notes here: - there is no evidence, nor words from ASUS, that indicates calling CSEE twice could be a problem, on the contrary, there is strong evidence that doing it only once causes issues such as what I am having and/or controller returning "every other sleep". - This has never been a problem before and my device has been entering and exiting s2idle just fine for the previous months. > @Mario what are your thoughts? > > The controller is expected to disconnect and reconnect to the device, > we are not trying to avoid that. > The controller is indeed expected to disconnect upon entering s2idle and the fact that I can wake up the device using A,B,X,Y buttons is concerning in this regard: such behavior has never come up in past months. > We plan to do a lot more thorough testing next week on Bazzite (on all > devices), however our kernel maintainer is feeling under the weather > so it will take a couple more days for that to start. > Please, do so before submitting any patch that is meant to change the s2idle path for every existing device. > Also, a small comment on this new firmware. This patch has merit > regardless of what Asus does with the ROG Ally, it is both a feature > and bug fix contribution. I would like to see suspend in Linux become > a lot more modern, as it is a feature we are often asked about. Users > want to be able to download games suspended for example. The Switch > can do it, the Playstation 5 can do it, but uh Linux cannot? It will > not cause regressions regardless of what Asus does, as this is making > Linux mimic Windows more. > This patch might have merit if you properly identify and address regressions you introduced. Windows does not show any sign of spurious wakeup nor aborts while entering modern standby, therefore there is still some difference that must be identified and addressed accordingly if this is the path to be followed going forward. > Then, to move on to your other concern. Hopefully, you conducted these > tests with a stock firmware. I think you did. Secondly, to me, > requiring a firmware update + kernel quirk + software quirk (as > extreme mode will be MCU version dependent) is something that I do not > find to be a very satisfactory solution. In any case, we are happy to > hold this patch out-of-tree for our users with older firmware and then > tack on whatever else is required for the newer firmware. > I conducted these tests with the lastest available firmware that is available to every user: exactly the firmware this patchset is meant to target. - Main Firmware: GA80100.RC71LM.318 - Second Firmware: GA80100.RC71LS.318 On that note I want to add that the path taken for entering s2idle by RC71L and RC72L is different as in the case of RC71L two MCUs are involved, while with what is available to you (RC72L) only one MCU is present and therefore you cannot assume that what works for your device also works on the other. Keeping the firmware updated is something users do regularly on this device, even if it means booting to windows. Beside, as for necessary kernel/software quirks, the new firmware is expected to require none, at least for asus-wmi, so I ask you to leave csee calls where they are now as in the future those will be used only on non-updated firmware. I also did the main testing with the previous 339 BIOS update, but just finished confirming all regressions are still happening with the latest available 341 BIOS update too. > I will post an updated version of the patch later today, although it > is functionally identical. > If it's functionally identical there is no point in it: it cannot be merged with these kind of regressions happening. > Antheas Best regards, Denis Benato
Hi Denis, > Beside, as for necessary kernel/software quirks, the new firmware is > expected to require none, at least for asus-wmi, so I ask you to leave > csee calls where they are now as in the future those will be used only > on non-updated firmware. I'm happy you said that, as it means this patch will not conflict with the new firmware and will just fix older devices. The only change my patch does on the Ally specifically is pull CSEE earlier and remove the extra call. There is nothing in this patch to explain what you are experiencing. There are a lot of causes I could point my finger to, but there is no point. We will conduct our own testing, and this will include the original Ally as well. A lot of them actually, and after initial testing this will include thousands of devices, as we plan to fully dogfood this patch. I was a bit busy today, so I did not update the patch. I want to rewrite part of the cover letter, as it includes some inconsistencies, and rename some variables. The inconsistencies have to do with how I describe the sleep stage, as I read up on some additional documentation, it is not related to the contents of the patch. In addition, it seems those sleep _DSMs cause problems on the Ally too, related to TDP. And no, I will not wait half a year for a BIOS update to fix those. I am also looking into how to integrate Modern Standby into the kernel, in a more full featured way. Downloading games in the background is a very requested feature after all, and since looking into the Ally's _DSM entries, it seems like it is built to support it. Background here would mean the fan will be off and the suspend light will be pulsing, so you can safely stow it in a bag while it is downloading games. However, this is conjecture until the patch for that is built and tested. Antheas
On 21/09/24 21:44, Antheas Kapenekakis wrote: > Hi Denis, > Hello Antheas, >> Beside, as for necessary kernel/software quirks, the new firmware is >> expected to require none, at least for asus-wmi, so I ask you to leave >> csee calls where they are now as in the future those will be used only >> on non-updated firmware. > I'm happy you said that, as it means this patch will not conflict with > the new firmware and will just fix older devices. > You are currently attempting to fix a problem that does not exists (or you haven't made us aware of), in a way that is causing regressions: no -- calling csee twice is not a problem and we have absolutely no reasons to suspect it is. First: the onus is on you to justify, with logs and a thorough explanation, why you are modifying a module. You have so far failed to do so. Second: give us explanation of why you think downloading games with screen off has something to do with the asus-wmi driver and belongs to the same patch series. > The only change my patch does on the Ally specifically is pull CSEE > earlier and remove the extra call. There is nothing in this patch to > explain what you are experiencing. There are a lot of causes I could > point my finger to, but there is no point. > A compiled version of the kernel tag v6.11 has mcu_powersave=0 working flawlessly, that very same kernel, compiled with the very same .config, with your patches on top exhibits what was documented: there is no reason to use fingers here. So... for one your patches do something else (and they seems to be related to activities done in background while in s2idle), and two you are saying that since you don't understand why a regression is happening it is okay to ignore that. > We will conduct our own testing, and this will include the original > Ally as well. A lot of them actually, and after initial testing this > will include thousands of devices, as we plan to fully dogfood this > patch. > I will be here waiting for the result, when you have identified the reason of regression I documented, include me in CC or reach me out privately with the work ready to test. > I was a bit busy today, so I did not update the patch. I want to > rewrite part of the cover letter, as it includes some inconsistencies, > and rename some variables. The inconsistencies have to do with how I > describe the sleep stage, as I read up on some additional > documentation, it is not related to the contents of the patch. In > addition, it seems those sleep _DSMs cause problems on the Ally too, > related to TDP. And no, I will not wait half a year for a BIOS update > to fix those. > > I am also looking into how to integrate Modern Standby into the > kernel, in a more full featured way. Downloading games in the > background is a very requested feature after all, and since looking > into the Ally's _DSM entries, it seems like it is built to support it. > Background here would mean the fan will be off and the suspend light > will be pulsing, so you can safely stow it in a bag while it is > downloading games. However, this is conjecture until the patch for > that is built and tested. > Again, userspace software running in s2idle has absolutely nothing to do with acpi entries. You need to decide what this patch is all about: If it is about asus-wmi then identify and solve regressions before resubmitting. If it is about downloading games while sleeping then create a new idle driver (this way you won't risk breaking what has been confirmed working for months or years), do a request for comment on a proposal, or propose an interface to the userspace that applications will use to carry on tasks while hardware is in s2idle and drop the useless asus-wmi part as it does not belong there. As it stands this work does not solve any problem and does not allow downloads to happen while the console is sleeping: pick one and follow that route. > Antheas Best regards, Denis
> I will be here waiting for the result, when you have identified the reason of regression I documented, include me in CC or reach me out privately with the work ready to test. Hi Dennis, as I said yesterday I would continue testing and I did. You were right, there are two pieces to this puzzle. Let's make something clear. Asus is not the only one using _DSM 3,4 this way. Asus is just the case where the deviation in behavior is more clear. I suspect OneXplayer is using it as well for the controller (USBP variable). And Ayaneo is notorious for unreliable S0ix to the point where people recommend bios mods to manually force S3 through PBS. First one is that calling Display Off/On while the kernel is partly suspended prevents the controller from coming back online. My patch fixes that, and I am sure you can attest. Second one is that asus plays an RGB suspend animation before suspending the controller, which lasts for over 1 second. If the device suspends before that, when it wakes up, it keeps playing the animation the disconnection happens then. Since XInput does not support selective suspend, this might lead to higher battery draw and, as you noticed, being able to wake up the device with the controller. For the Ally X, it means the controller takes around 2s more to wake up (in addition to the animation quirk). Thank you for catching that. I will update the patch series to take that into account. If you want I can add a Reported-by for you, and if you test the next revision out-of-band, I can also add Tested-by. In addition, if you give me some details about the next firmware asus is preparing, I can make sure the (pre-suspend) delay only applies to the older firmware. There is no need for a delay post-suspend. If Asus is removing the CSEE call for backwards compatibility, this might also work. I still think there is value to this patch, as it tries to study Modern Standby behavior further. And I would suspect Asus is not the only manufacturer being creative. > do a request for comment on a proposal, Yes, that will come after some time as well, but it will build on this patch. Antheas
diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c index dd0b40b9bbe8..aa448ed58077 100644 --- a/drivers/acpi/x86/s2idle.c +++ b/drivers/acpi/x86/s2idle.c @@ -60,6 +60,7 @@ static int lps0_dsm_func_mask; static guid_t lps0_dsm_guid_microsoft; static int lps0_dsm_func_mask_microsoft; static int lps0_dsm_state; +static bool lsp0_dsm_in_screen_off; /* Device constraint entry structure */ struct lpi_device_info { @@ -539,15 +540,19 @@ static struct acpi_scan_handler lps0_handler = { .attach = lps0_device_attach, }; -int acpi_s2idle_prepare_late(void) +static int acpi_s2idle_screen_off(void) { - struct acpi_s2idle_dev_ops *handler; - if (!lps0_device_handle || sleep_no_lps0) return 0; - if (pm_debug_messages_on) - lpi_check_constraints(); + if (lsp0_dsm_in_screen_off) { + acpi_handle_info(lps0_device_handle, + "called screen off call twice before calling screen on.\n"); + // fallthrough for debugging, calling twice should be gated by the caller + } + + lsp0_dsm_in_screen_off = true; + acpi_scan_lock_acquire(); /* Screen off */ if (lps0_dsm_func_mask > 0) @@ -560,6 +565,50 @@ int acpi_s2idle_prepare_late(void) acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF, lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); + acpi_scan_lock_release(); + + return 0; +} + +static int acpi_s2idle_screen_on(void) +{ + if (!lps0_device_handle || sleep_no_lps0) + return 0; + + if (!lsp0_dsm_in_screen_off) { + acpi_handle_info(lps0_device_handle, + "called screen on before calling screen off.\n"); + // fallthrough for debugging, calling twice should be gated by the caller + } + + lsp0_dsm_in_screen_off = false; + acpi_scan_lock_acquire(); + + /* Screen on */ + if (lps0_dsm_func_mask_microsoft > 0) + acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON, + lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); + if (lps0_dsm_func_mask > 0) + acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ? + ACPI_LPS0_SCREEN_ON_AMD : + ACPI_LPS0_SCREEN_ON, + lps0_dsm_func_mask, lps0_dsm_guid); + + acpi_scan_lock_release(); + + return 0; +} + +int acpi_s2idle_prepare_late(void) +{ + struct acpi_s2idle_dev_ops *handler; + + if (!lps0_device_handle || sleep_no_lps0) + return 0; + + if (pm_debug_messages_on) + lpi_check_constraints(); + /* LPS0 entry */ if (lps0_dsm_func_mask > 0 && acpi_s2idle_vendor_amd()) acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY_AMD, @@ -623,19 +672,10 @@ void acpi_s2idle_restore_early(void) acpi_sleep_run_lps0_dsm(ACPI_LPS0_MS_EXIT, lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); } - - /* Screen on */ - if (lps0_dsm_func_mask_microsoft > 0) - acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON, - lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft); - if (lps0_dsm_func_mask > 0) - acpi_sleep_run_lps0_dsm(acpi_s2idle_vendor_amd() ? - ACPI_LPS0_SCREEN_ON_AMD : - ACPI_LPS0_SCREEN_ON, - lps0_dsm_func_mask, lps0_dsm_guid); } static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { + .screen_off = acpi_s2idle_screen_off, .begin = acpi_s2idle_begin, .prepare = acpi_s2idle_prepare, .prepare_late = acpi_s2idle_prepare_late, @@ -644,10 +684,12 @@ static const struct platform_s2idle_ops acpi_s2idle_ops_lps0 = { .restore_early = acpi_s2idle_restore_early, .restore = acpi_s2idle_restore, .end = acpi_s2idle_end, + .screen_on = acpi_s2idle_screen_on, }; void __init acpi_s2idle_setup(void) { + lsp0_dsm_in_screen_off = false; acpi_scan_add_handler(&lps0_handler); s2idle_set_ops(&acpi_s2idle_ops_lps0); }
Move the screen on and off calls into dedicated callbacks that gate the ACPI mutex, so they can be called outside of the suspend path. This fixes issues on certain devices that expect kernel drivers to be fully active during the calls, and allows for the flexibility of calling them as part of a more elaborate userspace suspend sequence (such as with "Screen Off" in Windows Modern Standby). Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- drivers/acpi/x86/s2idle.c | 72 +++++++++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 15 deletions(-)