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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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;
> > >  }

-- 
Regards,

Laurent Pinchart