| Message ID | 20231117111433.1561669-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Small Runtime PM API changes | expand |
On Fri, 17 Nov 2023 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > pm_runtime_get_if_in_use() returns an error if Runtime PM is disabled for > the device, in which case it won't increment the use count. > pm_runtime_put() does that unconditionally however. Only call > pm_runtime_put() in case pm_runtime_get_if_in_use() has returned a value > > 0. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/imx219.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 8436880dcf7a..9865d0b41244 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -371,7 +371,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > const struct v4l2_mbus_framefmt *format; > struct v4l2_subdev_state *state; > - int ret = 0; > + int ret = 0, pm_status; > > state = v4l2_subdev_get_locked_active_state(&imx219->sd); > format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0); > @@ -393,7 +393,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > * Applying V4L2 control value only happens > * when power is up for streaming > */ > - if (pm_runtime_get_if_in_use(&client->dev) == 0) > + pm_status = pm_runtime_get_if_in_use(&client->dev); > + if (!pm_status) > return 0; > > switch (ctrl->id) { > @@ -446,7 +447,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > - pm_runtime_put(&client->dev); > + if (pm_status > 0) > + pm_runtime_put(&client->dev); > > return ret; > } > -- > 2.39.2 > >
Hi Sakari, Thank you for the patch. On Fri, Nov 17, 2023 at 01:14:27PM +0200, Sakari Ailus wrote: > The majority of the callers currently using pm_runtime_get_if_active() > call it with ign_usage_count argument set to true. There's only one driver > using this feature (and natually implementation of > pm_runtime_get_if_in_use()). > > To make this function more practical to use, remove the ign_usage_count > argument from the function. The main implementation is renamed as > __pm_runtime_get_conditional(). > > This function is expected to gain a large number of users in the future > --- camera sensor drivers using runtime autosuspend have a need to get the s/---/-/ > device's usage_count conditionally if it is enabled. Most of these > currently use pm_runtime_get_if_in_use(), which is a bug. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > since v1: > > - Fix __pm_runtime_get_if_conditional() un-CONFIG_PM implementation. > --- > Documentation/power/runtime_pm.rst | 5 ++-- > drivers/base/power/runtime.c | 9 ++++--- > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- > drivers/media/i2c/ccs/ccs-core.c | 2 +- > drivers/net/ipa/ipa_smp2p.c | 2 +- > drivers/pci/pci.c | 2 +- > include/linux/pm_runtime.h | 32 +++++++++++++++++++++---- > sound/hda/hdac_device.c | 2 +- > 8 files changed, 40 insertions(+), 16 deletions(-) > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst > index 65b86e487afe..da99379071a4 100644 > --- a/Documentation/power/runtime_pm.rst > +++ b/Documentation/power/runtime_pm.rst > @@ -396,10 +396,9 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: > nonzero, increment the counter and return 1; otherwise return 0 without > changing the counter > > - `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);` > + `int pm_runtime_get_if_active(struct device *dev);` > - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the > - runtime PM status is RPM_ACTIVE, and either ign_usage_count is true > - or the device's usage_count is non-zero, increment the counter and > + runtime PM status is RPM_ACTIVE, increment the counter and > return 1; otherwise return 0 without changing the counter > > `void pm_runtime_put_noidle(struct device *dev);` > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 4545669cb973..8b56468eca9d 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -1175,7 +1175,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags) > EXPORT_SYMBOL_GPL(__pm_runtime_resume); > > /** > - * pm_runtime_get_if_active - Conditionally bump up device usage counter. > + * __pm_runtime_get_conditional - Conditionally bump up device usage counter. > * @dev: Device to handle. > * @ign_usage_count: Whether or not to look at the current usage counter value. > * > @@ -1195,8 +1195,11 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume); > * > * The caller is responsible for decrementing the runtime PM usage counter of > * @dev after this function has returned a positive value for it. > + * > + * This function is not intended to be called by drivers, use > + * pm_runtime_get_if_active() or pm_runtime_get_if_in_use() instead. > */ > -int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count) > +int __pm_runtime_get_conditional(struct device *dev, bool ign_usage_count) > { > unsigned long flags; > int retval; > @@ -1217,7 +1220,7 @@ int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count) > > return retval; > } > -EXPORT_SYMBOL_GPL(pm_runtime_get_if_active); > +EXPORT_SYMBOL_GPL(__pm_runtime_get_conditional); > > /** > * __pm_runtime_set_status - Set runtime PM status of a device. > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 6d8e5e5c0cba..b163efe80975 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -434,7 +434,7 @@ static intel_wakeref_t __intel_runtime_pm_get_if_active(struct intel_runtime_pm > * function, since the power state is undefined. This applies > * atm to the late/early system suspend/resume handlers. > */ > - if (pm_runtime_get_if_active(rpm->kdev, ignore_usecount) <= 0) > + if (__pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0) > return 0; > } > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index 12e6f0a26fc8..45701a18c845 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -664,7 +664,7 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > - pm_status = pm_runtime_get_if_active(&client->dev, true); > + pm_status = pm_runtime_get_if_active(&client->dev); > if (!pm_status) > return 0; > > diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c > index 5620dc271fac..cbf3d4761ce3 100644 > --- a/drivers/net/ipa/ipa_smp2p.c > +++ b/drivers/net/ipa/ipa_smp2p.c > @@ -92,7 +92,7 @@ static void ipa_smp2p_notify(struct ipa_smp2p *smp2p) > return; > > dev = &smp2p->ipa->pdev->dev; > - smp2p->power_on = pm_runtime_get_if_active(dev, true) > 0; > + smp2p->power_on = pm_runtime_get_if_active(dev) > 0; > > /* Signal whether the IPA power is enabled */ > mask = BIT(smp2p->enabled_bit); > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 59c01d68c6d5..d8d4abbc936f 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2439,7 +2439,7 @@ static void pci_pme_list_scan(struct work_struct *work) > * If the device is in a low power state it > * should not be polled either. > */ > - pm_status = pm_runtime_get_if_active(dev, true); > + pm_status = pm_runtime_get_if_active(dev); > if (!pm_status) > continue; > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 7c9b35448563..13cd526634c1 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -72,7 +72,8 @@ extern int pm_runtime_force_resume(struct device *dev); > extern int __pm_runtime_idle(struct device *dev, int rpmflags); > extern int __pm_runtime_suspend(struct device *dev, int rpmflags); > extern int __pm_runtime_resume(struct device *dev, int rpmflags); > -extern int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count); > +extern int __pm_runtime_get_conditional(struct device *dev, > + bool ign_usage_count); > extern int pm_schedule_suspend(struct device *dev, unsigned int delay); > extern int __pm_runtime_set_status(struct device *dev, unsigned int status); > extern int pm_runtime_barrier(struct device *dev); > @@ -94,16 +95,33 @@ extern void pm_runtime_release_supplier(struct device_link *link); > > extern int devm_pm_runtime_enable(struct device *dev); > > +/** > + * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is > + * in active state > + * @dev: Target device. > + * > + * Increment the runtime PM usage counter of @dev if its runtime PM status is > + * %RPM_ACTIVE, in which case it returns 1. If the device is in a different > + * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the > + * device. > + */ > +static inline int pm_runtime_get_if_active(struct device *dev) > +{ > + return __pm_runtime_get_conditional(dev, true); > +} > + > /** > * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter. > * @dev: Target device. > * > * Increment the runtime PM usage counter of @dev if its runtime PM status is > - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0. > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case > + * it returns 1. If the device is in a different state or its usage_count is 0, > + * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device. It would be nice to explain the difference between RPM_ACTIVE and usage_count. It can be done in a separate patch. > */ > static inline int pm_runtime_get_if_in_use(struct device *dev) > { > - return pm_runtime_get_if_active(dev, false); > + return __pm_runtime_get_conditional(dev, false); > } > > /** > @@ -275,8 +293,12 @@ static inline int pm_runtime_get_if_in_use(struct device *dev) > { > return -EINVAL; > } > -static inline int pm_runtime_get_if_active(struct device *dev, > - bool ign_usage_count) > +static inline int pm_runtime_get_if_active(struct device *dev) > +{ > + return -EINVAL; > +} The pm_runtime_get_if_in_use() and pm_runtime_get_if_active() wrappers could be defined once, outside of the CONFIG_PM conditional section, nistead of having a stub version when !CONFIG_PM. Lowering the amount of conditional code helps minimizing the risk of build errors in exotic configurations. This can also be done in a separate patch. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +static inline int __pm_runtime_get_conditional(struct device *dev, > + bool ign_usage_count) > { > return -EINVAL; > } > diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c > index bbf7bcdb449a..0a9223c18d77 100644 > --- a/sound/hda/hdac_device.c > +++ b/sound/hda/hdac_device.c > @@ -611,7 +611,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_power_up_pm); > int snd_hdac_keep_power_up(struct hdac_device *codec) > { > if (!atomic_inc_not_zero(&codec->in_pm)) { > - int ret = pm_runtime_get_if_active(&codec->dev, true); > + int ret = pm_runtime_get_if_active(&codec->dev); > if (!ret) > return -1; > if (ret < 0)
Hi Rafael, On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > wish to set the last_busy timestamp to current time and put the > > > usage_count of the device and set the autosuspend timer. > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > calling first pm_runtime_mark_last_busy() and then > > > pm_runtime_put_autosuspend(). > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > pm_runtime_mark_last_busy() right before. Let's make the > > pm_runtime_put_autosuspend() function do that by default, and add a > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > of cases where updating the last busy timestamp isn't desired. We want > > to simplify the API, not make it more complex. > > I would also prefer it to be done this way if not too problematic. I'm glad you agree :-) The change will probably be a bit painful, but I think it's for the best. Sakari, please let me know if I can help.
Hi Laurent, On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > Hi Rafael, > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > wish to set the last_busy timestamp to current time and put the > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > calling first pm_runtime_mark_last_busy() and then > > > > pm_runtime_put_autosuspend(). > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > pm_runtime_mark_last_busy() right before. Let's make the > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > of cases where updating the last busy timestamp isn't desired. We want > > > to simplify the API, not make it more complex. > > > > I would also prefer it to be done this way if not too problematic. > > I'm glad you agree :-) The change will probably be a bit painful, but I > think it's for the best. Sakari, please let me know if I can help. I actually do prefer this approach, too. There about 350 drivers using pm_runtime_autosuspend() currently. Around 150 uses pm_runtime_autosuspend() which is not preceded by pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. I checked some of what's left: most do still call both, but in a way that evades Coccinelle matching. Some omissions seem to remain. Given that there are way more users that do also call pm_runtime_mark_last_busy(), I think I'll try to introduce __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() documentation change first and then rename the callers that don't use pm_runtime_mark_last_busy().
Hi Sakari, On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > wish to set the last_busy timestamp to current time and put the > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > pm_runtime_put_autosuspend(). > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > to simplify the API, not make it more complex. > > > > > > I would also prefer it to be done this way if not too problematic. > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > think it's for the best. Sakari, please let me know if I can help. > > I actually do prefer this approach, too. > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > 150 uses pm_runtime_autosuspend() which is not preceded by > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > I checked some of what's left: most do still call both, but in a way that > evades Coccinelle matching. Some omissions seem to remain. > > Given that there are way more users that do also call > pm_runtime_mark_last_busy(), I think I'll try to introduce > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > documentation change first and then rename the callers that don't use > pm_runtime_mark_last_busy(). And also drop pm_runtime_mark_last_busy() from the drivers that call pm_runtime_put_autosuspend(), right ? This sounds good to me. Thank you for working on this. Two RPM API simplifications in a week, it feels like Christmas is coming :-)
Hi Laurent, On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > > wish to set the last_busy timestamp to current time and put the > > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > > pm_runtime_put_autosuspend(). > > > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > > to simplify the API, not make it more complex. > > > > > > > > I would also prefer it to be done this way if not too problematic. > > > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > > think it's for the best. Sakari, please let me know if I can help. > > > > I actually do prefer this approach, too. > > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > > 150 uses pm_runtime_autosuspend() which is not preceded by > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > > > I checked some of what's left: most do still call both, but in a way that > > evades Coccinelle matching. Some omissions seem to remain. > > > > Given that there are way more users that do also call > > pm_runtime_mark_last_busy(), I think I'll try to introduce > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > > documentation change first and then rename the callers that don't use > > pm_runtime_mark_last_busy(). > > And also drop pm_runtime_mark_last_busy() from the drivers that call > pm_runtime_put_autosuspend(), right ? That should be done but as it doesn't affect the functionality, it can (and may only) be done later on --- the current users need to be converted to use the to-be-added __pm_runtime_put_autosuspend() first. > > This sounds good to me. Thank you for working on this. Two RPM API > simplifications in a week, it feels like Christmas is coming :-) Yes. And it's always the case actually! Only the time that it takes differs.
Hi Sakari, On Tue, Nov 21, 2023 at 08:41:34AM +0000, Sakari Ailus wrote: > On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote: > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > > > wish to set the last_busy timestamp to current time and put the > > > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > > > pm_runtime_put_autosuspend(). > > > > > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > > > to simplify the API, not make it more complex. > > > > > > > > > > I would also prefer it to be done this way if not too problematic. > > > > > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > > > think it's for the best. Sakari, please let me know if I can help. > > > > > > I actually do prefer this approach, too. > > > > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > > > 150 uses pm_runtime_autosuspend() which is not preceded by > > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > > > > > I checked some of what's left: most do still call both, but in a way that > > > evades Coccinelle matching. Some omissions seem to remain. > > > > > > Given that there are way more users that do also call > > > pm_runtime_mark_last_busy(), I think I'll try to introduce > > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > > > documentation change first and then rename the callers that don't use > > > pm_runtime_mark_last_busy(). > > > > And also drop pm_runtime_mark_last_busy() from the drivers that call > > pm_runtime_put_autosuspend(), right ? > > That should be done but as it doesn't affect the functionality, it can (and > may only) be done later on --- the current users need to be converted to > use the to-be-added __pm_runtime_put_autosuspend() first. True. If you're going to send a series that change things tree-wide I was thinking that it would be best to address multiple tree-wide changes at the same time, that would be less churn, especially if it can be mostly scripted with Coccinelle. That would be my preference (especially because the issue will then be solved and we'll be able to move to something else, instead of leaving old code lingering on for a long time), but it's up to you. > > This sounds good to me. Thank you for working on this. Two RPM API > > simplifications in a week, it feels like Christmas is coming :-) > > Yes. And it's always the case actually! Only the time that it takes > differs.
Hi Laurent, On Tue, Nov 21, 2023 at 10:50:56AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Nov 21, 2023 at 08:41:34AM +0000, Sakari Ailus wrote: > > On Mon, Nov 20, 2023 at 11:47:43AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 20, 2023 at 09:27:51AM +0000, Sakari Ailus wrote: > > > > On Sat, Nov 18, 2023 at 11:30:31PM +0200, Laurent Pinchart wrote: > > > > > On Sat, Nov 18, 2023 at 10:20:46PM +0100, Rafael J. Wysocki wrote: > > > > > > On Sat, Nov 18, 2023 at 6:49 PM Laurent Pinchart wrote: > > > > > > > On Fri, Nov 17, 2023 at 01:14:28PM +0200, Sakari Ailus wrote: > > > > > > > > Add pm_runtime_put_mark_busy_autosusp() helper function for users that > > > > > > > > wish to set the last_busy timestamp to current time and put the > > > > > > > > usage_count of the device and set the autosuspend timer. > > > > > > > > > > > > > > > > Essentially calling pm_runtime_suspend_mark_busy_autosusp() equal to > > > > > > > > calling first pm_runtime_mark_last_busy() and then > > > > > > > > pm_runtime_put_autosuspend(). > > > > > > > > > > > > > > The vast majority if the pm_runtime_put_autosuspend() users call > > > > > > > pm_runtime_mark_last_busy() right before. Let's make the > > > > > > > pm_runtime_put_autosuspend() function do that by default, and add a > > > > > > > __pm_runtime_put_autosuspend() (name to be bikshedded) for the minority > > > > > > > of cases where updating the last busy timestamp isn't desired. We want > > > > > > > to simplify the API, not make it more complex. > > > > > > > > > > > > I would also prefer it to be done this way if not too problematic. > > > > > > > > > > I'm glad you agree :-) The change will probably be a bit painful, but I > > > > > think it's for the best. Sakari, please let me know if I can help. > > > > > > > > I actually do prefer this approach, too. > > > > > > > > There about 350 drivers using pm_runtime_autosuspend() currently. Around > > > > 150 uses pm_runtime_autosuspend() which is not preceded by > > > > pm_runtime_mark_last_busy(). Call-wise the numbers are ~ 1050 and ~ 330. > > > > > > > > I checked some of what's left: most do still call both, but in a way that > > > > evades Coccinelle matching. Some omissions seem to remain. > > > > > > > > Given that there are way more users that do also call > > > > pm_runtime_mark_last_busy(), I think I'll try to introduce > > > > __pm_runtime_put_autosuspend() and pm_runtime_put_autosuspend() > > > > documentation change first and then rename the callers that don't use > > > > pm_runtime_mark_last_busy(). > > > > > > And also drop pm_runtime_mark_last_busy() from the drivers that call > > > pm_runtime_put_autosuspend(), right ? > > > > That should be done but as it doesn't affect the functionality, it can (and > > may only) be done later on --- the current users need to be converted to > > use the to-be-added __pm_runtime_put_autosuspend() first. > > True. If you're going to send a series that change things tree-wide I > was thinking that it would be best to address multiple tree-wide changes > at the same time, that would be less churn, especially if it can be > mostly scripted with Coccinelle. That would be my preference (especially > because the issue will then be solved and we'll be able to move to > something else, instead of leaving old code lingering on for a long > time), but it's up to you. This will mean around 1000 changed lines in 350 files. Given the number of changes and how they're scattered around, I'd expect to merge first the Runtime PM API change, then later on the driver specific changes via their own trees. Doing it differently risks a large number of conflicts. Hopefully faster than changing the I²C driver probe function though. We also need to have some time before all users of pm_runtime_put_autosuspend() have been converted, including new ones merged meanwhile.
Hi folks, This small set happily mixes Runtime PM and media patches. The set does two main things Runtime PM API-wise. Firstly, pm_runtime_get_if_active() is made more user-friendly by removing the ign_use_count argument so the users no longer need to call it with that set to true. Secondly, pm_runtime_put_mark_busy_autosusp() helper is added to avoid drivers having to call pm_runtime_mark_last_busy() only to be followed by pm_runtime_autosuspend(). The vast majority of the users of pm_runtime_autosuspend() would probably have been fine with making pm_runtime_autosuspend() do the last busy stamping, too, but given the sheer number of users it's hard to tell if there could be problems here and there. On the other hand, there are probably a sizable proportion of call sites where the missing pm_runtime_mark_last_busy() call is simply a bug. The three last patches are addressing Runtime PM issues in a few sensor drivers. Comments would be welcome. since v1: - Fix a compilation issue when CONFIG_PM is disabled in the first patch. - Improve the documentation patch, assume the use of autosuspend (this generally makes sense for camera sensor drivers). - Keep using pm_runtime_get_if_in_use() in imx319 and imx219 drivers (they don't use autosuspend). - Add a patch to document acpi_dev_state_d0() in conjunction of non-D0 probe. Sakari Ailus (7): pm: runtime: Simplify pm_runtime_get_if_active() usage pm: runtime: Add pm_runtime_put_mark_busy_autosusp() helper ACPI: Documentation: Document acpi_dev_state_d0() media: Documentation: Improve camera sensor runtime PM documentation media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly media: imx319: Put usage_count correctly in s_ctrl callback media: imx219: Put usage_count correctly in s_ctrl callback .../driver-api/media/camera-sensor.rst | 76 +++++++++++++------ .../firmware-guide/acpi/non-d0-probe.rst | 10 +++ Documentation/power/runtime_pm.rst | 5 +- drivers/base/power/runtime.c | 9 ++- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- drivers/media/i2c/ccs/ccs-core.c | 2 +- drivers/media/i2c/imx219.c | 8 +- drivers/media/i2c/imx319.c | 8 +- drivers/media/i2c/ov8858.c | 8 +- drivers/net/ipa/ipa_smp2p.c | 2 +- drivers/pci/pci.c | 2 +- include/linux/pm_runtime.h | 49 ++++++++++-- sound/hda/hdac_device.c | 2 +- 13 files changed, 133 insertions(+), 50 deletions(-) base-commit: 3e238417254bfdcc23fe207780b59cbb08656762