[PATCH 0/4] media: dw100: Dynamic vertex map updates and fixes for PREEMPT_RT

[PATCH 0/4] media: dw100: Dynamic vertex map updates and fixes for PREEMPT_RT

[Stefan Klug]

Hi all,

This series contains a few updates to the dw100 dewarper. Patches 1-2
implement V4L2 requests support and dynamic updates of the vertex map.
This enables advanced use cases like interactive pan, zoom and rotate. A
first attempt to implement the dynamic update was posted a while back in
[1] but wasn't merged due to races in the implementation. By switching
to V4L2 requests the implementation is a lot simpler now.

Patches 3-4 fix an issue on PREEMPT_RT enabled kernels. These patches
should possibly be squashed into one. I expected patch 3 to be
sufficient but during testing I hit an error condition that is worked
around in patch 4. As I don't have documentation for the hardware it is
difficult to decide on the proper fix. So feedback on that is greatly
appreciated.

Best regards,
Stefan

[1] https://lore.kernel.org/linux-media/20241022063155.506191-1-umang.jain@ideasonboard.com/

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
Stefan Klug (4):
      media: dw100: Implement V4L2 requests support
      media: dw100: Implement dynamic vertex map update
      media: dw100: Fix kernel oops with PREEMPT_RT enabled
      media: dw100: Split interrupt handler to fix timeout error

 drivers/media/platform/nxp/dw100/dw100.c | 89 ++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 16 deletions(-)
---
base-commit: 9ace4753a5202b02191d54e9fdf7f9e3d02b85eb
change-id: 20260105-sklug-v6-16-topic-dw100-v3-1-dev-b856657db695

Best regards,
-- 
Stefan Klug <stefan.klug@ideasonboard.com>

[PATCH 1/4] media: dw100: Implement V4L2 requests support

[Stefan Klug]

The dw100 dewarper hardware present on the NXP i.MX8MP allows very
flexible dewarping using a freely configurable vertex map. Aside from
lens dewarping the vertex map can be used to implement things like
arbitrary zoom, pan and rotation. The current driver supports setting
that vertex map before calling VIDIOC_STREAMON.

To control above mentioned features during streaming it is necessary to
update the vertex map dynamically. To do that in a race free manner V4L2
requests support is required. This patch adds V4L2 requests support to
prepare for dynamic vertex map updates.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v1:
- Moved v4l2_ctrl_request_complete into dw100_device_run
---
 drivers/media/platform/nxp/dw100/dw100.c | 41 ++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
index 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f8003856c4fc0 100644
--- a/drivers/media/platform/nxp/dw100/dw100.c
+++ b/drivers/media/platform/nxp/dw100/dw100.c
@@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
 	return 0;
 }
 
+static int dw100_buf_out_validate(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+
+	vbuf->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
 static int dw100_buf_prepare(struct vb2_buffer *vb)
 {
 	unsigned int i;
@@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
 	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
 }
 
+static void dw100_buf_request_complete(struct vb2_buffer *vb)
+{
+	struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
+
+	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
+}
+
 static void dw100_return_all_buffers(struct vb2_queue *q,
 				     enum vb2_buffer_state state)
 {
@@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue *q)
 }
 
 static const struct vb2_ops dw100_qops = {
-	.queue_setup	 = dw100_queue_setup,
-	.buf_prepare	 = dw100_buf_prepare,
-	.buf_queue	 = dw100_buf_queue,
-	.start_streaming = dw100_start_streaming,
-	.stop_streaming  = dw100_stop_streaming,
+	.queue_setup	      = dw100_queue_setup,
+	.buf_out_validate     = dw100_buf_out_validate,
+	.buf_prepare	      = dw100_buf_prepare,
+	.buf_queue	      = dw100_buf_queue,
+	.start_streaming      = dw100_start_streaming,
+	.stop_streaming       = dw100_stop_streaming,
+	.buf_request_complete = dw100_buf_request_complete,
 };
 
 static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
@@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	src_vq->lock = &ctx->vq_mutex;
 	src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
+	src_vq->supports_requests = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
@@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
 
+	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
+				&ctx->hdl);
+
+	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
+				   &ctx->hdl);
+
 	dw100_start(ctx, src_buf, dst_buf);
 }
 
@@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops = {
 	.device_run	= dw100_device_run,
 };
 
+static const struct media_device_ops dw100_m2m_media_ops = {
+	.req_validate = vb2_request_validate,
+	.req_queue = v4l2_m2m_request_queue,
+};
+
 static struct video_device *dw100_init_video_device(struct dw100_device *dw_dev)
 {
 	struct video_device *vfd = &dw_dev->vfd;
@@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device *pdev)
 	dw_dev->mdev.dev = &pdev->dev;
 	strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
 	media_device_init(&dw_dev->mdev);
+	dw_dev->mdev.ops = &dw100_m2m_media_ops;
 	dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
 
 	ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);

-- 
2.51.0

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Nicolas Dufresne]

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

Hi,

Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> The dw100 dewarper hardware present on the NXP i.MX8MP allows very
> flexible dewarping using a freely configurable vertex map. Aside from
> lens dewarping the vertex map can be used to implement things like
> arbitrary zoom, pan and rotation. The current driver supports setting
> that vertex map before calling VIDIOC_STREAMON.
> 
> To control above mentioned features during streaming it is necessary to
> update the vertex map dynamically. To do that in a race free manner V4L2
> requests support is required. This patch adds V4L2 requests support to
> prepare for dynamic vertex map updates.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v1:
> - Moved v4l2_ctrl_request_complete into dw100_device_run
> ---
>  drivers/media/platform/nxp/dw100/dw100.c | 41 ++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> index 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f8003856c4fc0 100644
> --- a/drivers/media/platform/nxp/dw100/dw100.c
> +++ b/drivers/media/platform/nxp/dw100/dw100.c
> @@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
>  	return 0;
>  }
>  
> +static int dw100_buf_out_validate(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +
> +	vbuf->field = V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
>  static int dw100_buf_prepare(struct vb2_buffer *vb)
>  {
>  	unsigned int i;
> @@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
>  	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
>  }
>  
> +static void dw100_buf_request_complete(struct vb2_buffer *vb)
> +{
> +	struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> +
> +	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> +}
> +
>  static void dw100_return_all_buffers(struct vb2_queue *q,
>  				     enum vb2_buffer_state state)
>  {
> @@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue *q)
>  }
>  
>  static const struct vb2_ops dw100_qops = {
> -	.queue_setup	 = dw100_queue_setup,
> -	.buf_prepare	 = dw100_buf_prepare,
> -	.buf_queue	 = dw100_buf_queue,
> -	.start_streaming = dw100_start_streaming,
> -	.stop_streaming  = dw100_stop_streaming,
> +	.queue_setup	      = dw100_queue_setup,
> +	.buf_out_validate     = dw100_buf_out_validate,
> +	.buf_prepare	      = dw100_buf_prepare,
> +	.buf_queue	      = dw100_buf_queue,
> +	.start_streaming      = dw100_start_streaming,
> +	.stop_streaming       = dw100_stop_streaming,
> +	.buf_request_complete = dw100_buf_request_complete,
>  };
>  
>  static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> @@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
>  	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	src_vq->lock = &ctx->vq_mutex;
>  	src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
> +	src_vq->supports_requests = true;
>  
>  	ret = vb2_queue_init(src_vq);
>  	if (ret)
> @@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
>  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>  	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  
> +	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> +				&ctx->hdl);
> +
> +	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> +				   &ctx->hdl);

The request should always be signalled last, so nothing wrong with applying the
controls as soon as possible in this case. Complete is a bit of a miss-leading
name, this function actually changes the global controls value (apply) and
removes its participation in request completion. Since the OUTPUT buffer for
that request is still queued, the request is not signalled yet.

But you have to flip over the order to buffer signalling in dw100_job_finish()
though. My recommendation is to use the helper
v4l2_m2m_buf_done_and_job_finish(). Something like this (not tested):

   diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
   index 4aaf9c3fff53..c5f9ee238345 100644
   --- a/drivers/media/platform/nxp/dw100/dw100.c
   +++ b/drivers/media/platform/nxp/dw100/dw100.c
   @@ -1058,7 +1058,6 @@ static const struct v4l2_ioctl_ops dw100_ioctl_ops = {
    static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
    {
           struct dw100_ctx *curr_ctx;
   -       struct vb2_v4l2_buffer *src_vb, *dst_vb;
           enum vb2_buffer_state buf_state;
    
           curr_ctx = v4l2_m2m_get_curr_priv(dw_dev->m2m_dev);
   @@ -1069,16 +1068,12 @@ static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
                   return;
           }
    
   -       src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
   -       dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
   -
           if (likely(!with_error))
                   buf_state = VB2_BUF_STATE_DONE;
           else
                   buf_state = VB2_BUF_STATE_ERROR;
    
   -       v4l2_m2m_buf_done(src_vb, buf_state);
   -       v4l2_m2m_buf_done(dst_vb, buf_state);
   +       v4l2_m2m_buf_done_and_job_finish(dw_dev->m2m_dev, buf_state);
    
           dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s error(s)\n",
                   with_error ? "" : "out");
   
You might be tempted to keep the logical order, and move the
v4l2_ctrl_request_complete() call into dw100_job_finish(), unfortunately this
does not work, since nothing mandate that any control was included in this
request, and that will lead to calling v4l2_ctrl_request_complete() on an
already completed request. Since its a single function HW, I don't see why you'd
want this, but it would require the manual request completion.

> +
>  	dw100_start(ctx, src_buf, dst_buf);

nit: I really don't see why this is a separate function ...

>  }
>  
> @@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops = {
>  	.device_run	= dw100_device_run,
>  };
>  
> +static const struct media_device_ops dw100_m2m_media_ops = {
> +	.req_validate = vb2_request_validate,
> +	.req_queue = v4l2_m2m_request_queue,
> +};
> +
>  static struct video_device *dw100_init_video_device(struct dw100_device *dw_dev)
>  {
>  	struct video_device *vfd = &dw_dev->vfd;
> @@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device *pdev)
>  	dw_dev->mdev.dev = &pdev->dev;
>  	strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
>  	media_device_init(&dw_dev->mdev);
> +	dw_dev->mdev.ops = &dw100_m2m_media_ops;
>  	dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
>  
>  	ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);

regards,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 01:46:46PM -0500, Nicolas Dufresne wrote:
> Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > The dw100 dewarper hardware present on the NXP i.MX8MP allows very
> > flexible dewarping using a freely configurable vertex map. Aside from
> > lens dewarping the vertex map can be used to implement things like
> > arbitrary zoom, pan and rotation. The current driver supports setting
> > that vertex map before calling VIDIOC_STREAMON.
> > 
> > To control above mentioned features during streaming it is necessary to
> > update the vertex map dynamically. To do that in a race free manner V4L2
> > requests support is required. This patch adds V4L2 requests support to
> > prepare for dynamic vertex map updates.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v1:
> > - Moved v4l2_ctrl_request_complete into dw100_device_run
> > ---
> >  drivers/media/platform/nxp/dw100/dw100.c | 41 ++++++++++++++++++++++++++++----
> >  1 file changed, 36 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> > index 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f8003856c4fc0 100644
> > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > @@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
> >  	return 0;
> >  }
> >  
> > +static int dw100_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > +	vbuf->field = V4L2_FIELD_NONE;
> > +
> > +	return 0;
> > +}

Stefan, how is this related to requests support ?

> > +
> >  static int dw100_buf_prepare(struct vb2_buffer *vb)
> >  {
> >  	unsigned int i;
> > @@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
> >  	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> >  }
> >  
> > +static void dw100_buf_request_complete(struct vb2_buffer *vb)
> > +{
> > +	struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > +	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);

Unless I'm missing something, this will call
v4l2_ctrl_request_complete() twice, once on each of the source and
destination buffers, for the same request and control handler. Is that
desired ?

> > +}
> > +
> >  static void dw100_return_all_buffers(struct vb2_queue *q,
> >  				     enum vb2_buffer_state state)
> >  {
> > @@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue *q)
> >  }
> >  
> >  static const struct vb2_ops dw100_qops = {
> > -	.queue_setup	 = dw100_queue_setup,
> > -	.buf_prepare	 = dw100_buf_prepare,
> > -	.buf_queue	 = dw100_buf_queue,
> > -	.start_streaming = dw100_start_streaming,
> > -	.stop_streaming  = dw100_stop_streaming,
> > +	.queue_setup	      = dw100_queue_setup,
> > +	.buf_out_validate     = dw100_buf_out_validate,
> > +	.buf_prepare	      = dw100_buf_prepare,
> > +	.buf_queue	      = dw100_buf_queue,
> > +	.start_streaming      = dw100_start_streaming,
> > +	.stop_streaming       = dw100_stop_streaming,
> > +	.buf_request_complete = dw100_buf_request_complete,
> >  };
> >  
> >  static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > @@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> >  	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >  	src_vq->lock = &ctx->vq_mutex;
> >  	src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
> > +	src_vq->supports_requests = true;
> >  
> >  	ret = vb2_queue_init(src_vq);
> >  	if (ret)
> > @@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
> >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> >  	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> >  
> > +	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > +				&ctx->hdl);
> > +
> > +	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > +				   &ctx->hdl);
> 
> The request should always be signalled last, so nothing wrong with applying the
> controls as soon as possible in this case. Complete is a bit of a miss-leading
> name, this function actually changes the global controls value (apply) and
> removes its participation in request completion. Since the OUTPUT buffer for
> that request is still queued, the request is not signalled yet.

I'm very confused here. As far as I can tell,
v4l2_ctrl_request_complete() doesn't apply controls (i.e. cause
.s_ctrl() to be called), it copies the value of controls back to the
request to report them to the application. Am I missing something ?

As there's nothing to report back to the application (no volatile
control whose value will come from the hardware), calling
v4l2_ctrl_request_complete() here seems fine to me. Is that what you
were trying to explain ?

> But you have to flip over the order to buffer signalling in dw100_job_finish()
> though. My recommendation is to use the helper
> v4l2_m2m_buf_done_and_job_finish(). Something like this (not tested):
> 
>    diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
>    index 4aaf9c3fff53..c5f9ee238345 100644
>    --- a/drivers/media/platform/nxp/dw100/dw100.c
>    +++ b/drivers/media/platform/nxp/dw100/dw100.c
>    @@ -1058,7 +1058,6 @@ static const struct v4l2_ioctl_ops dw100_ioctl_ops = {
>     static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
>     {
>            struct dw100_ctx *curr_ctx;
>    -       struct vb2_v4l2_buffer *src_vb, *dst_vb;
>            enum vb2_buffer_state buf_state;
>     
>            curr_ctx = v4l2_m2m_get_curr_priv(dw_dev->m2m_dev);
>    @@ -1069,16 +1068,12 @@ static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
>                    return;
>            }
>     
>    -       src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
>    -       dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
>    -
>            if (likely(!with_error))
>                    buf_state = VB2_BUF_STATE_DONE;
>            else
>                    buf_state = VB2_BUF_STATE_ERROR;
>     
>    -       v4l2_m2m_buf_done(src_vb, buf_state);
>    -       v4l2_m2m_buf_done(dst_vb, buf_state);
>    +       v4l2_m2m_buf_done_and_job_finish(dw_dev->m2m_dev, buf_state);
>     
>            dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s error(s)\n",
>                    with_error ? "" : "out");
>    
> You might be tempted to keep the logical order, and move the
> v4l2_ctrl_request_complete() call into dw100_job_finish(), unfortunately this
> does not work, since nothing mandate that any control was included in this
> request, and that will lead to calling v4l2_ctrl_request_complete() on an
> already completed request. Since its a single function HW, I don't see why you'd
> want this, but it would require the manual request completion.
> 
> > +
> >  	dw100_start(ctx, src_buf, dst_buf);
> 
> nit: I really don't see why this is a separate function ...
> 
> >  }
> >  
> > @@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops = {
> >  	.device_run	= dw100_device_run,
> >  };
> >  
> > +static const struct media_device_ops dw100_m2m_media_ops = {
> > +	.req_validate = vb2_request_validate,
> > +	.req_queue = v4l2_m2m_request_queue,
> > +};
> > +
> >  static struct video_device *dw100_init_video_device(struct dw100_device *dw_dev)
> >  {
> >  	struct video_device *vfd = &dw_dev->vfd;
> > @@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device *pdev)
> >  	dw_dev->mdev.dev = &pdev->dev;
> >  	strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
> >  	media_device_init(&dw_dev->mdev);
> > +	dw_dev->mdev.ops = &dw100_m2m_media_ops;
> >  	dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
> >  
> >  	ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Stefan Klug]

