On Sat, Oct 26, 2024 at 09:52:43PM +0200, Xavier Roumegue wrote:
> Hi Umang,
>
> Thanks for the patch, this feature sounds promising.
>
> 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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.
> > + 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
