| Message ID | 20231101-tidss-probe-v1-2-45149e0f9415@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Probe related fixes and cleanups | expand |
Hi Tomi, Thank you for the patch. 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@ideasonboard.com> > --- > 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); 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. > pm_runtime_disable(dev); > > /* devm allocated dispc goes away with the dev so mark it NULL */ >
On 01/11/2023 15:54, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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@ideasonboard.com> >> --- >> 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. And even if it wouldn't, we don't want to call dispc_runtime_resume() in probe when we have PM. > 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. Tomi
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@ideasonboard.com> > >> --- > >> 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.
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@ideasonboard.com> >>>> --- >>>> 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
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); @@ -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); pm_runtime_disable(dev); /* devm allocated dispc goes away with the dev so mark it NULL */
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@ideasonboard.com> --- drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)