| Message ID | 20220222141838.1.If784ba19e875e8ded4ec4931601ce6d255845245@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Properly undo autosuspend | expand |
On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson <dianders@chromium.org> wrote: > > The PM Runtime docs say: > Drivers in ->remove() callback should undo the runtime PM changes done > in ->probe(). Usually this means calling pm_runtime_disable(), > pm_runtime_dont_use_autosuspend() etc. > > We weren't doing that for autosuspend. Let's do it. > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Hm. I know a few places in drivers where I don't do this :/ Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, Feb 22, 2022 at 11:44:54PM +0100, Linus Walleij wrote: > On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > The PM Runtime docs say: > > Drivers in ->remove() callback should undo the runtime PM changes done > > in ->probe(). Usually this means calling pm_runtime_disable(), > > pm_runtime_dont_use_autosuspend() etc. > > > > We weren't doing that for autosuspend. Let's do it. > > > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Hm. I know a few places in drivers where I don't do this :/ It seems to be a very common problem indeed, I haven't seen any driver yet that uses pm_runtime_dont_use_autosuspend(). We could play a game of whack-a-mole, but we'll never win. Could this be solved in the runtime PM framework instead ? pm_runtime_disable() could disable auto-suspend. If there are legitimate use cases for disabling runtime PM temporarily without disabling auto-suspend, then a new function designed specifically for remove() that would take care of cleaning everything up could be another option. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Hi, On Tue, Feb 22, 2022 at 9:08 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Feb 22, 2022 at 11:44:54PM +0100, Linus Walleij wrote: > > On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > The PM Runtime docs say: > > > Drivers in ->remove() callback should undo the runtime PM changes done > > > in ->probe(). Usually this means calling pm_runtime_disable(), > > > pm_runtime_dont_use_autosuspend() etc. > > > > > > We weren't doing that for autosuspend. Let's do it. > > > > > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > Hm. I know a few places in drivers where I don't do this :/ > > It seems to be a very common problem indeed, I haven't seen any driver > yet that uses pm_runtime_dont_use_autosuspend(). We could play a game of > whack-a-mole, but we'll never win. Could this be solved in the runtime > PM framework instead ? pm_runtime_disable() could disable auto-suspend. > If there are legitimate use cases for disabling runtime PM temporarily > without disabling auto-suspend, then a new function designed > specifically for remove() that would take care of cleaning everything up > could be another option. Yeah, it would be good. It's probably not a yak I have time to shave right now, though. :( I _suspect_ that there are legitimate reasons we can't just magically do it in pm_runtime_disable(). If nothing else I believe there are legitimate code paths during normal operation that look like this: pm_runtime_disable(); do_something_that_needs_pm_runtime_disabled(); pm_runtime_enable(); Also: if it were really a simple problem to solve one would have thought that it would have been solved initially instead of adding documentation particularly mentioning pm_runtime_dont_use_autosuspend() How about a middle ground, though: we could add a devm function that does all the magic. Somewhat recently devm_pm_runtime_enable() was added. What if we add a variant for those that use autosuspend, like: devm_pm_runtime_enable_with_autosuspend(dev, initial_delay) That would: * pm_runtime_enable() * pm_runtime_set_autosuspend_delay() * pm_runtime_use_autosuspend() * Use devm_add_action_or_reset() to undo everything. Assuming that the pm_runtime folks are OK with that, we could transition things over to the new function once it rolls into mainline. So this doesn't magically fix all existing code but provides a path to make this more discoverable. -Doug
Hi Doug, On Wed, Feb 23, 2022 at 07:43:27AM -0800, Doug Anderson wrote: > On Tue, Feb 22, 2022 at 9:08 PM Laurent Pinchart wrote: > > On Tue, Feb 22, 2022 at 11:44:54PM +0100, Linus Walleij wrote: > > > On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson wrote: > > > > > > > > The PM Runtime docs say: > > > > Drivers in ->remove() callback should undo the runtime PM changes done > > > > in ->probe(). Usually this means calling pm_runtime_disable(), > > > > pm_runtime_dont_use_autosuspend() etc. > > > > > > > > We weren't doing that for autosuspend. Let's do it. > > > > > > > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend") > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > Hm. I know a few places in drivers where I don't do this :/ > > > > It seems to be a very common problem indeed, I haven't seen any driver > > yet that uses pm_runtime_dont_use_autosuspend(). We could play a game of > > whack-a-mole, but we'll never win. Could this be solved in the runtime > > PM framework instead ? pm_runtime_disable() could disable auto-suspend. > > If there are legitimate use cases for disabling runtime PM temporarily > > without disabling auto-suspend, then a new function designed > > specifically for remove() that would take care of cleaning everything up > > could be another option. > > Yeah, it would be good. It's probably not a yak I have time to shave > right now, though. :( I don't insist on shaving that yak right now :-) This patch is fine. > I _suspect_ that there are legitimate reasons we can't just magically > do it in pm_runtime_disable(). If nothing else I believe there are > legitimate code paths during normal operation that look like this: > > pm_runtime_disable(); > do_something_that_needs_pm_runtime_disabled(); > pm_runtime_enable(); > > Also: if it were really a simple problem to solve one would have > thought that it would have been solved initially instead of adding > documentation particularly mentioning > pm_runtime_dont_use_autosuspend() I'm not sure, look at how long it took for us to get pm_runtime_resume_and_get(). The problem has been considered for years as a non-issue by the runtime PM developers. It feels like the API is developed without considering its users. > How about a middle ground, though: we could add a devm function that > does all the magic. Somewhat recently devm_pm_runtime_enable() was > added. What if we add a variant for those that use autosuspend, like: > > devm_pm_runtime_enable_with_autosuspend(dev, initial_delay) > > That would: > * pm_runtime_enable() > * pm_runtime_set_autosuspend_delay() > * pm_runtime_use_autosuspend() > * Use devm_add_action_or_reset() to undo everything. > > Assuming that the pm_runtime folks are OK with that, we could > transition things over to the new function once it rolls into > mainline. > > So this doesn't magically fix all existing code but provides a path to > make this more discoverable. Sounds like a good idea. I wonder if this could be handled by devm_pm_runtime_enable() actually. If a driver calls devm_pm_runtime_enable() and then enables auto-suspend, can't the runtime PM core reasonably expect that auto-suspend should be disabled at .remove() time ? The pm_runtime_disable_action() function could disable auto-suspend unconditionally (assuming pm_runtime_use_autosuspend() and pm_runtime_dont_use_autosuspend() don't need to be balanced, if they do, then I'll just go cry in a corner).
Hi, On Wed, Feb 23, 2022 at 7:55 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > > How about a middle ground, though: we could add a devm function that > > does all the magic. Somewhat recently devm_pm_runtime_enable() was > > added. What if we add a variant for those that use autosuspend, like: > > > > devm_pm_runtime_enable_with_autosuspend(dev, initial_delay) > > > > That would: > > * pm_runtime_enable() > > * pm_runtime_set_autosuspend_delay() > > * pm_runtime_use_autosuspend() > > * Use devm_add_action_or_reset() to undo everything. > > > > Assuming that the pm_runtime folks are OK with that, we could > > transition things over to the new function once it rolls into > > mainline. > > > > So this doesn't magically fix all existing code but provides a path to > > make this more discoverable. > > Sounds like a good idea. I wonder if this could be handled by > devm_pm_runtime_enable() actually. If a driver calls > devm_pm_runtime_enable() and then enables auto-suspend, can't the > runtime PM core reasonably expect that auto-suspend should be disabled > at .remove() time ? The pm_runtime_disable_action() function could > disable auto-suspend unconditionally (assuming > pm_runtime_use_autosuspend() and pm_runtime_dont_use_autosuspend() don't > need to be balanced, if they do, then I'll just go cry in a corner). I like your idea. I think you're right that we can just leverage the existing function. This yak didn't look to hairy, so I posted a patch: https://lore.kernel.org/r/20220223083441.1.I925ce9fa12992a58caed6b297e0171d214866fe7@changeid I guess now we see what Rafael thinks. ;-) -Doug
Hi, On Wed, Feb 23, 2022 at 7:55 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Doug, > > On Wed, Feb 23, 2022 at 07:43:27AM -0800, Doug Anderson wrote: > > On Tue, Feb 22, 2022 at 9:08 PM Laurent Pinchart wrote: > > > On Tue, Feb 22, 2022 at 11:44:54PM +0100, Linus Walleij wrote: > > > > On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson wrote: > > > > > > > > > > The PM Runtime docs say: > > > > > Drivers in ->remove() callback should undo the runtime PM changes done > > > > > in ->probe(). Usually this means calling pm_runtime_disable(), > > > > > pm_runtime_dont_use_autosuspend() etc. > > > > > > > > > > We weren't doing that for autosuspend. Let's do it. > > > > > > > > > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend") > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > > > Hm. I know a few places in drivers where I don't do this :/ > > > > > > It seems to be a very common problem indeed, I haven't seen any driver > > > yet that uses pm_runtime_dont_use_autosuspend(). We could play a game of > > > whack-a-mole, but we'll never win. Could this be solved in the runtime > > > PM framework instead ? pm_runtime_disable() could disable auto-suspend. > > > If there are legitimate use cases for disabling runtime PM temporarily > > > without disabling auto-suspend, then a new function designed > > > specifically for remove() that would take care of cleaning everything up > > > could be another option. > > > > Yeah, it would be good. It's probably not a yak I have time to shave > > right now, though. :( > > I don't insist on shaving that yak right now :-) This patch is fine. Landed in drm-misc-fixes: 26d347434829 drm/bridge: ti-sn65dsi86: Properly undo autosuspend -Doug
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 38616aab12ac..fb6c588b0f71 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1758,6 +1758,7 @@ static inline void ti_sn_gpio_unregister(void) {} static void ti_sn65dsi86_runtime_disable(void *data) { + pm_runtime_dont_use_autosuspend(data); pm_runtime_disable(data); } @@ -1817,11 +1818,11 @@ static int ti_sn65dsi86_probe(struct i2c_client *client, "failed to get reference clock\n"); pm_runtime_enable(dev); + pm_runtime_set_autosuspend_delay(pdata->dev, 500); + pm_runtime_use_autosuspend(pdata->dev); ret = devm_add_action_or_reset(dev, ti_sn65dsi86_runtime_disable, dev); if (ret) return ret; - pm_runtime_set_autosuspend_delay(pdata->dev, 500); - pm_runtime_use_autosuspend(pdata->dev); ti_sn65dsi86_debugfs_init(pdata);
The PM Runtime docs say: Drivers in ->remove() callback should undo the runtime PM changes done in ->probe(). Usually this means calling pm_runtime_disable(), pm_runtime_dont_use_autosuspend() etc. We weren't doing that for autosuspend. Let's do it. Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime autosuspend") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)