On 06/11/2023 00:53, Laurent Pinchart wrote:
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().
(The text below is more about the end result of this series, rather than
this specific patch):
Hmm, no, I don't think that's how it works. My understanding is this:
There are multiple parts "enabling the hardware", and I think they
usually need to be done in this order: 1) enabling the parent devices,
2) system level HW module enable (this is possibly really part of the
1), 3) clk/regulator/register setup.
3) is handled by the driver, but 1) and 2) are handled via the runtime
PM framework. Calling dispc_runtime_resume() as the first thing could
mean that DSS's parents are not enabled or that the DSS HW module is not
enabled at the system control level.
That's why I first call pm_runtime_set_active(), which should handle 1)
and 2).
The only thing dispc_runtime_resume() does wrt. enabling the hardware is
enabling the fclk. It does a lot more, but all the rest is just
configuring the hardware to settings that we always want to use (e.g.
fifo management).
Now, if the bootloader had enabled the display, and the driver did:
- pm_runtime_enable()
- pm_runtime_get()
- dispc_reset()
it would cause dispc_runtime_resume() to be called before the reset.
This would mean that the dispc_runtime_resume() would be changing
settings that must not be changed while streaming is enabled.
We could do a DSS reset always as the first thing in
dispc_runtime_resume() (after enabling the fclk), but that feels a bit
pointless as after the first reset the DSS is in a known state.
Also, if we don't do a reset at probe time, there are things we need to
take care of: at least we need to mask the IRQs (presuming we register
the DSS interrupt at probe time). But generally speaking, I feel a bit
uncomfortable leaving an IP possibly running in an unknown state after
probe. I'd much rather just reset it at probe.
Tomi
