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

[Stefan Klug <stefan.klug@ideasonboard.com>]

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);
> > > >