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
