Message-ID: <542f97c1b2dee066f9961b508958f7397faaaf8f.camel@ndufresne.ca>
Date: Tue, 06 Jan 2026 10:27:31 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Stefan Klug <stefan.klug@...asonboard.com>, Laurent Pinchart
	 <laurent.pinchart@...asonboard.com>
Cc: 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
Subject: Re: [PATCH 2/4] media: dw100: Implement dynamic vertex map update

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists