Re: [PATCH] media: dw100: Enable dynamic vertex map

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Oct 28, 2024 at 08:32:51PM +0530, Umang Jain wrote:
> Hi Laurent,
> 
> On 28/10/24 8:11 pm, Laurent Pinchart wrote:
> > Hi Umang,
> >
> > On Mon, Oct 28, 2024 at 07:47:46PM +0530, Umang Jain wrote:
> >> On 27/10/24 8:10 pm, Laurent Pinchart wrote:
> >>> On Sat, Oct 26, 2024 at 09:52:43PM +0200, Xavier Roumegue wrote:
> >>>> On 10/22/24 8:31 AM, Umang Jain wrote:
> >>>>> Currently, vertex maps cannot be updated dynamically while dw100
> >>>>> is streaming. This patch enables the support to update the vertex
> >>>>> map dynamically at runtime.
> >>>>>
> >>>>> To support this functionality, we need to allocate and track two
> >>>>> sets of DMA-allocated vertex maps, one for the currently applied map
> >>>>> and another for the updated (pending) map. Before the start of next
> >>>>> frame, if a new user-supplied vertex map is available, the hardware
> >>>>> mapping is changed to use new vertex map, thus enabling the user to
> >>>>> update the vertex map at runtime.
> >>> How do you synchronize the new map with the jobs ? That doesn't seem to
> >>> be supported by the patch, is it a feature that you don't need ?
> >>>
> >>>>> We should ensure no race occurs when the vertex map is updated multiple
> >>>>> times when a frame is processing. Hence, vertex map is never updated to
> >>>>> the applied vertex map index in .s_ctrl(). It is always updated on the
> >>>>> pending vertex map slot, with `maps_mutex` lock held. `maps_mutex` lock
> >>>>> is also held when the pending vertex map is applied to the hardware in
> >>>>> dw100_start().
> >>>>>
> >>>>> Ability to update the vertex map at runtime, enables abritrary rotation
> >>> s/abritrary/arbitrary/
> >>>
> >>>>> and digital zoom features for the input frames, through the dw100
> >>>>> hardware.
> >>>>>
> >>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >>>>> ---
> >>>>>     drivers/media/platform/nxp/dw100/dw100.c | 73 ++++++++++++++++++------
> >>>>>     1 file changed, 56 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> >>>>> index 54ebf59df682..42712ccff754 100644
> >>>>> --- a/drivers/media/platform/nxp/dw100/dw100.c
> >>>>> +++ b/drivers/media/platform/nxp/dw100/dw100.c
> >>>>> @@ -83,6 +83,11 @@ struct dw100_q_data {
> >>>>>     	struct v4l2_rect		crop;
> >>>>>     };
> >>>>>     
> >>>>> +struct dw100_map {
> >>>>> +	unsigned int *map;
> >>>>> +	dma_addr_t map_dma;
> >>> I would have called the field just 'dma' as it's already qualified by
> >>> the structure name or the field name in dw100_ctx.
> >>>
> >>>>> +};
> >>>>> +
> >>>>>     struct dw100_ctx {
> >>>>>     	struct v4l2_fh			fh;
> >>>>>     	struct dw100_device		*dw_dev;
> >>>>> @@ -92,12 +97,14 @@ struct dw100_ctx {
> >>>>>     	struct mutex			vq_mutex;
> >>>>>     
> >>>>>     	/* Look Up Table for pixel remapping */
> >>>>> -	unsigned int			*map;
> >>>>> -	dma_addr_t			map_dma;
> >>>>> +	struct dw100_map		maps[2];
> >>>>> +	unsigned int			applied_map_id;
> >>>>>     	size_t				map_size;
> >>>>>     	unsigned int			map_width;
> >>>>>     	unsigned int			map_height;
> >>>>>     	bool				user_map_is_set;
> >>>>> +	bool				user_map_is_updated;
> >>>>> +	struct mutex			maps_mutex;
> >>>>>     
> >>>>>     	/* Source and destination queue data */
> >>>>>     	struct dw100_q_data		q_data[2];
> >>>>> @@ -308,24 +315,31 @@ static int dw100_create_mapping(struct dw100_ctx *ctx)
> >>>>>     {
> >>>>>     	u32 *user_map;
> >>>>>     
> >>>>> -	if (ctx->map)
> >>>>> -		dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >>>>> -				  ctx->map, ctx->map_dma);
> >>>>> +	for (unsigned int i = 0; i < 2; i++) {
> >>> 	for (unsigned int i = 0; i < ARRAY_SIZE(ctx->maps); i++) {
> >>> 		struct dw100_map *map = &ctx->maps[i];
> >>>
> >>> and use map below.
> >>>
> >>>
> >>>>> +		if (ctx->maps[i].map)
> >>>>> +			dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >>>>> +					  ctx->maps[i].map, ctx->maps[i].map_dma);
> >>>>>     
> >>>>> -	ctx->map = dma_alloc_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >>>>> -				      &ctx->map_dma, GFP_KERNEL);
> >>>>> +		ctx->maps[i].map = dma_alloc_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >>>>> +						      &ctx->maps[i].map_dma, GFP_KERNEL);
> >>>>>     
> >>>>> -	if (!ctx->map)
> >>>>> -		return -ENOMEM;
> >>>>> +		if (!ctx->maps[i].map)
> >>>>> +			return -ENOMEM;
> >>>>> +	}
> >>>>>     
> >>>>>     	user_map = dw100_get_user_map(ctx);
> >>>>> -	memcpy(ctx->map, user_map, ctx->map_size);
> >>>>> +
> >>>>> +	mutex_lock(&ctx->maps_mutex);
> >>>>> +	ctx->applied_map_id = 0;
> >>>>> +	memcpy(ctx->maps[ctx->applied_map_id].map, user_map, ctx->map_size);
> >>>>> +	mutex_unlock(&ctx->maps_mutex);
> >>>>>     
> >>>>>     	dev_dbg(&ctx->dw_dev->pdev->dev,
> >>>>>     		"%ux%u %s mapping created (d:%pad-c:%p) for stream %ux%u->%ux%u\n",
> >>>>>     		ctx->map_width, ctx->map_height,
> >>>>>     		ctx->user_map_is_set ? "user" : "identity",
> >>>>> -		&ctx->map_dma, ctx->map,
> >>>>> +		&ctx->maps[ctx->applied_map_id].map_dma,
> >>>>> +		ctx->maps[ctx->applied_map_id].map,
> >>>>>     		ctx->q_data[DW100_QUEUE_SRC].pix_fmt.width,
> >>>>>     		ctx->q_data[DW100_QUEUE_DST].pix_fmt.height,
> >>>>>     		ctx->q_data[DW100_QUEUE_SRC].pix_fmt.width,
> >>>>> @@ -336,10 +350,12 @@ static int dw100_create_mapping(struct dw100_ctx *ctx)
> >>>>>     
> >>>>>     static void dw100_destroy_mapping(struct dw100_ctx *ctx)
> >>>>>     {
> >>>>> -	if (ctx->map) {
> >>>>> -		dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >>>>> -				  ctx->map, ctx->map_dma);
> >>>>> -		ctx->map = NULL;
> >>>>> +	for (unsigned int i = 0; i < 2; i++) {
> >>> 	for (unsigned int i = 0; i < ARRAY_SIZE(ctx->maps); i++) {
> >>> 		struct dw100_map *map = &ctx->maps[i];
> >>>
> >>> and use map below.
> >>>
> >>>>> +		if (ctx->maps[i].map)
> >>>>> +			dma_free_coherent(&ctx->dw_dev->pdev->dev, ctx->map_size,
> >>>>> +					  ctx->maps[i].map, ctx->maps[i].map_dma);
> >>>>> +
> >>>>> +		ctx->maps[i].map = NULL;
> >>>>>     	}
> >>>>>     }
> >>>>>     
> >>>>> @@ -350,6 +366,15 @@ static int dw100_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>>     
> >>>>>     	switch (ctrl->id) {
> >>>>>     	case V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP:
> >>>>> +		u32 *user_map = ctrl->p_new.p_u32;
> >>>> A warning to fix here.
> >>>>
> >>>>> +		unsigned int id;
> >>>>> +
> >>>>> +		mutex_lock(&ctx->maps_mutex);
> >>>>> +		id = ctx->applied_map_id ? 0 : 1;
> >>>>> +		memcpy(ctx->maps[id].map, user_map, ctx->map_size);
> >>>>> +		ctx->user_map_is_updated = true;
> >>>> If you call the control before to start the stream, the dma mapping is
> >>>> not yet done(dw100_create_mapping not yet called). Hence, copying the
> >>>> user map to a NULL pointer.
> >>> The maps could be allocated in dw100_open() when creating the context.
> >>> That would likely require moving the initialization of ctx->map_width,
> >>> ctx->map_height and ctx->map_size as well. The handling of the identity
> >>> map would probably need to be rewritten too.
> >> The ctx->map_width, ctx->map_height and ctx->map_size would be updated
> >> on s_fmt().
> > I saw that ctx->map_width, ctx->map_height and ctx->map_size are set in
> > dw100_ctrl_dewarping_map_init(), with
> >
> > 	mw = ctrl->dims[0];
> > 	mh = ctrl->dims[1];
> >
> > 	[...]
> >
> > 	ctx->map_width = mw;
> > 	ctx->map_height = mh;
> > 	ctx->map_size = mh * mw * sizeof(u32);
> >
> > but overlooked the fact that the dimensions are set in dw100_s_fmt().
> >
> >> I think we can solve the NULL pointer issue by allocating when creating
> >> the context (open()), however, it would require updating (re-allocation)
> >> again before the map can be memcpy()ed before streaming. Because the map
> >> dimensions would have changed.
> > If the map dimensions change, that invalidates the map contents set by
> > userspace. This is currently handled by dw100_ctrl_dewarping_map_init().
> > You won't be able to just memcpy() the previous control to the new one.
> >
> >> See dw100_s_fmt()
> >>
> >> ...
> >> dims[0] = dw100_get_n_vertices_from_length(q_data->pix_fmt.width);
> >> dims[1] = dw100_get_n_vertices_from_length(q_data->pix_fmt.height);
> >>
> >> ret = v4l2_ctrl_modify_dimensions(ctrl, dims);
> >> ```
> >>
> >> I checked the v4l2_ctrl_modify_dimensions definition to check if it
> >> issues a call v4l2_ctrl_type_ops.init  (where the map dimensions are
> >> updated for dw100) and it does.
> >>
> >> So, I think I will have to introduce allocations in dw100_open() so that
> >> NULL pointer issue doesn't occur and let the dma allocation get
> >> re-allocated with new dimensions just before stream start.
> >
> > That seems a bit pointless, if the map will be invalidated by a call to
> > VIDIOC_S_FMT anyway. The only case where it would be useful is if
> > userspace sets the control before starting streaming and doesn't call
> > VIDIOC_S_FMT.
> 
> I was indeed not comfortable with the .open() dma-allocation approach 
> and hence, I summarised it here for discussion.
> 
> > I'm increasingly thinking the driver should use the request API to
> > synchronize the control with the jobs.
> 
> I will atleast consider and estimate how much complex it would be!

Good idea, let's make a decision based on the need and complexity.

> >> Also, we do not have to move the ctx->map_width, ctx->height abd
> >> ctx->map_size inititlisation, since they are already gets initialised to
> >> defaults, on the open() path when v4l2_ctrl_new_custom() is done.
> >>
> >>>>> +		mutex_unlock(&ctx->maps_mutex);
> >>>>> +
> >>>>>     		ctx->user_map_is_set = true;
> >>>>>     		break;
> >>>>>     	}
> >>>>> @@ -655,6 +680,8 @@ static int dw100_open(struct file *file)
> >>>>>     
> >>>>>     	v4l2_fh_add(&ctx->fh);
> >>>>>     
> >>>>> +	mutex_init(&ctx->maps_mutex);
> >>>>> +
> >>>>>     	return 0;
> >>>>>     
> >>>>>     err:
> >>>>> @@ -675,6 +702,7 @@ static int dw100_release(struct file *file)
> >>>>>     	v4l2_ctrl_handler_free(&ctx->hdl);
> >>>>>     	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> >>>>>     	mutex_destroy(&ctx->vq_mutex);
> >>>>> +	mutex_destroy(&ctx->maps_mutex);
> >>>>>     	kfree(ctx);
> >>>>>     
> >>>>>     	return 0;
> >>>>> @@ -1453,8 +1481,19 @@ static void dw100_start(struct dw100_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
> >>>>>     	dw100_hw_set_destination(dw_dev, &ctx->q_data[DW100_QUEUE_DST],
> >>>>>     				 ctx->q_data[DW100_QUEUE_SRC].fmt,
> >>>>>     				 &out_vb->vb2_buf);
> >>>>> -	dw100_hw_set_mapping(dw_dev, ctx->map_dma,
> >>>>> -			     ctx->map_width, ctx->map_height);
> >>>>> +
> >>>>> +
> >>>>> +	mutex_lock(&ctx->maps_mutex);
> >>>>> +	if (ctx->user_map_is_updated) {
> >>>> The hardware register must unconditionally be updated while starting a
> >>>> new context, as a v4l2 m2m supports multi context operations. Otherwise,
> >>>> you may be running with the user mapping used by the previous context.
> >>>>
> >>>> Moreover, the hardware mapping will not be set in case you use the
> >>>> driver as a simple scaler without user mapping, which causes the process
> >>>> to hang as the run does not start and never completes.
> >>>>
> >>>>> +		unsigned int id = ctx->applied_map_id ? 0 : 1;
> >>>>> +
> >>>>> +		dw100_hw_set_mapping(dw_dev, ctx->maps[id].map_dma,
> >>>>> +				     ctx->map_width, ctx->map_height);
> >>>>> +		ctx->applied_map_id = id;
> >>>>> +		ctx->user_map_is_updated = false;
> >>>>> +	}
> >>>>> +	mutex_unlock(&ctx->maps_mutex);
> >>>>> +
> >>>>>     	dw100_hw_enable_irq(dw_dev);
> >>>>>     	dw100_hw_dewarp_start(dw_dev);
> >>>>>     
> >>>> It sounds as this patch requires a collaborative application for running
> >>>> well. All my simple tests failed.
> >>>>
> >>>> You can test a simple scaler/pixfmt conversion operation with v4l2 utils:
> >>>>
> >>>>
> >>>> v4l2-ctl \
> >>>> -d 0 \
> >>>> --set-fmt-video-out width=640,height=480,pixelformat=NV12,field=none \
> >>>> --set-fmt-video width=640,height=480,pixelformat=NV21,field=none \
> >>>> --stream-out-pattern 3 \
> >>>> --set-selection-output\
> >>>> target=crop,top=0,left=0,width=640,height=480,flags= \
> >>>> --stream-count 100 \
> >>>> --stream-mmap \
> >>>> --stream-out-mmap

-- 
Regards,

Laurent Pinchart