Hi Nicolas, hi Laurent,

Thank you for the review.

Quoting Laurent Pinchart (2026-01-06 01:33:02)
> On Mon, Jan 05, 2026 at 01:46:46PM -0500, Nicolas Dufresne wrote:
> > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > The dw100 dewarper hardware present on the NXP i.MX8MP allows very
> > > flexible dewarping using a freely configurable vertex map. Aside from
> > > lens dewarping the vertex map can be used to implement things like
> > > arbitrary zoom, pan and rotation. The current driver supports setting
> > > that vertex map before calling VIDIOC_STREAMON.
> > > 
> > > To control above mentioned features during streaming it is necessary to
> > > update the vertex map dynamically. To do that in a race free manner V4L2
> > > requests support is required. This patch adds V4L2 requests support to
> > > prepare for dynamic vertex map updates.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v1:
> > > - Moved v4l2_ctrl_request_complete into dw100_device_run
> > > ---
> > >  drivers/media/platform/nxp/dw100/dw100.c | 41 ++++++++++++++++++++++++++++----
> > >  1 file changed, 36 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> > > index 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f8003856c4fc0 100644
> > > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > > @@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
> > >     return 0;
> > >  }
> > >  
> > > +static int dw100_buf_out_validate(struct vb2_buffer *vb)
> > > +{
> > > +   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > +
> > > +   vbuf->field = V4L2_FIELD_NONE;
> > > +
> > > +   return 0;
> > > +}
> 
> Stefan, how is this related to requests support ?

vb2_queue_or_prepare_buf() errors out if this is not implemented and the
buffer uses requests. This was the implementation I saw on most drivers.
And as I don't expect anyone to try to use the dewarper with interleaved
data I didn't bother to add a warning.

> 
> > > +
> > >  static int dw100_buf_prepare(struct vb2_buffer *vb)
> > >  {
> > >     unsigned int i;
> > > @@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
> > >     v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > >  }
> > >  
> > > +static void dw100_buf_request_complete(struct vb2_buffer *vb)
> > > +{
> > > +   struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > +
> > > +   v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> 
> Unless I'm missing something, this will call
> v4l2_ctrl_request_complete() twice, once on each of the source and
> destination buffers, for the same request and control handler. Is that
> desired ?

The docs say "a buffer that was never queued to the driver but is
associated with a queued request was canceled..." So to my understanding
the only purpose of this function is to mark all controls in the request
as being handled, so that the request can be finished.

All the implementations I found in the kernel are exactly the same and
are to my understanding only necessary to map from a vb2_buffer to a
v4l2_ctrl_handler (which is then passed to v4l2_ctrl_request_complete())

> 
> > > +}
> > > +
> > >  static void dw100_return_all_buffers(struct vb2_queue *q,
> > >                                  enum vb2_buffer_state state)
> > >  {
> > > @@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue *q)
> > >  }
> > >  
> > >  static const struct vb2_ops dw100_qops = {
> > > -   .queue_setup     = dw100_queue_setup,
> > > -   .buf_prepare     = dw100_buf_prepare,
> > > -   .buf_queue       = dw100_buf_queue,
> > > -   .start_streaming = dw100_start_streaming,
> > > -   .stop_streaming  = dw100_stop_streaming,
> > > +   .queue_setup          = dw100_queue_setup,
> > > +   .buf_out_validate     = dw100_buf_out_validate,
> > > +   .buf_prepare          = dw100_buf_prepare,
> > > +   .buf_queue            = dw100_buf_queue,
> > > +   .start_streaming      = dw100_start_streaming,
> > > +   .stop_streaming       = dw100_stop_streaming,
> > > +   .buf_request_complete = dw100_buf_request_complete,
> > >  };
> > >  
> > >  static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > > @@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > >     src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > >     src_vq->lock = &ctx->vq_mutex;
> > >     src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
> > > +   src_vq->supports_requests = true;
> > >  
> > >     ret = vb2_queue_init(src_vq);
> > >     if (ret)
> > > @@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
> > >     src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > >     dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > >  
> > > +   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > +                           &ctx->hdl);
> > > +
> > > +   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > +                              &ctx->hdl);
> > 
> > The request should always be signalled last, so nothing wrong with applying the
> > controls as soon as possible in this case. Complete is a bit of a miss-leading
> > name, this function actually changes the global controls value (apply) and
> > removes its participation in request completion. Since the OUTPUT buffer for
> > that request is still queued, the request is not signalled yet.
> 
> I'm very confused here. As far as I can tell,
> v4l2_ctrl_request_complete() doesn't apply controls (i.e. cause
> .s_ctrl() to be called), it copies the value of controls back to the
> request to report them to the application. Am I missing something ?
> 
> As there's nothing to report back to the application (no volatile
> control whose value will come from the hardware), calling
> v4l2_ctrl_request_complete() here seems fine to me. Is that what you
> were trying to explain ?

I think that was meant, yes (see next comment)

> 
> > But you have to flip over the order to buffer signalling in dw100_job_finish()
> > though. My recommendation is to use the helper
> > v4l2_m2m_buf_done_and_job_finish(). Something like this (not tested):
> > 
> >    diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> >    index 4aaf9c3fff53..c5f9ee238345 100644
> >    --- a/drivers/media/platform/nxp/dw100/dw100.c
> >    +++ b/drivers/media/platform/nxp/dw100/dw100.c
> >    @@ -1058,7 +1058,6 @@ static const struct v4l2_ioctl_ops dw100_ioctl_ops = {
> >     static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
> >     {
> >            struct dw100_ctx *curr_ctx;
> >    -       struct vb2_v4l2_buffer *src_vb, *dst_vb;
> >            enum vb2_buffer_state buf_state;
> >     
> >            curr_ctx = v4l2_m2m_get_curr_priv(dw_dev->m2m_dev);
> >    @@ -1069,16 +1068,12 @@ static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
> >                    return;
> >            }
> >     
> >    -       src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
> >    -       dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
> >    -
> >            if (likely(!with_error))
> >                    buf_state = VB2_BUF_STATE_DONE;
> >            else
> >                    buf_state = VB2_BUF_STATE_ERROR;
> >     
> >    -       v4l2_m2m_buf_done(src_vb, buf_state);
> >    -       v4l2_m2m_buf_done(dst_vb, buf_state);
> >    +       v4l2_m2m_buf_done_and_job_finish(dw_dev->m2m_dev, buf_state);
> >     
> >            dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s error(s)\n",
> >                    with_error ? "" : "out");
> >    
> > You might be tempted to keep the logical order, and move the
> > v4l2_ctrl_request_complete() call into dw100_job_finish(), unfortunately this
> > does not work, since nothing mandate that any control was included in this
> > request, and that will lead to calling v4l2_ctrl_request_complete() on an
> > already completed request. Since its a single function HW, I don't see why you'd
> > want this, but it would require the manual request completion.
> > 

Nicolas, if I go you right you mean that one might be tempted to do

v4l2_ctrl_request_setup()
v4l2_m2m_buf_done(src)
v4l2_m2m_buf_done(dst)
v4l2_ctrl_request_complete()

which feels natural from the names of the functions, but the
v4l2_m2m_buf_done(src) might complete the request if it has no
associated controls and therefore the later v4l2_ctrl_request_complete()
would fail...

I see that the usage of v4l2_m2m_buf_done_and_job_finish() is more
compact and will use that in v2. For the sake of understanding: The only
functional issue with my code is that v4l2_m2m_buf_done(src_buf) is
called before v4l2_m2m_buf_done(dest_buf), right?

> > > +
> > >     dw100_start(ctx, src_buf, dst_buf);
> > 
> > nit: I really don't see why this is a separate function ...

I wondered that also, but didn't want to mess too much with existing
code. Maybe as a fixup on top?

Best regards,
Stefan

> > 
> > >  }
> > >  
> > > @@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops = {
> > >     .device_run     = dw100_device_run,
> > >  };
> > >  
> > > +static const struct media_device_ops dw100_m2m_media_ops = {
> > > +   .req_validate = vb2_request_validate,
> > > +   .req_queue = v4l2_m2m_request_queue,
> > > +};
> > > +
> > >  static struct video_device *dw100_init_video_device(struct dw100_device *dw_dev)
> > >  {
> > >     struct video_device *vfd = &dw_dev->vfd;
> > > @@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device *pdev)
> > >     dw_dev->mdev.dev = &pdev->dev;
> > >     strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
> > >     media_device_init(&dw_dev->mdev);
> > > +   dw_dev->mdev.ops = &dw100_m2m_media_ops;
> > >     dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
> > >  
> > >     ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Nicolas Dufresne]

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

Le mardi 06 janvier 2026 à 15:16 +0100, Stefan Klug a écrit :
> Hi Nicolas, hi Laurent,
> 
> Thank you for the review.
> 
> Quoting Laurent Pinchart (2026-01-06 01:33:02)
> > On Mon, Jan 05, 2026 at 01:46:46PM -0500, Nicolas Dufresne wrote:
> > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > The dw100 dewarper hardware present on the NXP i.MX8MP allows very
> > > > flexible dewarping using a freely configurable vertex map. Aside from
> > > > lens dewarping the vertex map can be used to implement things like
> > > > arbitrary zoom, pan and rotation. The current driver supports setting
> > > > that vertex map before calling VIDIOC_STREAMON.
> > > > 
> > > > To control above mentioned features during streaming it is necessary to
> > > > update the vertex map dynamically. To do that in a race free manner V4L2
> > > > requests support is required. This patch adds V4L2 requests support to
> > > > prepare for dynamic vertex map updates.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > 
> > > > ---
> > > > 
> > > > Changes in v1:
> > > > - Moved v4l2_ctrl_request_complete into dw100_device_run
> > > > ---
> > > >  drivers/media/platform/nxp/dw100/dw100.c | 41
> > > > ++++++++++++++++++++++++++++----
> > > >  1 file changed, 36 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> > > > b/drivers/media/platform/nxp/dw100/dw100.c
> > > > index
> > > > 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f80
> > > > 03856c4fc0 100644
> > > > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > > > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > > > @@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
> > > >     return 0;
> > > >  }
> > > >  
> > > > +static int dw100_buf_out_validate(struct vb2_buffer *vb)
> > > > +{
> > > > +   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > +
> > > > +   vbuf->field = V4L2_FIELD_NONE;
> > > > +
> > > > +   return 0;
> > > > +}
> > 
> > Stefan, how is this related to requests support ?
> 
> vb2_queue_or_prepare_buf() errors out if this is not implemented and the
> buffer uses requests. This was the implementation I saw on most drivers.
> And as I don't expect anyone to try to use the dewarper with interleaved
> data I didn't bother to add a warning.

I'm stumbled on this one too, but I completely forgot what this callback was
about and assume something like this. I suspect it might be some left over from
when we required all request to hold all the controls, we should find a way to
make that optional, and sort out this field thing, but I'd say its not a problem
for this serie.

> 
> > 
> > > > +
> > > >  static int dw100_buf_prepare(struct vb2_buffer *vb)
> > > >  {
> > > >     unsigned int i;
> > > > @@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
> > > >     v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > > >  }
> > > >  
> > > > +static void dw100_buf_request_complete(struct vb2_buffer *vb)
> > > > +{
> > > > +   struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > > +
> > > > +   v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > 
> > Unless I'm missing something, this will call
> > v4l2_ctrl_request_complete() twice, once on each of the source and
> > destination buffers, for the same request and control handler. Is that
> > desired ?
> 
> The docs say "a buffer that was never queued to the driver but is
> associated with a queued request was canceled..." So to my understanding
> the only purpose of this function is to mark all controls in the request
> as being handled, so that the request can be finished.
> 
> All the implementations I found in the kernel are exactly the same and
> are to my understanding only necessary to map from a vb2_buffer to a
> v4l2_ctrl_handler (which is then passed to v4l2_ctrl_request_complete())

vb2 does not have access to the control handler, so it cannot call
v4l2_ctrl_request_complete() when the request is never passed to the driver.
This is only called if the request is being discarded in the background. There
is certainly room for improvement here.

> 
> > 
> > > > +}
> > > > +
> > > >  static void dw100_return_all_buffers(struct vb2_queue *q,
> > > >                                  enum vb2_buffer_state state)
> > > >  {
> > > > @@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue
> > > > *q)
> > > >  }
> > > >  
> > > >  static const struct vb2_ops dw100_qops = {
> > > > -   .queue_setup     = dw100_queue_setup,
> > > > -   .buf_prepare     = dw100_buf_prepare,
> > > > -   .buf_queue       = dw100_buf_queue,
> > > > -   .start_streaming = dw100_start_streaming,
> > > > -   .stop_streaming  = dw100_stop_streaming,
> > > > +   .queue_setup          = dw100_queue_setup,
> > > > +   .buf_out_validate     = dw100_buf_out_validate,
> > > > +   .buf_prepare          = dw100_buf_prepare,
> > > > +   .buf_queue            = dw100_buf_queue,
> > > > +   .start_streaming      = dw100_start_streaming,
> > > > +   .stop_streaming       = dw100_stop_streaming,
> > > > +   .buf_request_complete = dw100_buf_request_complete,
> > > >  };
> > > >  
> > > >  static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > > > @@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct
> > > > vb2_queue *src_vq,
> > > >     src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > > >     src_vq->lock = &ctx->vq_mutex;
> > > >     src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
> > > > +   src_vq->supports_requests = true;
> > > >  
> > > >     ret = vb2_queue_init(src_vq);
> > > >     if (ret)
> > > > @@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
> > > >     src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > > >     dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > > >  
> > > > +   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > > +                           &ctx->hdl);
> > > > +
> > > > +   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > > +                              &ctx->hdl);
> > > 
> > > The request should always be signalled last, so nothing wrong with
> > > applying the
> > > controls as soon as possible in this case. Complete is a bit of a miss-
> > > leading
> > > name, this function actually changes the global controls value (apply) and
> > > removes its participation in request completion. Since the OUTPUT buffer
> > > for
> > > that request is still queued, the request is not signalled yet.
> > 
> > I'm very confused here. As far as I can tell,
> > v4l2_ctrl_request_complete() doesn't apply controls (i.e. cause
> > .s_ctrl() to be called), it copies the value of controls back to the
> > request to report them to the application. Am I missing something ?
> > 
> > As there's nothing to report back to the application (no volatile
> > control whose value will come from the hardware), calling
> > v4l2_ctrl_request_complete() here seems fine to me. Is that what you
> > were trying to explain ?
> 
> I think that was meant, yes (see next comment)
> 
> > 
> > > But you have to flip over the order to buffer signalling in
> > > dw100_job_finish()
> > > though. My recommendation is to use the helper
> > > v4l2_m2m_buf_done_and_job_finish(). Something like this (not tested):
> > > 
> > >    diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> > > b/drivers/media/platform/nxp/dw100/dw100.c
> > >    index 4aaf9c3fff53..c5f9ee238345 100644
> > >    --- a/drivers/media/platform/nxp/dw100/dw100.c
> > >    +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > >    @@ -1058,7 +1058,6 @@ static const struct v4l2_ioctl_ops
> > > dw100_ioctl_ops = {
> > >     static void dw100_job_finish(struct dw100_device *dw_dev, bool
> > > with_error)
> > >     {
> > >            struct dw100_ctx *curr_ctx;
> > >    -       struct vb2_v4l2_buffer *src_vb, *dst_vb;
> > >            enum vb2_buffer_state buf_state;
> > >     
> > >            curr_ctx = v4l2_m2m_get_curr_priv(dw_dev->m2m_dev);
> > >    @@ -1069,16 +1068,12 @@ static void dw100_job_finish(struct
> > > dw100_device *dw_dev, bool with_error)
> > >                    return;
> > >            }
> > >     
> > >    -       src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
> > >    -       dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
> > >    -
> > >            if (likely(!with_error))
> > >                    buf_state = VB2_BUF_STATE_DONE;
> > >            else
> > >                    buf_state = VB2_BUF_STATE_ERROR;
> > >     
> > >    -       v4l2_m2m_buf_done(src_vb, buf_state);
> > >    -       v4l2_m2m_buf_done(dst_vb, buf_state);
> > >    +       v4l2_m2m_buf_done_and_job_finish(dw_dev->m2m_dev, buf_state);
> > >     
> > >            dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s
> > > error(s)\n",
> > >                    with_error ? "" : "out");
> > >    
> > > You might be tempted to keep the logical order, and move the
> > > v4l2_ctrl_request_complete() call into dw100_job_finish(), unfortunately
> > > this
> > > does not work, since nothing mandate that any control was included in this
> > > request, and that will lead to calling v4l2_ctrl_request_complete() on an
> > > already completed request. Since its a single function HW, I don't see why
> > > you'd
> > > want this, but it would require the manual request completion.
> > > 
> 
> Nicolas, if I go you right you mean that one might be tempted to do
> 
> v4l2_ctrl_request_setup()
> v4l2_m2m_buf_done(src)
> v4l2_m2m_buf_done(dst)
> v4l2_ctrl_request_complete()
> 
> which feels natural from the names of the functions, but the
> v4l2_m2m_buf_done(src) might complete the request if it has no
> associated controls and therefore the later v4l2_ctrl_request_complete()
> would fail...
> 
> I see that the usage of v4l2_m2m_buf_done_and_job_finish() is more
> compact and will use that in v2. For the sake of understanding: The only
> functional issue with my code is that v4l2_m2m_buf_done(src_buf) is
> called before v4l2_m2m_buf_done(dest_buf), right?

