Message-ID: <20260106143539.GA26157@pendragon.ideasonboard.com>
Date: Tue, 6 Jan 2026 16:35:39 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>
Cc: Stefan Klug <stefan.klug@...asonboard.com>,
	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

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

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

Powered by blists - more mailing lists