| Message ID | 20240627-multistream-v2-1-6ae96c54c1c3@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: cadence,ti: CSI2RX Multistream Support | expand |
Hi, On 27/06/2024 16:09, Jai Luthra wrote: > From: Jayshri Pawar <jpawar@cadence.com> > > Use runtime power management hooks to save power when CSI-RX is not in > use. Also stop/start any in-progress streams, which might happen during > a system suspend/resume cycle. > > Signed-off-by: Jayshri Pawar <jpawar@cadence.com> > Co-developed-by: Jai Luthra <j-luthra@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 43 +++++++++++++++++++++++++++- > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 6f7d27a48eff..751eadbe61ef 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -366,6 +366,12 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > int ret = 0; > > + if (enable) { > + ret = pm_runtime_resume_and_get(csi2rx->dev); > + if (ret < 0) > + return ret; > + } > + > mutex_lock(&csi2rx->lock); > > if (enable) { > @@ -375,8 +381,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > */ > if (!csi2rx->count) { > ret = csi2rx_start(csi2rx); > - if (ret) > + if (ret) { > + pm_runtime_put(csi2rx->dev); > goto out; > + } > } > > csi2rx->count++; > @@ -388,6 +396,8 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > */ > if (!csi2rx->count) > csi2rx_stop(csi2rx); > + > + pm_runtime_put(csi2rx->dev); > } > > out: > @@ -661,6 +671,29 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx) > return ret; > } > > +static int csi2rx_suspend(struct device *dev) > +{ > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > + > + mutex_lock(&csi2rx->lock); > + if (csi2rx->count) > + csi2rx_stop(csi2rx); > + mutex_unlock(&csi2rx->lock); > + > + return 0; > +} > + > +static int csi2rx_resume(struct device *dev) > +{ > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > + > + mutex_lock(&csi2rx->lock); > + if (csi2rx->count) > + csi2rx_start(csi2rx); > + mutex_unlock(&csi2rx->lock); > + return 0; > +} > + I don't think this looks correct. Afaik the runtime suspend/resume is not called on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to use the same callbacks for runtime and system suspend, but I think that's a bad idea. Runtime suspend is not supposed to turn off the streaming. The driver is supposed to turn off the streaming, then call runtime_put, which would result in runtime suspend callback getting called. Tomi > static int csi2rx_probe(struct platform_device *pdev) > { > struct csi2rx_priv *csi2rx; > @@ -707,6 +740,7 @@ static int csi2rx_probe(struct platform_device *pdev) > if (ret) > goto err_cleanup; > > + pm_runtime_enable(csi2rx->dev); > ret = v4l2_async_register_subdev(&csi2rx->subdev); > if (ret < 0) > goto err_free_state; > @@ -721,6 +755,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > err_free_state: > v4l2_subdev_cleanup(&csi2rx->subdev); > + pm_runtime_disable(csi2rx->dev); > err_cleanup: > v4l2_async_nf_unregister(&csi2rx->notifier); > v4l2_async_nf_cleanup(&csi2rx->notifier); > @@ -739,9 +774,14 @@ static void csi2rx_remove(struct platform_device *pdev) > v4l2_async_unregister_subdev(&csi2rx->subdev); > v4l2_subdev_cleanup(&csi2rx->subdev); > media_entity_cleanup(&csi2rx->subdev.entity); > + pm_runtime_disable(csi2rx->dev); > kfree(csi2rx); > } > > +static const struct dev_pm_ops csi2rx_pm_ops = { > + SET_RUNTIME_PM_OPS(csi2rx_suspend, csi2rx_resume, NULL) > +}; > + > static const struct of_device_id csi2rx_of_table[] = { > { .compatible = "starfive,jh7110-csi2rx" }, > { .compatible = "cdns,csi2rx" }, > @@ -756,6 +796,7 @@ static struct platform_driver csi2rx_driver = { > .driver = { > .name = "cdns-csi2rx", > .of_match_table = csi2rx_of_table, > + .pm = &csi2rx_pm_ops, > }, > }; > module_platform_driver(csi2rx_driver); >
Hi Tomi, [...] > > +static int csi2rx_suspend(struct device *dev) { > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > + > > + mutex_lock(&csi2rx->lock); > > + if (csi2rx->count) > > + csi2rx_stop(csi2rx); > > + mutex_unlock(&csi2rx->lock); > > + > > + return 0; > > +} > > + > > +static int csi2rx_resume(struct device *dev) { > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > + > > + mutex_lock(&csi2rx->lock); > > + if (csi2rx->count) > > + csi2rx_start(csi2rx); > > + mutex_unlock(&csi2rx->lock); > > + return 0; > > +} > > + > > I don't think this looks correct. Afaik the runtime suspend/resume is not called > on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to > use the same callbacks for runtime and system suspend, but I think that's a > bad idea. Runtime suspend is not supposed to turn off the streaming. The > driver is supposed to turn off the streaming, then call runtime_put, which > would result in runtime suspend callback getting called. > I implemented system suspend/resume based on this patch, I feel good about it. https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@starfivetech.com/ Regards, Changhuang
Hi Tomi, On Jun 28, 2024 at 11:14:50 +0300, Tomi Valkeinen wrote: > Hi, > > On 27/06/2024 16:09, Jai Luthra wrote: > > From: Jayshri Pawar <jpawar@cadence.com> > > > > Use runtime power management hooks to save power when CSI-RX is not in > > use. Also stop/start any in-progress streams, which might happen during > > a system suspend/resume cycle. > > > > Signed-off-by: Jayshri Pawar <jpawar@cadence.com> > > Co-developed-by: Jai Luthra <j-luthra@ti.com> > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > --- > > drivers/media/platform/cadence/cdns-csi2rx.c | 43 +++++++++++++++++++++++++++- > > 1 file changed, 42 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 6f7d27a48eff..751eadbe61ef 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -366,6 +366,12 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > int ret = 0; > > + if (enable) { > > + ret = pm_runtime_resume_and_get(csi2rx->dev); > > + if (ret < 0) > > + return ret; > > + } > > + > > mutex_lock(&csi2rx->lock); > > if (enable) { > > @@ -375,8 +381,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > */ > > if (!csi2rx->count) { > > ret = csi2rx_start(csi2rx); > > - if (ret) > > + if (ret) { > > + pm_runtime_put(csi2rx->dev); > > goto out; > > + } > > } > > csi2rx->count++; > > @@ -388,6 +396,8 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > */ > > if (!csi2rx->count) > > csi2rx_stop(csi2rx); > > + > > + pm_runtime_put(csi2rx->dev); > > } > > out: > > @@ -661,6 +671,29 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx) > > return ret; > > } > > +static int csi2rx_suspend(struct device *dev) > > +{ > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > + > > + mutex_lock(&csi2rx->lock); > > + if (csi2rx->count) > > + csi2rx_stop(csi2rx); > > + mutex_unlock(&csi2rx->lock); > > + > > + return 0; > > +} > > + > > +static int csi2rx_resume(struct device *dev) > > +{ > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > + > > + mutex_lock(&csi2rx->lock); > > + if (csi2rx->count) > > + csi2rx_start(csi2rx); > > + mutex_unlock(&csi2rx->lock); > > + return 0; > > +} > > + > > I don't think this looks correct. Afaik the runtime suspend/resume is not > called on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to > use the same callbacks for runtime and system suspend, but I think that's a > bad idea. Runtime suspend is not supposed to turn off the streaming. The > driver is supposed to turn off the streaming, then call runtime_put, which > would result in runtime suspend callback getting called. Agreed, runtime PM should only be called once streams are stopped and device is not in use. These hooks look like system suspend/resume hooks to me. I don't think this patch makes much sense in this series anyway, I was carrying it to bring the drivers at parity with the vendor kernel. I will drop this from my series and let Changhuang carry it in his series for overall PM support for cdns-csi2rx. > > Tomi > > > static int csi2rx_probe(struct platform_device *pdev) > > { > > struct csi2rx_priv *csi2rx; > > @@ -707,6 +740,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > if (ret) > > goto err_cleanup; > > + pm_runtime_enable(csi2rx->dev); > > ret = v4l2_async_register_subdev(&csi2rx->subdev); > > if (ret < 0) > > goto err_free_state; > > @@ -721,6 +755,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > err_free_state: > > v4l2_subdev_cleanup(&csi2rx->subdev); > > + pm_runtime_disable(csi2rx->dev); > > err_cleanup: > > v4l2_async_nf_unregister(&csi2rx->notifier); > > v4l2_async_nf_cleanup(&csi2rx->notifier); > > @@ -739,9 +774,14 @@ static void csi2rx_remove(struct platform_device *pdev) > > v4l2_async_unregister_subdev(&csi2rx->subdev); > > v4l2_subdev_cleanup(&csi2rx->subdev); > > media_entity_cleanup(&csi2rx->subdev.entity); > > + pm_runtime_disable(csi2rx->dev); > > kfree(csi2rx); > > } > > +static const struct dev_pm_ops csi2rx_pm_ops = { > > + SET_RUNTIME_PM_OPS(csi2rx_suspend, csi2rx_resume, NULL) > > +}; > > + > > static const struct of_device_id csi2rx_of_table[] = { > > { .compatible = "starfive,jh7110-csi2rx" }, > > { .compatible = "cdns,csi2rx" }, > > @@ -756,6 +796,7 @@ static struct platform_driver csi2rx_driver = { > > .driver = { > > .name = "cdns-csi2rx", > > .of_match_table = csi2rx_of_table, > > + .pm = &csi2rx_pm_ops, > > }, > > }; > > module_platform_driver(csi2rx_driver); > > >
Hi Changhuang, On Jun 28, 2024 at 08:45:06 +0000, Changhuang Liang wrote: > Hi Tomi, > > [...] > > > +static int csi2rx_suspend(struct device *dev) { > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > + > > > + mutex_lock(&csi2rx->lock); > > > + if (csi2rx->count) > > > + csi2rx_stop(csi2rx); > > > + mutex_unlock(&csi2rx->lock); > > > + > > > + return 0; > > > +} > > > + > > > +static int csi2rx_resume(struct device *dev) { > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > + > > > + mutex_lock(&csi2rx->lock); > > > + if (csi2rx->count) > > > + csi2rx_start(csi2rx); > > > + mutex_unlock(&csi2rx->lock); > > > + return 0; > > > +} > > > + > > > > I don't think this looks correct. Afaik the runtime suspend/resume is not called > > on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to > > use the same callbacks for runtime and system suspend, but I think that's a > > bad idea. Runtime suspend is not supposed to turn off the streaming. The > > driver is supposed to turn off the streaming, then call runtime_put, which > > would result in runtime suspend callback getting called. > > > > I implemented system suspend/resume based on this patch, I feel good about it. > > https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@starfivetech.com/ Thanks for carrying this patch in your series. I think Tomi's point still holds - only the system suspend hook should try to stop/start the CSI2RX device. Runtime PM hooks are usually only called when there are no users, so no active streams. > > Regards, > Changhuang
Hi, Jai, > Hi Changhuang, > > On Jun 28, 2024 at 08:45:06 +0000, Changhuang Liang wrote: > > Hi Tomi, > > > > [...] > > > > +static int csi2rx_suspend(struct device *dev) { > > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > > + > > > > + mutex_lock(&csi2rx->lock); > > > > + if (csi2rx->count) > > > > + csi2rx_stop(csi2rx); > > > > + mutex_unlock(&csi2rx->lock); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int csi2rx_resume(struct device *dev) { > > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > > + > > > > + mutex_lock(&csi2rx->lock); > > > > + if (csi2rx->count) > > > > + csi2rx_start(csi2rx); > > > > + mutex_unlock(&csi2rx->lock); > > > > + return 0; > > > > +} > > > > + > > > > > > I don't think this looks correct. Afaik the runtime suspend/resume > > > is not called on system suspend/resume. You could change the > > > SET_RUNTIME_PM_OPS to use the same callbacks for runtime and > system > > > suspend, but I think that's a bad idea. Runtime suspend is not > > > supposed to turn off the streaming. The driver is supposed to turn > > > off the streaming, then call runtime_put, which would result in runtime > suspend callback getting called. > > > > > > > I implemented system suspend/resume based on this patch, I feel good > about it. > > > > https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@st > > arfivetech.com/ > > Thanks for carrying this patch in your series. > > I think Tomi's point still holds - only the system suspend hook should try to > stop/start the CSI2RX device. > > Runtime PM hooks are usually only called when there are no users, so no > active streams. > OK, I will modify this in my series. Regards, Changhuang
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c index 6f7d27a48eff..751eadbe61ef 100644 --- a/drivers/media/platform/cadence/cdns-csi2rx.c +++ b/drivers/media/platform/cadence/cdns-csi2rx.c @@ -366,6 +366,12 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); int ret = 0; + if (enable) { + ret = pm_runtime_resume_and_get(csi2rx->dev); + if (ret < 0) + return ret; + } + mutex_lock(&csi2rx->lock); if (enable) { @@ -375,8 +381,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) */ if (!csi2rx->count) { ret = csi2rx_start(csi2rx); - if (ret) + if (ret) { + pm_runtime_put(csi2rx->dev); goto out; + } } csi2rx->count++; @@ -388,6 +396,8 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) */ if (!csi2rx->count) csi2rx_stop(csi2rx); + + pm_runtime_put(csi2rx->dev); } out: @@ -661,6 +671,29 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx) return ret; } +static int csi2rx_suspend(struct device *dev) +{ + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); + + mutex_lock(&csi2rx->lock); + if (csi2rx->count) + csi2rx_stop(csi2rx); + mutex_unlock(&csi2rx->lock); + + return 0; +} + +static int csi2rx_resume(struct device *dev) +{ + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); + + mutex_lock(&csi2rx->lock); + if (csi2rx->count) + csi2rx_start(csi2rx); + mutex_unlock(&csi2rx->lock); + return 0; +} + static int csi2rx_probe(struct platform_device *pdev) { struct csi2rx_priv *csi2rx; @@ -707,6 +740,7 @@ static int csi2rx_probe(struct platform_device *pdev) if (ret) goto err_cleanup; + pm_runtime_enable(csi2rx->dev); ret = v4l2_async_register_subdev(&csi2rx->subdev); if (ret < 0) goto err_free_state; @@ -721,6 +755,7 @@ static int csi2rx_probe(struct platform_device *pdev) err_free_state: v4l2_subdev_cleanup(&csi2rx->subdev); + pm_runtime_disable(csi2rx->dev); err_cleanup: v4l2_async_nf_unregister(&csi2rx->notifier); v4l2_async_nf_cleanup(&csi2rx->notifier); @@ -739,9 +774,14 @@ static void csi2rx_remove(struct platform_device *pdev) v4l2_async_unregister_subdev(&csi2rx->subdev); v4l2_subdev_cleanup(&csi2rx->subdev); media_entity_cleanup(&csi2rx->subdev.entity); + pm_runtime_disable(csi2rx->dev); kfree(csi2rx); } +static const struct dev_pm_ops csi2rx_pm_ops = { + SET_RUNTIME_PM_OPS(csi2rx_suspend, csi2rx_resume, NULL) +}; + static const struct of_device_id csi2rx_of_table[] = { { .compatible = "starfive,jh7110-csi2rx" }, { .compatible = "cdns,csi2rx" }, @@ -756,6 +796,7 @@ static struct platform_driver csi2rx_driver = { .driver = { .name = "cdns-csi2rx", .of_match_table = csi2rx_of_table, + .pm = &csi2rx_pm_ops, }, }; module_platform_driver(csi2rx_driver);