Correct, swapping these two would work too, but as you said, I proposed
v4l2_m2m_buf_done_and_job_finish() simply because it is more compact and fits
your use case. The extra code in the helper is for capture buffer holding, which
at least for now does not apply to you.

> 
> > > > +
> > > >     dw100_start(ctx, src_buf, dst_buf);
> > > 
> > > nit: I really don't see why this is a separate function ...
> 
> I wondered that also, but didn't want to mess too much with existing
> code. Maybe as a fixup on top?

Feel free to do so.

Nicolas

> 
> Best regards,
> Stefan
> 
> > > 
> > > >  }
> > > >  
> > > > @@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops =
> > > > {
> > > >     .device_run     = dw100_device_run,
> > > >  };
> > > >  
> > > > +static const struct media_device_ops dw100_m2m_media_ops = {
> > > > +   .req_validate = vb2_request_validate,
> > > > +   .req_queue = v4l2_m2m_request_queue,
> > > > +};
> > > > +
> > > >  static struct video_device *dw100_init_video_device(struct dw100_device
> > > > *dw_dev)
> > > >  {
> > > >     struct video_device *vfd = &dw_dev->vfd;
> > > > @@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device
> > > > *pdev)
> > > >     dw_dev->mdev.dev = &pdev->dev;
> > > >     strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
> > > >     media_device_init(&dw_dev->mdev);
> > > > +   dw_dev->mdev.ops = &dw100_m2m_media_ops;
> > > >     dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
> > > >  
> > > >     ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Laurent Pinchart]

(CC'ing Hans)

On Tue, Jan 06, 2026 at 09:35:19AM -0500, Nicolas Dufresne wrote:
> Le mardi 06 janvier 2026 à 15:16 +0100, Stefan Klug a écrit :
> > Hi Nicolas, hi Laurent,
> > 
> > Thank you for the review.
> > 
> > Quoting Laurent Pinchart (2026-01-06 01:33:02)
> > > On Mon, Jan 05, 2026 at 01:46:46PM -0500, Nicolas Dufresne wrote:
> > > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > > The dw100 dewarper hardware present on the NXP i.MX8MP allows very
> > > > > flexible dewarping using a freely configurable vertex map. Aside from
> > > > > lens dewarping the vertex map can be used to implement things like
> > > > > arbitrary zoom, pan and rotation. The current driver supports setting
> > > > > that vertex map before calling VIDIOC_STREAMON.
> > > > > 
> > > > > To control above mentioned features during streaming it is necessary to
> > > > > update the vertex map dynamically. To do that in a race free manner V4L2
> > > > > requests support is required. This patch adds V4L2 requests support to
> > > > > prepare for dynamic vertex map updates.
> > > > > 
> > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Changes in v1:
> > > > > - Moved v4l2_ctrl_request_complete into dw100_device_run
> > > > > ---
> > > > >  drivers/media/platform/nxp/dw100/dw100.c | 41
> > > > > ++++++++++++++++++++++++++++----
> > > > >  1 file changed, 36 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> > > > > b/drivers/media/platform/nxp/dw100/dw100.c
> > > > > index
> > > > > 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f80
> > > > > 03856c4fc0 100644
> > > > > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > > > > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > > > > @@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
> > > > >     return 0;
> > > > >  }
> > > > >  
> > > > > +static int dw100_buf_out_validate(struct vb2_buffer *vb)
> > > > > +{
> > > > > +   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > +
> > > > > +   vbuf->field = V4L2_FIELD_NONE;
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > 
> > > Stefan, how is this related to requests support ?
> > 
> > vb2_queue_or_prepare_buf() errors out if this is not implemented and the
> > buffer uses requests. This was the implementation I saw on most drivers.
> > And as I don't expect anyone to try to use the dewarper with interleaved
> > data I didn't bother to add a warning.
> 
> I'm stumbled on this one too, but I completely forgot what this callback was
> about and assume something like this. I suspect it might be some left over from
> when we required all request to hold all the controls, we should find a way to
> make that optional, and sort out this field thing, but I'd say its not a problem
> for this serie.

Definitely not a problem for this series :-) Please see my other reply
about using .buf_prepare(), let's avoid splitting the discussion.

> > > > > +
> > > > >  static int dw100_buf_prepare(struct vb2_buffer *vb)
> > > > >  {
> > > > >     unsigned int i;
> > > > > @@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
> > > > >     v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > > > >  }
> > > > >  
> > > > > +static void dw100_buf_request_complete(struct vb2_buffer *vb)
> > > > > +{
> > > > > +   struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > > > +
> > > > > +   v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > > 
> > > Unless I'm missing something, this will call
> > > v4l2_ctrl_request_complete() twice, once on each of the source and
> > > destination buffers, for the same request and control handler. Is that
> > > desired ?
> > 
> > The docs say "a buffer that was never queued to the driver but is
> > associated with a queued request was canceled..." So to my understanding
> > the only purpose of this function is to mark all controls in the request
> > as being handled, so that the request can be finished.
> > 
> > All the implementations I found in the kernel are exactly the same and
> > are to my understanding only necessary to map from a vb2_buffer to a
> > v4l2_ctrl_handler (which is then passed to v4l2_ctrl_request_complete())
> 
> vb2 does not have access to the control handler, so it cannot call
> v4l2_ctrl_request_complete() when the request is never passed to the driver.
> This is only called if the request is being discarded in the background. There
> is certainly room for improvement here.

A media_request_object_ops operation could be a better candidate to
complete controls. This will likely require rethinking how cancellation
is implemented, as it still couldn't go through vb2 (otherwise the new
operation would still be called once for each queue, which isn't right).

> > > > > +}
> > > > > +
> > > > >  static void dw100_return_all_buffers(struct vb2_queue *q,
> > > > >                                  enum vb2_buffer_state state)
> > > > >  {
> > > > > @@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue
> > > > > *q)
> > > > >  }
> > > > >  
> > > > >  static const struct vb2_ops dw100_qops = {
> > > > > -   .queue_setup     = dw100_queue_setup,
> > > > > -   .buf_prepare     = dw100_buf_prepare,
> > > > > -   .buf_queue       = dw100_buf_queue,
> > > > > -   .start_streaming = dw100_start_streaming,
> > > > > -   .stop_streaming  = dw100_stop_streaming,
> > > > > +   .queue_setup          = dw100_queue_setup,
> > > > > +   .buf_out_validate     = dw100_buf_out_validate,
> > > > > +   .buf_prepare          = dw100_buf_prepare,
> > > > > +   .buf_queue            = dw100_buf_queue,
> > > > > +   .start_streaming      = dw100_start_streaming,
> > > > > +   .stop_streaming       = dw100_stop_streaming,
> > > > > +   .buf_request_complete = dw100_buf_request_complete,
> > > > >  };
> > > > >  
> > > > >  static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > > > > @@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct
> > > > > vb2_queue *src_vq,
> > > > >     src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > > > >     src_vq->lock = &ctx->vq_mutex;
> > > > >     src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
> > > > > +   src_vq->supports_requests = true;
> > > > >  
> > > > >     ret = vb2_queue_init(src_vq);
> > > > >     if (ret)
> > > > > @@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
> > > > >     src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > > > >     dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > > > >  
> > > > > +   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > > > +                           &ctx->hdl);
> > > > > +
> > > > > +   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > > > +                              &ctx->hdl);
> > > > 
> > > > The request should always be signalled last, so nothing wrong with
> > > > applying the
> > > > controls as soon as possible in this case. Complete is a bit of a miss-
> > > > leading
> > > > name, this function actually changes the global controls value (apply) and
> > > > removes its participation in request completion. Since the OUTPUT buffer
> > > > for
> > > > that request is still queued, the request is not signalled yet.
> > > 
> > > I'm very confused here. As far as I can tell,
> > > v4l2_ctrl_request_complete() doesn't apply controls (i.e. cause
> > > .s_ctrl() to be called), it copies the value of controls back to the
> > > request to report them to the application. Am I missing something ?
> > > 
> > > As there's nothing to report back to the application (no volatile
> > > control whose value will come from the hardware), calling
> > > v4l2_ctrl_request_complete() here seems fine to me. Is that what you
> > > were trying to explain ?
> > 
> > I think that was meant, yes (see next comment)
> > 
> > > 
> > > > But you have to flip over the order to buffer signalling in
> > > > dw100_job_finish()
> > > > though. My recommendation is to use the helper
> > > > v4l2_m2m_buf_done_and_job_finish(). Something like this (not tested):
> > > > 
> > > >    diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> > > > b/drivers/media/platform/nxp/dw100/dw100.c
> > > >    index 4aaf9c3fff53..c5f9ee238345 100644
> > > >    --- a/drivers/media/platform/nxp/dw100/dw100.c
> > > >    +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > > >    @@ -1058,7 +1058,6 @@ static const struct v4l2_ioctl_ops
> > > > dw100_ioctl_ops = {
> > > >     static void dw100_job_finish(struct dw100_device *dw_dev, bool
> > > > with_error)
> > > >     {
> > > >            struct dw100_ctx *curr_ctx;
> > > >    -       struct vb2_v4l2_buffer *src_vb, *dst_vb;
> > > >            enum vb2_buffer_state buf_state;
> > > >     
> > > >            curr_ctx = v4l2_m2m_get_curr_priv(dw_dev->m2m_dev);
> > > >    @@ -1069,16 +1068,12 @@ static void dw100_job_finish(struct
> > > > dw100_device *dw_dev, bool with_error)
> > > >                    return;
> > > >            }
> > > >     
> > > >    -       src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
> > > >    -       dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
> > > >    -
> > > >            if (likely(!with_error))
> > > >                    buf_state = VB2_BUF_STATE_DONE;
> > > >            else
> > > >                    buf_state = VB2_BUF_STATE_ERROR;
> > > >     
> > > >    -       v4l2_m2m_buf_done(src_vb, buf_state);
> > > >    -       v4l2_m2m_buf_done(dst_vb, buf_state);
> > > >    +       v4l2_m2m_buf_done_and_job_finish(dw_dev->m2m_dev, buf_state);
> > > >     
> > > >            dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s
> > > > error(s)\n",
> > > >                    with_error ? "" : "out");
> > > >    
> > > > You might be tempted to keep the logical order, and move the
> > > > v4l2_ctrl_request_complete() call into dw100_job_finish(), unfortunately
> > > > this
> > > > does not work, since nothing mandate that any control was included in this
> > > > request, and that will lead to calling v4l2_ctrl_request_complete() on an
> > > > already completed request. Since its a single function HW, I don't see why
> > > > you'd
> > > > want this, but it would require the manual request completion.
> > 
> > Nicolas, if I go you right you mean that one might be tempted to do
> > 
> > v4l2_ctrl_request_setup()
> > v4l2_m2m_buf_done(src)
> > v4l2_m2m_buf_done(dst)
> > v4l2_ctrl_request_complete()
> > 
> > which feels natural from the names of the functions, but the
> > v4l2_m2m_buf_done(src) might complete the request if it has no
> > associated controls and therefore the later v4l2_ctrl_request_complete()
> > would fail...
> > 
> > I see that the usage of v4l2_m2m_buf_done_and_job_finish() is more
> > compact and will use that in v2. For the sake of understanding: The only
> > functional issue with my code is that v4l2_m2m_buf_done(src_buf) is
> > called before v4l2_m2m_buf_done(dest_buf), right?
> 
> Correct, swapping these two would work too, but as you said, I proposed
> v4l2_m2m_buf_done_and_job_finish() simply because it is more compact and fits
> your use case. The extra code in the helper is for capture buffer holding, which
> at least for now does not apply to you.
> 
> > > > > +
> > > > >     dw100_start(ctx, src_buf, dst_buf);
> > > > 
> > > > nit: I really don't see why this is a separate function ...
> > 
> > I wondered that also, but didn't want to mess too much with existing
> > code. Maybe as a fixup on top?
> 
> Feel free to do so.
> 
> > > > >  }
> > > > >  
> > > > > @@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops =
> > > > > {
> > > > >     .device_run     = dw100_device_run,
> > > > >  };
> > > > >  
> > > > > +static const struct media_device_ops dw100_m2m_media_ops = {
> > > > > +   .req_validate = vb2_request_validate,
> > > > > +   .req_queue = v4l2_m2m_request_queue,
> > > > > +};
> > > > > +
> > > > >  static struct video_device *dw100_init_video_device(struct dw100_device
> > > > > *dw_dev)
> > > > >  {
> > > > >     struct video_device *vfd = &dw_dev->vfd;
> > > > > @@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device
> > > > > *pdev)
> > > > >     dw_dev->mdev.dev = &pdev->dev;
> > > > >     strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
> > > > >     media_device_init(&dw_dev->mdev);
> > > > > +   dw_dev->mdev.ops = &dw100_m2m_media_ops;
> > > > >     dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
> > > > >  
> > > > >     ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Nicolas Dufresne]

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

Le mardi 06 janvier 2026 à 16:59 +0200, Laurent Pinchart a écrit :
> > I'm stumbled on this one too, but I completely forgot what this callback was
> > about and assume something like this. I suspect it might be some left over
> > from
> > when we required all request to hold all the controls, we should find a way
> > to
> > make that optional, and sort out this field thing, but I'd say its not a
> > problem
> > for this serie.
> 
> Definitely not a problem for this series :-) Please see my other reply
> about using .buf_prepare(), let's avoid splitting the discussion.

I replied 22 minutes before you did, so to be accurate, you did split the
discussion.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Laurent Pinchart]

CC'ing Hans Verkuil for two questions below.

