diff mbox series

[v2,5/7] media: ov8858: Use pm_runtime_get_if_active(), put usage_count correctly

Message ID 20231117111433.1561669-6-sakari.ailus@linux.intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Small Runtime PM API changes | expand

Commit Message

Sakari Ailus Nov. 17, 2023, 11:14 a.m. UTC
Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
and set controls, then use runtime PM autosuspend once the controls have
been set (instead of likely transitioning to suspended state immediately).

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov8858.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi Nov. 17, 2023, 3:30 p.m. UTC | #1
Hi Sakari

On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> and set controls, then use runtime PM autosuspend once the controls have
> been set (instead of likely transitioning to suspended state immediately).
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/ov8858.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> index 3af6125a2eee..a99b91700a8d 100644
> --- a/drivers/media/i2c/ov8858.c
> +++ b/drivers/media/i2c/ov8858.c
> @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
>  	struct v4l2_subdev_state *state;
>  	u16 digi_gain;
>  	s64 max_exp;
> -	int ret;
> +	int ret, pm_status;
>
>  	/*
>  	 * The control handler and the subdev state use the same mutex and the
> @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>
> -	if (!pm_runtime_get_if_in_use(&client->dev))
> +	pm_status = pm_runtime_get_if_active(&client->dev);
> +	if (!pm_status)
>  		return 0;
>
>  	switch (ctrl->id) {
> @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>
> -	pm_runtime_put(&client->dev);
> +	if (pm_status > 0)

I'm not 100% sure I get this bit.

If we get here it means pm_status is either -EINVAL or > 0, otherwise
we would have exited earlier.

What's the point of checking for > 0 here ?

There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
the the below call is a nop, or if pm_runtime has not been enabled by
the driver, which means the driver doesn't use pm_runtime at all.

Are there other cases I have missed that require checking here for
pm_status > 0 ?

> +		pm_runtime_mark_busy_autosusp(&client->dev);
>
>  	return ret;
>  }
> --
> 2.39.2
>
Sakari Ailus Nov. 18, 2023, 11:12 a.m. UTC | #2
Hi Jacopo,

On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > and set controls, then use runtime PM autosuspend once the controls have
> > been set (instead of likely transitioning to suspended state immediately).
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/ov8858.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > index 3af6125a2eee..a99b91700a8d 100644
> > --- a/drivers/media/i2c/ov8858.c
> > +++ b/drivers/media/i2c/ov8858.c
> > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> >  	struct v4l2_subdev_state *state;
> >  	u16 digi_gain;
> >  	s64 max_exp;
> > -	int ret;
> > +	int ret, pm_status;
> >
> >  	/*
> >  	 * The control handler and the subdev state use the same mutex and the
> > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	}
> >
> > -	if (!pm_runtime_get_if_in_use(&client->dev))
> > +	pm_status = pm_runtime_get_if_active(&client->dev);
> > +	if (!pm_status)
> >  		return 0;
> >
> >  	switch (ctrl->id) {
> > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		break;
> >  	}
> >
> > -	pm_runtime_put(&client->dev);
> > +	if (pm_status > 0)
> 
> I'm not 100% sure I get this bit.
> 
> If we get here it means pm_status is either -EINVAL or > 0, otherwise
> we would have exited earlier.
> 
> What's the point of checking for > 0 here ?
> 
> There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> the the below call is a nop, or if pm_runtime has not been enabled by
> the driver, which means the driver doesn't use pm_runtime at all.
> 
> Are there other cases I have missed that require checking here for
> pm_status > 0 ?

Other than Runtime PM being disabled, I don't think that should happen.

So as such this patch does not fix a bug. I just prefer to be extra
cautious when it comes to use counts.

> 
> > +		pm_runtime_mark_busy_autosusp(&client->dev);
> >
> >  	return ret;
> >  }
Laurent Pinchart Nov. 18, 2023, 5:33 p.m. UTC | #3
Hi Sakari

On Sat, Nov 18, 2023 at 11:12:41AM +0000, Sakari Ailus wrote:
> On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> > On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > > and set controls, then use runtime PM autosuspend once the controls have
> > > been set (instead of likely transitioning to suspended state immediately).
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/i2c/ov8858.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > > index 3af6125a2eee..a99b91700a8d 100644
> > > --- a/drivers/media/i2c/ov8858.c
> > > +++ b/drivers/media/i2c/ov8858.c
> > > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  	struct v4l2_subdev_state *state;
> > >  	u16 digi_gain;
> > >  	s64 max_exp;
> > > -	int ret;
> > > +	int ret, pm_status;
> > >
> > >  	/*
> > >  	 * The control handler and the subdev state use the same mutex and the
> > > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  		break;
> > >  	}
> > >
> > > -	if (!pm_runtime_get_if_in_use(&client->dev))
> > > +	pm_status = pm_runtime_get_if_active(&client->dev);
> > > +	if (!pm_status)
> > >  		return 0;
> > >
> > >  	switch (ctrl->id) {
> > > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > >  		break;
> > >  	}
> > >
> > > -	pm_runtime_put(&client->dev);
> > > +	if (pm_status > 0)
> > 
> > I'm not 100% sure I get this bit.
> > 
> > If we get here it means pm_status is either -EINVAL or > 0, otherwise
> > we would have exited earlier.
> > 
> > What's the point of checking for > 0 here ?
> > 
> > There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> > the the below call is a nop, or if pm_runtime has not been enabled by
> > the driver, which means the driver doesn't use pm_runtime at all.
> > 
> > Are there other cases I have missed that require checking here for
> > pm_status > 0 ?
> 
> Other than Runtime PM being disabled, I don't think that should happen.
> 
> So as such this patch does not fix a bug. I just prefer to be extra
> cautious when it comes to use counts.

What happened to the old motto of "if it's not broken, don't fix it" ?
:-) I like how this series (slightly) simplifies the runtime PM API by
giving pm_runtime_get_if_active() the right behaviour for the most
common use cases. Let's continue in that direction, and evolve the API
to simplify driver, not render them more complex.

I would prefer refactoring this series to first switch drivers to
pm_runtime_get_if_active(), and then use autosuspend at the end of the
.s_ctrl() handler. That can be two patches, each modifying all relevant
sensor driver.

> > > +		pm_runtime_mark_busy_autosusp(&client->dev);
> > >
> > >  	return ret;
> > >  }
Sakari Ailus Nov. 20, 2023, 8:31 a.m. UTC | #4
Hi Laurent,

On Sat, Nov 18, 2023 at 07:33:15PM +0200, Laurent Pinchart wrote:
> Hi Sakari
> 
> On Sat, Nov 18, 2023 at 11:12:41AM +0000, Sakari Ailus wrote:
> > On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> > > On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > > > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > > > and set controls, then use runtime PM autosuspend once the controls have
> > > > been set (instead of likely transitioning to suspended state immediately).
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/i2c/ov8858.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > > > index 3af6125a2eee..a99b91700a8d 100644
> > > > --- a/drivers/media/i2c/ov8858.c
> > > > +++ b/drivers/media/i2c/ov8858.c
> > > > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  	struct v4l2_subdev_state *state;
> > > >  	u16 digi_gain;
> > > >  	s64 max_exp;
> > > > -	int ret;
> > > > +	int ret, pm_status;
> > > >
> > > >  	/*
> > > >  	 * The control handler and the subdev state use the same mutex and the
> > > > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		break;
> > > >  	}
> > > >
> > > > -	if (!pm_runtime_get_if_in_use(&client->dev))
> > > > +	pm_status = pm_runtime_get_if_active(&client->dev);
> > > > +	if (!pm_status)
> > > >  		return 0;
> > > >
> > > >  	switch (ctrl->id) {
> > > > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		break;
> > > >  	}
> > > >
> > > > -	pm_runtime_put(&client->dev);
> > > > +	if (pm_status > 0)
> > > 
> > > I'm not 100% sure I get this bit.
> > > 
> > > If we get here it means pm_status is either -EINVAL or > 0, otherwise
> > > we would have exited earlier.
> > > 
> > > What's the point of checking for > 0 here ?
> > > 
> > > There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> > > the the below call is a nop, or if pm_runtime has not been enabled by
> > > the driver, which means the driver doesn't use pm_runtime at all.
> > > 
> > > Are there other cases I have missed that require checking here for
> > > pm_status > 0 ?
> > 
> > Other than Runtime PM being disabled, I don't think that should happen.
> > 
> > So as such this patch does not fix a bug. I just prefer to be extra
> > cautious when it comes to use counts.
> 
> What happened to the old motto of "if it's not broken, don't fix it" ?

People tend to take patterns used in drivers and this indeed works here,
but only when taken with the rest of the Runtime PM API usage.

> :-) I like how this series (slightly) simplifies the runtime PM API by
> giving pm_runtime_get_if_active() the right behaviour for the most
> common use cases. Let's continue in that direction, and evolve the API
> to simplify driver, not render them more complex.
> 
> I would prefer refactoring this series to first switch drivers to
> pm_runtime_get_if_active(), and then use autosuspend at the end of the
> .s_ctrl() handler. That can be two patches, each modifying all relevant
> sensor driver.

I think it should be fine to do both in the same patch indeed.
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
index 3af6125a2eee..a99b91700a8d 100644
--- a/drivers/media/i2c/ov8858.c
+++ b/drivers/media/i2c/ov8858.c
@@ -1538,7 +1538,7 @@  static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct v4l2_subdev_state *state;
 	u16 digi_gain;
 	s64 max_exp;
-	int ret;
+	int ret, pm_status;
 
 	/*
 	 * The control handler and the subdev state use the same mutex and the
@@ -1561,7 +1561,8 @@  static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
-	if (!pm_runtime_get_if_in_use(&client->dev))
+	pm_status = pm_runtime_get_if_active(&client->dev);
+	if (!pm_status)
 		return 0;
 
 	switch (ctrl->id) {
@@ -1601,7 +1602,8 @@  static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	}
 
-	pm_runtime_put(&client->dev);
+	if (pm_status > 0)
+		pm_runtime_mark_busy_autosusp(&client->dev);
 
 	return ret;
 }