diff mbox series

[1/5] drm/omap: Fix suspend resume regression after platform data removal

Message ID 20200531193941.13179-2-tony@atomide.com
State New, archived
Headers show
Series Suspend and resume fixes for omapdrm pdata removal | expand

Commit Message

Tony Lindgren May 31, 2020, 7:39 p.m. UTC
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(-)

Comments

Tomi Valkeinen June 3, 2020, 12:33 p.m. UTC | #1
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
Tony Lindgren June 3, 2020, 2:06 p.m. UTC | #2
* 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
Tomi Valkeinen June 9, 2020, 7:04 a.m. UTC | #3
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
Tony Lindgren June 9, 2020, 3:19 p.m. UTC | #4
* 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
Tomi Valkeinen June 9, 2020, 3:26 p.m. UTC | #5
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
Tony Lindgren June 9, 2020, 4:52 p.m. UTC | #6
* 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 June 9, 2020, 5:10 p.m. UTC | #7
* 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 June 9, 2020, 5:26 p.m. UTC | #8
* 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
Tomi Valkeinen June 10, 2020, 11:47 a.m. UTC | #9
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
Tony Lindgren June 10, 2020, 10:41 p.m. UTC | #10
* 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
Grygorii Strashko June 11, 2020, 2 p.m. UTC | #11
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?
Tony Lindgren June 11, 2020, 2:32 p.m. UTC | #12
* 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
Tomi Valkeinen June 16, 2020, 1:01 p.m. UTC | #13
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
Tony Lindgren June 16, 2020, 3:30 p.m. UTC | #14
* 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
Grygorii Strashko June 16, 2020, 4:56 p.m. UTC | #15
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.
Tomi Valkeinen June 17, 2020, 6:04 a.m. UTC | #16
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
Grygorii Strashko June 17, 2020, 12:49 p.m. UTC | #17
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 mbox series

Patch

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 = {