While working on the TI BSP kernel, adding bootload splash screen
support, I noticed some issues with the driver and opportunities for
cleanups and improvements.
Tomi
Signed-off-by: Tomi Valkeinen <[email protected]>
---
Tomi Valkeinen (10):
drm/tidss: Use pm_runtime_resume_and_get()
drm/tidss: Use PM autosuspend
drm/tidss: Drop useless variable init
drm/tidss: Move reset to the end of dispc_init()
drm/tidss: Return error value from from softreset
drm/tidss: Check for K2G in in dispc_softreset()
drm/tidss: Fix dss reset
drm/tidss: Add dispc_is_idle()
drm/tidss: IRQ code cleanup
drm/tidss: Fix atomic_flush check
drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++---
drivers/gpu/drm/tidss/tidss_dispc.c | 76 +++++++++++++++++++++++++++++++++----
drivers/gpu/drm/tidss/tidss_drv.c | 14 +++++--
drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++----------------------
4 files changed, 92 insertions(+), 61 deletions(-)
---
base-commit: 9d7c8c066916f231ca0ed4e4fce6c4b58ca3e451
change-id: 20231030-tidss-probe-854b1098c3af
Best regards,
--
Tomi Valkeinen <[email protected]>
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 <[email protected]>
---
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);
@@ -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 */
--
2.34.1
The IRQ setup code is overly complex. All we really need to do is
initialize the related fields in struct tidss_device, and request the
IRQ.
We can drop all the HW accesses, as they are pointless: the driver will
set the IRQs correctly when it needs any of the IRQs, and at probe time
we have done a reset, so we know that all the IRQs are masked by default
in the hardware.
Thus we can combine the tidss_irq_preinstall() and
tidss_irq_postinstall() into the tidss_irq_install() function, drop the
HW accesses, and drop the use of spinlock, as this is done at init time
and there can be no races.
We can also drop the HW access from the tidss_irq_uninstall(), as the
driver will anyway disable and suspend the hardware at remove time.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/tidss/tidss_drv.c | 2 ++
drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
2 files changed, 10 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 64914331715a..37693f30d98b 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, tidss);
+ spin_lock_init(&tidss->wait_lock);
+
ret = dispc_init(tidss);
if (ret) {
dev_err(dev, "failed to initialize dispc: %d\n", ret);
diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
index 0c681c7600bc..604334ef526a 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.c
+++ b/drivers/gpu/drm/tidss/tidss_irq.c
@@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
spin_unlock_irqrestore(&tidss->wait_lock, flags);
}
-static void tidss_irq_preinstall(struct drm_device *ddev)
-{
- struct tidss_device *tidss = to_tidss(ddev);
-
- spin_lock_init(&tidss->wait_lock);
-
- tidss_runtime_get(tidss);
-
- dispc_set_irqenable(tidss->dispc, 0);
- dispc_read_and_clear_irqstatus(tidss->dispc);
-
- tidss_runtime_put(tidss);
-}
-
-static void tidss_irq_postinstall(struct drm_device *ddev)
+int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
{
struct tidss_device *tidss = to_tidss(ddev);
- unsigned long flags;
- unsigned int i;
+ int ret;
- tidss_runtime_get(tidss);
+ if (irq == IRQ_NOTCONNECTED)
+ return -ENOTCONN;
- spin_lock_irqsave(&tidss->wait_lock, flags);
+ ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
+ if (ret)
+ return ret;
tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
- for (i = 0; i < tidss->num_crtcs; ++i) {
+ for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
@@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
}
- tidss_irq_update(tidss);
-
- spin_unlock_irqrestore(&tidss->wait_lock, flags);
-
- tidss_runtime_put(tidss);
-}
-
-int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
-{
- int ret;
-
- if (irq == IRQ_NOTCONNECTED)
- return -ENOTCONN;
-
- tidss_irq_preinstall(ddev);
-
- ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
- if (ret)
- return ret;
-
- tidss_irq_postinstall(ddev);
-
return 0;
}
@@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
{
struct tidss_device *tidss = to_tidss(ddev);
- tidss_runtime_get(tidss);
- dispc_set_irqenable(tidss->dispc, 0);
- tidss_runtime_put(tidss);
-
free_irq(tidss->irq, ddev);
}
--
2.34.1
Add a helper function, dispc_is_idle(), which returns whether the DSS is
idle (i.e. is any video port enabled).
For now we add a call to it in the suspend and resume callbacks, and
print a warning if in either place the DSS is not idle. In the future
this can be used to detect if the bootloader had enabled the DSS, and
the driver can act on that.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 13db062892e3..a527c28c8833 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2603,10 +2603,18 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
}
+static bool dispc_is_idle(struct dispc_device *dispc)
+{
+ return REG_GET(dispc, DSS_SYSSTATUS, 9, 9);
+}
+
int dispc_runtime_suspend(struct dispc_device *dispc)
{
dev_dbg(dispc->dev, "suspend\n");
+ if (!dispc_is_idle(dispc))
+ dev_warn(dispc->dev, "Bad HW state: DSS not idle when suspending");
+
dispc->is_enabled = false;
clk_disable_unprepare(dispc->fclk);
@@ -2620,6 +2628,9 @@ int dispc_runtime_resume(struct dispc_device *dispc)
clk_prepare_enable(dispc->fclk);
+ if (!dispc_is_idle(dispc))
+ dev_warn(dispc->dev, "Bad HW state: DSS not idle when resuming");
+
if (REG_GET(dispc, DSS_SYSSTATUS, 0, 0) == 0)
dev_warn(dispc->dev, "DSS FUNC RESET not done!\n");
--
2.34.1
The probe function calls dispc_softreset() before runtime PM is enabled
and without enabling any of the DSS clocks. This happens to work by
luck, and we need to make sure the DSS HW is active and the fclk is
enabled.
To fix the above, add a new function, dispc_init_hw(), which does:
- pm_runtime_set_active()
- clk_prepare_enable(fclk)
- dispc_softreset().
This ensures that the reset can be successfully accomplished.
Note that we use pm_runtime_set_active(), not the normal
pm_runtime_get(). The reason for this is that at this point we haven't
enabled the runtime PM yet and also we don't want the normal resume
callback to be called: the dispc resume callback does some initial HW
setup, and it expects that the HW was off (no video ports are
streaming). If the bootloader has enabled the DSS and has set up a
boot time splash-screen, the DSS would be enabled and streaming which
might lead to issues with the normal resume callback.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index f204a0701e9f..13db062892e3 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc)
return 0;
}
+static int dispc_init_hw(struct dispc_device *dispc)
+{
+ struct device *dev = dispc->dev;
+ int ret;
+
+ ret = pm_runtime_set_active(dev);
+ if (ret) {
+ dev_err(dev, "Failed to set DSS PM to active\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(dispc->fclk);
+ if (ret) {
+ dev_err(dev, "Failed to enable DSS fclk\n");
+ goto err_runtime_suspend;
+ }
+
+ ret = dispc_softreset(dispc);
+ if (ret)
+ goto err_clk_disable;
+
+ clk_disable_unprepare(dispc->fclk);
+ ret = pm_runtime_set_suspended(dev);
+ if (ret) {
+ dev_err(dev, "Failed to set DSS PM to suspended\n");
+ return ret;
+ }
+
+ return 0;
+
+err_clk_disable:
+ clk_disable_unprepare(dispc->fclk);
+
+err_runtime_suspend:
+ ret = pm_runtime_set_suspended(dev);
+ if (ret) {
+ dev_err(dev, "Failed to set DSS PM to suspended\n");
+ return ret;
+ }
+
+ return ret;
+}
+
int dispc_init(struct tidss_device *tidss)
{
struct device *dev = tidss->dev;
@@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss)
tidss->dispc = dispc;
- r = dispc_softreset(dispc);
+ r = dispc_init_hw(dispc);
if (r)
return r;
--
2.34.1
tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
returns immediately as there's no reason to do any register changes.
However, the code checks for 'crtc->state->enable', which does not
reflect the actual HW state. We should instead look at the
'crtc->state->active' flag.
This causes the tidss_crtc_atomic_flush() to proceed with the flush even
if the active state is false, which then causes us to hit the
WARN_ON(!crtc->state->event) check.
Fix this by checking the active flag, and while at it, fix the related
debug print which had "active" and "needs modeset" wrong way.
Signed-off-by: Tomi Valkeinen <[email protected]>
---
drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 5e5e466f35d1..4c7009a5d643 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
struct tidss_device *tidss = to_tidss(ddev);
unsigned long flags;
- dev_dbg(ddev->dev,
- "%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
- crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
- crtc->state->enable, crtc->state->event);
+ dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
+ __func__, crtc->name, crtc->state->active,
+ drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);
/* There is nothing to do if CRTC is not going to be enabled. */
- if (!crtc->state->enable)
+ if (!crtc->state->active)
return;
/*
--
2.34.1
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 <[email protected]>
> ---
> 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 */
>
--
Regards,
Laurent Pinchart
Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
> The probe function calls dispc_softreset() before runtime PM is enabled
> and without enabling any of the DSS clocks. This happens to work by
> luck, and we need to make sure the DSS HW is active and the fclk is
> enabled.
>
> To fix the above, add a new function, dispc_init_hw(), which does:
>
> - pm_runtime_set_active()
> - clk_prepare_enable(fclk)
> - dispc_softreset().
>
> This ensures that the reset can be successfully accomplished.
>
> Note that we use pm_runtime_set_active(), not the normal
> pm_runtime_get(). The reason for this is that at this point we haven't
> enabled the runtime PM yet and also we don't want the normal resume
> callback to be called: the dispc resume callback does some initial HW
> setup, and it expects that the HW was off (no video ports are
> streaming). If the bootloader has enabled the DSS and has set up a
> boot time splash-screen, the DSS would be enabled and streaming which
> might lead to issues with the normal resume callback.
I think the right way to do this would be, in probe(), to
- power on the device
- enable runtime PM, masking the device as active
- at end of probe, calling pm_runtime_put_autosuspend()
Please also see my review of patch 02/10.
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index f204a0701e9f..13db062892e3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc)
> return 0;
> }
>
> +static int dispc_init_hw(struct dispc_device *dispc)
> +{
> + struct device *dev = dispc->dev;
> + int ret;
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set DSS PM to active\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(dispc->fclk);
> + if (ret) {
> + dev_err(dev, "Failed to enable DSS fclk\n");
> + goto err_runtime_suspend;
> + }
> +
> + ret = dispc_softreset(dispc);
> + if (ret)
> + goto err_clk_disable;
> +
> + clk_disable_unprepare(dispc->fclk);
> + ret = pm_runtime_set_suspended(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set DSS PM to suspended\n");
> + return ret;
> + }
> +
> + return 0;
> +
> +err_clk_disable:
> + clk_disable_unprepare(dispc->fclk);
> +
> +err_runtime_suspend:
> + ret = pm_runtime_set_suspended(dev);
> + if (ret) {
> + dev_err(dev, "Failed to set DSS PM to suspended\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> int dispc_init(struct tidss_device *tidss)
> {
> struct device *dev = tidss->dev;
> @@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss)
>
> tidss->dispc = dispc;
>
> - r = dispc_softreset(dispc);
> + r = dispc_init_hw(dispc);
> if (r)
> return r;
>
--
Regards,
Laurent Pinchart
Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:45AM +0200, Tomi Valkeinen wrote:
> Add a helper function, dispc_is_idle(), which returns whether the DSS is
> idle (i.e. is any video port enabled).
>
> For now we add a call to it in the suspend and resume callbacks, and
> print a warning if in either place the DSS is not idle.
Could you please explain here why these checks are needed/useful ? Why
would the dispc not be idle ?
> In the future
> this can be used to detect if the bootloader had enabled the DSS, and
> the driver can act on that.
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_dispc.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 13db062892e3..a527c28c8833 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2603,10 +2603,18 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
> dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
> }
>
> +static bool dispc_is_idle(struct dispc_device *dispc)
> +{
> + return REG_GET(dispc, DSS_SYSSTATUS, 9, 9);
> +}
> +
> int dispc_runtime_suspend(struct dispc_device *dispc)
> {
> dev_dbg(dispc->dev, "suspend\n");
>
> + if (!dispc_is_idle(dispc))
> + dev_warn(dispc->dev, "Bad HW state: DSS not idle when suspending");
> +
> dispc->is_enabled = false;
>
> clk_disable_unprepare(dispc->fclk);
> @@ -2620,6 +2628,9 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>
> clk_prepare_enable(dispc->fclk);
>
> + if (!dispc_is_idle(dispc))
> + dev_warn(dispc->dev, "Bad HW state: DSS not idle when resuming");
> +
> if (REG_GET(dispc, DSS_SYSSTATUS, 0, 0) == 0)
> dev_warn(dispc->dev, "DSS FUNC RESET not done!\n");
>
--
Regards,
Laurent Pinchart
Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
> The IRQ setup code is overly complex. All we really need to do is
> initialize the related fields in struct tidss_device, and request the
> IRQ.
>
> We can drop all the HW accesses, as they are pointless: the driver will
> set the IRQs correctly when it needs any of the IRQs, and at probe time
> we have done a reset, so we know that all the IRQs are masked by default
> in the hardware.
Even for K2G ?
> Thus we can combine the tidss_irq_preinstall() and
> tidss_irq_postinstall() into the tidss_irq_install() function, drop the
> HW accesses, and drop the use of spinlock, as this is done at init time
> and there can be no races.
>
> We can also drop the HW access from the tidss_irq_uninstall(), as the
> driver will anyway disable and suspend the hardware at remove time.
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_drv.c | 2 ++
> drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
> 2 files changed, 10 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 64914331715a..37693f30d98b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, tidss);
>
> + spin_lock_init(&tidss->wait_lock);
> +
> ret = dispc_init(tidss);
> if (ret) {
> dev_err(dev, "failed to initialize dispc: %d\n", ret);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 0c681c7600bc..604334ef526a 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
> spin_unlock_irqrestore(&tidss->wait_lock, flags);
> }
>
> -static void tidss_irq_preinstall(struct drm_device *ddev)
> -{
> - struct tidss_device *tidss = to_tidss(ddev);
> -
> - spin_lock_init(&tidss->wait_lock);
> -
> - tidss_runtime_get(tidss);
> -
> - dispc_set_irqenable(tidss->dispc, 0);
> - dispc_read_and_clear_irqstatus(tidss->dispc);
> -
> - tidss_runtime_put(tidss);
> -}
> -
> -static void tidss_irq_postinstall(struct drm_device *ddev)
> +int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> {
> struct tidss_device *tidss = to_tidss(ddev);
> - unsigned long flags;
> - unsigned int i;
> + int ret;
>
> - tidss_runtime_get(tidss);
> + if (irq == IRQ_NOTCONNECTED)
> + return -ENOTCONN;
>
> - spin_lock_irqsave(&tidss->wait_lock, flags);
> + ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> + if (ret)
> + return ret;
>
> tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
>
> - for (i = 0; i < tidss->num_crtcs; ++i) {
> + for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
> struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
>
> tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
> @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
> tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
> }
>
> - tidss_irq_update(tidss);
> -
> - spin_unlock_irqrestore(&tidss->wait_lock, flags);
> -
> - tidss_runtime_put(tidss);
> -}
> -
> -int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> -{
> - int ret;
> -
> - if (irq == IRQ_NOTCONNECTED)
> - return -ENOTCONN;
> -
> - tidss_irq_preinstall(ddev);
> -
> - ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> - if (ret)
> - return ret;
> -
> - tidss_irq_postinstall(ddev);
> -
> return 0;
> }
>
> @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
> {
> struct tidss_device *tidss = to_tidss(ddev);
>
> - tidss_runtime_get(tidss);
> - dispc_set_irqenable(tidss->dispc, 0);
> - tidss_runtime_put(tidss);
> -
> free_irq(tidss->irq, ddev);
> }
>
--
Regards,
Laurent Pinchart
Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
> tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
> returns immediately as there's no reason to do any register changes.
>
> However, the code checks for 'crtc->state->enable', which does not
> reflect the actual HW state. We should instead look at the
> 'crtc->state->active' flag.
>
> This causes the tidss_crtc_atomic_flush() to proceed with the flush even
> if the active state is false, which then causes us to hit the
> WARN_ON(!crtc->state->event) check.
>
> Fix this by checking the active flag, and while at it, fix the related
> debug print which had "active" and "needs modeset" wrong way.
>
> Signed-off-by: Tomi Valkeinen <[email protected]>
> ---
> drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 5e5e466f35d1..4c7009a5d643 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
> struct tidss_device *tidss = to_tidss(ddev);
> unsigned long flags;
>
> - dev_dbg(ddev->dev,
> - "%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
> - crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
> - crtc->state->enable, crtc->state->event);
> + dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
> + __func__, crtc->name, crtc->state->active,
> + drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);
While at it, how about this ?
dev_dbg(ddev->dev, "%s: %s is %sactive, %s modeset, event %p\n",
__func__, crtc->name, crtc->state->active ? "" : "not ",
drm_atomic_crtc_needs_modeset(crtc->state) ? "needs", "doesn't need",
crtc->state->event);
>
> /* There is nothing to do if CRTC is not going to be enabled. */
> - if (!crtc->state->enable)
> + if (!crtc->state->active)
I think the drm_atomic_helper_commit_planes() helper will handle this if
you pass it the DRM_PLANE_COMMIT_ACTIVE_ONLY flag.
> return;
>
> /*
--
Regards,
Laurent Pinchart
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 <[email protected]>
>> ---
>> 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
On 01/11/2023 16:52, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
>> The IRQ setup code is overly complex. All we really need to do is
>> initialize the related fields in struct tidss_device, and request the
>> IRQ.
>>
>> We can drop all the HW accesses, as they are pointless: the driver will
>> set the IRQs correctly when it needs any of the IRQs, and at probe time
>> we have done a reset, so we know that all the IRQs are masked by default
>> in the hardware.
>
> Even for K2G ?
Good point. I'll add a simple manual reset for k2g, masking the IRQs and
disabling the VPs.
Tomi
On 01/11/2023 16:32, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Nov 01, 2023 at 11:17:45AM +0200, Tomi Valkeinen wrote:
>> Add a helper function, dispc_is_idle(), which returns whether the DSS is
>> idle (i.e. is any video port enabled).
>>
>> For now we add a call to it in the suspend and resume callbacks, and
>> print a warning if in either place the DSS is not idle.
>
> Could you please explain here why these checks are needed/useful ? Why
> would the dispc not be idle ?
I'll drop this. This was mostly a debugging aid for myself when testing
runtime PM.
Tomi
On 01/11/2023 16:30, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
>> The probe function calls dispc_softreset() before runtime PM is enabled
>> and without enabling any of the DSS clocks. This happens to work by
>> luck, and we need to make sure the DSS HW is active and the fclk is
>> enabled.
>>
>> To fix the above, add a new function, dispc_init_hw(), which does:
>>
>> - pm_runtime_set_active()
>> - clk_prepare_enable(fclk)
>> - dispc_softreset().
>>
>> This ensures that the reset can be successfully accomplished.
>>
>> Note that we use pm_runtime_set_active(), not the normal
>> pm_runtime_get(). The reason for this is that at this point we haven't
>> enabled the runtime PM yet and also we don't want the normal resume
>> callback to be called: the dispc resume callback does some initial HW
>> setup, and it expects that the HW was off (no video ports are
>> streaming). If the bootloader has enabled the DSS and has set up a
>> boot time splash-screen, the DSS would be enabled and streaming which
>> might lead to issues with the normal resume callback.
>
> I think the right way to do this would be, in probe(), to
>
> - power on the device
> - enable runtime PM, masking the device as active
> - at end of probe, calling pm_runtime_put_autosuspend()
Can you explain what that would accomplish, or why the code in this
patch is wrong?
If I understand it right, you're suggesting a more "normal" power up at
the probe time, and then leaving the DSS enabled, but with autosuspend.
That would require powering up, doing a reset, and calling
dispc_runtime_resume. Which can be done, but I'm not sure why it's
better, as we're not interested in "normal" power up at probe time.
But I can see that my approach looks perhaps a bit odd just by looking
at these patches. This work was related to keeping the bootloader's
splash screen on the screen for a longer time, i.e. delaying reset.
For that, I wanted an early function (dispc_init_hw) which would,
instead of always resetting the DSS as it does in this version, peek at
the DSS hardware, and see if the DSS is already streaming. If no, do a
reset and proceed normally. If yes, skip the reset, leave the clocks
enabled, and keep DSS PM active.
Later, when we'd be doing the first modeset, the driver would do the
initial reset.
So, that's why I wanted an independent function for the HW probing/init,
which is called before runtime PM is enabled, and I did not want normal
runtime resume to be called as dispc_runtime_resume() would break the
display.
I think a better solution would be to set up the fb of tidss's fbdev to
use the reserved memory, used for the boot splash screen. But I didn't
figure out a way to do this. But even there we'd like to delay the reset
until the first modeset (when the fbdev display is getting enabled).
Tomi
On 01/11/2023 16:56, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
>> tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
>> returns immediately as there's no reason to do any register changes.
>>
>> However, the code checks for 'crtc->state->enable', which does not
>> reflect the actual HW state. We should instead look at the
>> 'crtc->state->active' flag.
>>
>> This causes the tidss_crtc_atomic_flush() to proceed with the flush even
>> if the active state is false, which then causes us to hit the
>> WARN_ON(!crtc->state->event) check.
>>
>> Fix this by checking the active flag, and while at it, fix the related
>> debug print which had "active" and "needs modeset" wrong way.
>>
>> Signed-off-by: Tomi Valkeinen <[email protected]>
>> ---
>> drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 5e5e466f35d1..4c7009a5d643 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>> struct tidss_device *tidss = to_tidss(ddev);
>> unsigned long flags;
>>
>> - dev_dbg(ddev->dev,
>> - "%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
>> - crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
>> - crtc->state->enable, crtc->state->event);
>> + dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
>> + __func__, crtc->name, crtc->state->active,
>> + drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);
>
> While at it, how about this ?
Why not. The active part won't be needed if we use
DRM_PLANE_COMMIT_ACTIVE_ONLY, though.
> dev_dbg(ddev->dev, "%s: %s is %sactive, %s modeset, event %p\n",
> __func__, crtc->name, crtc->state->active ? "" : "not ",
> drm_atomic_crtc_needs_modeset(crtc->state) ? "needs", "doesn't need",
> crtc->state->event);
>
>>
>> /* There is nothing to do if CRTC is not going to be enabled. */
>> - if (!crtc->state->enable)
>> + if (!crtc->state->active)
>
> I think the drm_atomic_helper_commit_planes() helper will handle this if
> you pass it the DRM_PLANE_COMMIT_ACTIVE_ONLY flag.
I considered it, but it does a bit more, as it also affects the plane
updates. We specifically did not use DRM_PLANE_COMMIT_ACTIVE_ONLY as it
didn't work with DSS.
That said, I can't figure out what was the issue. It's possible the
issue was only on the older DSS HW, with omapdrm (tidss code was
originally somewhat based on omapdrm). I'm pretty sure the issue was
related to multi-display systems and the plane updates there. But I
don't have any multi-display board which uses tidss.
So, I'll keep this patch, but add another on top, which uses
DRM_PLANE_COMMIT_ACTIVE_ONLY. Then it'll be easy to revert the
DRM_PLANE_COMMIT_ACTIVE_ONLY one if needed, while still keeping this fix.
Tomi
On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
> The probe function calls dispc_softreset() before runtime PM is enabled
> and without enabling any of the DSS clocks. This happens to work by
> luck, and we need to make sure the DSS HW is active and the fclk is
> enabled.
Not sure on the exact implication here, however from the description
this seems a candidate for stable and would need a Fixes tag.
Francesco
On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
> tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
> returns immediately as there's no reason to do any register changes.
>
> However, the code checks for 'crtc->state->enable', which does not
> reflect the actual HW state. We should instead look at the
> 'crtc->state->active' flag.
>
> This causes the tidss_crtc_atomic_flush() to proceed with the flush even
> if the active state is false, which then causes us to hit the
> WARN_ON(!crtc->state->event) check.
>
> Fix this by checking the active flag, and while at it, fix the related
> debug print which had "active" and "needs modeset" wrong way.
Candidate for stable? Add a Fixes tag?
Francesco
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 <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
>>>> ---
>>>> 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