Message-ID: <20260106145354.GB26157@pendragon.ideasonboard.com>
Date: Tue, 6 Jan 2026 16:53:54 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Stefan Klug <stefan.klug@...asonboard.com>
Cc: Nicolas Dufresne <nicolas@...fresne.ca>,
	Xavier Roumegue <xavier.roumegue@....nxp.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Clark Williams <clrkwllms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	Hans Verkuil <hans@...erkuil.nl>
Subject: Re: [PATCH 1/4] media: dw100: Implement V4L2 requests support

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@...asonboard.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

Powered by blists - more mailing lists