On Tue, Jan 06, 2026 at 03:16:17PM +0100, Stefan Klug wrote:
> Hi Nicolas, hi Laurent,
> 
> Thank you for the review.
> 
> Quoting Laurent Pinchart (2026-01-06 01:33:02)
> > On Mon, Jan 05, 2026 at 01:46:46PM -0500, Nicolas Dufresne wrote:
> > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > The dw100 dewarper hardware present on the NXP i.MX8MP allows very
> > > > flexible dewarping using a freely configurable vertex map. Aside from
> > > > lens dewarping the vertex map can be used to implement things like
> > > > arbitrary zoom, pan and rotation. The current driver supports setting
> > > > that vertex map before calling VIDIOC_STREAMON.
> > > > 
> > > > To control above mentioned features during streaming it is necessary to
> > > > update the vertex map dynamically. To do that in a race free manner V4L2
> > > > requests support is required. This patch adds V4L2 requests support to
> > > > prepare for dynamic vertex map updates.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > 
> > > > ---
> > > > 
> > > > Changes in v1:
> > > > - Moved v4l2_ctrl_request_complete into dw100_device_run
> > > > ---
> > > >  drivers/media/platform/nxp/dw100/dw100.c | 41 ++++++++++++++++++++++++++++----
> > > >  1 file changed, 36 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> > > > index 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..7f14b82c15a071605c124dbb868f8003856c4fc0 100644
> > > > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > > > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > > > @@ -459,6 +459,15 @@ static int dw100_queue_setup(struct vb2_queue *vq,
> > > >     return 0;
> > > >  }
> > > >  
> > > > +static int dw100_buf_out_validate(struct vb2_buffer *vb)
> > > > +{
> > > > +   struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > +
> > > > +   vbuf->field = V4L2_FIELD_NONE;
> > > > +
> > > > +   return 0;
> > > > +}
> > 
> > Stefan, how is this related to requests support ?
> 
> vb2_queue_or_prepare_buf() errors out if this is not implemented and the
> buffer uses requests. This was the implementation I saw on most drivers.
> And as I don't expect anyone to try to use the dewarper with interleaved
> data I didn't bother to add a warning.

I wasn't aware of that. Reading the code, I'm really puzzled by why
.buf_out_validate() was added, it seems that validating .field in
.buf_prepare() would be enough. Hans, could you shed some light on that
?

> > > > +
> > > >  static int dw100_buf_prepare(struct vb2_buffer *vb)
> > > >  {
> > > >     unsigned int i;
> > > > @@ -500,6 +509,13 @@ static void dw100_buf_queue(struct vb2_buffer *vb)
> > > >     v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > > >  }
> > > >  
> > > > +static void dw100_buf_request_complete(struct vb2_buffer *vb)
> > > > +{
> > > > +   struct dw100_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > > +
> > > > +   v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > 
> > Unless I'm missing something, this will call
> > v4l2_ctrl_request_complete() twice, once on each of the source and
> > destination buffers, for the same request and control handler. Is that
> > desired ?
> 
> The docs say "a buffer that was never queued to the driver but is
> associated with a queued request was canceled..." So to my understanding
> the only purpose of this function is to mark all controls in the request
> as being handled, so that the request can be finished.

That doesn't explain why it should be done twice per request. Hans,
could you clarify this ?

> All the implementations I found in the kernel are exactly the same and
> are to my understanding only necessary to map from a vb2_buffer to a
> v4l2_ctrl_handler (which is then passed to v4l2_ctrl_request_complete())
> 
> > > > +}
> > > > +
> > > >  static void dw100_return_all_buffers(struct vb2_queue *q,
> > > >                                  enum vb2_buffer_state state)
> > > >  {
> > > > @@ -553,11 +569,13 @@ static void dw100_stop_streaming(struct vb2_queue *q)
> > > >  }
> > > >  
> > > >  static const struct vb2_ops dw100_qops = {
> > > > -   .queue_setup     = dw100_queue_setup,
> > > > -   .buf_prepare     = dw100_buf_prepare,
> > > > -   .buf_queue       = dw100_buf_queue,
> > > > -   .start_streaming = dw100_start_streaming,
> > > > -   .stop_streaming  = dw100_stop_streaming,
> > > > +   .queue_setup          = dw100_queue_setup,
> > > > +   .buf_out_validate     = dw100_buf_out_validate,
> > > > +   .buf_prepare          = dw100_buf_prepare,
> > > > +   .buf_queue            = dw100_buf_queue,
> > > > +   .start_streaming      = dw100_start_streaming,
> > > > +   .stop_streaming       = dw100_stop_streaming,
> > > > +   .buf_request_complete = dw100_buf_request_complete,
> > > >  };
> > > >  
> > > >  static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > > > @@ -575,6 +593,7 @@ static int dw100_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
> > > >     src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > > >     src_vq->lock = &ctx->vq_mutex;
> > > >     src_vq->dev = ctx->dw_dev->v4l2_dev.dev;
> > > > +   src_vq->supports_requests = true;
> > > >  
> > > >     ret = vb2_queue_init(src_vq);
> > > >     if (ret)
> > > > @@ -1460,6 +1479,12 @@ static void dw100_device_run(void *priv)
> > > >     src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > > >     dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > > >  
> > > > +   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > > +                           &ctx->hdl);
> > > > +
> > > > +   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > > +                              &ctx->hdl);
> > > 
> > > The request should always be signalled last, so nothing wrong with applying the
> > > controls as soon as possible in this case. Complete is a bit of a miss-leading
> > > name, this function actually changes the global controls value (apply) and
> > > removes its participation in request completion. Since the OUTPUT buffer for
> > > that request is still queued, the request is not signalled yet.
> > 
> > I'm very confused here. As far as I can tell,
> > v4l2_ctrl_request_complete() doesn't apply controls (i.e. cause
> > .s_ctrl() to be called), it copies the value of controls back to the
> > request to report them to the application. Am I missing something ?
> > 
> > As there's nothing to report back to the application (no volatile
> > control whose value will come from the hardware), calling
> > v4l2_ctrl_request_complete() here seems fine to me. Is that what you
> > were trying to explain ?
> 
> I think that was meant, yes (see next comment)
> 
> > > But you have to flip over the order to buffer signalling in dw100_job_finish()
> > > though. My recommendation is to use the helper
> > > v4l2_m2m_buf_done_and_job_finish(). Something like this (not tested):
> > > 
> > >    diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> > >    index 4aaf9c3fff53..c5f9ee238345 100644
> > >    --- a/drivers/media/platform/nxp/dw100/dw100.c
> > >    +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > >    @@ -1058,7 +1058,6 @@ static const struct v4l2_ioctl_ops dw100_ioctl_ops = {
> > >     static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
> > >     {
> > >            struct dw100_ctx *curr_ctx;
> > >    -       struct vb2_v4l2_buffer *src_vb, *dst_vb;
> > >            enum vb2_buffer_state buf_state;
> > >     
> > >            curr_ctx = v4l2_m2m_get_curr_priv(dw_dev->m2m_dev);
> > >    @@ -1069,16 +1068,12 @@ static void dw100_job_finish(struct dw100_device *dw_dev, bool with_error)
> > >                    return;
> > >            }
> > >     
> > >    -       src_vb = v4l2_m2m_src_buf_remove(curr_ctx->fh.m2m_ctx);
> > >    -       dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->fh.m2m_ctx);
> > >    -
> > >            if (likely(!with_error))
> > >                    buf_state = VB2_BUF_STATE_DONE;
> > >            else
> > >                    buf_state = VB2_BUF_STATE_ERROR;
> > >     
> > >    -       v4l2_m2m_buf_done(src_vb, buf_state);
> > >    -       v4l2_m2m_buf_done(dst_vb, buf_state);
> > >    +       v4l2_m2m_buf_done_and_job_finish(dw_dev->m2m_dev, buf_state);
> > >     
> > >            dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s error(s)\n",
> > >                    with_error ? "" : "out");
> > >    
> > > You might be tempted to keep the logical order, and move the
> > > v4l2_ctrl_request_complete() call into dw100_job_finish(), unfortunately this
> > > does not work, since nothing mandate that any control was included in this
> > > request, and that will lead to calling v4l2_ctrl_request_complete() on an
> > > already completed request. Since its a single function HW, I don't see why you'd
> > > want this, but it would require the manual request completion.
> > > 
> 
> Nicolas, if I go you right you mean that one might be tempted to do
> 
> v4l2_ctrl_request_setup()
> v4l2_m2m_buf_done(src)
> v4l2_m2m_buf_done(dst)
> v4l2_ctrl_request_complete()
> 
> which feels natural from the names of the functions, but the
> v4l2_m2m_buf_done(src) might complete the request if it has no
> associated controls and therefore the later v4l2_ctrl_request_complete()
> would fail...
> 
> I see that the usage of v4l2_m2m_buf_done_and_job_finish() is more
> compact and will use that in v2. For the sake of understanding: The only
> functional issue with my code is that v4l2_m2m_buf_done(src_buf) is
> called before v4l2_m2m_buf_done(dest_buf), right?

Is that an issue, why would the destination buffer need to be completed
first ?

> > > > +
> > > >     dw100_start(ctx, src_buf, dst_buf);
> > > 
> > > nit: I really don't see why this is a separate function ...
> 
> I wondered that also, but didn't want to mess too much with existing
> code. Maybe as a fixup on top?

A separate patch would be better.

> > > >  }
> > > >  
> > > > @@ -1467,6 +1492,11 @@ static const struct v4l2_m2m_ops dw100_m2m_ops = {
> > > >     .device_run     = dw100_device_run,
> > > >  };
> > > >  
> > > > +static const struct media_device_ops dw100_m2m_media_ops = {
> > > > +   .req_validate = vb2_request_validate,
> > > > +   .req_queue = v4l2_m2m_request_queue,
> > > > +};
> > > > +
> > > >  static struct video_device *dw100_init_video_device(struct dw100_device *dw_dev)
> > > >  {
> > > >     struct video_device *vfd = &dw_dev->vfd;
> > > > @@ -1578,6 +1608,7 @@ static int dw100_probe(struct platform_device *pdev)
> > > >     dw_dev->mdev.dev = &pdev->dev;
> > > >     strscpy(dw_dev->mdev.model, "dw100", sizeof(dw_dev->mdev.model));
> > > >     media_device_init(&dw_dev->mdev);
> > > > +   dw_dev->mdev.ops = &dw100_m2m_media_ops;
> > > >     dw_dev->v4l2_dev.mdev = &dw_dev->mdev;
> > > >  
> > > >     ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Nicolas Dufresne]

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

Hi Laurent,

Le mardi 06 janvier 2026 à 16:53 +0200, Laurent Pinchart a écrit :
> CC'ing Hans Verkuil for two questions below.
> > 

[...]

> > The docs say "a buffer that was never queued to the driver but is
> > associated with a queued request was canceled..." So to my understanding
> > the only purpose of this function is to mark all controls in the request
> > as being handled, so that the request can be finished.
> 
> That doesn't explain why it should be done twice per request. Hans,
> could you clarify this ?

I explained it in another thread, it is only called if device_run() is not going
to be called, so only once. vb2 does not have access to the the control handler,
so it can't call the v4l2_ctrl_request_complete() fonction directly.

> 
[...]

> > Nicolas, if I go you right you mean that one might be tempted to do
> > 
> > v4l2_ctrl_request_setup()
> > v4l2_m2m_buf_done(src)
> > v4l2_m2m_buf_done(dst)
> > v4l2_ctrl_request_complete()
> > 
> > which feels natural from the names of the functions, but the
> > v4l2_m2m_buf_done(src) might complete the request if it has no
> > associated controls and therefore the later v4l2_ctrl_request_complete()
> > would fail...
> > 
> > I see that the usage of v4l2_m2m_buf_done_and_job_finish() is more
> > compact and will use that in v2. For the sake of understanding: The only
> > functional issue with my code is that v4l2_m2m_buf_done(src_buf) is
> > called before v4l2_m2m_buf_done(dest_buf), right?
> 
> Is that an issue, why would the destination buffer need to be completed
> first ?

The VB2 media_request_object is being removed from the request once the OUTPUT
(src) buffer is marked done. If this was the last media_request_object, the
request will move to completed state, which will signal its FD.

If you do that before you mark the CAPTURE (dst) buffer as done, an application
that uses non blocking IOs may endup calling DQBUF(dst) too soon, which will
return EBUSY. Since we really want the request FD to be used as the only
synchronisation point, we made the rule that the request FD must be signalled
last. Since its error prone, and its not illegal to synchronise on the device
read/write/pri state, we made a condensed helper for it.

Alternatively, the manual request completion is being added for cases this
implicit request completion does not work, or when it makes everything too
complicated to adhere to this rule. This is the case for dual-stage video
decoders (MTK/RPi).

Nicolas


[...]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Laurent Pinchart]

On Tue, Jan 06, 2026 at 10:41:56AM -0500, Nicolas Dufresne wrote:
> Hi Laurent,
> 
> Le mardi 06 janvier 2026 à 16:53 +0200, Laurent Pinchart a écrit :
> > CC'ing Hans Verkuil for two questions below.
> > > 
> 
> [...]
> 
> > > The docs say "a buffer that was never queued to the driver but is
> > > associated with a queued request was canceled..." So to my understanding
> > > the only purpose of this function is to mark all controls in the request
> > > as being handled, so that the request can be finished.
> > 
> > That doesn't explain why it should be done twice per request. Hans,
> > could you clarify this ?
> 
> I explained it in another thread, it is only called if device_run() is not going
> to be called, so only once. vb2 does not have access to the the control handler,
> so it can't call the v4l2_ctrl_request_complete() fonction directly.

But the function is called per queue. If a buffer has been queued on
both the capture and the output queue for a request, won't the operation
be called twice with the same request ?

> [...]
> 
> > > Nicolas, if I go you right you mean that one might be tempted to do
> > > 
> > > v4l2_ctrl_request_setup()
> > > v4l2_m2m_buf_done(src)
> > > v4l2_m2m_buf_done(dst)
> > > v4l2_ctrl_request_complete()
> > > 
> > > which feels natural from the names of the functions, but the
> > > v4l2_m2m_buf_done(src) might complete the request if it has no
> > > associated controls and therefore the later v4l2_ctrl_request_complete()
> > > would fail...
> > > 
> > > I see that the usage of v4l2_m2m_buf_done_and_job_finish() is more
> > > compact and will use that in v2. For the sake of understanding: The only
> > > functional issue with my code is that v4l2_m2m_buf_done(src_buf) is
> > > called before v4l2_m2m_buf_done(dest_buf), right?
> > 
> > Is that an issue, why would the destination buffer need to be completed
> > first ?
> 
> The VB2 media_request_object is being removed from the request once the OUTPUT
> (src) buffer is marked done. If this was the last media_request_object, the
> request will move to completed state, which will signal its FD.

I'll reply to this separately, I need to read the code first.

> If you do that before you mark the CAPTURE (dst) buffer as done, an application
> that uses non blocking IOs may endup calling DQBUF(dst) too soon, which will
> return EBUSY. Since we really want the request FD to be used as the only
> synchronisation point, we made the rule that the request FD must be signalled
> last. Since its error prone, and its not illegal to synchronise on the device
> read/write/pri state, we made a condensed helper for it.
> 
> Alternatively, the manual request completion is being added for cases this
> implicit request completion does not work, or when it makes everything too
> complicated to adhere to this rule. This is the case for dual-stage video
> decoders (MTK/RPi).
> 
> [...]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Nicolas Dufresne]

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

Hi,

Le mardi 06 janvier 2026 à 17:45 +0200, Laurent Pinchart a écrit :
> > I explained it in another thread, it is only called if device_run() is not going
> > to be called, so only once. vb2 does not have access to the the control handler,
> > so it can't call the v4l2_ctrl_request_complete() fonction directly.
> 
> But the function is called per queue. If a buffer has been queued on
> both the capture and the output queue for a request, won't the operation
> be called twice with the same request ?

Only the OUTPUT queues allow for requests at the moment.

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

[Laurent Pinchart]

On Tue, Jan 06, 2026 at 10:56:09AM -0500, Nicolas Dufresne wrote:
> Le mardi 06 janvier 2026 à 17:45 +0200, Laurent Pinchart a écrit :
> > > I explained it in another thread, it is only called if device_run() is not going
> > > to be called, so only once. vb2 does not have access to the the control handler,
> > > so it can't call the v4l2_ctrl_request_complete() fonction directly.
> > 
> > But the function is called per queue. If a buffer has been queued on
> > both the capture and the output queue for a request, won't the operation
> > be called twice with the same request ?
> 
> Only the OUTPUT queues allow for requests at the moment.

