Re: [PATCH] media: Split v4l2_pipeline_pm_use into v4l2_pipeline_pm_{get, put}

[Ezequiel Garcia <ezequiel@collabora.com>]

Hi Hans,

On Tue, 2019-11-26 at 06:47 +0900, Ezequiel Garcia wrote:
> Currently, v4l2_pipeline_pm_use() prototype is:
> 
>   int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> 
> Where the 'use' argument shall only be set to '1' for enable/power-on,
> or to '0' for disable/power-off. The integer return is specified
> as only meaningful when 'use' is set to '1'.
> 
> Let's enforce this semantic by splitting the function in two:
> v4l2_pipeline_pm_get and v4l2_pipeline_pm_put. This is done
> for several reasons.
> 
> It makes the API easier to use (or harder to misuse).
> It removes the constraint on the values the 'use' argument
> shall take. Also, it removes the need to constraint
> the return value, by making v4l2_pipeline_pm_put void return.
> 
> And last, it's more consistent with other kernel APIs, such
> as the runtime pm APIs, which makes the code more symmetric.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Any feedback on this? No hurries, just pinging in case
it fell thru the cracks.

Thanks,
Ezequiel

> ---
>  Documentation/media/kapi/csi2.rst             |  2 +-
>  drivers/media/platform/omap3isp/ispvideo.c    |  4 +-
>  .../media/platform/qcom/camss/camss-video.c   |  4 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c   |  6 +--
>  .../platform/sunxi/sun4i-csi/sun4i_v4l2.c     |  6 +--
>  .../platform/sunxi/sun6i-csi/sun6i_video.c    |  4 +-
>  drivers/media/v4l2-core/v4l2-mc.c             | 18 +++++++--
>  drivers/staging/media/imx/imx-media-capture.c |  4 +-
>  drivers/staging/media/omap4iss/iss_video.c    |  4 +-
>  include/media/v4l2-mc.h                       | 40 ++++++++++++-------
>  10 files changed, 57 insertions(+), 35 deletions(-)
> 
> diff --git a/Documentation/media/kapi/csi2.rst b/Documentation/media/kapi/csi2.rst
> index 030a5c41ec75..e111ff7bfd3d 100644
> --- a/Documentation/media/kapi/csi2.rst
> +++ b/Documentation/media/kapi/csi2.rst
> @@ -74,7 +74,7 @@ Before the receiver driver may enable the CSI-2 transmitter by using
>  the :c:type:`v4l2_subdev_video_ops`->s_stream(), it must have powered
>  the transmitter up by using the
>  :c:type:`v4l2_subdev_core_ops`->s_power() callback. This may take
> -place either indirectly by using :c:func:`v4l2_pipeline_pm_use` or
> +place either indirectly by using :c:func:`v4l2_pipeline_pm_get` or
>  directly.
>  
>  Formats
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index ee183c35ff3b..16efd18f1e88 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1311,7 +1311,7 @@ static int isp_video_open(struct file *file)
>  		goto done;
>  	}
>  
> -	ret = v4l2_pipeline_pm_use(&video->video.entity, 1);
> +	ret = v4l2_pipeline_pm_get(&video->video.entity);
>  	if (ret < 0) {
>  		omap3isp_put(video->isp);
>  		goto done;
> @@ -1363,7 +1363,7 @@ static int isp_video_release(struct file *file)
>  	vb2_queue_release(&handle->queue);
>  	mutex_unlock(&video->queue_lock);
>  
> -	v4l2_pipeline_pm_use(&video->video.entity, 0);
> +	v4l2_pipeline_pm_put(&video->video.entity);
>  
>  	/* Release the file handle. */
>  	v4l2_fh_del(vfh);
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 1d50dfbbb762..a019dbab5e04 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -745,7 +745,7 @@ static int video_open(struct file *file)
>  
>  	file->private_data = vfh;
>  
> -	ret = v4l2_pipeline_pm_use(&vdev->entity, 1);
> +	ret = v4l2_pipeline_pm_get(&vdev->entity);
>  	if (ret < 0) {
>  		dev_err(video->camss->dev, "Failed to power up pipeline: %d\n",
>  			ret);
> @@ -771,7 +771,7 @@ static int video_release(struct file *file)
>  
>  	vb2_fop_release(file);
>  
> -	v4l2_pipeline_pm_use(&vdev->entity, 0);
> +	v4l2_pipeline_pm_put(&vdev->entity);
>  
>  	file->private_data = NULL;
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 9e2e63ffcc47..2a5be6334f72 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -826,7 +826,7 @@ static int rvin_open(struct file *file)
>  		goto err_unlock;
>  
>  	if (vin->info->use_mc)
> -		ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
> +		ret = v4l2_pipeline_pm_get(&vin->vdev.entity);
>  	else if (v4l2_fh_is_singular_file(file))
>  		ret = rvin_power_parallel(vin, true);
>  
> @@ -842,7 +842,7 @@ static int rvin_open(struct file *file)
>  	return 0;
>  err_power:
>  	if (vin->info->use_mc)
> -		v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> +		v4l2_pipeline_pm_put(&vin->vdev.entity);
>  	else if (v4l2_fh_is_singular_file(file))
>  		rvin_power_parallel(vin, false);
>  err_open:
> @@ -870,7 +870,7 @@ static int rvin_release(struct file *file)
>  	ret = _vb2_fop_release(file, NULL);
>  
>  	if (vin->info->use_mc) {
> -		v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> +		v4l2_pipeline_pm_put(&vin->vdev.entity);
>  	} else {
>  		if (fh_singular)
>  			rvin_power_parallel(vin, false);
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> index 83a3a0257c7b..8dfc2877d4c6 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> @@ -214,7 +214,7 @@ static int sun4i_csi_open(struct file *file)
>  	if (ret < 0)
>  		goto err_pm_put;
>  
> -	ret = v4l2_pipeline_pm_use(&csi->vdev.entity, 1);
> +	ret = v4l2_pipeline_pm_get(&csi->vdev.entity);
>  	if (ret)
>  		goto err_pm_put;
>  
> @@ -227,7 +227,7 @@ static int sun4i_csi_open(struct file *file)
>  	return 0;
>  
>  err_pipeline_pm_put:
> -	v4l2_pipeline_pm_use(&csi->vdev.entity, 0);
> +	v4l2_pipeline_pm_put(&csi->vdev.entity);
>  
>  err_pm_put:
>  	pm_runtime_put(csi->dev);
> @@ -243,7 +243,7 @@ static int sun4i_csi_release(struct file *file)
>  	mutex_lock(&csi->lock);
>  
>  	v4l2_fh_release(file);
> -	v4l2_pipeline_pm_use(&csi->vdev.entity, 0);
> +	v4l2_pipeline_pm_put(&csi->vdev.entity);
>  	pm_runtime_put(csi->dev);
>  
>  	mutex_unlock(&csi->lock);
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> index f0dfe68486d1..3d619ad08c9f 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> @@ -474,7 +474,7 @@ static int sun6i_video_open(struct file *file)
>  	if (ret < 0)
>  		goto unlock;
>  
> -	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
> +	ret = v4l2_pipeline_pm_get(&video->vdev.entity);
>  	if (ret < 0)
>  		goto fh_release;
>  
> @@ -507,7 +507,7 @@ static int sun6i_video_close(struct file *file)
>  
>  	_vb2_fop_release(file, NULL);
>  
> -	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
> +	v4l2_pipeline_pm_put(&video->vdev.entity);
>  
>  	if (last_fh)
>  		sun6i_csi_set_power(video->csi, false);
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 014a2a97cadd..0fffdd3ce6a4 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -321,7 +321,7 @@ EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source);
>   * use_count field stores the total number of users of all video device nodes
>   * in the pipeline.
>   *
> - * The v4l2_pipeline_pm_use() function must be called in the open() and
> + * The v4l2_pipeline_pm_{get, put}() functions must be called in the open() and
>   * close() handlers of video device nodes. It increments or decrements the use
>   * count of all subdev entities in the pipeline.
>   *
> @@ -423,7 +423,7 @@ static int pipeline_pm_power(struct media_entity *entity, int change,
>  	return ret;
>  }
>  
> -int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> +static int v4l2_pipeline_pm_use(struct media_entity *entity, unsigned int use)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;
>  	int change = use ? 1 : -1;
> @@ -444,7 +444,19 @@ int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
> +
> +int v4l2_pipeline_pm_get(struct media_entity *entity)
> +{
> +	return v4l2_pipeline_pm_use(entity, 1);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_get);
> +
> +void v4l2_pipeline_pm_put(struct media_entity *entity)
> +{
> +	/* Powering off entities shouldn't fail. */
> +	WARN_ON(v4l2_pipeline_pm_use(entity, 0));
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_put);
>  
>  int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
>  			      unsigned int notification)
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index 7712e7be8625..8aac4a3df7ca 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -643,7 +643,7 @@ static int capture_open(struct file *file)
>  	if (ret)
>  		v4l2_err(priv->src_sd, "v4l2_fh_open failed\n");
>  
> -	ret = v4l2_pipeline_pm_use(&vfd->entity, 1);
> +	ret = v4l2_pipeline_pm_get(&vfd->entity);
>  	if (ret)
>  		v4l2_fh_release(file);
>  
> @@ -664,7 +664,7 @@ static int capture_release(struct file *file)
>  		vq->owner = NULL;
>  	}
>  
> -	v4l2_pipeline_pm_use(&vfd->entity, 0);
> +	v4l2_pipeline_pm_put(&vfd->entity);
>  
>  	v4l2_fh_release(file);
>  	mutex_unlock(&priv->mutex);
> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
> index 673aa3a5f2bd..9578b8d22f25 100644
> --- a/drivers/staging/media/omap4iss/iss_video.c
> +++ b/drivers/staging/media/omap4iss/iss_video.c
> @@ -1111,7 +1111,7 @@ static int iss_video_open(struct file *file)
>  		goto done;
>  	}
>  
> -	ret = v4l2_pipeline_pm_use(&video->video.entity, 1);
> +	ret = v4l2_pipeline_pm_get(&video->video.entity);
>  	if (ret < 0) {
>  		omap4iss_put(video->iss);
>  		goto done;
> @@ -1160,7 +1160,7 @@ static int iss_video_release(struct file *file)
>  	/* Disable streaming and free the buffers queue resources. */
>  	iss_video_streamoff(file, vfh, video->type);
>  
> -	v4l2_pipeline_pm_use(&video->video.entity, 0);
> +	v4l2_pipeline_pm_put(&video->video.entity);
>  
>  	/* Release the videobuf2 queue */
>  	vb2_queue_release(&handle->queue);
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 384960249f01..5e73eb8e28f6 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -86,23 +86,30 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q);
>  
>  
>  /**
> - * v4l2_pipeline_pm_use - Update the use count of an entity
> - * @entity: The entity
> - * @use: Use (1) or stop using (0) the entity
> + * v4l2_pipeline_pm_get - Increase the use count of a pipeline
> + * @entity: The root entity of a pipeline
>   *
> - * Update the use count of all entities in the pipeline and power entities on or
> - * off accordingly.
> + * Update the use count of all entities in the pipeline and power entities on.
>   *
> - * This function is intended to be called in video node open (use ==
> - * 1) and release (use == 0). It uses struct media_entity.use_count to
> - * track the power status. The use of this function should be paired
> - * with v4l2_pipeline_link_notify().
> + * This function is intended to be called in video node open. It uses
> + * struct media_entity.use_count to track the power status. The use
> + * of this function should be paired with v4l2_pipeline_link_notify().
>   *
> - * Return 0 on success or a negative error code on failure. Powering entities
> - * off is assumed to never fail. No failure can occur when the use parameter is
> - * set to 0.
> + * Return 0 on success or a negative error code on failure.
>   */
> -int v4l2_pipeline_pm_use(struct media_entity *entity, int use);
> +int v4l2_pipeline_pm_get(struct media_entity *entity);
> +
> +/**
> + * v4l2_pipeline_pm_put - Decrease the use count of a pipeline
> + * @entity: The root entity of a pipeline
> + *
> + * Update the use count of all entities in the pipeline and power entities off.
> + *
> + * This function is intended to be called in video node release. It uses
> + * struct media_entity.use_count to track the power status. The use
> + * of this function should be paired with v4l2_pipeline_link_notify().
> + */
> +void v4l2_pipeline_pm_put(struct media_entity *entity);
>  
>  
>  /**
> @@ -114,7 +121,7 @@ int v4l2_pipeline_pm_use(struct media_entity *entity, int use);
>   * React to link management on powered pipelines by updating the use count of
>   * all entities in the source and sink sides of the link. Entities are powered
>   * on or off accordingly. The use of this function should be paired
> - * with v4l2_pipeline_pm_use().
> + * with v4l2_pipeline_pm_{get,put}().
>   *
>   * Return 0 on success or a negative error code on failure. Powering entities
>   * off is assumed to never fail. This function will not fail for disconnection
> @@ -144,11 +151,14 @@ static inline int v4l_vb2q_enable_media_source(struct vb2_queue *q)
>  	return 0;
>  }
>  
> -static inline int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> +static inline int v4l2_pipeline_pm_get(struct media_entity *entity)
>  {
>  	return 0;
>  }
>  
> +static inline void v4l2_pipeline_pm_put(struct media_entity *entity)
> +{}
> +
>  static inline int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
>  					    unsigned int notification)
>  {
> -- 
> 2.22.0
> 
>