Re: [PATCH] media: v4l2-mem2mem: Apply DST_QUEUE_OFF_BASE on MMAP buffers across ioctls

[Chen-Yu Tsai]

On Thu, Dec 9, 2021 at 8:06 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> On 09/12/2021 07:29, Chen-Yu Tsai wrote:
> > DST_QUEUE_OFF_BASE is applied to offset/mem_offset on MMAP capture buffers
> > only for the VIDIOC_QUERYBUF ioctl, while the userspace fields (including
> > offset/mem_offset) are filled in for VIDIOC_{QUERY,PREPARE,Q,DQ}BUF
> > ioctls. This leads to differences in the values presented to userspace.
> > If userspace attempts to mmap the capture buffer directly using values
> > from DQBUF, it will fail.
> >
> > Move the code that applies the magic offset into a helper, and call
> > that helper from all four ioctl entry points.
> >
> > Fixes: 7f98639def42 ("V4L/DVB: add memory-to-memory device helper framework for videobuf")
> > Fixes: 908a0d7c588e ("[media] v4l: mem2mem: port to videobuf2")
> > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > ---
> > This was tested on RK3399 with
> >
> >     gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! fakesink
> >
> > and verifying the values using the V4L2 debug messages:
> >
> >     video2: VIDIOC_QUERYBUF: 00:00:00.000000 index=0, type=vid-cap-mplane, request_fd=0, flags=0x00004000, field=any, sequence=0, memory=mmap
> >     plane 0: bytesused=0, data_offset=0x00000000, offset/userptr=0x40000000, length=2097152
> >     timecode=00:00:00 type=0, flags=0x00000000, frames=0, userbits=0x00000000
> >     video2: VIDIOC_QUERYBUF: 00:00:00.000000 index=0, type=vid-out-mplane, request_fd=0, flags=0x00004000, field=any, sequence=0, memory=mmap
> >     plane 0: bytesused=0, data_offset=0x00000000, offset/userptr=0x0, length=153600
> >     timecode=00:00:00 type=0, flags=0x00000000, frames=0, userbits=0x00000000
> >
> >     video2: VIDIOC_QBUF: 00:00:00.000000 index=0, type=vid-cap-mplane, request_fd=0, flags=0x00004003, field=any, sequence=0, memory=mmap
> >     plane 0: bytesused=2097152, data_offset=0x00000000, offset/userptr=0x40000000, length=2097152
> >     timecode=00:00:00 type=0, flags=0x00000000, frames=0, userbits=0x00000000
> >     video2: VIDIOC_QBUF: 00:00:00.000000 index=0, type=vid-out-mplane, request_fd=0, flags=0x00004003, field=none, sequence=0, memory=mmap
> >     plane 0: bytesused=153600, data_offset=0x00000000, offset/userptr=0x0, length=153600
> >     timecode=00:00:00 type=0, flags=0x00000000, frames=0, userbits=0x00000000
> >
> >     video2: VIDIOC_DQBUF: 00:00:00.000000 index=0, type=vid-cap-mplane, request_fd=0, flags=0x00004001, field=none, sequence=0, memory=mmap
> >     plane 0: bytesused=6324, data_offset=0x00000000, offset/userptr=0x40000000, length=2097152
> >     timecode=00:00:00 type=0, flags=0x00000000, frames=0, userbits=0x00000000
> >     video2: VIDIOC_DQBUF: 00:00:00.000000 index=0, type=vid-out-mplane, request_fd=0, flags=0x00004001, field=none, sequence=0, memory=mmap
> >     plane 0: bytesused=153600, data_offset=0x00000000, offset/userptr=0x0, length=153600
> >     timecode=00:00:00 type=0, flags=0x00000000, frames=0, userbits=0x00000000
> >
> > Gstreamer doesn't do PREPAREBUF calls, so that path was not verified.
> > However the code changes are exactly the same, so I'm quite confident
> > about them.
> >
> > ---
> >  drivers/media/v4l2-core/v4l2-mem2mem.c | 46 ++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > index e2654b422334..b47f25297c43 100644
> > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > @@ -585,19 +585,14 @@ int v4l2_m2m_reqbufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_m2m_reqbufs);
> >
> > -int v4l2_m2m_querybuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> > -                   struct v4l2_buffer *buf)
> > +static void v4l2_m2m_adjust_mem_offset(struct vb2_queue *vq,
> > +                                    struct v4l2_buffer *buf)
> >  {
> > -     struct vb2_queue *vq;
> > -     int ret = 0;
> > -     unsigned int i;
> > -
> > -     vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
> > -     ret = vb2_querybuf(vq, buf);
> > -
> >       /* Adjust MMAP memory offsets for the CAPTURE queue */
> >       if (buf->memory == V4L2_MEMORY_MMAP && V4L2_TYPE_IS_CAPTURE(vq->type)) {
> >               if (V4L2_TYPE_IS_MULTIPLANAR(vq->type)) {
> > +                     unsigned int i;
> > +
> >                       for (i = 0; i < buf->length; ++i)
> >                               buf->m.planes[i].m.mem_offset
> >                                       += DST_QUEUE_OFF_BASE;
> > @@ -605,6 +600,19 @@ int v4l2_m2m_querybuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >                       buf->m.offset += DST_QUEUE_OFF_BASE;
> >               }
> >       }
> > +}
> > +
> > +int v4l2_m2m_querybuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> > +                   struct v4l2_buffer *buf)
> > +{
> > +     struct vb2_queue *vq;
> > +     int ret = 0;
> > +
> > +     vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
> > +     ret = vb2_querybuf(vq, buf);
> > +
> > +     /* Adjust MMAP memory offsets for the CAPTURE queue */
> > +     v4l2_m2m_adjust_mem_offset(vq, buf);
> >
> >       return ret;
> >  }
> > @@ -760,6 +768,10 @@ int v4l2_m2m_qbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >       }
> >
> >       ret = vb2_qbuf(vq, vdev->v4l2_dev->mdev, buf);
> > +
> > +     /* Adjust MMAP memory offsets for the CAPTURE queue */
> > +     v4l2_m2m_adjust_mem_offset(vq, buf);
> > +
> >       if (ret)
> >               return ret;
> >
> > @@ -784,9 +796,15 @@ int v4l2_m2m_dqbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >                  struct v4l2_buffer *buf)
> >  {
> >       struct vb2_queue *vq;
> > +     int ret;
> >
> >       vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
> > -     return vb2_dqbuf(vq, buf, file->f_flags & O_NONBLOCK);
> > +     ret = vb2_dqbuf(vq, buf, file->f_flags & O_NONBLOCK);
> > +
> > +     /* Adjust MMAP memory offsets for the CAPTURE queue */
> > +     v4l2_m2m_adjust_mem_offset(vq, buf);
> > +
> > +     return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_m2m_dqbuf);
> >
> > @@ -795,9 +813,15 @@ int v4l2_m2m_prepare_buf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >  {
> >       struct video_device *vdev = video_devdata(file);
> >       struct vb2_queue *vq;
> > +     int ret;
> >
> >       vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
> > -     return vb2_prepare_buf(vq, vdev->v4l2_dev->mdev, buf);
> > +     ret = vb2_prepare_buf(vq, vdev->v4l2_dev->mdev, buf);
> > +
> > +     /* Adjust MMAP memory offsets for the CAPTURE queue */
> > +     v4l2_m2m_adjust_mem_offset(vq, buf);
> > +
> > +     return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_m2m_prepare_buf);
> >
> >
>
> For all these functions you should only call v4l2_m2m_adjust_mem_offset() if !ret.
> If the vb2_* function returned an error, then the offset fields aren't filled in,
> so it makes no sense to update them. And besides, the struct isn't copied back to
> userspace anyway on error.

Got it. That makes more sense than the original code.

ChenYu