Aahhh that's what I was missing. I thought the request would contain
buffers for both the output and capture queues.

-- 
Regards,

Laurent Pinchart

[PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Stefan Klug]

Implement dynamic vertex map updates by handling the
V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
allows to implement features like dynamic zoom, pan, rotate and dewarp.

To stay compatible with the old version, updates of
V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
when requests are not used. Print a corresponding warning once.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
index 7f14b82c15a071605c124dbb868f8003856c4fc0..8a421059a1c9b55f514a29d3c2c5a6ffb76e0a64 100644
--- a/drivers/media/platform/nxp/dw100/dw100.c
+++ b/drivers/media/platform/nxp/dw100/dw100.c
@@ -98,6 +98,8 @@ struct dw100_ctx {
 	unsigned int			map_width;
 	unsigned int			map_height;
 	bool				user_map_is_set;
+	bool				user_map_needs_update;
+	bool				warned_dynamic_update;
 
 	/* Source and destination queue data */
 	struct dw100_q_data		q_data[2];
@@ -293,11 +295,15 @@ static u32 dw100_map_format_coordinates(u16 xq, u16 yq)
 	return (u32)((yq << 16) | xq);
 }
 
-static u32 *dw100_get_user_map(struct dw100_ctx *ctx)
+static void dw100_update_mapping(struct dw100_ctx *ctx)
 {
 	struct v4l2_ctrl *ctrl = ctx->ctrls[DW100_CTRL_DEWARPING_MAP];
 
-	return ctrl->p_cur.p_u32;
+	if (!ctx->user_map_needs_update)
+		return;
+
+	memcpy(ctx->map, ctrl->p_cur.p_u32, ctx->map_size);
+	ctx->user_map_needs_update = false;
 }
 
 /*
@@ -306,8 +312,6 @@ static u32 *dw100_get_user_map(struct dw100_ctx *ctx)
  */
 static int dw100_create_mapping(struct dw100_ctx *ctx)
 {
-	u32 *user_map;
-
 	if (ctx->map)
 		dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
 				  ctx->map, ctx->map_dma);
@@ -318,8 +322,8 @@ static int dw100_create_mapping(struct dw100_ctx *ctx)
 	if (!ctx->map)
 		return -ENOMEM;
 
-	user_map = dw100_get_user_map(ctx);
-	memcpy(ctx->map, user_map, ctx->map_size);
+	ctx->user_map_needs_update = true;
+	dw100_update_mapping(ctx);
 
 	dev_dbg(&ctx->dw_dev->pdev->dev,
 		"%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
@@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
 	switch (ctrl->id) {
 	case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
 		ctx->user_map_is_set = true;
+		ctx->user_map_needs_update = true;
 		break;
 	}
 
@@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
 	}
 
 	ctx->user_map_is_set = false;
+	ctx->user_map_needs_update = true;
 }
 
 static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
@@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
 	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
 				&ctx->hdl);
 
+	if (src_buf->vb2_buf.req_obj.req) {
+		dw100_update_mapping(ctx);
+	} else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
+		ctx->warned_dynamic_update = true;
+		dev_warn(&ctx->dw_dev->pdev->dev,
+			"V4L2 requests are required to update the vertex map dynamically"
+		);
+	}
+
 	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
 				   &ctx->hdl);
 

-- 
2.51.0

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Nicolas Dufresne]

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

Hi,

Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> Implement dynamic vertex map updates by handling the
> V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> allows to implement features like dynamic zoom, pan, rotate and dewarp.
> 
> To stay compatible with the old version, updates of
> V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> when requests are not used. Print a corresponding warning once.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> index 7f14b82c15a071605c124dbb868f8003856c4fc0..8a421059a1c9b55f514a29d3c2c5a6ffb76e0a64 100644
> --- a/drivers/media/platform/nxp/dw100/dw100.c
> +++ b/drivers/media/platform/nxp/dw100/dw100.c
> @@ -98,6 +98,8 @@ struct dw100_ctx {
>  	unsigned int			map_width;
>  	unsigned int			map_height;
>  	bool				user_map_is_set;
> +	bool				user_map_needs_update;
> +	bool				warned_dynamic_update;
>  
>  	/* Source and destination queue data */
>  	struct dw100_q_data		q_data[2];
> @@ -293,11 +295,15 @@ static u32 dw100_map_format_coordinates(u16 xq, u16 yq)
>  	return (u32)((yq << 16) | xq);
>  }
>  
> -static u32 *dw100_get_user_map(struct dw100_ctx *ctx)
> +static void dw100_update_mapping(struct dw100_ctx *ctx)
>  {
>  	struct v4l2_ctrl *ctrl = ctx->ctrls[DW100_CTRL_DEWARPING_MAP];
>  
> -	return ctrl->p_cur.p_u32;
> +	if (!ctx->user_map_needs_update)
> +		return;
> +
> +	memcpy(ctx->map, ctrl->p_cur.p_u32, ctx->map_size);
> +	ctx->user_map_needs_update = false;
>  }
>  
>  /*
> @@ -306,8 +312,6 @@ static u32 *dw100_get_user_map(struct dw100_ctx *ctx)
>   */
>  static int dw100_create_mapping(struct dw100_ctx *ctx)
>  {
> -	u32 *user_map;
> -
>  	if (ctx->map)
>  		dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
>  				  ctx->map, ctx->map_dma);
> @@ -318,8 +322,8 @@ static int dw100_create_mapping(struct dw100_ctx *ctx)
>  	if (!ctx->map)
>  		return -ENOMEM;
>  
> -	user_map = dw100_get_user_map(ctx);
> -	memcpy(ctx->map, user_map, ctx->map_size);
> +	ctx->user_map_needs_update = true;
> +	dw100_update_mapping(ctx);
>  
>  	dev_dbg(&ctx->dw_dev->pdev->dev,
>  		"%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
>  	switch (ctrl->id) {
>  	case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
>  		ctx->user_map_is_set = true;
> +		ctx->user_map_needs_update = true;

This will be called before the new mapping is applied by
v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
depth is high enough.

Instead, you should check in the request for the presence of
V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
set this to true if if there is no request, in the case you also wanted to
change the original behaviour of only updating that vertex on streamon, but I
don't see much point though.

>  		break;
>  	}
>  
> @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
>  	}
>  
>  	ctx->user_map_is_set = false;
> +	ctx->user_map_needs_update = true;
>  }
>  
>  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
>  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
>  				&ctx->hdl);
>  
> +	if (src_buf->vb2_buf.req_obj.req) {
> +		dw100_update_mapping(ctx);
> +	} else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> +		ctx->warned_dynamic_update = true;
> +		dev_warn(&ctx->dw_dev->pdev->dev,
> +			"V4L2 requests are required to update the vertex map dynamically"

Do you know about dev_warn_once() ? That being said, the driver is usable
without requests and a static vertex (and must stay this way to not break the
ABI). I don't think you should warn here at all.

Nicolas

> +		);
> +	}
> +
>  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
>  				   &ctx->hdl);
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 01:58:25PM -0500, Nicolas Dufresne wrote:
> Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > Implement dynamic vertex map updates by handling the
> > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> > allows to implement features like dynamic zoom, pan, rotate and dewarp.
> > 
> > To stay compatible with the old version, updates of
> > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> > when requests are not used. Print a corresponding warning once.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> > index 7f14b82c15a071605c124dbb868f8003856c4fc0..8a421059a1c9b55f514a29d3c2c5a6ffb76e0a64 100644
> > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > @@ -98,6 +98,8 @@ struct dw100_ctx {
> >  	unsigned int			map_width;
> >  	unsigned int			map_height;
> >  	bool				user_map_is_set;
> > +	bool				user_map_needs_update;
> > +	bool				warned_dynamic_update;
> >  
> >  	/* Source and destination queue data */
> >  	struct dw100_q_data		q_data[2];
> > @@ -293,11 +295,15 @@ static u32 dw100_map_format_coordinates(u16 xq, u16 yq)
> >  	return (u32)((yq << 16) | xq);
> >  }
> >  
> > -static u32 *dw100_get_user_map(struct dw100_ctx *ctx)
> > +static void dw100_update_mapping(struct dw100_ctx *ctx)
> >  {
> >  	struct v4l2_ctrl *ctrl = ctx->ctrls[DW100_CTRL_DEWARPING_MAP];
> >  
> > -	return ctrl->p_cur.p_u32;
> > +	if (!ctx->user_map_needs_update)
> > +		return;
> > +
> > +	memcpy(ctx->map, ctrl->p_cur.p_u32, ctx->map_size);
> > +	ctx->user_map_needs_update = false;
> >  }
> >  
> >  /*
> > @@ -306,8 +312,6 @@ static u32 *dw100_get_user_map(struct dw100_ctx *ctx)
> >   */
> >  static int dw100_create_mapping(struct dw100_ctx *ctx)
> >  {
> > -	u32 *user_map;
> > -
> >  	if (ctx->map)
> >  		dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >  				  ctx->map, ctx->map_dma);
> > @@ -318,8 +322,8 @@ static int dw100_create_mapping(struct dw100_ctx *ctx)
> >  	if (!ctx->map)
> >  		return -ENOMEM;
> >  
> > -	user_map = dw100_get_user_map(ctx);
> > -	memcpy(ctx->map, user_map, ctx->map_size);
> > +	ctx->user_map_needs_update = true;
> > +	dw100_update_mapping(ctx);
> >  
> >  	dev_dbg(&ctx->dw_dev->pdev->dev,
> >  		"%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> > @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> >  		ctx->user_map_is_set = true;
> > +		ctx->user_map_needs_update = true;
> 
> This will be called before the new mapping is applied by
> v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
> depth is high enough.

v4l2_ctrl_request_complete() does not apply a mapping, what am I missing
?

> Instead, you should check in the request for the presence of
> V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
> set this to true if if there is no request, in the case you also wanted to
> change the original behaviour of only updating that vertex on streamon, but I
> don't see much point though.
> 
> >  		break;
> >  	}
> >  
> > @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
> >  	}
> >  
> >  	ctx->user_map_is_set = false;
> > +	ctx->user_map_needs_update = true;
> >  }
> >  
> >  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> > @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
> >  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> >  				&ctx->hdl);
> >  
> > +	if (src_buf->vb2_buf.req_obj.req) {
> > +		dw100_update_mapping(ctx);
> > +	} else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> > +		ctx->warned_dynamic_update = true;
> > +		dev_warn(&ctx->dw_dev->pdev->dev,
> > +			"V4L2 requests are required to update the vertex map dynamically"
> 
> Do you know about dev_warn_once() ? That being said, the driver is usable
> without requests and a static vertex (and must stay this way to not break the
> ABI). I don't think you should warn here at all.

Applications should move to using requests. We'll do so in libcamera
unconditionally. I don't expect many other direct users, so warning that
the mapping won't be applied when an application sets the corresponding
control during streaming without using requests seems fair to me. It
will help debugging issues.

> > +		);
> > +	}
> > +
> >  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> >  				   &ctx->hdl);
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Nicolas Dufresne]

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

Hi,

Le mardi 06 janvier 2026 à 02:42 +0200, Laurent Pinchart a écrit :
> On Mon, Jan 05, 2026 at 01:58:25PM -0500, Nicolas Dufresne wrote:
> > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > Implement dynamic vertex map updates by handling the
> > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> > > allows to implement features like dynamic zoom, pan, rotate and dewarp.
> > > 
> > > To stay compatible with the old version, updates of
> > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> > > when requests are not used. Print a corresponding warning once.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
> > > 

[...]

> > >  	dev_dbg(&ctx->dw_dev->pdev->dev,
> > >  		"%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> > > @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  	switch (ctrl->id) {
> > >  	case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> > >  		ctx->user_map_is_set = true;
> > > +		ctx->user_map_needs_update = true;
> > 
> > This will be called before the new mapping is applied by
> > v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
> > depth is high enough.
> 
> v4l2_ctrl_request_complete() does not apply a mapping, what am I missing
> ?

Sorry for my confusion, after reading back, you are correct, this is called
v4l2_ctrl_request_setup() inside the device_run() function. You can dismiss the
bit about user_map_needs_update in my review (the paragraph below).

> 
> > Instead, you should check in the request for the presence of
> > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
> > set this to true if if there is no request, in the case you also wanted to
> > change the original behaviour of only updating that vertex on streamon, but I
> > don't see much point though.
> > 
> > >  		break;
> > >  	}
> > >  
> > > @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
> > >  	}
> > >  
> > >  	ctx->user_map_is_set = false;
> > > +	ctx->user_map_needs_update = true;
> > >  }
> > >  
> > >  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> > > @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
> > >  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > >  				&ctx->hdl);
> > >  
> > > +	if (src_buf->vb2_buf.req_obj.req) {
> > > +		dw100_update_mapping(ctx);
> > > +	} else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> > > +		ctx->warned_dynamic_update = true;
> > > +		dev_warn(&ctx->dw_dev->pdev->dev,
> > > +			"V4L2 requests are required to update the vertex map dynamically"
> > 
> > Do you know about dev_warn_once() ? That being said, the driver is usable
> > without requests and a static vertex (and must stay this way to not break the
> > ABI). I don't think you should warn here at all.
> 
> Applications should move to using requests. We'll do so in libcamera
> unconditionally. I don't expect many other direct users, so warning that
> the mapping won't be applied when an application sets the corresponding
> control during streaming without using requests seems fair to me. It
> will help debugging issues.

It is also a miss-use of dev_warn which is meant to indicate a problem with the
driver or the hardware. The V4L2 core uses dev_dbg() or customer log level for
that in general. Also, don't re-implement _once() variants with
warned_dynamic_update please. Personally, I would just let the out-of request
change the control on the next device_run(), even if that means its out of sync.

Nicolas

> 
> > > +		);
> > > +	}
> > > +
> > >  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > >  				   &ctx->hdl);
> > >  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Stefan Klug]

Hi,

Quoting Nicolas Dufresne (2026-01-06 14:47:59)
> Hi,
> 
> Le mardi 06 janvier 2026 à 02:42 +0200, Laurent Pinchart a écrit :
> > On Mon, Jan 05, 2026 at 01:58:25PM -0500, Nicolas Dufresne wrote:
> > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > Implement dynamic vertex map updates by handling the
> > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> > > > allows to implement features like dynamic zoom, pan, rotate and dewarp.
> > > > 
> > > > To stay compatible with the old version, updates of
> > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> > > > when requests are not used. Print a corresponding warning once.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > ---
> > > >  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
> > > > 
> 
> [...]
> 
> > > >   dev_dbg(&ctx->dw_dev->pdev->dev,
> > > >           "%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> > > > @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >   switch (ctrl->id) {
> > > >   case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> > > >           ctx->user_map_is_set = true;
> > > > +         ctx->user_map_needs_update = true;
> > > 
> > > This will be called before the new mapping is applied by
> > > v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
> > > depth is high enough.
> > 
> > v4l2_ctrl_request_complete() does not apply a mapping, what am I missing
> > ?
> 
> Sorry for my confusion, after reading back, you are correct, this is called
> v4l2_ctrl_request_setup() inside the device_run() function. You can dismiss the
> bit about user_map_needs_update in my review (the paragraph below).
> 

That means nothing has to change here, right?

> > 
> > > Instead, you should check in the request for the presence of
> > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
> > > set this to true if if there is no request, in the case you also wanted to
> > > change the original behaviour of only updating that vertex on streamon, but I
> > > don't see much point though.
> > > 
> > > >           break;
> > > >   }
> > > >  
> > > > @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
> > > >   }
> > > >  
> > > >   ctx->user_map_is_set = false;
> > > > + ctx->user_map_needs_update = true;
> > > >  }
> > > >  
> > > >  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> > > > @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
> > > >   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > >                           &ctx->hdl);
> > > >  
> > > > + if (src_buf->vb2_buf.req_obj.req) {
> > > > +         dw100_update_mapping(ctx);
> > > > + } else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> > > > +         ctx->warned_dynamic_update = true;
> > > > +         dev_warn(&ctx->dw_dev->pdev->dev,
> > > > +                 "V4L2 requests are required to update the vertex map dynamically"
> > > 
> > > Do you know about dev_warn_once() ? That being said, the driver is usable
> > > without requests and a static vertex (and must stay this way to not break the
> > > ABI). I don't think you should warn here at all.

My idea was that I'd like to see the warning once per context and not
once per boot. Afaik I can't use dev_warn_once() for that.

> > 
> > Applications should move to using requests. We'll do so in libcamera
> > unconditionally. I don't expect many other direct users, so warning that
> > the mapping won't be applied when an application sets the corresponding
> > control during streaming without using requests seems fair to me. It
> > will help debugging issues.
> 
> It is also a miss-use of dev_warn which is meant to indicate a problem with the
> driver or the hardware. The V4L2 core uses dev_dbg() or customer log level for
> that in general. Also, don't re-implement _once() variants with
> warned_dynamic_update please. Personally, I would just let the out-of request
> change the control on the next device_run(), even if that means its out of sync.

But then you end up with potentially difficult to debug issues, because
users would not know that they should use requests. Not warning (or
dev_dbg) has the same effect from my point of view, because users just
see a device not working as expected. Is a customer log level the
solution?

Best regards,
Stefan

> 
> Nicolas
> 
> > 
> > > > +         );
> > > > + }
> > > > +
> > > >   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > >                              &ctx->hdl);
> > > >

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Nicolas Dufresne]

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

	Le mardi 06 janvier 2026 à 15:29 +0100, Stefan Klug a écrit :
