| Message ID | 20231218174042.794012-10-mike.rudenko@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/20] media: i2c: ov4689: Clean up and annotate the register table | expand |
Hi Mikhail, On Mon, Dec 18, 2023 at 08:40:30PM +0300, Mikhail Rudenko wrote: > Use runtime PM autosuspend to avoid powering off the sensor during > fast stop-reconfigure-restart cycles. > > Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > --- > drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c > index 5300e621ff90..64cc6d9e48cc 100644 > --- a/drivers/media/i2c/ov4689.c > +++ b/drivers/media/i2c/ov4689.c > @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) > ov4689->cur_mode->num_regs, > NULL); > if (ret) { > - pm_runtime_put(dev); > + pm_runtime_put_sync(dev); Why are you switching to pm_runtime_put_sync() here? That isn't covered by the commit message (nor I think should be done). > goto unlock_and_return; > } > > ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); > if (ret) { > - pm_runtime_put(dev); > + pm_runtime_put_sync(dev); > goto unlock_and_return; > } > > ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, > OV4689_MODE_STREAMING, NULL); > if (ret) { > - pm_runtime_put(dev); > + pm_runtime_put_sync(dev); > goto unlock_and_return; > } > } else { > cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, > OV4689_MODE_SW_STANDBY, NULL); > - pm_runtime_put(dev); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > } > > unlock_and_return: > @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > - pm_runtime_put(dev); > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); Also note that with runtime PM autosuspend, you have to use pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use(). > + > return ret; > } > > @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) > } > > pm_runtime_set_active(dev); > + pm_runtime_get_noresume(dev); > pm_runtime_enable(dev); > - pm_runtime_idle(dev); > + pm_runtime_set_autosuspend_delay(dev, 1000); > + pm_runtime_use_autosuspend(dev); > > ret = v4l2_async_register_subdev_sensor(sd); > if (ret) { > @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) > goto err_clean_subdev_pm; > } > > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + > return 0; > > err_clean_subdev_pm: > pm_runtime_disable(dev); > - pm_runtime_set_suspended(dev); > + pm_runtime_put_noidle(dev); > v4l2_subdev_cleanup(sd); > err_clean_entity: > media_entity_cleanup(&sd->entity);
Hi Sakari, Thanks for the review! On 2024-01-08 at 11:18 GMT, Sakari Ailus <sakari.ailus@iki.fi> wrote: > Hi Mikhail, > > On Mon, Dec 18, 2023 at 08:40:30PM +0300, Mikhail Rudenko wrote: >> Use runtime PM autosuspend to avoid powering off the sensor during >> fast stop-reconfigure-restart cycles. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> --- >> drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- >> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> index 5300e621ff90..64cc6d9e48cc 100644 >> --- a/drivers/media/i2c/ov4689.c >> +++ b/drivers/media/i2c/ov4689.c >> @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) >> ov4689->cur_mode->num_regs, >> NULL); >> if (ret) { >> - pm_runtime_put(dev); >> + pm_runtime_put_sync(dev); > > Why are you switching to pm_runtime_put_sync() here? That isn't covered by > the commit message (nor I think should be done). PM autosuspend conversion was suggested earlier by Laurent in his review of this series [1], and he adviced looking at how it was done for the imx290 driver. I followed along the lines of the corresponding patch [2]. >> goto unlock_and_return; >> } >> >> ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); >> if (ret) { >> - pm_runtime_put(dev); >> + pm_runtime_put_sync(dev); >> goto unlock_and_return; >> } >> >> ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> OV4689_MODE_STREAMING, NULL); >> if (ret) { >> - pm_runtime_put(dev); >> + pm_runtime_put_sync(dev); >> goto unlock_and_return; >> } >> } else { >> cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> OV4689_MODE_SW_STANDBY, NULL); >> - pm_runtime_put(dev); >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> } >> >> unlock_and_return: >> @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) >> break; >> } >> >> - pm_runtime_put(dev); >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); > > Also note that with runtime PM autosuspend, you have to use > pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use(). Noted, will do so in v3. >> + >> return ret; >> } >> >> @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) >> } >> >> pm_runtime_set_active(dev); >> + pm_runtime_get_noresume(dev); >> pm_runtime_enable(dev); >> - pm_runtime_idle(dev); >> + pm_runtime_set_autosuspend_delay(dev, 1000); >> + pm_runtime_use_autosuspend(dev); >> >> ret = v4l2_async_register_subdev_sensor(sd); >> if (ret) { >> @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) >> goto err_clean_subdev_pm; >> } >> >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> + >> return 0; >> >> err_clean_subdev_pm: >> pm_runtime_disable(dev); >> - pm_runtime_set_suspended(dev); >> + pm_runtime_put_noidle(dev); >> v4l2_subdev_cleanup(sd); >> err_clean_entity: >> media_entity_cleanup(&sd->entity); [1] https://lore.kernel.org/all/20231211181935.GG27535@pendragon.ideasonboard.com/ [2] https://lore.kernel.org/all/20230116144454.1012-14-laurent.pinchart@ideasonboard.com/ -- Best regards, Mikhail Rudenko
Hi Mikhail, On Mon, Jan 08, 2024 at 06:06:52PM +0300, Mikhail Rudenko wrote: > Hi Sakari, > > Thanks for the review! > > On 2024-01-08 at 11:18 GMT, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > Hi Mikhail, > > > > On Mon, Dec 18, 2023 at 08:40:30PM +0300, Mikhail Rudenko wrote: > >> Use runtime PM autosuspend to avoid powering off the sensor during > >> fast stop-reconfigure-restart cycles. > >> > >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > >> --- > >> drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- > >> 1 file changed, 15 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c > >> index 5300e621ff90..64cc6d9e48cc 100644 > >> --- a/drivers/media/i2c/ov4689.c > >> +++ b/drivers/media/i2c/ov4689.c > >> @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) > >> ov4689->cur_mode->num_regs, > >> NULL); > >> if (ret) { > >> - pm_runtime_put(dev); > >> + pm_runtime_put_sync(dev); > > > > Why are you switching to pm_runtime_put_sync() here? That isn't covered by > > the commit message (nor I think should be done). > > PM autosuspend conversion was suggested earlier by Laurent in his review > of this series [1], and he adviced looking at how it was done for the > imx290 driver. I followed along the lines of the corresponding patch > [2]. There's no need to use the _sync() variant here. And at least it wouldn't be related to autosuspend, were you to switch to that. > > >> goto unlock_and_return; > >> } > >> > >> ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); > >> if (ret) { > >> - pm_runtime_put(dev); > >> + pm_runtime_put_sync(dev); > >> goto unlock_and_return; > >> } > >> > >> ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, > >> OV4689_MODE_STREAMING, NULL); > >> if (ret) { > >> - pm_runtime_put(dev); > >> + pm_runtime_put_sync(dev); > >> goto unlock_and_return; > >> } > >> } else { > >> cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, > >> OV4689_MODE_SW_STANDBY, NULL); > >> - pm_runtime_put(dev); > >> + pm_runtime_mark_last_busy(dev); > >> + pm_runtime_put_autosuspend(dev); > >> } > >> > >> unlock_and_return: > >> @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) > >> break; > >> } > >> > >> - pm_runtime_put(dev); > >> + pm_runtime_mark_last_busy(dev); > >> + pm_runtime_put_autosuspend(dev); > > > > Also note that with runtime PM autosuspend, you have to use > > pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use(). > > Noted, will do so in v3. > > >> + > >> return ret; > >> } > >> > >> @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) > >> } > >> > >> pm_runtime_set_active(dev); > >> + pm_runtime_get_noresume(dev); > >> pm_runtime_enable(dev); > >> - pm_runtime_idle(dev); > >> + pm_runtime_set_autosuspend_delay(dev, 1000); > >> + pm_runtime_use_autosuspend(dev); > >> > >> ret = v4l2_async_register_subdev_sensor(sd); > >> if (ret) { > >> @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) > >> goto err_clean_subdev_pm; > >> } > >> > >> + pm_runtime_mark_last_busy(dev); > >> + pm_runtime_put_autosuspend(dev); > >> + > >> return 0; > >> > >> err_clean_subdev_pm: > >> pm_runtime_disable(dev); > >> - pm_runtime_set_suspended(dev); > >> + pm_runtime_put_noidle(dev); > >> v4l2_subdev_cleanup(sd); > >> err_clean_entity: > >> media_entity_cleanup(&sd->entity); > > [1] https://lore.kernel.org/all/20231211181935.GG27535@pendragon.ideasonboard.com/ > [2] https://lore.kernel.org/all/20230116144454.1012-14-laurent.pinchart@ideasonboard.com/ >
Hi Sakari, and thanks for the review! On 2024-02-23 at 08:19 GMT, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi Mikhail, > > On Mon, Jan 08, 2024 at 06:06:52PM +0300, Mikhail Rudenko wrote: >> Hi Sakari, >> >> Thanks for the review! >> >> On 2024-01-08 at 11:18 GMT, Sakari Ailus <sakari.ailus@iki.fi> wrote: >> >> > Hi Mikhail, >> > >> > On Mon, Dec 18, 2023 at 08:40:30PM +0300, Mikhail Rudenko wrote: >> >> Use runtime PM autosuspend to avoid powering off the sensor during >> >> fast stop-reconfigure-restart cycles. >> >> >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> >> --- >> >> drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- >> >> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> >> index 5300e621ff90..64cc6d9e48cc 100644 >> >> --- a/drivers/media/i2c/ov4689.c >> >> +++ b/drivers/media/i2c/ov4689.c >> >> @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) >> >> ov4689->cur_mode->num_regs, >> >> NULL); >> >> if (ret) { >> >> - pm_runtime_put(dev); >> >> + pm_runtime_put_sync(dev); >> > >> > Why are you switching to pm_runtime_put_sync() here? That isn't covered by >> > the commit message (nor I think should be done). >> >> PM autosuspend conversion was suggested earlier by Laurent in his review >> of this series [1], and he adviced looking at how it was done for the >> imx290 driver. I followed along the lines of the corresponding patch >> [2]. > > There's no need to use the _sync() variant here. And at least it wouldn't > be related to autosuspend, were you to switch to that. Ok, will use pm_runtime_put in v3. Or do you suggest dropping this patch altogether? Laurent? >> >> >> goto unlock_and_return; >> >> } >> >> >> >> ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); >> >> if (ret) { >> >> - pm_runtime_put(dev); >> >> + pm_runtime_put_sync(dev); >> >> goto unlock_and_return; >> >> } >> >> >> >> ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> >> OV4689_MODE_STREAMING, NULL); >> >> if (ret) { >> >> - pm_runtime_put(dev); >> >> + pm_runtime_put_sync(dev); >> >> goto unlock_and_return; >> >> } >> >> } else { >> >> cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> >> OV4689_MODE_SW_STANDBY, NULL); >> >> - pm_runtime_put(dev); >> >> + pm_runtime_mark_last_busy(dev); >> >> + pm_runtime_put_autosuspend(dev); >> >> } >> >> >> >> unlock_and_return: >> >> @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) >> >> break; >> >> } >> >> >> >> - pm_runtime_put(dev); >> >> + pm_runtime_mark_last_busy(dev); >> >> + pm_runtime_put_autosuspend(dev); >> > >> > Also note that with runtime PM autosuspend, you have to use >> > pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use(). >> >> Noted, will do so in v3. >> >> >> + >> >> return ret; >> >> } >> >> >> >> @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) >> >> } >> >> >> >> pm_runtime_set_active(dev); >> >> + pm_runtime_get_noresume(dev); >> >> pm_runtime_enable(dev); >> >> - pm_runtime_idle(dev); >> >> + pm_runtime_set_autosuspend_delay(dev, 1000); >> >> + pm_runtime_use_autosuspend(dev); >> >> >> >> ret = v4l2_async_register_subdev_sensor(sd); >> >> if (ret) { >> >> @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) >> >> goto err_clean_subdev_pm; >> >> } >> >> >> >> + pm_runtime_mark_last_busy(dev); >> >> + pm_runtime_put_autosuspend(dev); >> >> + >> >> return 0; >> >> >> >> err_clean_subdev_pm: >> >> pm_runtime_disable(dev); >> >> - pm_runtime_set_suspended(dev); >> >> + pm_runtime_put_noidle(dev); >> >> v4l2_subdev_cleanup(sd); >> >> err_clean_entity: >> >> media_entity_cleanup(&sd->entity); >> >> [1] https://lore.kernel.org/all/20231211181935.GG27535@pendragon.ideasonboard.com/ >> [2] https://lore.kernel.org/all/20230116144454.1012-14-laurent.pinchart@ideasonboard.com/ >> -- Best regards, Mikhail Rudenko
Hi Mikhail, On Fri, Feb 23, 2024 at 06:18:12PM +0300, Mikhail Rudenko wrote: > > Hi Sakari, > > and thanks for the review! > > On 2024-02-23 at 08:19 GMT, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > Hi Mikhail, > > > > On Mon, Jan 08, 2024 at 06:06:52PM +0300, Mikhail Rudenko wrote: > >> Hi Sakari, > >> > >> Thanks for the review! > >> > >> On 2024-01-08 at 11:18 GMT, Sakari Ailus <sakari.ailus@iki.fi> wrote: > >> > >> > Hi Mikhail, > >> > > >> > On Mon, Dec 18, 2023 at 08:40:30PM +0300, Mikhail Rudenko wrote: > >> >> Use runtime PM autosuspend to avoid powering off the sensor during > >> >> fast stop-reconfigure-restart cycles. > >> >> > >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > >> >> --- > >> >> drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- > >> >> 1 file changed, 15 insertions(+), 7 deletions(-) > >> >> > >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c > >> >> index 5300e621ff90..64cc6d9e48cc 100644 > >> >> --- a/drivers/media/i2c/ov4689.c > >> >> +++ b/drivers/media/i2c/ov4689.c > >> >> @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) > >> >> ov4689->cur_mode->num_regs, > >> >> NULL); > >> >> if (ret) { > >> >> - pm_runtime_put(dev); > >> >> + pm_runtime_put_sync(dev); > >> > > >> > Why are you switching to pm_runtime_put_sync() here? That isn't covered by > >> > the commit message (nor I think should be done). > >> > >> PM autosuspend conversion was suggested earlier by Laurent in his review > >> of this series [1], and he adviced looking at how it was done for the > >> imx290 driver. I followed along the lines of the corresponding patch > >> [2]. > > > > There's no need to use the _sync() variant here. And at least it wouldn't > > be related to autosuspend, were you to switch to that. > > Ok, will use pm_runtime_put in v3. Or do you suggest dropping this patch > altogether? Laurent? Using autosuspend is preferred. Much of the benefit come from avoiding redundant register writes but that's a separate matter. > > >> > >> >> goto unlock_and_return; > >> >> } > >> >> > >> >> ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); > >> >> if (ret) { > >> >> - pm_runtime_put(dev); > >> >> + pm_runtime_put_sync(dev); > >> >> goto unlock_and_return; > >> >> } > >> >> > >> >> ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, > >> >> OV4689_MODE_STREAMING, NULL); > >> >> if (ret) { > >> >> - pm_runtime_put(dev); > >> >> + pm_runtime_put_sync(dev); > >> >> goto unlock_and_return; > >> >> } > >> >> } else { > >> >> cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, > >> >> OV4689_MODE_SW_STANDBY, NULL); > >> >> - pm_runtime_put(dev); > >> >> + pm_runtime_mark_last_busy(dev); > >> >> + pm_runtime_put_autosuspend(dev); > >> >> } > >> >> > >> >> unlock_and_return: > >> >> @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) > >> >> break; > >> >> } > >> >> > >> >> - pm_runtime_put(dev); > >> >> + pm_runtime_mark_last_busy(dev); > >> >> + pm_runtime_put_autosuspend(dev); > >> > > >> > Also note that with runtime PM autosuspend, you have to use > >> > pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use(). > >> > >> Noted, will do so in v3. > >> > >> >> + > >> >> return ret; > >> >> } > >> >> > >> >> @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) > >> >> } > >> >> > >> >> pm_runtime_set_active(dev); > >> >> + pm_runtime_get_noresume(dev); > >> >> pm_runtime_enable(dev); > >> >> - pm_runtime_idle(dev); > >> >> + pm_runtime_set_autosuspend_delay(dev, 1000); > >> >> + pm_runtime_use_autosuspend(dev); > >> >> > >> >> ret = v4l2_async_register_subdev_sensor(sd); > >> >> if (ret) { > >> >> @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) > >> >> goto err_clean_subdev_pm; > >> >> } > >> >> > >> >> + pm_runtime_mark_last_busy(dev); > >> >> + pm_runtime_put_autosuspend(dev); > >> >> + > >> >> return 0; > >> >> > >> >> err_clean_subdev_pm: > >> >> pm_runtime_disable(dev); > >> >> - pm_runtime_set_suspended(dev); > >> >> + pm_runtime_put_noidle(dev); > >> >> v4l2_subdev_cleanup(sd); > >> >> err_clean_entity: > >> >> media_entity_cleanup(&sd->entity); > >> > >> [1] https://lore.kernel.org/all/20231211181935.GG27535@pendragon.ideasonboard.com/ > >> [2] https://lore.kernel.org/all/20230116144454.1012-14-laurent.pinchart@ideasonboard.com/ > >> > >
On 2024-02-24 at 19:38 GMT, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi Mikhail, > > On Fri, Feb 23, 2024 at 06:18:12PM +0300, Mikhail Rudenko wrote: >> >> Hi Sakari, >> >> and thanks for the review! >> >> On 2024-02-23 at 08:19 GMT, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: >> >> > Hi Mikhail, >> > >> > On Mon, Jan 08, 2024 at 06:06:52PM +0300, Mikhail Rudenko wrote: >> >> Hi Sakari, >> >> >> >> Thanks for the review! >> >> >> >> On 2024-01-08 at 11:18 GMT, Sakari Ailus <sakari.ailus@iki.fi> wrote: >> >> >> >> > Hi Mikhail, >> >> > >> >> > On Mon, Dec 18, 2023 at 08:40:30PM +0300, Mikhail Rudenko wrote: >> >> >> Use runtime PM autosuspend to avoid powering off the sensor during >> >> >> fast stop-reconfigure-restart cycles. >> >> >> >> >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> >> >> --- >> >> >> drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- >> >> >> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> >> >> >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> >> >> index 5300e621ff90..64cc6d9e48cc 100644 >> >> >> --- a/drivers/media/i2c/ov4689.c >> >> >> +++ b/drivers/media/i2c/ov4689.c >> >> >> @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) >> >> >> ov4689->cur_mode->num_regs, >> >> >> NULL); >> >> >> if (ret) { >> >> >> - pm_runtime_put(dev); >> >> >> + pm_runtime_put_sync(dev); >> >> > >> >> > Why are you switching to pm_runtime_put_sync() here? That isn't covered by >> >> > the commit message (nor I think should be done). >> >> >> >> PM autosuspend conversion was suggested earlier by Laurent in his review >> >> of this series [1], and he adviced looking at how it was done for the >> >> imx290 driver. I followed along the lines of the corresponding patch >> >> [2]. >> > >> > There's no need to use the _sync() variant here. And at least it wouldn't >> > be related to autosuspend, were you to switch to that. >> >> Ok, will use pm_runtime_put in v3. Or do you suggest dropping this patch >> altogether? Laurent? > > Using autosuspend is preferred. > > Much of the benefit come from avoiding redundant register writes but that's > a separate matter. I see, will try to do it in a separate patch outside this series. Could you please point to a driver which does this right? > >> >> >> >> >> >> goto unlock_and_return; >> >> >> } >> >> >> >> >> >> ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); >> >> >> if (ret) { >> >> >> - pm_runtime_put(dev); >> >> >> + pm_runtime_put_sync(dev); >> >> >> goto unlock_and_return; >> >> >> } >> >> >> >> >> >> ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> >> >> OV4689_MODE_STREAMING, NULL); >> >> >> if (ret) { >> >> >> - pm_runtime_put(dev); >> >> >> + pm_runtime_put_sync(dev); >> >> >> goto unlock_and_return; >> >> >> } >> >> >> } else { >> >> >> cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> >> >> OV4689_MODE_SW_STANDBY, NULL); >> >> >> - pm_runtime_put(dev); >> >> >> + pm_runtime_mark_last_busy(dev); >> >> >> + pm_runtime_put_autosuspend(dev); >> >> >> } >> >> >> >> >> >> unlock_and_return: >> >> >> @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) >> >> >> break; >> >> >> } >> >> >> >> >> >> - pm_runtime_put(dev); >> >> >> + pm_runtime_mark_last_busy(dev); >> >> >> + pm_runtime_put_autosuspend(dev); >> >> > >> >> > Also note that with runtime PM autosuspend, you have to use >> >> > pm_runtime_get_if_active() instead of pm_runtime_get_if_in_use(). >> >> >> >> Noted, will do so in v3. >> >> >> >> >> + >> >> >> return ret; >> >> >> } >> >> >> >> >> >> @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) >> >> >> } >> >> >> >> >> >> pm_runtime_set_active(dev); >> >> >> + pm_runtime_get_noresume(dev); >> >> >> pm_runtime_enable(dev); >> >> >> - pm_runtime_idle(dev); >> >> >> + pm_runtime_set_autosuspend_delay(dev, 1000); >> >> >> + pm_runtime_use_autosuspend(dev); >> >> >> >> >> >> ret = v4l2_async_register_subdev_sensor(sd); >> >> >> if (ret) { >> >> >> @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) >> >> >> goto err_clean_subdev_pm; >> >> >> } >> >> >> >> >> >> + pm_runtime_mark_last_busy(dev); >> >> >> + pm_runtime_put_autosuspend(dev); >> >> >> + >> >> >> return 0; >> >> >> >> >> >> err_clean_subdev_pm: >> >> >> pm_runtime_disable(dev); >> >> >> - pm_runtime_set_suspended(dev); >> >> >> + pm_runtime_put_noidle(dev); >> >> >> v4l2_subdev_cleanup(sd); >> >> >> err_clean_entity: >> >> >> media_entity_cleanup(&sd->entity); >> >> >> >> [1] https://lore.kernel.org/all/20231211181935.GG27535@pendragon.ideasonboard.com/ >> >> [2] https://lore.kernel.org/all/20230116144454.1012-14-laurent.pinchart@ideasonboard.com/ >> >> >> >> -- Best regards, Mikhail Rudenko
diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c index 5300e621ff90..64cc6d9e48cc 100644 --- a/drivers/media/i2c/ov4689.c +++ b/drivers/media/i2c/ov4689.c @@ -407,26 +407,27 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on) ov4689->cur_mode->num_regs, NULL); if (ret) { - pm_runtime_put(dev); + pm_runtime_put_sync(dev); goto unlock_and_return; } ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); if (ret) { - pm_runtime_put(dev); + pm_runtime_put_sync(dev); goto unlock_and_return; } ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, OV4689_MODE_STREAMING, NULL); if (ret) { - pm_runtime_put(dev); + pm_runtime_put_sync(dev); goto unlock_and_return; } } else { cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, OV4689_MODE_SW_STANDBY, NULL); - pm_runtime_put(dev); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); } unlock_and_return: @@ -606,7 +607,9 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl) break; } - pm_runtime_put(dev); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return ret; } @@ -877,8 +880,10 @@ static int ov4689_probe(struct i2c_client *client) } pm_runtime_set_active(dev); + pm_runtime_get_noresume(dev); pm_runtime_enable(dev); - pm_runtime_idle(dev); + pm_runtime_set_autosuspend_delay(dev, 1000); + pm_runtime_use_autosuspend(dev); ret = v4l2_async_register_subdev_sensor(sd); if (ret) { @@ -886,11 +891,14 @@ static int ov4689_probe(struct i2c_client *client) goto err_clean_subdev_pm; } + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return 0; err_clean_subdev_pm: pm_runtime_disable(dev); - pm_runtime_set_suspended(dev); + pm_runtime_put_noidle(dev); v4l2_subdev_cleanup(sd); err_clean_entity: media_entity_cleanup(&sd->entity);
Use runtime PM autosuspend to avoid powering off the sensor during fast stop-reconfigure-restart cycles. Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> --- drivers/media/i2c/ov4689.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)