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

[Stefan Klug]

Hi Laurent,

Thank you for the review.

Quoting Laurent Pinchart (2026-02-10 18:50:35)
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Thu, Jan 29, 2026 at 12:43:10PM +0100, Stefan Klug wrote:
> > 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
> 
> Once this gets merged it won't be a patch any more :-) As commit
> messages are written in an imperative mood style,
> 
> s/This patch adds/Add/
> 
> > prepare for dynamic vertex map updates.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@xxxxxxxxxxxxxxxx>
> > 
> > ---
> > 
> > Changes in v2:
> > - Use v4l2_m2m_buf_done_and_job_finish() to mark the buffers as done in
> >   the correct order.
> > 
> > Changes in v1:
> > - Moved v4l2_ctrl_request_complete into dw100_device_run
> > ---
> >  drivers/media/platform/nxp/dw100/dw100.c | 49 +++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> > index 4aaf9c3fff5397f0441944ee926f2c8ba6fc864a..1cb895da9912371a2b23ca62412c572d9cb75c00 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)
> > @@ -1058,7 +1077,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 +1087,13 @@ 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, curr_ctx->fh.m2m_ctx,
> > +                                      buf_state);
> >  
> >       dev_dbg(&dw_dev->pdev->dev, "Finishing transaction with%s error(s)\n",
> >               with_error ? "" : "out");
> > @@ -1460,6 +1475,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);
> >  
> 
> I'd add a comment here, as the complete() call before start() can be
> confusing.
> 
>         /*
>          * Apply controls from the request to the device and copy back the value
>          * of volatile controls to the request. We can do the latter before
>          * starting the dewarper as no controls are updated as a result of the
>          * hardware operation.
>          /
> 
> It could be me not being familiar enough with the API, but I think
> discussions during review of v2 showed that this confused other people
> too.

I was just applying the comment to the code when I realized that I had
difficulties parsing it. Main cause is that it is just above the call to
v4l2_ctrl_request_setup() which is imho not the questionable call. Would
you be fine with a shorter comment above v4l2_ctrl_request_complete()
only?

	/*
         * As the hardware does not update any volatile controls, we can
         * complete control handling before starting the dewarper.
         */
	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
				   &ctx->hdl);

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

Thanks,
Stefan

> 
> > +     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 +1488,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 +1604,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