> Hi,
> 
> Quoting Nicolas Dufresne (2026-01-06 14:47:59)
> > Hi,
> > 
> > Le mardi 06 janvier 2026 à 02:42 +0200, Laurent Pinchart a écrit :
> > > On Mon, Jan 05, 2026 at 01:58:25PM -0500, Nicolas Dufresne wrote:
> > > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > > Implement dynamic vertex map updates by handling the
> > > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> > > > > allows to implement features like dynamic zoom, pan, rotate and dewarp.
> > > > > 
> > > > > To stay compatible with the old version, updates of
> > > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> > > > > when requests are not used. Print a corresponding warning once.
> > > > > 
> > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > ---
> > > > >  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
> > > > > 
> > 
> > [...]
> > 
> > > > >   dev_dbg(&ctx->dw_dev->pdev->dev,
> > > > >           "%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> > > > > @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > >   switch (ctrl->id) {
> > > > >   case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> > > > >           ctx->user_map_is_set = true;
> > > > > +         ctx->user_map_needs_update = true;
> > > > 
> > > > This will be called before the new mapping is applied by
> > > > v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
> > > > depth is high enough.
> > > 
> > > v4l2_ctrl_request_complete() does not apply a mapping, what am I missing
> > > ?
> > 
> > Sorry for my confusion, after reading back, you are correct, this is called
> > v4l2_ctrl_request_setup() inside the device_run() function. You can dismiss the
> > bit about user_map_needs_update in my review (the paragraph below).
> > 
> 
> That means nothing has to change here, right?

Correct.

> 
> > > 
> > > > Instead, you should check in the request for the presence of
> > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
> > > > set this to true if if there is no request, in the case you also wanted to
> > > > change the original behaviour of only updating that vertex on streamon, but I
> > > > don't see much point though.
> > > > 
> > > > >           break;
> > > > >   }
> > > > >  
> > > > > @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
> > > > >   }
> > > > >  
> > > > >   ctx->user_map_is_set = false;
> > > > > + ctx->user_map_needs_update = true;
> > > > >  }
> > > > >  
> > > > >  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> > > > > @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
> > > > >   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > > >                           &ctx->hdl);
> > > > >  
> > > > > + if (src_buf->vb2_buf.req_obj.req) {
> > > > > +         dw100_update_mapping(ctx);
> > > > > + } else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> > > > > +         ctx->warned_dynamic_update = true;
> > > > > +         dev_warn(&ctx->dw_dev->pdev->dev,
> > > > > +                 "V4L2 requests are required to update the vertex map dynamically"
> > > > 
> > > > Do you know about dev_warn_once() ? That being said, the driver is usable
> > > > without requests and a static vertex (and must stay this way to not break the
> > > > ABI). I don't think you should warn here at all.
> 
> My idea was that I'd like to see the warning once per context and not
> once per boot. Afaik I can't use dev_warn_once() for that.

I didn't catch this detail (commit comment welcome), fair enough if that's the
direction we are heading. Again, I don't understand why we don't just apply that
on next device_run in "legacy" mode.

> 
> > > 
> > > Applications should move to using requests. We'll do so in libcamera
> > > unconditionally. I don't expect many other direct users, so warning that
> > > the mapping won't be applied when an application sets the corresponding
> > > control during streaming without using requests seems fair to me. It
> > > will help debugging issues.
> > 
> > It is also a miss-use of dev_warn which is meant to indicate a problem with the
> > driver or the hardware. The V4L2 core uses dev_dbg() or customer log level for
> > that in general. Also, don't re-implement _once() variants with
> > warned_dynamic_update please. Personally, I would just let the out-of request
> > change the control on the next device_run(), even if that means its out of sync.
> 
> But then you end up with potentially difficult to debug issues, because
> users would not know that they should use requests. Not warning (or
> dev_dbg) has the same effect from my point of view, because users just
> see a device not working as expected. Is a customer log level the
> solution?

Custom level are hard to discover for sure, but the rules around dev_warn()
aren't mine. We can perhaps makes an argument that the driver is broken, but why
don't we fix it to match the V4L2 spec is still unknown, specially that its an
easy fix match the V4L2 spec (even if in practice, this is not quite usable).
Normally everything else we add above the V4L2 spec, specially stuff using
request get a domain specific spec. With that in hand, we could create better
rules, but otherwise I don't have much foundation to make a good call here.

Nicolas

> 
> Best regards,
> Stefan
> 
> > 
> > Nicolas
> > 
> > > 
> > > > > +         );
> > > > > + }
> > > > > +
> > > > >   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > > >                              &ctx->hdl);
> > > > > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Laurent Pinchart]

On Tue, Jan 06, 2026 at 10:27:31AM -0500, Nicolas Dufresne wrote:
> Le mardi 06 janvier 2026 à 15:29 +0100, Stefan Klug a écrit :
> > Quoting Nicolas Dufresne (2026-01-06 14:47:59)
> > > Le mardi 06 janvier 2026 à 02:42 +0200, Laurent Pinchart a écrit :
> > > > On Mon, Jan 05, 2026 at 01:58:25PM -0500, Nicolas Dufresne wrote:
> > > > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > > > Implement dynamic vertex map updates by handling the
> > > > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> > > > > > allows to implement features like dynamic zoom, pan, rotate and dewarp.
> > > > > > 
> > > > > > To stay compatible with the old version, updates of
> > > > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> > > > > > when requests are not used. Print a corresponding warning once.
> > > > > > 
> > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > > ---
> > > > > >  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
> > > > > > 
> > > 
> > > [...]
> > > 
> > > > > >   dev_dbg(&ctx->dw_dev->pdev->dev,
> > > > > >           "%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> > > > > > @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > >   switch (ctrl->id) {
> > > > > >   case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> > > > > >           ctx->user_map_is_set = true;
> > > > > > +         ctx->user_map_needs_update = true;
> > > > > 
> > > > > This will be called before the new mapping is applied by
> > > > > v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
> > > > > depth is high enough.
> > > > 
> > > > v4l2_ctrl_request_complete() does not apply a mapping, what am I missing
> > > > ?
> > > 
> > > Sorry for my confusion, after reading back, you are correct, this is called
> > > v4l2_ctrl_request_setup() inside the device_run() function. You can dismiss the
> > > bit about user_map_needs_update in my review (the paragraph below).
> > 
> > That means nothing has to change here, right?
> 
> Correct.
> 
> > > > > Instead, you should check in the request for the presence of
> > > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
> > > > > set this to true if if there is no request, in the case you also wanted to
> > > > > change the original behaviour of only updating that vertex on streamon, but I
> > > > > don't see much point though.
> > > > > 
> > > > > >           break;
> > > > > >   }
> > > > > >  
> > > > > > @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
> > > > > >   }
> > > > > >  
> > > > > >   ctx->user_map_is_set = false;
> > > > > > + ctx->user_map_needs_update = true;
> > > > > >  }
> > > > > >  
> > > > > >  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> > > > > > @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
> > > > > >   v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > > > >                           &ctx->hdl);
> > > > > >  
> > > > > > + if (src_buf->vb2_buf.req_obj.req) {
> > > > > > +         dw100_update_mapping(ctx);
> > > > > > + } else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> > > > > > +         ctx->warned_dynamic_update = true;
> > > > > > +         dev_warn(&ctx->dw_dev->pdev->dev,
> > > > > > +                 "V4L2 requests are required to update the vertex map dynamically"
> > > > > 
> > > > > Do you know about dev_warn_once() ? That being said, the driver is usable
> > > > > without requests and a static vertex (and must stay this way to not break the
> > > > > ABI). I don't think you should warn here at all.
> > 
> > My idea was that I'd like to see the warning once per context and not
> > once per boot. Afaik I can't use dev_warn_once() for that.
> 
> I didn't catch this detail (commit comment welcome), fair enough if that's the
> direction we are heading. Again, I don't understand why we don't just apply that
> on next device_run in "legacy" mode.

Because to be really useful, dynamic mode requires synchronization with
buffers. Sure, it's possible to implement a non-synchronized legacy mode
and let userspace handle synchronization, but that's suboptimal as it
will require never queuing more than one buffer to the kernel at a time.
I don't think we should implement APIs that let userspace do things in
the wrong way when it's possible and simple enough to do them right.
That would add code to the kernel that would increase the driver
complexity with no real upside.

> > > > Applications should move to using requests. We'll do so in libcamera
> > > > unconditionally. I don't expect many other direct users, so warning that
> > > > the mapping won't be applied when an application sets the corresponding
> > > > control during streaming without using requests seems fair to me. It
> > > > will help debugging issues.
> > > 
> > > It is also a miss-use of dev_warn which is meant to indicate a problem with the
> > > driver or the hardware. The V4L2 core uses dev_dbg() or customer log level for
> > > that in general. Also, don't re-implement _once() variants with
> > > warned_dynamic_update please. Personally, I would just let the out-of request
> > > change the control on the next device_run(), even if that means its out of sync.
> > 
> > But then you end up with potentially difficult to debug issues, because
> > users would not know that they should use requests. Not warning (or
> > dev_dbg) has the same effect from my point of view, because users just
> > see a device not working as expected. Is a customer log level the
> > solution?
> 
> Custom level are hard to discover for sure, but the rules around dev_warn()
> aren't mine. We can perhaps makes an argument that the driver is broken, but why
> don't we fix it to match the V4L2 spec is still unknown, specially that its an
> easy fix match the V4L2 spec (even if in practice, this is not quite usable).
> Normally everything else we add above the V4L2 spec, specially stuff using
> request get a domain specific spec. With that in hand, we could create better
> rules, but otherwise I don't have much foundation to make a good call here.
> 
> > > > > > +         );
> > > > > > + }
> > > > > > +
> > > > > >   v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > > > >                              &ctx->hdl);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

[Laurent Pinchart]

Hi Nicolas,

On Tue, Jan 06, 2026 at 08:47:59AM -0500, Nicolas Dufresne wrote:
> Le mardi 06 janvier 2026 à 02:42 +0200, Laurent Pinchart a écrit :
> > On Mon, Jan 05, 2026 at 01:58:25PM -0500, Nicolas Dufresne wrote:
> > > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > > Implement dynamic vertex map updates by handling the
> > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP control during streaming. This
> > > > allows to implement features like dynamic zoom, pan, rotate and dewarp.
> > > > 
> > > > To stay compatible with the old version, updates of
> > > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP are ignored during streaming
> > > > when requests are not used. Print a corresponding warning once.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > ---
> > > >  drivers/media/platform/nxp/dw100/dw100.c | 27 +++++++++++++++++++++------
> > > > 
> 
> [...]
> 
> > > >  	dev_dbg(&ctx->dw_dev->pdev->dev,
> > > >  		"%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> > > > @@ -351,6 +355,7 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >  	switch (ctrl->id) {
> > > >  	case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> > > >  		ctx->user_map_is_set = true;
> > > > +		ctx->user_map_needs_update = true;
> > > 
> > > This will be called before the new mapping is applied by
> > > v4l2_ctrl_request_complete(), and then may be cleared too soon if you queue
> > > depth is high enough.
> > 
> > v4l2_ctrl_request_complete() does not apply a mapping, what am I missing
> > ?
> 
> Sorry for my confusion, after reading back, you are correct, this is called
> v4l2_ctrl_request_setup() inside the device_run() function. You can dismiss the
> bit about user_map_needs_update in my review (the paragraph below).

Thanks for the clarification.

> > > Instead, you should check in the request for the presence of
> > > V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP to optimize the updates. Yo may still
> > > set this to true if if there is no request, in the case you also wanted to
> > > change the original behaviour of only updating that vertex on streamon, but I
> > > don't see much point though.
> > > 
> > > >  		break;
> > > >  	}
> > > >  
> > > > @@ -405,6 +410,7 @@ static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
> > > >  	}
> > > >  
> > > >  	ctx->user_map_is_set = false;
> > > > +	ctx->user_map_needs_update = true;
> > > >  }
> > > >  
> > > >  static const struct v4l2_ctrl_type_ops dw100_ctrl_type_ops = {
> > > > @@ -1482,6 +1488,15 @@ static void dw100_device_run(void *priv)
> > > >  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> > > >  				&ctx->hdl);
> > > >  
> > > > +	if (src_buf->vb2_buf.req_obj.req) {
> > > > +		dw100_update_mapping(ctx);
> > > > +	} else if (ctx->user_map_needs_update && !ctx->warned_dynamic_update) {
> > > > +		ctx->warned_dynamic_update = true;
> > > > +		dev_warn(&ctx->dw_dev->pdev->dev,
> > > > +			"V4L2 requests are required to update the vertex map dynamically"
> > > 
> > > Do you know about dev_warn_once() ? That being said, the driver is usable
> > > without requests and a static vertex (and must stay this way to not break the
> > > ABI). I don't think you should warn here at all.
> > 
> > Applications should move to using requests. We'll do so in libcamera
> > unconditionally. I don't expect many other direct users, so warning that
> > the mapping won't be applied when an application sets the corresponding
> > control during streaming without using requests seems fair to me. It
> > will help debugging issues.
> 
> It is also a miss-use of dev_warn which is meant to indicate a problem with the
> driver or the hardware. The V4L2 core uses dev_dbg() or customer log level for
> that in general. Also, don't re-implement _once() variants with
> warned_dynamic_update please.

Agreed about the _once() variant. That will move to printing the message
once per context to once globally, but I think it's fine given the
purpose of warning application developers of incorrect usage.

> Personally, I would just let the out-of request
> change the control on the next device_run(), even if that means its out of sync.

So you mean changing the current behaviour of applying the control at
next streamon to next .device_run() even if it means synchronization
between the control and the frame being processed can't be guaranteed ?
I don't think that's a good idea, synchronization is a key requirement.
We carried a local kernel patch that would do just that, and to make it
usable we had to ensure in libcamera that we never queued more than one
buffer at a time, to guarantee synchronous behaviour. I think
applications should use the request API for dynamic updates of the
dewarp map.

> > > > +		);
> > > > +	}
> > > > +
> > > >  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> > > >  				   &ctx->hdl);
> > > >  

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Stefan Klug]

On kernels with PREEMPT_RT enabled, a "BUG: scheduling while atomic"
kernel oops occurs inside dw100_irq_handler -> vb2_buffer_done. This is
because vb2_buffer_done takes a spinlock which is not allowed within
interrupt context on PREEMPT_RT.

