Re: [PATCH 06/19] media: amlogic: c3: isp: drop vb2_queue_release calls

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

Hi Hans,

Thank you for the patch.

On Fri, Jul 17, 2026 at 10:19:29AM +0200, Hans Verkuil wrote:
> Either that function was called when the video device was not
> yet registered (so nothing to release), or use vb2_video_unregister_device
> instead of the video_unregister_device/vb2_queue_release pair.
> 
> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
> Cc: Keke Li <keke.li@amlogic.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../media/platform/amlogic/c3/isp/c3-isp-capture.c    |  7 ++-----
>  drivers/media/platform/amlogic/c3/isp/c3-isp-params.c | 11 ++++-------
>  drivers/media/platform/amlogic/c3/isp/c3-isp-stats.c  |  7 ++-----
>  3 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-capture.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-capture.c
> index 11d85f5342f0..a0d92ac1b78c 100644
> --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-capture.c
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-capture.c
> @@ -720,7 +720,7 @@ static int c3_isp_register_capture(struct c3_isp_capture *cap)
>  	cap->pad.flags = MEDIA_PAD_FL_SINK;
>  	ret = media_entity_pads_init(&vdev->entity, 1, &cap->pad);
>  	if (ret)
> -		goto err_queue_release;
> +		goto err_destroy;
>  
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
> @@ -733,8 +733,6 @@ static int c3_isp_register_capture(struct c3_isp_capture *cap)
>  
>  err_entity_cleanup:
>  	media_entity_cleanup(&vdev->entity);
> -err_queue_release:
> -	vb2_queue_release(vb2_q);
>  err_destroy:
>  	mutex_destroy(&cap->lock);
>  	return ret;
> @@ -789,9 +787,8 @@ void c3_isp_captures_unregister(struct c3_isp_device *isp)
>  
>  		if (!cap->isp)
>  			continue;
> -		vb2_queue_release(&cap->vb2_q);
>  		media_entity_cleanup(&cap->vdev.entity);
> -		video_unregister_device(&cap->vdev);
> +		vb2_video_unregister_device(&cap->vdev);

Is it safe to change the order of operations here, can't driver code
called from queue release access the media entity ?

Same below.

>  		mutex_destroy(&cap->lock);
>  	}
>  }
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> index aec3eed0e443..782f18c90995 100644
> --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> @@ -876,12 +876,12 @@ int c3_isp_params_register(struct c3_isp_device *isp)
>  
>  	ret = vb2_queue_init(vb2_q);
>  	if (ret)
> -		goto err_detroy;
> +		goto err_destroy;
>  
>  	params->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&vdev->entity, 1, &params->pad);
>  	if (ret)
> -		goto err_queue_release;
> +		goto err_destroy;
>  
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret < 0) {
> @@ -894,9 +894,7 @@ int c3_isp_params_register(struct c3_isp_device *isp)
>  
>  err_entity_cleanup:
>  	media_entity_cleanup(&vdev->entity);
> -err_queue_release:
> -	vb2_queue_release(vb2_q);
> -err_detroy:
> +err_destroy:
>  	mutex_destroy(&params->lock);
>  	return ret;
>  }
> @@ -905,9 +903,8 @@ void c3_isp_params_unregister(struct c3_isp_device *isp)
>  {
>  	struct c3_isp_params *params = &isp->params;
>  
> -	vb2_queue_release(&params->vb2_q);
>  	media_entity_cleanup(&params->vdev.entity);
> -	video_unregister_device(&params->vdev);
> +	vb2_video_unregister_device(&params->vdev);
>  	mutex_destroy(&params->lock);
>  }
>  
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-stats.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-stats.c
> index 8a5d7e1a30c9..78722cbe44b4 100644
> --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-stats.c
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-stats.c
> @@ -279,7 +279,7 @@ int c3_isp_stats_register(struct c3_isp_device *isp)
>  	stats->pad.flags = MEDIA_PAD_FL_SINK;
>  	ret = media_entity_pads_init(&vdev->entity, 1, &stats->pad);
>  	if (ret)
> -		goto err_queue_release;
> +		goto err_destroy;
>  
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
> @@ -292,8 +292,6 @@ int c3_isp_stats_register(struct c3_isp_device *isp)
>  
>  err_entity_cleanup:
>  	media_entity_cleanup(&vdev->entity);
> -err_queue_release:
> -	vb2_queue_release(vb2_q);
>  err_destroy:
>  	mutex_destroy(&stats->lock);
>  	return ret;
> @@ -303,9 +301,8 @@ void c3_isp_stats_unregister(struct c3_isp_device *isp)
>  {
>  	struct c3_isp_stats *stats = &isp->stats;
>  
> -	vb2_queue_release(&stats->vb2_q);
>  	media_entity_cleanup(&stats->vdev.entity);
> -	video_unregister_device(&stats->vdev);
> +	vb2_video_unregister_device(&stats->vdev);
>  	mutex_destroy(&stats->lock);
>  }
>  

-- 
Regards,

Laurent Pinchart