| Message ID | 20240902112408.493201-5-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: pisp-be: Split jobs creation and scheduling | expand |
Hi Jacopo, Thank you for the patch. On Mon, Sep 02, 2024 at 01:24:06PM +0200, Jacopo Mondi wrote: > During the probe() routine, the driver needs to power up the interface > in order to identify and initialize the hardware and it later suspends > it at the end of probe(). > > The driver erroneously resumes the interface by calling the > pispbe_runtime_resume() function directly but suspends it by > calling pm_runtime_put_autosuspend(). > > This causes a PM usage count imbalance at probe time, notified by the > runtime_pm framework with the below message in the system log: > > pispbe 1000880000.pisp_be: Runtime PM usage count underflow! > > Fix this by suspending the interface using pm_runtime_idle() which > doesn't decrease the pm_runtime usage count and inform the PM framework > that the device is active by calling pm_runtime_set_active(). > > Adjust the pispbe_remove() function as well to disable > the pm_runtime in the correct order, s/,$/./ > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > v3->v4: > - Instead of using pm_runtime for resuming, suspend using > pm_runtime_idle() to support !CONFIG_PM > > v2->v3: > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > the kernel test robot <lkp@intel.com> > --- > .../platform/raspberrypi/pisp_be/pisp_be.c | 26 +++++++++---------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > index d614f53f0f68..1c19ca946bd4 100644 > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > @@ -1720,36 +1720,32 @@ static int pispbe_probe(struct platform_device *pdev) > "Failed to get clock"); > > /* Hardware initialisation */ > - pm_runtime_set_autosuspend_delay(pispbe->dev, 200); > - pm_runtime_use_autosuspend(pispbe->dev); > - pm_runtime_enable(pispbe->dev); > - > ret = pispbe_runtime_resume(pispbe->dev); > if (ret) > - goto pm_runtime_disable_err; > + return ret; > > pispbe->hw_busy = false; > spin_lock_init(&pispbe->hw_lock); > ret = pispbe_hw_init(pispbe); > if (ret) > - goto pm_runtime_suspend_err; > + goto runtime_suspend_err; > > ret = pispbe_init_devices(pispbe); > if (ret) > goto disable_devs_err; > > - pm_runtime_mark_last_busy(pispbe->dev); > - pm_runtime_put_autosuspend(pispbe->dev); > + pm_runtime_set_autosuspend_delay(pispbe->dev, 200); > + pm_runtime_use_autosuspend(pispbe->dev); > + pm_runtime_set_active(pispbe->dev); > + pm_runtime_enable(pispbe->dev); > + pm_runtime_idle(pispbe->dev); > > return 0; > > disable_devs_err: It would be nice to rename the error labels to start with err_, like commonly done (in another patch of course). > pispbe_destroy_devices(pispbe); > -pm_runtime_suspend_err: > +runtime_suspend_err: > pispbe_runtime_suspend(pispbe->dev); > -pm_runtime_disable_err: > - pm_runtime_dont_use_autosuspend(pispbe->dev); > - pm_runtime_disable(pispbe->dev); > > return ret; > } > @@ -1760,9 +1756,11 @@ static void pispbe_remove(struct platform_device *pdev) > > pispbe_destroy_devices(pispbe); > > - pispbe_runtime_suspend(pispbe->dev); > pm_runtime_dont_use_autosuspend(pispbe->dev); > - pm_runtime_disable(pispbe->dev); > + pm_runtime_disable(pispbe->dev); > + if (!pm_runtime_status_suspended(pispbe->dev)) > + pispbe_runtime_suspend(pispbe->dev); > + pm_runtime_set_suspended(pispbe->dev); Wrong indentation, you're using spaces instead of tabs. With those issues fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > static const struct dev_pm_ops pispbe_pm_ops = {
Hi Laurent, On Tue, Sep 03, 2024 at 02:13:16AM GMT, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Sep 02, 2024 at 01:24:06PM +0200, Jacopo Mondi wrote: > > During the probe() routine, the driver needs to power up the interface > > in order to identify and initialize the hardware and it later suspends > > it at the end of probe(). > > > > The driver erroneously resumes the interface by calling the > > pispbe_runtime_resume() function directly but suspends it by > > calling pm_runtime_put_autosuspend(). > > > > This causes a PM usage count imbalance at probe time, notified by the > > runtime_pm framework with the below message in the system log: > > > > pispbe 1000880000.pisp_be: Runtime PM usage count underflow! > > > > Fix this by suspending the interface using pm_runtime_idle() which > > doesn't decrease the pm_runtime usage count and inform the PM framework > > that the device is active by calling pm_runtime_set_active(). > > > > Adjust the pispbe_remove() function as well to disable > > the pm_runtime in the correct order, > > s/,$/./ > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > v3->v4: > > - Instead of using pm_runtime for resuming, suspend using > > pm_runtime_idle() to support !CONFIG_PM > > > > v2->v3: > > - Mark pispbe_runtime_resume() as __maybe_unused as reported by > > the kernel test robot <lkp@intel.com> > > --- > > .../platform/raspberrypi/pisp_be/pisp_be.c | 26 +++++++++---------- > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > index d614f53f0f68..1c19ca946bd4 100644 > > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > @@ -1720,36 +1720,32 @@ static int pispbe_probe(struct platform_device *pdev) > > "Failed to get clock"); > > > > /* Hardware initialisation */ > > - pm_runtime_set_autosuspend_delay(pispbe->dev, 200); > > - pm_runtime_use_autosuspend(pispbe->dev); > > - pm_runtime_enable(pispbe->dev); > > - > > ret = pispbe_runtime_resume(pispbe->dev); > > if (ret) > > - goto pm_runtime_disable_err; > > + return ret; > > > > pispbe->hw_busy = false; > > spin_lock_init(&pispbe->hw_lock); > > ret = pispbe_hw_init(pispbe); > > if (ret) > > - goto pm_runtime_suspend_err; > > + goto runtime_suspend_err; > > > > ret = pispbe_init_devices(pispbe); > > if (ret) > > goto disable_devs_err; > > > > - pm_runtime_mark_last_busy(pispbe->dev); > > - pm_runtime_put_autosuspend(pispbe->dev); > > + pm_runtime_set_autosuspend_delay(pispbe->dev, 200); > > + pm_runtime_use_autosuspend(pispbe->dev); > > + pm_runtime_set_active(pispbe->dev); > > + pm_runtime_enable(pispbe->dev); > > + pm_runtime_idle(pispbe->dev); > > > > return 0; > > > > disable_devs_err: > > It would be nice to rename the error labels to start with err_, like > commonly done (in another patch of course). > > > pispbe_destroy_devices(pispbe); > > -pm_runtime_suspend_err: > > +runtime_suspend_err: > > pispbe_runtime_suspend(pispbe->dev); > > -pm_runtime_disable_err: > > - pm_runtime_dont_use_autosuspend(pispbe->dev); > > - pm_runtime_disable(pispbe->dev); > > > > return ret; > > } > > @@ -1760,9 +1756,11 @@ static void pispbe_remove(struct platform_device *pdev) > > > > pispbe_destroy_devices(pispbe); > > > > - pispbe_runtime_suspend(pispbe->dev); > > pm_runtime_dont_use_autosuspend(pispbe->dev); > > - pm_runtime_disable(pispbe->dev); > > + pm_runtime_disable(pispbe->dev); > > + if (!pm_runtime_status_suspended(pispbe->dev)) > > + pispbe_runtime_suspend(pispbe->dev); > > + pm_runtime_set_suspended(pispbe->dev); > > Wrong indentation, you're using spaces instead of tabs. Oh crabs, how did I missed that ? > > With those issues fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks > > > } > > > > static const struct dev_pm_ops pispbe_pm_ops = { > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c index d614f53f0f68..1c19ca946bd4 100644 --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c @@ -1720,36 +1720,32 @@ static int pispbe_probe(struct platform_device *pdev) "Failed to get clock"); /* Hardware initialisation */ - pm_runtime_set_autosuspend_delay(pispbe->dev, 200); - pm_runtime_use_autosuspend(pispbe->dev); - pm_runtime_enable(pispbe->dev); - ret = pispbe_runtime_resume(pispbe->dev); if (ret) - goto pm_runtime_disable_err; + return ret; pispbe->hw_busy = false; spin_lock_init(&pispbe->hw_lock); ret = pispbe_hw_init(pispbe); if (ret) - goto pm_runtime_suspend_err; + goto runtime_suspend_err; ret = pispbe_init_devices(pispbe); if (ret) goto disable_devs_err; - pm_runtime_mark_last_busy(pispbe->dev); - pm_runtime_put_autosuspend(pispbe->dev); + pm_runtime_set_autosuspend_delay(pispbe->dev, 200); + pm_runtime_use_autosuspend(pispbe->dev); + pm_runtime_set_active(pispbe->dev); + pm_runtime_enable(pispbe->dev); + pm_runtime_idle(pispbe->dev); return 0; disable_devs_err: pispbe_destroy_devices(pispbe); -pm_runtime_suspend_err: +runtime_suspend_err: pispbe_runtime_suspend(pispbe->dev); -pm_runtime_disable_err: - pm_runtime_dont_use_autosuspend(pispbe->dev); - pm_runtime_disable(pispbe->dev); return ret; } @@ -1760,9 +1756,11 @@ static void pispbe_remove(struct platform_device *pdev) pispbe_destroy_devices(pispbe); - pispbe_runtime_suspend(pispbe->dev); pm_runtime_dont_use_autosuspend(pispbe->dev); - pm_runtime_disable(pispbe->dev); + pm_runtime_disable(pispbe->dev); + if (!pm_runtime_status_suspended(pispbe->dev)) + pispbe_runtime_suspend(pispbe->dev); + pm_runtime_set_suspended(pispbe->dev); } static const struct dev_pm_ops pispbe_pm_ops = {
During the probe() routine, the driver needs to power up the interface in order to identify and initialize the hardware and it later suspends it at the end of probe(). The driver erroneously resumes the interface by calling the pispbe_runtime_resume() function directly but suspends it by calling pm_runtime_put_autosuspend(). This causes a PM usage count imbalance at probe time, notified by the runtime_pm framework with the below message in the system log: pispbe 1000880000.pisp_be: Runtime PM usage count underflow! Fix this by suspending the interface using pm_runtime_idle() which doesn't decrease the pm_runtime usage count and inform the PM framework that the device is active by calling pm_runtime_set_active(). Adjust the pispbe_remove() function as well to disable the pm_runtime in the correct order, Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- v3->v4: - Instead of using pm_runtime for resuming, suspend using pm_runtime_idle() to support !CONFIG_PM v2->v3: - Mark pispbe_runtime_resume() as __maybe_unused as reported by the kernel test robot <lkp@intel.com> --- .../platform/raspberrypi/pisp_be/pisp_be.c | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-)