Re: [PATCH] media: vim2m: keep transaction buffer count stable while streaming

[Nicolas Dufresne]

Hi,

Le mardi 26 mai 2026 à 21:22 +0900, Younho Choi a écrit :
> V4L2_CID_TRANS_NUM_BUFS controls how many buffer pairs a vim2m
> mem2mem job processes before the job is completed. The driver stores
> the value in ctx->translen and device_work() uses it later to decide
> whether the current transaction should continue.
> 
> Letting userspace change this control while streaming is active can
> make a queued job observe a different transaction length than the one
> it started with. That leaves the transaction state inconsistent with
> the buffers currently queued for the job.
> 
> Grab the transaction buffer count control while either queue is
> streaming, and release it only after both queues have stopped
> streaming. The V4L2 control framework then rejects changes with
> -EBUSY while the value is in use, while still allowing userspace to
> configure the value before streaming starts.
> 
> Keep the control handler alive until after v4l2_m2m_ctx_release(),
> since releasing the mem2mem context can call stop_streaming(), which
> now ungrabs the control.
> 
> Fixes: 96d8eab5d0a1 ("V4L/DVB: [v5,2/2] v4l: Add a mem-to-mem videobuf
> framework test device")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Younho Choi <gdool88@xxxxxxxxx>

I think this patch make sense, though I was pretty surprise of the private
control offset, which I've tracked down to an undocumented change made by Hans
in 2017. I've delegated this patch to him so we can sort this out first.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>

> ---
>  drivers/media/test-drivers/vim2m.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vim2m.c b/drivers/media/test-
> drivers/vim2m.c
> index bb2dd11eef0e..f4a2c4083829 100644
> --- a/drivers/media/test-drivers/vim2m.c
> +++ b/drivers/media/test-drivers/vim2m.c
> @@ -205,6 +205,7 @@ struct vim2m_ctx {
>  	struct vim2m_dev	*dev;
>  
>  	struct v4l2_ctrl_handler hdl;
> +	struct v4l2_ctrl	*trans_num_bufs_ctrl;
>  
>  	/* Processed buffers in this transaction */
>  	u8			num_processed;
> @@ -1258,9 +1259,27 @@ static int vim2m_start_streaming(struct vb2_queue *q,
> unsigned int count)
>  		ctx->aborting = 0;
>  
>  	q_data->sequence = 0;
> +	v4l2_ctrl_grab(ctx->trans_num_bufs_ctrl, true);
> +
>  	return 0;
>  }
>  
> +static bool vim2m_other_queue_is_streaming(struct vim2m_ctx *ctx,
> +					   struct vb2_queue *q)
> +{
> +	struct vb2_queue *other_vq;
> +
> +	if (!ctx->fh.m2m_ctx)
> +		return false;
> +
> +	if (V4L2_TYPE_IS_OUTPUT(q->type))
> +		other_vq = v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx);
> +	else
> +		other_vq = v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx);
> +
> +	return vb2_is_streaming(other_vq);
> +}
> +
>  static void vim2m_stop_streaming(struct vb2_queue *q)
>  {
>  	struct vim2m_ctx *ctx = vb2_get_drv_priv(q);
> @@ -1274,11 +1293,14 @@ static void vim2m_stop_streaming(struct vb2_queue *q)
>  		else
>  			vbuf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>  		if (!vbuf)
> -			return;
> +			break;
>  		v4l2_ctrl_request_complete(vbuf->vb2_buf.req_obj.req,
>  					   &ctx->hdl);
>  		v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_ERROR);
>  	}
> +
> +	if (!vim2m_other_queue_is_streaming(ctx, q))
> +		v4l2_ctrl_grab(ctx->trans_num_bufs_ctrl, false);
>  }
>  
>  static void vim2m_buf_request_complete(struct vb2_buffer *vb)
> @@ -1380,7 +1402,8 @@ static int vim2m_open(struct file *file)
>  
>  	vim2m_ctrl_trans_time_msec.def = default_transtime;
>  	v4l2_ctrl_new_custom(hdl, &vim2m_ctrl_trans_time_msec, NULL);
> -	v4l2_ctrl_new_custom(hdl, &vim2m_ctrl_trans_num_bufs, NULL);
> +	ctx->trans_num_bufs_ctrl =
> +		v4l2_ctrl_new_custom(hdl, &vim2m_ctrl_trans_num_bufs, NULL);
>  	if (hdl->error) {
>  		rc = hdl->error;
>  		v4l2_ctrl_handler_free(hdl);
> @@ -1435,10 +1458,10 @@ static int vim2m_release(struct file *file)
>  
>  	v4l2_fh_del(&ctx->fh, file);
>  	v4l2_fh_exit(&ctx->fh);
> -	v4l2_ctrl_handler_free(&ctx->hdl);
>  	mutex_lock(&dev->dev_mutex);
>  	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>  	mutex_unlock(&dev->dev_mutex);
> +	v4l2_ctrl_handler_free(&ctx->hdl);
>  	kfree(ctx);
>  
>  	atomic_dec(&dev->num_inst);
> 
> base-commit: 5d6919055dec134de3c40167a490f33c74c12581

Attachment: signature.asc
Description: This is a digitally signed message part