Fix that by making the irq handler threaded. The threaded interrupt
handling might cause the interrupt line to be disabled a little longer
than before. As the line is not shared, this has no negative side
effects.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 drivers/media/platform/nxp/dw100/dw100.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
index 8a421059a1c9b55f514a29d3c2c5a6ffb76e0a64..4f5ef70e5f4a052fb5f208e35f8785f9d30dc54e 100644
--- a/drivers/media/platform/nxp/dw100/dw100.c
+++ b/drivers/media/platform/nxp/dw100/dw100.c
@@ -1600,8 +1600,9 @@ static int dw100_probe(struct platform_device *pdev)
 
 	pm_runtime_put_sync(&pdev->dev);
 
-	ret = devm_request_irq(&pdev->dev, irq, dw100_irq_handler, IRQF_ONESHOT,
-			       dev_name(&pdev->dev), dw_dev);
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					dw100_irq_handler, IRQF_ONESHOT,
+					dev_name(&pdev->dev), dw_dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
 		goto err_pm;

-- 
2.51.0

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Nicolas Dufresne]

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

Hi,

Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> On kernels with PREEMPT_RT enabled, a "BUG: scheduling while atomic"
> kernel oops occurs inside dw100_irq_handler -> vb2_buffer_done. This is
> because vb2_buffer_done takes a spinlock which is not allowed within
> interrupt context on PREEMPT_RT.
> 
> Fix that by making the irq handler threaded. The threaded interrupt
> handling might cause the interrupt line to be disabled a little longer
> than before. As the line is not shared, this has no negative side
> effects.

That's interesting, do you plan to update more drivers ? There is a lot of m2m
using hard IRQ to minimize the idle time (save a context switch), but RT support
might be more worth then that.

> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

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

> ---
>  drivers/media/platform/nxp/dw100/dw100.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> b/drivers/media/platform/nxp/dw100/dw100.c
> index
> 8a421059a1c9b55f514a29d3c2c5a6ffb76e0a64..4f5ef70e5f4a052fb5f208e35f8785f9d30d
> c54e 100644
> --- a/drivers/media/platform/nxp/dw100/dw100.c
> +++ b/drivers/media/platform/nxp/dw100/dw100.c
> @@ -1600,8 +1600,9 @@ static int dw100_probe(struct platform_device *pdev)
>  
>  	pm_runtime_put_sync(&pdev->dev);
>  
> -	ret = devm_request_irq(&pdev->dev, irq, dw100_irq_handler,
> IRQF_ONESHOT,
> -			       dev_name(&pdev->dev), dw_dev);
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					dw100_irq_handler, IRQF_ONESHOT,
> +					dev_name(&pdev->dev), dw_dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
>  		goto err_pm;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 02:02:38PM -0500, Nicolas Dufresne wrote:
> Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > On kernels with PREEMPT_RT enabled, a "BUG: scheduling while atomic"
> > kernel oops occurs inside dw100_irq_handler -> vb2_buffer_done. This is
> > because vb2_buffer_done takes a spinlock which is not allowed within
> > interrupt context on PREEMPT_RT.
> > 
> > Fix that by making the irq handler threaded. The threaded interrupt
> > handling might cause the interrupt line to be disabled a little longer
> > than before. As the line is not shared, this has no negative side
> > effects.
> 
> That's interesting, do you plan to update more drivers ? There is a lot of m2m
> using hard IRQ to minimize the idle time (save a context switch), but RT support
> might be more worth then that.

This is a part of PREEMPT_RT that puzzles me. By turning regular
spinlocks into mutexes, RT seems to break drivers that use those
spinlocks in hard IRQ handlers. That's a very large number of drivers
given how widespread regular spinlock usage is. Do drivers need to be
manually converted to either raw spinlocks or threaded IRQ handlers ?
What about non-RT kernels, how can a driver avoid the thread scheduling
penalty in those cases, do they need to manually select between
request_irq() and request_threaded_irq() based on if RT is enabled ?
This puzzles me, it feels like I must be missing something.

> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> > ---
> >  drivers/media/platform/nxp/dw100/dw100.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> > b/drivers/media/platform/nxp/dw100/dw100.c
> > index
> > 8a421059a1c9b55f514a29d3c2c5a6ffb76e0a64..4f5ef70e5f4a052fb5f208e35f8785f9d30d
> > c54e 100644
> > --- a/drivers/media/platform/nxp/dw100/dw100.c
> > +++ b/drivers/media/platform/nxp/dw100/dw100.c
> > @@ -1600,8 +1600,9 @@ static int dw100_probe(struct platform_device *pdev)
> >  
> >  	pm_runtime_put_sync(&pdev->dev);
> >  
> > -	ret = devm_request_irq(&pdev->dev, irq, dw100_irq_handler, IRQF_ONESHOT,
> > -			       dev_name(&pdev->dev), dw_dev);
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +					dw100_irq_handler, IRQF_ONESHOT,
> > +					dev_name(&pdev->dev), dw_dev);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
> >  		goto err_pm;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Steven Rostedt]

On Tue, 6 Jan 2026 01:59:21 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> > That's interesting, do you plan to update more drivers ? There is a lot of m2m
> > using hard IRQ to minimize the idle time (save a context switch), but RT support
> > might be more worth then that.  
> 
> This is a part of PREEMPT_RT that puzzles me. By turning regular
> spinlocks into mutexes, RT seems to break drivers that use those
> spinlocks in hard IRQ handlers. That's a very large number of drivers
> given how widespread regular spinlock usage is. Do drivers need to be
> manually converted to either raw spinlocks or threaded IRQ handlers ?

No. Pretty much all interrupts are converted into threaded interrupt
handlers unless IRQF_NO_THREAD, IRQF_PERCPU, or IRQF_ONESHOT are specified.

The interrupt line is disabled until the thread handler is called.


> What about non-RT kernels, how can a driver avoid the thread scheduling
> penalty in those cases, do they need to manually select between
> request_irq() and request_threaded_irq() based on if RT is enabled ?
> This puzzles me, it feels like I must be missing something.

The issue here is that the interrupt handler specifies ONESHOT which causes
the handler to be executed in hard interrupt context.

-- Steve

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 07:39:33PM -0500, Steven Rostedt wrote:
> On Tue, 6 Jan 2026 01:59:21 +0200 Laurent Pinchart wrote:
> 
> > > That's interesting, do you plan to update more drivers ? There is a lot of m2m
> > > using hard IRQ to minimize the idle time (save a context switch), but RT support
> > > might be more worth then that.  
> > 
> > This is a part of PREEMPT_RT that puzzles me. By turning regular
> > spinlocks into mutexes, RT seems to break drivers that use those
> > spinlocks in hard IRQ handlers. That's a very large number of drivers
> > given how widespread regular spinlock usage is. Do drivers need to be
> > manually converted to either raw spinlocks or threaded IRQ handlers ?
> 
> No. Pretty much all interrupts are converted into threaded interrupt
> handlers unless IRQF_NO_THREAD, IRQF_PERCPU, or IRQF_ONESHOT are specified.
> 
> The interrupt line is disabled until the thread handler is called.
> 
> > What about non-RT kernels, how can a driver avoid the thread scheduling
> > penalty in those cases, do they need to manually select between
> > request_irq() and request_threaded_irq() based on if RT is enabled ?
> > This puzzles me, it feels like I must be missing something.
> 
> The issue here is that the interrupt handler specifies ONESHOT which causes
> the handler to be executed in hard interrupt context.

Gotcha.

Stefan, please explain in the commit message why the ONESHOT flag is
set by the driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Stefan Klug]

Quoting Laurent Pinchart (2026-01-06 01:49:28)
> On Mon, Jan 05, 2026 at 07:39:33PM -0500, Steven Rostedt wrote:
> > On Tue, 6 Jan 2026 01:59:21 +0200 Laurent Pinchart wrote:
> > 
> > > > That's interesting, do you plan to update more drivers ? There is a lot of m2m
> > > > using hard IRQ to minimize the idle time (save a context switch), but RT support
> > > > might be more worth then that.  
> > > 
> > > This is a part of PREEMPT_RT that puzzles me. By turning regular
> > > spinlocks into mutexes, RT seems to break drivers that use those
> > > spinlocks in hard IRQ handlers. That's a very large number of drivers
> > > given how widespread regular spinlock usage is. Do drivers need to be
> > > manually converted to either raw spinlocks or threaded IRQ handlers ?
> > 
> > No. Pretty much all interrupts are converted into threaded interrupt
> > handlers unless IRQF_NO_THREAD, IRQF_PERCPU, or IRQF_ONESHOT are specified.
> > 
> > The interrupt line is disabled until the thread handler is called.
> > 
> > > What about non-RT kernels, how can a driver avoid the thread scheduling
> > > penalty in those cases, do they need to manually select between
> > > request_irq() and request_threaded_irq() based on if RT is enabled ?
> > > This puzzles me, it feels like I must be missing something.
> > 
> > The issue here is that the interrupt handler specifies ONESHOT which causes
> > the handler to be executed in hard interrupt context.
> 
> Gotcha.
> 
> Stefan, please explain in the commit message why the ONESHOT flag is
> set by the driver.

Hah, if I knew that :-).

The pieces I have are:
In the DT the interrupt line is marked as IRQ_TYPE_LEVEL_HIGH. I don't
know why and couldn't find a reference to that in the reference manual.
Assuming it is a level interrupt, then it makes sense to treat it as ONESHOT,
otherwise it would fire again immediately after handling the hard
interrupt...but it was a hard interrupt in first place - huh.

I just realize that I still miss a bit of the puzzle:
ONESHOT is doumented as:

"Interrupt is not reenabled after the hardirq handler finished. Used by
threaded interrupts which need to keep the irq line disabled until the
threaded handler has been run."

That makes perfect sense. So ONESHOT disables the irq line until the
thread_fn has completed (if it was set). Now on preempt_rt inside
irq_setup_forced_threading() we don't force threading if ONESHOT is
requested. Why is that?

So I'm left with two questions:
- Why aren't ONESHOT irq handlers forced to threaded on preempt_rt?
- Why was ONESHOT requested in first place as to my current knowledge it
  really only makes sense if a thread_fn is defined.

Did I just answer my own question? ONESHOT only makes sense if there is
a thread_fn and it is assumed that the hard handler is necessary. So
preempt_rt doesn't try to change that?

That would mean the ONESHOT in the dw100 was not necessary in first
place but didn't do any harm until preempt_rt was enabled... And if
ONSHOT is *not* set preempt_rt would automatically force the irq handler
to be threaded and set the ONESHOT flag in irq_setup_forced_threading().

So everything would be fine except that we'd still hit the timeout issue
from patch 4/4.

So if I got that right, the dw100 driver is in the unfortunate
situation, that the irq handler consists of two parts where the first
part *must* run in hard interrupt context and the second part *should* run
in hard interrupt context but it is fine if it becomes threaded due to
preempt_rt. As we can't model that, the best we can do is to always run
the second part threaded...

So patch 4/4 seems correct until we get new information about the
hardware.

Any thoughts?

Best regards,
Stefan

> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Sebastian Andrzej Siewior]

On 2026-01-06 18:11:27 [+0100], Stefan Klug wrote:
> Hah, if I knew that :-).
> 
> The pieces I have are:
> In the DT the interrupt line is marked as IRQ_TYPE_LEVEL_HIGH. I don't
> know why and couldn't find a reference to that in the reference manual.

It is either a LEVEL interrupt or just marked as such. But it seems to
behave as such.

> Assuming it is a level interrupt, then it makes sense to treat it as ONESHOT,
> otherwise it would fire again immediately after handling the hard
> interrupt...but it was a hard interrupt in first place - huh.

So setting it ONESHOT while it is non-threaded does not seem to make
sense, correct.

> I just realize that I still miss a bit of the puzzle:
> ONESHOT is doumented as:
> 
> "Interrupt is not reenabled after the hardirq handler finished. Used by
> threaded interrupts which need to keep the irq line disabled until the
> threaded handler has been run."
> 
> That makes perfect sense. So ONESHOT disables the irq line until the
> thread_fn has completed (if it was set). Now on preempt_rt inside
> irq_setup_forced_threading() we don't force threading if ONESHOT is
> requested. Why is that?

Because ONESHOT is usually used where there is no primary handler/ the
primary handler does just a wake of thread.

> So I'm left with two questions:
> - Why aren't ONESHOT irq handlers forced to threaded on preempt_rt?

See above. Also PREEMPT_RT just enforces the kernel command line
threadirqs

> - Why was ONESHOT requested in first place as to my current knowledge it
>   really only makes sense if a thread_fn is defined.

I would say it was a mistake and nobody noticed it. There is no visible
difference if there is just the primary handler and the system does not
use threadirqs (or PREEMPT_RT which enforces it).

> Did I just answer my own question? ONESHOT only makes sense if there is
> a thread_fn and it is assumed that the hard handler is necessary. So
> preempt_rt doesn't try to change that?

Yes. ONESHOT is used if the interrupt source within the IRQ chip has to
be masked until after the thread completed. So setting ONESHOT without a
threaded handler is dubious.

> That would mean the ONESHOT in the dw100 was not necessary in first
> place but didn't do any harm until preempt_rt was enabled... And if
> ONSHOT is *not* set preempt_rt would automatically force the irq handler
> to be threaded and set the ONESHOT flag in irq_setup_forced_threading().

correct.

> So everything would be fine except that we'd still hit the timeout issue
> from patch 4/4.
> 
> So if I got that right, the dw100 driver is in the unfortunate
> situation, that the irq handler consists of two parts where the first
> part *must* run in hard interrupt context and the second part *should* run
> in hard interrupt context but it is fine if it becomes threaded due to
> preempt_rt. As we can't model that, the best we can do is to always run
> the second part threaded...

So happens if you avoid the IRQF_ONESHOT? Do you still get these
timeout errors?

> So patch 4/4 seems correct until we get new information about the
> hardware.
> 
> Any thoughts?
> 
> Best regards,
> Stefan

Sebastian

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Stefan Klug]

Hi Sebastian,

Thanks for your support.

Quoting Sebastian Andrzej Siewior (2026-01-12 12:43:13)
> On 2026-01-06 18:11:27 [+0100], Stefan Klug wrote:
> > Hah, if I knew that :-).
> > 
> > The pieces I have are:
> > In the DT the interrupt line is marked as IRQ_TYPE_LEVEL_HIGH. I don't
> > know why and couldn't find a reference to that in the reference manual.
> 
> It is either a LEVEL interrupt or just marked as such. But it seems to
> behave as such.
> 
> > Assuming it is a level interrupt, then it makes sense to treat it as ONESHOT,
> > otherwise it would fire again immediately after handling the hard
> > interrupt...but it was a hard interrupt in first place - huh.
> 
> So setting it ONESHOT while it is non-threaded does not seem to make
> sense, correct.
> 
> > I just realize that I still miss a bit of the puzzle:
> > ONESHOT is doumented as:
> > 
> > "Interrupt is not reenabled after the hardirq handler finished. Used by
> > threaded interrupts which need to keep the irq line disabled until the
> > threaded handler has been run."
> > 
> > That makes perfect sense. So ONESHOT disables the irq line until the
> > thread_fn has completed (if it was set). Now on preempt_rt inside
> > irq_setup_forced_threading() we don't force threading if ONESHOT is
> > requested. Why is that?
> 
> Because ONESHOT is usually used where there is no primary handler/ the
> primary handler does just a wake of thread.
> 
> > So I'm left with two questions:
> > - Why aren't ONESHOT irq handlers forced to threaded on preempt_rt?
> 
> See above. Also PREEMPT_RT just enforces the kernel command line
> threadirqs
> 
> > - Why was ONESHOT requested in first place as to my current knowledge it
> >   really only makes sense if a thread_fn is defined.
> 
> I would say it was a mistake and nobody noticed it. There is no visible
> difference if there is just the primary handler and the system does not
> use threadirqs (or PREEMPT_RT which enforces it).
> 
> > Did I just answer my own question? ONESHOT only makes sense if there is
> > a thread_fn and it is assumed that the hard handler is necessary. So
> > preempt_rt doesn't try to change that?
> 
> Yes. ONESHOT is used if the interrupt source within the IRQ chip has to
> be masked until after the thread completed. So setting ONESHOT without a
> threaded handler is dubious.
> 
> > That would mean the ONESHOT in the dw100 was not necessary in first
> > place but didn't do any harm until preempt_rt was enabled... And if
> > ONSHOT is *not* set preempt_rt would automatically force the irq handler
> > to be threaded and set the ONESHOT flag in irq_setup_forced_threading().
> 
> correct.
> 
> > So everything would be fine except that we'd still hit the timeout issue
> > from patch 4/4.
> > 
> > So if I got that right, the dw100 driver is in the unfortunate
> > situation, that the irq handler consists of two parts where the first
> > part *must* run in hard interrupt context and the second part *should* run
> > in hard interrupt context but it is fine if it becomes threaded due to
> > preempt_rt. As we can't model that, the best we can do is to always run
> > the second part threaded...
> 
> So happens if you avoid the IRQF_ONESHOT? Do you still get these
> timeout errors?

