| Message ID | 20200531193941.13179-2-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Suspend and resume fixes for omapdrm pdata removal | expand |
Hi Tony, On 31/05/2020 22:39, Tony Lindgren wrote: > When booting without legacy platform data, we no longer have omap_device > calling PM runtime suspend for us on suspend. This causes the driver > context not be saved as we have no suspend and resume functions defined. > > Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it > will call the existing PM runtime suspend functions on suspend. I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS modules in any order, but things have to be shut down in orderly manner. omapdrm hasn't relied on omap_device calling runtime suspend for us (I didn't know it does that). We have system suspend hooks in omap_drv.c: SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume) omap_drm_suspend() is supposed to turn off the displays, which then cause dispc_runtime_put (and other runtime_puts) to be called, which result in dispc_runtime_suspend (and other runtime PM suspends). So... For some reason that's no longer happening? I need to try to find a board with which suspend/resume works (without DSS)... Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200603 12:34]: > Hi Tony, > > On 31/05/2020 22:39, Tony Lindgren wrote: > > When booting without legacy platform data, we no longer have omap_device > > calling PM runtime suspend for us on suspend. This causes the driver > > context not be saved as we have no suspend and resume functions defined. > > > > Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it > > will call the existing PM runtime suspend functions on suspend. > > I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS > modules in any order, but things have to be shut down in orderly manner. OK. I presume you talk about the order of dss child devices here. > omapdrm hasn't relied on omap_device calling runtime suspend for us (I > didn't know it does that). We have system suspend hooks in omap_drv.c: We had omap_device sort of brute forcing things to idle on suspend which only really works for interconnect target modules with one device in them. > SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume) > > omap_drm_suspend() is supposed to turn off the displays, which then cause > dispc_runtime_put (and other runtime_puts) to be called, which result in > dispc_runtime_suspend (and other runtime PM suspends). OK thanks for explaining, I missed that part. > So... For some reason that's no longer happening? I need to try to find a > board with which suspend/resume works (without DSS)... Yes it seems something has changed. When diffing the dmesg debug output on suspend and resume, context save and restore functions are no longer called as the PM runtime suspend and resume functions are no longer called on suspend and resume. I'll drop this patch, and will be applying the rest of the series to fixes if no objections. Thanks, Tony
On 03/06/2020 17:06, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200603 12:34]: >> Hi Tony, >> >> On 31/05/2020 22:39, Tony Lindgren wrote: >>> When booting without legacy platform data, we no longer have omap_device >>> calling PM runtime suspend for us on suspend. This causes the driver >>> context not be saved as we have no suspend and resume functions defined. >>> >>> Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it >>> will call the existing PM runtime suspend functions on suspend. >> >> I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS >> modules in any order, but things have to be shut down in orderly manner. > > OK. I presume you talk about the order of dss child devices here. Yes, but not only that. E.g. the dispc driver hasn't been designed to be suspended while active. The only way to properly suspend the dispc HW is to first disable the outputs, wait until they've finished with their current frame, and only then can things be shut down. The suspend machinery doesn't handle all that (and it couldn't anyway, due to the dependencies to other DSS devices in the pipeline). >> omapdrm hasn't relied on omap_device calling runtime suspend for us (I >> didn't know it does that). We have system suspend hooks in omap_drv.c: > > We had omap_device sort of brute forcing things to idle on suspend > which only really works for interconnect target modules with one > device in them. > >> SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume) >> >> omap_drm_suspend() is supposed to turn off the displays, which then cause >> dispc_runtime_put (and other runtime_puts) to be called, which result in >> dispc_runtime_suspend (and other runtime PM suspends). > > OK thanks for explaining, I missed that part. > >> So... For some reason that's no longer happening? I need to try to find a >> board with which suspend/resume works (without DSS)... > > Yes it seems something has changed. When diffing the dmesg debug output > on suspend and resume, context save and restore functions are no longer > called as the PM runtime suspend and resume functions are no longer > called on suspend and resume. I now tested with AM4 SK, and I still can't get system suspend/resume work (without DSS). I have no clue about how to fix that. But if I use pm_test to prevent total suspend, I can reproduce this (or at least looks the same). And now that I look at this, I have a recollection that I've seen this before. What happens is that the system suspend hook (omap_drm_suspend) gets called fine, and it turns off the displays, which leads to dispc_runtime_puts etc. All goes fine. But there's an extra runtime PM reference (dev.power.usage_count) that seems to come out of nowhere. So when omap_drm_suspend is finished, there's still usage_count of 1, and dispc never suspends fully. I think the PM framework does this when starting system suspend process. Maybe this was also happening earlier, but omap_device used to do the final suspend (so omapdrm depended on that functionality, after all...). Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 07:05]: > On 03/06/2020 17:06, Tony Lindgren wrote: > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200603 12:34]: > > > Hi Tony, > > > > > > On 31/05/2020 22:39, Tony Lindgren wrote: > > > > When booting without legacy platform data, we no longer have omap_device > > > > calling PM runtime suspend for us on suspend. This causes the driver > > > > context not be saved as we have no suspend and resume functions defined. > > > > > > > > Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it > > > > will call the existing PM runtime suspend functions on suspend. > > > > > > I don't think we can use UNIVERSAL_DEV_PM_OPS, as we can't disable DSS > > > modules in any order, but things have to be shut down in orderly manner. > > > > OK. I presume you talk about the order of dss child devices here. > > Yes, but not only that. > > E.g. the dispc driver hasn't been designed to be suspended while active. The > only way to properly suspend the dispc HW is to first disable the outputs, > wait until they've finished with their current frame, and only then can > things be shut down. OK > The suspend machinery doesn't handle all that (and it couldn't anyway, due > to the dependencies to other DSS devices in the pipeline). OK I replied to your patch with some untested comments that might simplify it potentially. > > > omapdrm hasn't relied on omap_device calling runtime suspend for us (I > > > didn't know it does that). We have system suspend hooks in omap_drv.c: > > > > We had omap_device sort of brute forcing things to idle on suspend > > which only really works for interconnect target modules with one > > device in them. > > > > > SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume) > > > > > > omap_drm_suspend() is supposed to turn off the displays, which then cause > > > dispc_runtime_put (and other runtime_puts) to be called, which result in > > > dispc_runtime_suspend (and other runtime PM suspends). > > > > OK thanks for explaining, I missed that part. > > > > > So... For some reason that's no longer happening? I need to try to find a > > > board with which suspend/resume works (without DSS)... > > > > Yes it seems something has changed. When diffing the dmesg debug output > > on suspend and resume, context save and restore functions are no longer > > called as the PM runtime suspend and resume functions are no longer > > called on suspend and resume. > > I now tested with AM4 SK, and I still can't get system suspend/resume work > (without DSS). I have no clue about how to fix that. But if I use pm_test to > prevent total suspend, I can reproduce this (or at least looks the same). OK > And now that I look at this, I have a recollection that I've seen this > before. What happens is that the system suspend hook (omap_drm_suspend) gets > called fine, and it turns off the displays, which leads to > dispc_runtime_puts etc. All goes fine. > > But there's an extra runtime PM reference (dev.power.usage_count) that seems > to come out of nowhere. So when omap_drm_suspend is finished, there's still > usage_count of 1, and dispc never suspends fully. Hmm no idea about that. My guess is that there might be an issue that was masked earlier with omap_device calling the child runtime_suspend. Currently I'm only able to rmmod -f omapdrm, not sure if these issues might be related. As we now have the interconnect target module as the parent so usage counts might be different but should balance out the same way as earlier. > I think the PM framework does this when starting system suspend process. > Maybe this was also happening earlier, but omap_device used to do the final > suspend (so omapdrm depended on that functionality, after all...). Yes the different handling seems to be the main part of the issue. Regards, Tony
On 09/06/2020 18:19, Tony Lindgren wrote: >> But there's an extra runtime PM reference (dev.power.usage_count) that seems >> to come out of nowhere. So when omap_drm_suspend is finished, there's still >> usage_count of 1, and dispc never suspends fully. > > Hmm no idea about that. My guess is that there might be an issue that was > masked earlier with omap_device calling the child runtime_suspend. Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device. > Currently I'm only able to rmmod -f omapdrm, not sure if these issues might > be related. Hmm, I always use modules, and can unload omapdrm and drm fine. But there's a sequence that must be followed. However, the sequence starts with unloading omapdrm... What behavior you see with rmmod? Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 15:27]: > On 09/06/2020 18:19, Tony Lindgren wrote: > > Currently I'm only able to rmmod -f omapdrm, not sure if these issues might > > be related. > > Hmm, I always use modules, and can unload omapdrm and drm fine. But there's > a sequence that must be followed. However, the sequence starts with > unloading omapdrm... What behavior you see with rmmod? Hmm maybe it's output specific somehow? I just tried again with the following with v5.7. I see the omapdrm usage count issue happen at least on duovero, but don't seem to currently get /dev/fb0 initialized on x15 with these: modprobe omapdrm #modprobe connector_hdmi # up to v5.6 modprobe display-connector # starting with v5.7-rc1 modprobe ti-tpd12s015 # beagle-x15 modprobe omapdss # rmmod omapdrm rmmod: ERROR: Module omapdrm is in use # lsmod | grep omapdrm omapdrm 65536 1 omapdss_base 16384 2 omapdrm,omapdss drm_kms_helper 155648 3 omapdss_base,omapdrm,omapdss drm 372736 7 ti_tpd12s015,omapdss_base,display_connector,omapdrm,omapdss,drm_kms_helper On beagle-x15 I see these errors after modprobe: DSS: OMAP DSS rev 6.1 omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes aic_dvdd_fixed: disabling ldousb: disabling Maybe I'm missing some related module on x15? Regards, Tony
* Tony Lindgren <tony@atomide.com> [200609 16:53]: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200609 15:27]: > > On 09/06/2020 18:19, Tony Lindgren wrote: > > > Currently I'm only able to rmmod -f omapdrm, not sure if these issues might > > > be related. > > > > Hmm, I always use modules, and can unload omapdrm and drm fine. But there's > > a sequence that must be followed. However, the sequence starts with > > unloading omapdrm... What behavior you see with rmmod? > > Hmm maybe it's output specific somehow? > > I just tried again with the following with v5.7. I see the omapdrm > usage count issue happen at least on duovero, but don't seem to > currently get /dev/fb0 initialized on x15 with these: > > modprobe omapdrm > #modprobe connector_hdmi # up to v5.6 > modprobe display-connector # starting with v5.7-rc1 > modprobe ti-tpd12s015 # beagle-x15 > modprobe omapdss > > # rmmod omapdrm > rmmod: ERROR: Module omapdrm is in use > > # lsmod | grep omapdrm > omapdrm 65536 1 > omapdss_base 16384 2 omapdrm,omapdss > drm_kms_helper 155648 3 omapdss_base,omapdrm,omapdss > drm 372736 7 ti_tpd12s015,omapdss_base,display_connector,omapdrm,omapdss,drm_kms_helper I'm also seeing the rmmod omapdrm issue on am437x-sk-evm: modprobe pwm-omap-dmtimer modprobe pwm-tiecap modprobe pwm_bl modprobe omapdrm modprobe panel-simple modprobe display-connector # starting with v5.7-rc1 modprobe omapdss # rmmod omapdrm rmmod: ERROR: Module omapdrm is in use > On beagle-x15 I see these errors after modprobe: > > DSS: OMAP DSS rev 6.1 > omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) > omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) > [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes > [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 > omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes > aic_dvdd_fixed: disabling > ldousb: disabling > > Maybe I'm missing some related module on x15? Still did not figure what I might be missing on x15 :) Regards, Tony
* Tony Lindgren <tony@atomide.com> [200609 17:11]: > I'm also seeing the rmmod omapdrm issue on am437x-sk-evm: Oops sorry this is a user error. I've forgotten I need to unbind the fb vtcon first :) thanks for hinting that Tomi! I can rmmod omapdrm just fine after doing: # echo 0 > /sys/class/vtconsole/vtcon1/bind > Still did not figure what I might be missing on x15 :) So this is really the only issue that I have but sounds like Tomi has it working and I'm just missing some module. Regards, Tony
On 09/06/2020 20:10, Tony Lindgren wrote: >> On beagle-x15 I see these errors after modprobe: >> >> DSS: OMAP DSS rev 6.1 >> omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) >> omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) >> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >> omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes >> [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 >> omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes >> aic_dvdd_fixed: disabling >> ldousb: disabling >> >> Maybe I'm missing some related module on x15? > > Still did not figure what I might be missing on x15 :) The log above shows that nothing is missing, omapdrm has probed fine. But it cannot see anything connected to the hdmi port. Are you booting with correct dtb for your x15 revision? And you have a monitor connected? =) Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200610 11:48]: > On 09/06/2020 20:10, Tony Lindgren wrote: > > > > On beagle-x15 I see these errors after modprobe: > > > > > > DSS: OMAP DSS rev 6.1 > > > omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops [omapdss]) > > > omapdss_dss 58000000.dss: bound 58040000.encoder (ops hdmi5_component_ops [omapdss]) > > > [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > > > omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes > > > [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0 > > > omapdrm omapdrm.0: [drm] Cannot find any crtc or sizes > > > aic_dvdd_fixed: disabling > > > ldousb: disabling > > > > > > Maybe I'm missing some related module on x15? > > > > Still did not figure what I might be missing on x15 :) > > The log above shows that nothing is missing, omapdrm has probed fine. But it > cannot see anything connected to the hdmi port. Are you booting with correct > dtb for your x15 revision? And you have a monitor connected? =) Oh you're right, I forgot to connect the HDMI cable back to X15 :) No wonder it's no longer working for me.. Regards, Tony
On 09/06/2020 18:26, Tomi Valkeinen wrote: > On 09/06/2020 18:19, Tony Lindgren wrote: >>> But there's an extra runtime PM reference (dev.power.usage_count) that seems >>> to come out of nowhere. So when omap_drm_suspend is finished, there's still >>> usage_count of 1, and dispc never suspends fully. >> >> Hmm no idea about that. My guess is that there might be an issue that was >> masked earlier with omap_device calling the child runtime_suspend. > > Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device. > I think I might have an idea what is going wrong. Before: +----------------------+ |omap_device_pm_domain | +---------------+------+------+ | device | +-------------+ | omap_device | +-------------+ omap_device is embedded in DD device and PM handled by omap_device_pm_domain. static int _od_suspend_noirq(struct device *dev) { ... ret = pm_generic_suspend_noirq(dev); [1] ^^ device suspend_noirq call if (!ret && !pm_runtime_status_suspended(dev)) { if (pm_generic_runtime_suspend(dev) == 0) { [2] ^^ device pm_runtime_suspend force call omap_device_idle(pdev); [3] ^^ omap_device disable od->flags |= OMAP_DEVICE_SUSPENDED; } } return ret; } Now: +------------+ |ti sysc dev | +-+----------+ | | | +-------------+ | | device | +-->+ | +-------------+ With new approach the omap_device is not embedded in DD Device anymore, instead ti-sysc (hwmod replacement) became parent of DD Device. As result suspend sequence became the following (Note. All PM runtime PUT calls became NOP during suspend by design): device |-> suspend() - in case of dss omap_drm_suspend() and Co if defined |-> suspend_noirq() - in case of dss *not defined", equal to step [1] above .. ti sysc dev (ti-sysc is parent, so called after device) |-> sysc_noirq_suspend |-> pm_runtime_force_suspend() |-> sysc_runtime_suspend() - equal to step [3] above And step [2] is missing as of now! I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do pm_runtime_force_xxx() calls at noirq suspend stage by adding: SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) Am I missing smth?
* Grygorii Strashko <grygorii.strashko@ti.com> [200611 13:59]: > I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do > pm_runtime_force_xxx() calls at noirq suspend stage by adding: > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume) > > Am I missing smth? Sounds good to me, makes it behave like a standard Linux driver now :) Regards, Tony
On 11/06/2020 17:00, Grygorii Strashko wrote: > > > On 09/06/2020 18:26, Tomi Valkeinen wrote: >> On 09/06/2020 18:19, Tony Lindgren wrote: >>>> But there's an extra runtime PM reference (dev.power.usage_count) that seems >>>> to come out of nowhere. So when omap_drm_suspend is finished, there's still >>>> usage_count of 1, and dispc never suspends fully. >>> >>> Hmm no idea about that. My guess is that there might be an issue that was >>> masked earlier with omap_device calling the child runtime_suspend. >> >> Yes. It's how PM works. It calls pm_runtime_get_noresume() before starting the suspend of a >> device. So I guess omapdrm's suspend has been broken all the time, but it was "fixed" by omap_device. >> > > I think I might have an idea what is going wrong. > > Before: > +----------------------+ > |omap_device_pm_domain | > +---------------+------+------+ > | device | > +-------------+ > | omap_device | > +-------------+ > > omap_device is embedded in DD device and PM handled by omap_device_pm_domain. > > static int _od_suspend_noirq(struct device *dev) > { > ... > > ret = pm_generic_suspend_noirq(dev); > [1] ^^ device suspend_noirq call > > if (!ret && !pm_runtime_status_suspended(dev)) { > if (pm_generic_runtime_suspend(dev) == 0) { > [2] ^^ device pm_runtime_suspend force call > > omap_device_idle(pdev); > [3] ^^ omap_device disable > od->flags |= OMAP_DEVICE_SUSPENDED; > } > } > > return ret; > } > > Now: > +------------+ > |ti sysc dev | > +-+----------+ > | > | > | +-------------+ > | | device | > +-->+ | > +-------------+ > > With new approach the omap_device is not embedded in DD Device anymore, > instead ti-sysc (hwmod replacement) became parent of DD Device. > > As result suspend sequence became the following > (Note. All PM runtime PUT calls became NOP during suspend by design): > > device > |-> suspend() - in case of dss omap_drm_suspend() and Co if defined > |-> suspend_noirq() - in case of dss *not defined", equal to step [1] above > .. > > ti sysc dev (ti-sysc is parent, so called after device) > |-> sysc_noirq_suspend > |-> pm_runtime_force_suspend() > |-> sysc_runtime_suspend() - equal to step [3] above > > And step [2] is missing as of now! > > I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do > pm_runtime_force_xxx() calls at noirq suspend stage by adding: > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume) > > Am I missing smth? Isn't this almost exactly the same my patch does? I just used suspend_late and resume_early. Is noirq phase better than late & early? Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [200616 13:02]: > On 11/06/2020 17:00, Grygorii Strashko wrote: > > I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do > > pm_runtime_force_xxx() calls at noirq suspend stage by adding: > > > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > pm_runtime_force_resume) > > > > Am I missing smth? > > Isn't this almost exactly the same my patch does? I just used suspend_late > and resume_early. Is noirq phase better than late & early? Well up to you as far as I'm concerned. The noirq phase comes with serious limitations, for let's say i2c bus usage if needed. Probably also harder to debug for suspend and resume. Regards, Tony
On 16/06/2020 18:30, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [200616 13:02]: >> On 11/06/2020 17:00, Grygorii Strashko wrote: >>> I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do >>> pm_runtime_force_xxx() calls at noirq suspend stage by adding: >>> >>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>> pm_runtime_force_resume) >>> >>> Am I missing smth? >> >> Isn't this almost exactly the same my patch does? I just used suspend_late >> and resume_early. Is noirq phase better than late & early? > > Well up to you as far as I'm concerned. The noirq phase comes with serious > limitations, for let's say i2c bus usage if needed. Probably also harder > to debug for suspend and resume. Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still work and there is no sync between suspend and pm_runtime. The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled.
On 16/06/2020 19:56, Grygorii Strashko wrote: > > > On 16/06/2020 18:30, Tony Lindgren wrote: >> * Tomi Valkeinen <tomi.valkeinen@ti.com> [200616 13:02]: >>> On 11/06/2020 17:00, Grygorii Strashko wrote: >>>> I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do >>>> pm_runtime_force_xxx() calls at noirq suspend stage by adding: >>>> >>>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>>> pm_runtime_force_resume) >>>> >>>> Am I missing smth? >>> >>> Isn't this almost exactly the same my patch does? I just used suspend_late >>> and resume_early. Is noirq phase better than late & early? >> >> Well up to you as far as I'm concerned. The noirq phase comes with serious >> limitations, for let's say i2c bus usage if needed. Probably also harder >> to debug for suspend and resume. > > Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still > work and > there is no sync between suspend and pm_runtime. > The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled. Yes, but which one... Do you know what the diff is with late/noirq from driver's perspective? I guess noirq is atomic context, late is nto? Dispc's suspend uses synchronize_irq(), so that rules out noirq. Although the call is not needed if using noirq version, so that could also be managed with small change. But I wonder if there's any benefit in using noirq versus late. Tomi
On 17/06/2020 09:04, Tomi Valkeinen wrote: > On 16/06/2020 19:56, Grygorii Strashko wrote: >> >> >> On 16/06/2020 18:30, Tony Lindgren wrote: >>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [200616 13:02]: >>>> On 11/06/2020 17:00, Grygorii Strashko wrote: >>>>> I think, suspend might be fixed if all devices, which are now child of ti-sysc, will do >>>>> pm_runtime_force_xxx() calls at noirq suspend stage by adding: >>>>> >>>>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>>>> pm_runtime_force_resume) >>>>> >>>>> Am I missing smth? >>>> >>>> Isn't this almost exactly the same my patch does? I just used suspend_late >>>> and resume_early. Is noirq phase better than late & early? >>> >>> Well up to you as far as I'm concerned. The noirq phase comes with serious >>> limitations, for let's say i2c bus usage if needed. Probably also harder >>> to debug for suspend and resume. >> >> Unfortunately, you can't use PM runtime force API at .suspend() stage as pm_runtime_get will still work and >> there is no sync between suspend and pm_runtime. >> The PM runtime force API can be used only during late/noirq as at this time pm_runtime is disabled. > > Yes, but which one... Do you know what the diff is with late/noirq from driver's perspective? I guess noirq is atomic context, late is nto? noirq is *not* atomic, jus IRQs (non-wakeup) will be disabled (disbale_irq()) > > Dispc's suspend uses synchronize_irq(), so that rules out noirq. Although the call is not needed if using noirq version, so that could also be managed with small change. But I wonder if there's any benefit in using noirq versus late.
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -4933,10 +4933,8 @@ static int dispc_runtime_resume(struct device *dev) return 0; } -static const struct dev_pm_ops dispc_pm_ops = { - .runtime_suspend = dispc_runtime_suspend, - .runtime_resume = dispc_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(dispc_pm_ops, dispc_runtime_suspend, + dispc_runtime_resume, NULL); struct platform_driver omap_dispchw_driver = { .probe = dispc_probe, diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5464,10 +5464,8 @@ static int dsi_runtime_resume(struct device *dev) return 0; } -static const struct dev_pm_ops dsi_pm_ops = { - .runtime_suspend = dsi_runtime_suspend, - .runtime_resume = dsi_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(dsi_pm_ops, dsi_runtime_suspend, + dsi_runtime_resume, NULL); struct platform_driver omap_dsihw_driver = { .probe = dsi_probe, diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c --- a/drivers/gpu/drm/omapdrm/dss/dss.c +++ b/drivers/gpu/drm/omapdrm/dss/dss.c @@ -1611,10 +1611,8 @@ static int dss_runtime_resume(struct device *dev) return 0; } -static const struct dev_pm_ops dss_pm_ops = { - .runtime_suspend = dss_runtime_suspend, - .runtime_resume = dss_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(dss_pm_ops, dss_runtime_suspend, + dss_runtime_resume, NULL); struct platform_driver omap_dsshw_driver = { .probe = dss_probe, diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c --- a/drivers/gpu/drm/omapdrm/dss/venc.c +++ b/drivers/gpu/drm/omapdrm/dss/venc.c @@ -942,10 +942,8 @@ static int venc_runtime_resume(struct device *dev) return 0; } -static const struct dev_pm_ops venc_pm_ops = { - .runtime_suspend = venc_runtime_suspend, - .runtime_resume = venc_runtime_resume, -}; +static UNIVERSAL_DEV_PM_OPS(venc_pm_ops, venc_runtime_suspend, + venc_runtime_resume, NULL); static const struct of_device_id venc_of_match[] = { { .compatible = "ti,omap2-venc", }, diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -1169,7 +1169,6 @@ int tiler_map_show(struct seq_file *s, void *arg) } #endif -#ifdef CONFIG_PM_SLEEP static int omap_dmm_resume(struct device *dev) { struct tcm_area area; @@ -1193,9 +1192,8 @@ static int omap_dmm_resume(struct device *dev) return 0; } -#endif -static SIMPLE_DEV_PM_OPS(omap_dmm_pm_ops, NULL, omap_dmm_resume); +static UNIVERSAL_DEV_PM_OPS(omap_dmm_pm_ops, NULL, omap_dmm_resume, NULL); #if defined(CONFIG_OF) static const struct dmm_platform_data dmm_omap4_platform_data = {
When booting without legacy platform data, we no longer have omap_device calling PM runtime suspend for us on suspend. This causes the driver context not be saved as we have no suspend and resume functions defined. Let's fix the issue by switching over to use UNIVERSAL_DEV_PM_OPS as it will call the existing PM runtime suspend functions on suspend. Fixes: cef766300353 ("drm/omap: Prepare DSS for probing without legacy platform data") Reported-by: Faiz Abbas <faiz_abbas@ti.com> Cc: dri-devel@lists.freedesktop.org Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/gpu/drm/omapdrm/dss/dispc.c | 6 ++---- drivers/gpu/drm/omapdrm/dss/dsi.c | 6 ++---- drivers/gpu/drm/omapdrm/dss/dss.c | 6 ++---- drivers/gpu/drm/omapdrm/dss/venc.c | 6 ++---- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 4 +--- 5 files changed, 9 insertions(+), 19 deletions(-)