Hi Tomi, CC'ing Sakari for his expertise on runtime PM (I think he will soon start wishing he would be ignorant in this area). On Thu, Nov 02, 2023 at 08:34:45AM +0200, Tomi Valkeinen wrote: > On 01/11/2023 15:54, Laurent Pinchart wrote: > > On Wed, Nov 01, 2023 at 11:17:39AM +0200, Tomi Valkeinen wrote: > >> Use runtime PM autosuspend feature, with 1s timeout, to avoid > >> unnecessary suspend-resume cycles when, e.g. the userspace temporarily > >> turns off the crtcs when configuring the outputs. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c > >> index f403db11b846..64914331715a 100644 > >> --- a/drivers/gpu/drm/tidss/tidss_drv.c > >> +++ b/drivers/gpu/drm/tidss/tidss_drv.c > >> @@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss) > >> > >> dev_dbg(tidss->dev, "%s\n", __func__); > >> > >> - r = pm_runtime_put_sync(tidss->dev); > >> + pm_runtime_mark_last_busy(tidss->dev); > >> + > >> + r = pm_runtime_put_autosuspend(tidss->dev); > >> WARN_ON(r < 0); > >> } > >> > >> @@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev) > >> > >> pm_runtime_enable(dev); > >> > >> + pm_runtime_set_autosuspend_delay(dev, 1000); > >> + pm_runtime_use_autosuspend(dev); > >> + > >> #ifndef CONFIG_PM > >> /* If we don't have PM, we need to call resume manually */ > >> dispc_runtime_resume(tidss->dispc); > > > > By the way, there's a way to handle this without any ifdef: > > > > dispc_runtime_resume(tidss->dispc); > > > > pm_runtime_set_active(dev); > > pm_runtime_get_noresume(dev); > > pm_runtime_enable(dev); > > pm_runtime_set_autosuspend_delay(dev, 1000); > > pm_runtime_use_autosuspend(dev); > > I'm not sure I follow what you are trying to do here. The call to > dispc_runtime_resume() would crash if we have PM, as the HW would not be > enabled at that point. Isn't dispc_runtime_resume() meant to enable the hardware ? The idea is to enable the hardware, then enable runtime PM, and tell the runtime PM framework that the device is enabled. If CONFIG_PM is not set, the RPM calls will be no-ops, and the device will stay enable. If CONFIG_PM is set, the device will be enabled, and will get disabled at end of probe by a call to pm_runtime_put_autosuspend(). > And even if it wouldn't, we don't want to call dispc_runtime_resume() > in probe when we have PM. Don't you need to enable the device at probe time in order to reset it, as done in later patches in the series ? > > Then, in the error path, > > > > pm_runtime_dont_use_autosuspend(dev); > > pm_runtime_disable(dev); > > pm_runtime_put_noidle(dev); > > > > dispc_runtime_suspend(tidss->dispc); > > > > And in remove: > > > > pm_runtime_dont_use_autosuspend(dev); > > pm_runtime_disable(dev); > > if (!pm_runtime_status_suspended(dev)) > > dispc_runtime_suspend(tidss->dispc); > > pm_runtime_set_suspended(dev); > > > > And yes, runtime PM is a horrible API. > > > >> @@ -215,6 +220,7 @@ static void tidss_remove(struct platform_device *pdev) > >> /* If we don't have PM, we need to call suspend manually */ > >> dispc_runtime_suspend(tidss->dispc); > >> #endif > >> + pm_runtime_dont_use_autosuspend(dev); > > > > This also needs to be done in the probe error path. > > Oops. Right, I'll add that. -- Regards, Laurent Pinchart