I did a bit more testing and got results that I fail to completely
understand.

If I enable IRQF_ONESHOT and use the threaded_fn, on a non PREEMPT_RT
system I regularly observe the timeout message.

If I pass irqflags=0 and use the hard handler on a PREEMPT_RT system I
expected the same behavior (as the hard handler gets changed to be
threaded and implicitely ONESHOT is set). But I don't see the timeout
messages.

Is there anything else that I need to do on PREEMPT_RT to force the
threaded behavior besides enabling the config? Or is the irq thread
running with higher priority and therefore possibly faster?

Running irqflags=0 and the hard handler on a non PREEMPT_RT system
didn't have any negative side effects. So maybe that is really the
solution...

I'll ping Xavier if he has more details on the hardware.

Best regards,
Stefan

> 
> > So patch 4/4 seems correct until we get new information about the
> > hardware.
> > 
> > Any thoughts?
> > 
> > Best regards,
> > Stefan
> 
> Sebastian
>

Re: [PATCH 3/4] media: dw100: Fix kernel oops with PREEMPT_RT enabled

[Sebastian Andrzej Siewior]

On 2026-01-14 18:22:34 [+0100], Stefan Klug wrote:
> Hi Sebastian,
Hi,

sorry for being late…

> I did a bit more testing and got results that I fail to completely
> understand.
> 
> If I enable IRQF_ONESHOT and use the threaded_fn, on a non PREEMPT_RT
> system I regularly observe the timeout message.
> 
> If I pass irqflags=0 and use the hard handler on a PREEMPT_RT system I
> expected the same behavior (as the hard handler gets changed to be
> threaded and implicitely ONESHOT is set). But I don't see the timeout
> messages.

If you have the driver with the removed ONESHOT and boot !RT with
threadirqs then you should have the same behaviour. RT threads all
handler not just one. This might change the behaviour if all interrupts
are served by one CPU and some interrupts are handled prior and have an
effect on this (threaded) one.

> Is there anything else that I need to do on PREEMPT_RT to force the
> threaded behavior besides enabling the config? Or is the irq thread
> running with higher priority and therefore possibly faster?

In both configurations (RT and !RT with threaded interrupts) the
interrupt handed is running at SCHED_FIFO prio 50. This has a higher
priority than the "normal" userland which runs at SCHED_OTHER but the
same priority as the remaining threaded interrupts.

> Running irqflags=0 and the hard handler on a non PREEMPT_RT system
> didn't have any negative side effects. So maybe that is really the
> solution...
> 
> I'll ping Xavier if he has more details on the hardware.
> 
> Best regards,
> Stefan

Sebastian

[PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Stefan Klug]

In the previous commit, the interrupt handler was changed to threaded.
This sometimes leads to DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT being
set after changing the vertex map. This can be seen by repeated error
outputs in dmesg:

dw100 32e30000.dwe: Interrupt error: 0x1

As there is no documentation available, it is unclear why that happens
and if this condition can simply be ignored. By splitting the interrupt
handling into two parts and only handling the dw100_job_finish() within
the threaded part, the error does not occur anymore.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

As noted on the cover letter, this patch still is intended to start the
discussion for a proper fix.

While writing this I noted that when
DW100_INTERRUPT_STATUS_INT_FRAME_DONE is set, the job gets finished
without error even when err_irqs != 0. Is that on purpose?
---
 drivers/media/platform/nxp/dw100/dw100.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
index 4f5ef70e5f4a052fb5f208e35f8785f9d30dc54e..67d941bdf768398edc611c94896cc42a70b88225 100644
--- a/drivers/media/platform/nxp/dw100/dw100.c
+++ b/drivers/media/platform/nxp/dw100/dw100.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/debugfs.h>
 #include <linux/interrupt.h>
+#include <linux/irqreturn.h>
 #include <linux/io.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
@@ -74,6 +75,7 @@ struct dw100_device {
 	struct clk_bulk_data		*clks;
 	int				num_clks;
 	struct dentry			*debugfs_root;
+	bool				frame_failed;
 };
 
 struct dw100_q_data {
@@ -1411,7 +1413,8 @@ static irqreturn_t dw100_irq_handler(int irq, void *dev_id)
 {
 	struct dw100_device *dw_dev = dev_id;
 	u32 pending_irqs, err_irqs, frame_done_irq;
-	bool with_error = true;
+
+	dw_dev->frame_failed = true;
 
 	pending_irqs = dw_hw_get_pending_irqs(dw_dev);
 	frame_done_irq = pending_irqs & DW100_INTERRUPT_STATUS_INT_FRAME_DONE;
@@ -1419,7 +1422,7 @@ static irqreturn_t dw100_irq_handler(int irq, void *dev_id)
 
 	if (frame_done_irq) {
 		dev_dbg(&dw_dev->pdev->dev, "Frame done interrupt\n");
-		with_error = false;
+		dw_dev->frame_failed = false;
 		err_irqs &= ~DW100_INTERRUPT_STATUS_INT_ERR_STATUS
 			(DW100_INTERRUPT_STATUS_INT_ERR_FRAME_DONE);
 	}
@@ -1432,7 +1435,14 @@ static irqreturn_t dw100_irq_handler(int irq, void *dev_id)
 	dw100_hw_clear_irq(dw_dev, pending_irqs |
 			   DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT);
 
-	dw100_job_finish(dw_dev, with_error);
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t dw100_irq_thread_fn(int irq, void *dev_id)
+{
+	struct dw100_device *dw_dev = dev_id;
+
+	dw100_job_finish(dw_dev, dw_dev->frame_failed);
 
 	return IRQ_HANDLED;
 }
@@ -1600,8 +1610,8 @@ static int dw100_probe(struct platform_device *pdev)
 
 	pm_runtime_put_sync(&pdev->dev);
 
-	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
-					dw100_irq_handler, IRQF_ONESHOT,
+	ret = devm_request_threaded_irq(&pdev->dev, irq, dw100_irq_handler,
+					dw100_irq_thread_fn, IRQF_ONESHOT,
 					dev_name(&pdev->dev), dw_dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);

-- 
2.51.0

Re: [PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Nicolas Dufresne]

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

Hi,

Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> In the previous commit, the interrupt handler was changed to threaded.
> This sometimes leads to DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT being
> set after changing the vertex map. This can be seen by repeated error
> outputs in dmesg:
> 
> dw100 32e30000.dwe: Interrupt error: 0x1
> 
> As there is no documentation available, it is unclear why that happens
> and if this condition can simply be ignored. By splitting the interrupt
> handling into two parts and only handling the dw100_job_finish() within
> the threaded part, the error does not occur anymore.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Ok, but arguably, this could be squashed.

Nicolas

> 
> ---
> 
> As noted on the cover letter, this patch still is intended to start the
> discussion for a proper fix.
> 
> While writing this I noted that when
> DW100_INTERRUPT_STATUS_INT_FRAME_DONE is set, the job gets finished
> without error even when err_irqs != 0. Is that on purpose?
> ---
>  drivers/media/platform/nxp/dw100/dw100.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/dw100/dw100.c
> b/drivers/media/platform/nxp/dw100/dw100.c
> index
> 4f5ef70e5f4a052fb5f208e35f8785f9d30dc54e..67d941bdf768398edc611c94896cc42a70b8
> 8225 100644
> --- a/drivers/media/platform/nxp/dw100/dw100.c
> +++ b/drivers/media/platform/nxp/dw100/dw100.c
> @@ -10,6 +10,7 @@
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
>  #include <linux/interrupt.h>
> +#include <linux/irqreturn.h>
>  #include <linux/io.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> @@ -74,6 +75,7 @@ struct dw100_device {
>  	struct clk_bulk_data		*clks;
>  	int				num_clks;
>  	struct dentry			*debugfs_root;
> +	bool				frame_failed;
>  };
>  
>  struct dw100_q_data {
> @@ -1411,7 +1413,8 @@ static irqreturn_t dw100_irq_handler(int irq, void
> *dev_id)
>  {
>  	struct dw100_device *dw_dev = dev_id;
>  	u32 pending_irqs, err_irqs, frame_done_irq;
> -	bool with_error = true;
> +
> +	dw_dev->frame_failed = true;
>  
>  	pending_irqs = dw_hw_get_pending_irqs(dw_dev);
>  	frame_done_irq = pending_irqs &
> DW100_INTERRUPT_STATUS_INT_FRAME_DONE;
> @@ -1419,7 +1422,7 @@ static irqreturn_t dw100_irq_handler(int irq, void
> *dev_id)
>  
>  	if (frame_done_irq) {
>  		dev_dbg(&dw_dev->pdev->dev, "Frame done interrupt\n");
> -		with_error = false;
> +		dw_dev->frame_failed = false;
>  		err_irqs &= ~DW100_INTERRUPT_STATUS_INT_ERR_STATUS
>  			(DW100_INTERRUPT_STATUS_INT_ERR_FRAME_DONE);
>  	}
> @@ -1432,7 +1435,14 @@ static irqreturn_t dw100_irq_handler(int irq, void
> *dev_id)
>  	dw100_hw_clear_irq(dw_dev, pending_irqs |
>  			   DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT);
>  
> -	dw100_job_finish(dw_dev, with_error);
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t dw100_irq_thread_fn(int irq, void *dev_id)
> +{
> +	struct dw100_device *dw_dev = dev_id;
> +
> +	dw100_job_finish(dw_dev, dw_dev->frame_failed);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1600,8 +1610,8 @@ static int dw100_probe(struct platform_device *pdev)
>  
>  	pm_runtime_put_sync(&pdev->dev);
>  
> -	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> -					dw100_irq_handler, IRQF_ONESHOT,
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, dw100_irq_handler,
> +					dw100_irq_thread_fn, IRQF_ONESHOT,
>  					dev_name(&pdev->dev), dw_dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Steven Rostedt]

On Mon, 05 Jan 2026 14:03:58 -0500
Nicolas Dufresne <nicolas@ndufresne.ca> wrote:

> Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > In the previous commit, the interrupt handler was changed to threaded.
> > This sometimes leads to DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT being
> > set after changing the vertex map. This can be seen by repeated error
> > outputs in dmesg:
> > 
> > dw100 32e30000.dwe: Interrupt error: 0x1
> > 
> > As there is no documentation available, it is unclear why that happens
> > and if this condition can simply be ignored. By splitting the interrupt
> > handling into two parts and only handling the dw100_job_finish() within
> > the threaded part, the error does not occur anymore.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>  
> 
> Ok, but arguably, this could be squashed.

Agreed. Because it doesn't seem to make sense to have a oneshot threaded
irq handler that doesn't have the two parts (non-threaded to acknowledge the
irq, and the threaded to handle it and re-enable it).

-- Steve

Re: [PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 04:37:48PM -0500, Steven Rostedt wrote:
> On Mon, 05 Jan 2026 14:03:58 -0500 Nicolas Dufresne wrote:
> > Le lundi 05 janvier 2026 à 12:35 +0100, Stefan Klug a écrit :
> > > In the previous commit, the interrupt handler was changed to threaded.
> > > This sometimes leads to DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT being
> > > set after changing the vertex map. This can be seen by repeated error
> > > outputs in dmesg:
> > > 
> > > dw100 32e30000.dwe: Interrupt error: 0x1
> > > 
> > > As there is no documentation available, it is unclear why that happens
> > > and if this condition can simply be ignored. By splitting the interrupt
> > > handling into two parts and only handling the dw100_job_finish() within
> > > the threaded part, the error does not occur anymore.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>  
> > 
> > Ok, but arguably, this could be squashed.

Stefan mentioned that in the cover letter, yes. The patches are
currently split because 4/4 shouldn't be needed based on our
understanding of the hardware. We're hoping for feedback on the issue
from someone with knowledge of the DW100 and access to its
documentation.

> Agreed. Because it doesn't seem to make sense to have a oneshot threaded
> irq handler that doesn't have the two parts (non-threaded to acknowledge the
> irq, and the threaded to handle it and re-enable it).

Why is so ? Isn't oneshot meant exactly for this purpose ? It's
documented as not reenabling the interrupt after the hardirq handler
(which is absent after 3/4) returns, why would a hardirq handler be
mandatory then ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Steven Rostedt]

On Tue, 6 Jan 2026 01:44:52 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> > Agreed. Because it doesn't seem to make sense to have a oneshot threaded
> > irq handler that doesn't have the two parts (non-threaded to acknowledge the
> > irq, and the threaded to handle it and re-enable it).  
> 
> Why is so ? Isn't oneshot meant exactly for this purpose ? It's
> documented as not reenabling the interrupt after the hardirq handler
> (which is absent after 3/4) returns, why would a hardirq handler be
> mandatory then ?

Because it's timing out. The error in the change log states:

    In the previous commit, the interrupt handler was changed to threaded.
    This sometimes leads to DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT being
    set after changing the vertex map. This can be seen by repeated error
    outputs in dmesg:

    dw100 32e30000.dwe: Interrupt error: 0x1

It needs to be acknowledged in a timely manner. That is best done in the
hard irq context where no locks need to be taken. It looks like the handler
also disables the interrupt on the device and will be reenabled after the
handler has completed (in thread context).

-- Steve

Re: [PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 07:43:50PM -0500, Steven Rostedt wrote:
> On Tue, 6 Jan 2026 01:44:52 +0200 Laurent Pinchart wrote:
> 
> > > Agreed. Because it doesn't seem to make sense to have a oneshot threaded
> > > irq handler that doesn't have the two parts (non-threaded to acknowledge the
> > > irq, and the threaded to handle it and re-enable it).  
> > 
> > Why is so ? Isn't oneshot meant exactly for this purpose ? It's
> > documented as not reenabling the interrupt after the hardirq handler
> > (which is absent after 3/4) returns, why would a hardirq handler be
> > mandatory then ?
> 
> Because it's timing out. The error in the change log states:
> 
>     In the previous commit, the interrupt handler was changed to threaded.
>     This sometimes leads to DW100_INTERRUPT_STATUS_INT_ERR_TIME_OUT being
>     set after changing the vertex map. This can be seen by repeated error
>     outputs in dmesg:
> 
>     dw100 32e30000.dwe: Interrupt error: 0x1
> 
> It needs to be acknowledged in a timely manner. That is best done in the
> hard irq context where no locks need to be taken. It looks like the handler
> also disables the interrupt on the device and will be reenabled after the
> handler has completed (in thread context).

My point is that we (neither I nor Stefan) don't know why it's "timing
out" and what it means. There's no documentation publicly available.
I'd like to get to the bottom of this.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] media: dw100: Split interrupt handler to fix timeout error

[Steven Rostedt]

On Tue, 6 Jan 2026 02:51:35 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> My point is that we (neither I nor Stefan) don't know why it's "timing
> out" and what it means. There's no documentation publicly available.
> I'd like to get to the bottom of this.

OK, that's beyond my knowledge as it's independent from PREEMPT_RT ;-)

-- Steve