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

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

[Younho Choi]

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@vger.kernel.org
Signed-off-by: Younho Choi <gdool88@mju.ac.kr>
---
 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
-- 
2.43.0

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

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4403 bytes --]

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@vger.kernel.org
> Signed-off-by: Younho Choi <gdool88@mju.ac.kr>

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@collabora.com>

> ---
>  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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[hverkuil+cisco]

On 16/07/2026 00:14, Nicolas Dufresne wrote:
> 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@vger.kernel.org
>> Signed-off-by: Younho Choi <gdool88@mju.ac.kr>
> 
> 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.

Actually, that was 2012.

I suspect that at that time we didn't yet reserve control ranges in v4l2-controls.h.

In any case, we're in a bit of luck in that the 0x1000-0x100f range is reserved in
v4l2-controls.h for the removed meye driver. Best solution is to just add a new
#define V4L2_CID_USER_VIM2M_BASE there. No control IDs have to change, so this is
safe.

I'll post a patch for that.

Regards,

	Hans

> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
>> ---
>>  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