<div dir="auto">Ah, I see. </div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 12 Jan, 2022, 01:05 Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Vedant,<br>
<br>
On Wed, Jan 12, 2022 at 01:03:23AM +0530, Vedant Paranjape wrote:<br>
> On Wed, Jan 12, 2022 at 12:52 AM Laurent Pinchart wrote:<br>
> > On Tue, Jan 11, 2022 at 09:10:53PM +0200, Laurent Pinchart wrote:<br>
> > > On Tue, Jan 11, 2022 at 10:03:29PM +0530, Vedant Paranjape wrote:<br>
> > > > Add support for PREPARE_BUF as one of the ioctl. Since this is a compat<br>
> > > > layer, there doesn't seem to be an equivalent to the "transfer ownership<br>
> > > > of the buffer to kernel driver" in V4L2Camera class.<br>
> > > ><br>
> > > > To match the error checks done by kernel implementation, we'd have to<br>
> > > > check if dmabuf fd is valid and that the buffer size is large enough.<br>
> > > > Doing so will not add any particular value to the program as<br>
> > > > applications don't depend on these conditions being handled correctly.<br>
> > ><br>
> > > s/don't depend/most likely don't depend/ (there can always be corner<br>
> > > cases, developers sometimes get inventive)<br>
> > ><br>
> > > > Signed-off-by: Vedant Paranjape <<a href="mailto:vedantparanjape160201@gmail.com" target="_blank" rel="noreferrer">vedantparanjape160201@gmail.com</a>><br>
> > > > ---<br>
> > > > Source builds and tests fine, also passes the v4l2-compliance<br>
> > > ><br>
> > > > <snip><br>
> > > > vedant@pc ~/libcamera$ ./test/v4l2_compat/v4l2_compat_test.py ./build/src/v4l2/v4l2-compat.so<br>
> > > > Testing /dev/video0 with uvcvideo driver... success<br>
> > > > </snip><br>
> > > ><br>
> > > > Still figuring out how to test this with v4l2-ctl, if you have hints<br>
> > > > please let me know :)<br>
> > ><br>
> > > v4l2-compliance tests VIDIOC_PREPARE buf, that should be good enough as<br>
> > > a test.<br>
> > ><br>
> > > > ---<br>
> > > > src/v4l2/v4l2_camera_proxy.cpp | 28 ++++++++++++++++++++++++++++<br>
> > > > src/v4l2/v4l2_camera_proxy.h | 1 +<br>
> > > > 2 files changed, 29 insertions(+)<br>
> > > ><br>
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp<br>
> > > > index 4d529bc29a4d..8c28e8738a06 100644<br>
> > > > --- a/src/v4l2/v4l2_camera_proxy.cpp<br>
> > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp<br>
> > > > @@ -544,6 +544,33 @@ int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *a<br>
> > > > return 0;<br>
> > > > }<br>
> > > ><br>
> > > > +int V4L2CameraProxy::vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg)<br>
> > > > +{<br>
> > > > + LOG(V4L2Compat, Debug) << "Servicing vidioc_prepare_buf, index = "<br>
> > > > + << arg->index << " fd = " << file->efd();<br>
> > > > +<br>
> > > > + if (!hasOwnership(file))<br>
> > > > + return -EBUSY;<br>
> > > > +<br>
> > > > + if (arg->index >= bufferCount_)<br>
> > > > + return -EINVAL;<br>
> > > > +<br>
> > ><br>
> > > struct v4l2_buffer &buffer = buffers_[arg->index];<br>
> > ><br>
> > > and use buffer below.<br>
> > ><br>
> > > With this change, and v4l2-compliance passing,<br>
> > ><br>
> > > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank" rel="noreferrer">laurent.pinchart@ideasonboard.com</a>><br>
> > ><br>
> > > > + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_REQUEST_FD ||<br>
> ><br>
> > Actually, this isn't correct, you need to check arg->flags here.<br>
> <br>
> I missed this, but I can't figure out the reason why this is incorrect ?<br>
<br>
Because V4L2_BUF_FLAG_REQUEST_FD is a flag that is passed to<br>
VIDIOC_PREPARE_BUF, not a flag we set internally.<br>
<br>
> > > > + buffers_[arg->index].flags & V4L2_BUF_FLAG_PREPARED)<br>
> > > > + return -EINVAL;<br>
> > > > +<br>
> > > > + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED ||<br>
> > > > + !validateBufferType(arg->type) ||<br>
> > > > + !validateMemoryType(arg->memory))<br>
> > > > + return -EINVAL;<br>
> ><br>
> > And let's group all the flags check:<br>
> ><br>
> > if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)<br>
> > return -EINVAL;<br>
> ><br>
> > if (!validateBufferType(arg->type) || !validateMemoryType(arg->memory))<br>
> > return -EINVAL;<br>
> ><br>
> > struct v4l2_buffer &buffer = buffers_[arg->index];<br>
> ><br>
> > if (buffer.flags & V4L2_BUF_FLAG_PREPARED ||<br>
> > buffer.flags & V4L2_BUF_FLAG_QUEUED ||<br>
> > return -EINVAL;<br>
> ><br>
> > > > +<br>
> > > > + buffers_[arg->index].flags |= V4L2_BUF_FLAG_PREPARED;<br>
> > > > +<br>
> > > > + arg->flags = buffers_[arg->index].flags;<br>
> > > > +<br>
> > > > + return 0;<br>
> > > > +}<br>
> > > > +<br>
> > > > int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)<br>
> > > > {<br>
> > > > LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = "<br>
> > > > @@ -709,6 +736,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {<br>
> > > > VIDIOC_S_INPUT,<br>
> > > > VIDIOC_REQBUFS,<br>
> > > > VIDIOC_QUERYBUF,<br>
> > > > + VIDIOC_PREPARE_BUF,<br>
> > > > VIDIOC_QBUF,<br>
> > > > VIDIOC_DQBUF,<br>
> > > > VIDIOC_EXPBUF,<br>
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h<br>
> > > > index 14e027c3e7d1..6baba94262a9 100644<br>
> > > > --- a/src/v4l2/v4l2_camera_proxy.h<br>
> > > > +++ b/src/v4l2/v4l2_camera_proxy.h<br>
> > > > @@ -57,6 +57,7 @@ private:<br>
> > > > int vidioc_s_input(V4L2CameraFile *file, int *arg);<br>
> > > > int vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuffers *arg);<br>
> > > > int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);<br>
> > > > + int vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg);<br>
> > > > int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);<br>
> > > > int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,<br>
> > > > libcamera::Mutex *lock) LIBCAMERA_TSA_REQUIRES(*lock);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>