[libcamera-devel,v2] v4l2: V4L2CameraProxy: Add EXPBUF as one of the supported ioctl
diff mbox series

Message ID 20211128061556.503443-1-vedantparanjape160201@gmail.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2] v4l2: V4L2CameraProxy: Add EXPBUF as one of the supported ioctl
Related show

Commit Message

Vedant Paranjape Nov. 28, 2021, 6:15 a.m. UTC
To support DMABUF as one of the memory buffer, we need to implement
EXPBUF in the v4l2 compat layer.

This patch implements vidioc_expbuf as one of the supported ioctls.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=89

Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
---
 src/v4l2/v4l2_camera_proxy.cpp | 37 ++++++++++++++++++++++++++++++++++
 src/v4l2/v4l2_camera_proxy.h   |  1 +
 2 files changed, 38 insertions(+)

Comments

Laurent Pinchart Nov. 28, 2021, 1:31 p.m. UTC | #1
Hi Vedant,

Thank you for the patch.

On Sun, Nov 28, 2021 at 11:45:56AM +0530, Vedant Paranjape wrote:
> To support DMABUF as one of the memory buffer, we need to implement
> EXPBUF in the v4l2 compat layer.
> 
> This patch implements vidioc_expbuf as one of the supported ioctls.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 37 ++++++++++++++++++++++++++++++++++
>  src/v4l2/v4l2_camera_proxy.h   |  1 +
>  2 files changed, 38 insertions(+)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 3610e63cade3..cb2f93631ab9 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -624,6 +624,39 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
>  	return 0;
>  }
>  
> +int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
> +
> +	if (!hasOwnership(file))
> +		return -EBUSY;
> +
> +	/* \todo Verify that the memory type is MMAP when adding DMABUF support */
> +	if (!validateBufferType(arg->type))
> +		return -EINVAL;
> +
> +	if (arg->index >= bufferCount_)
> +		return -EINVAL;
> +
> +	if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
> +		return -EINVAL;
> +
> +	LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";

I'd either drop this, or combine it with the message at the top of the
function.

> +
> +	if (file->priority() < maxPriority())
> +		return -EBUSY;

Why is this chech needed ?

> +
> +	memset(arg->reserved, 0, sizeof(arg->reserved));
> +
> +	arg->fd = fcntl(buffers_[arg->index].m.fd,
> +			arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);
> +	arg->fd = fcntl(arg->fd, F_SETFL, 0);

0 isn't the right value.

> +
> +	LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;

I'd drop this too.

> +
> +	return 0;
> +}
> +
>  int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
>  {
>  	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
> @@ -685,6 +718,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
>  	VIDIOC_QUERYBUF,
>  	VIDIOC_QBUF,
>  	VIDIOC_DQBUF,
> +	VIDIOC_EXPBUF,
>  	VIDIOC_STREAMON,
>  	VIDIOC_STREAMOFF,
>  };
> @@ -755,6 +789,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
>  	case VIDIOC_DQBUF:
>  		ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
>  		break;
> +	case VIDIOC_EXPBUF:
> +		ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
> +		break;
>  	case VIDIOC_STREAMON:
>  		ret = vidioc_streamon(file, static_cast<int *>(arg));
>  		break;
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index fccec241879d..81ef7788e9fe 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -58,6 +58,7 @@ private:
>  	int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
>  	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
>  			 libcamera::MutexLocker *locker);
> +	int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
>  	int vidioc_streamon(V4L2CameraFile *file, int *arg);
>  	int vidioc_streamoff(V4L2CameraFile *file, int *arg);
>
Laurent Pinchart Nov. 28, 2021, 4:37 p.m. UTC | #2
On Sun, Nov 28, 2021 at 03:31:40PM +0200, Laurent Pinchart wrote:
> Hi Vedant,
> 
> Thank you for the patch.
> 
> On Sun, Nov 28, 2021 at 11:45:56AM +0530, Vedant Paranjape wrote:
> > To support DMABUF as one of the memory buffer, we need to implement
> > EXPBUF in the v4l2 compat layer.
> > 
> > This patch implements vidioc_expbuf as one of the supported ioctls.
> > 
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> > 
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 37 ++++++++++++++++++++++++++++++++++
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 3610e63cade3..cb2f93631ab9 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -624,6 +624,39 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >  	return 0;
> >  }
> >  
> > +int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
> > +
> > +	if (!hasOwnership(file))
> > +		return -EBUSY;
> > +
> > +	/* \todo Verify that the memory type is MMAP when adding DMABUF support */
> > +	if (!validateBufferType(arg->type))
> > +		return -EINVAL;
> > +
> > +	if (arg->index >= bufferCount_)
> > +		return -EINVAL;
> > +
> > +	if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
> > +		return -EINVAL;
> > +
> > +	LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";
> 
> I'd either drop this, or combine it with the message at the top of the
> function.
> 
> > +
> > +	if (file->priority() < maxPriority())
> > +		return -EBUSY;
> 
> Why is this chech needed ?
> 
> > +
> > +	memset(arg->reserved, 0, sizeof(arg->reserved));
> > +
> > +	arg->fd = fcntl(buffers_[arg->index].m.fd,
> > +			arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);

Actually I don't think this is correct. .m.fd is for importing dmabufs,
it's always 0 at the moment.

If this has passed v4l2-compliance, it means EXPBUF isn't properly
tested by it.

> > +	arg->fd = fcntl(arg->fd, F_SETFL, 0);
> 
> 0 isn't the right value.
> 
> > +
> > +	LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;
> 
> I'd drop this too.
> 
> > +
> > +	return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
> >  {
> >  	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
> > @@ -685,6 +718,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> >  	VIDIOC_QUERYBUF,
> >  	VIDIOC_QBUF,
> >  	VIDIOC_DQBUF,
> > +	VIDIOC_EXPBUF,
> >  	VIDIOC_STREAMON,
> >  	VIDIOC_STREAMOFF,
> >  };
> > @@ -755,6 +789,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> >  	case VIDIOC_DQBUF:
> >  		ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
> >  		break;
> > +	case VIDIOC_EXPBUF:
> > +		ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
> > +		break;
> >  	case VIDIOC_STREAMON:
> >  		ret = vidioc_streamon(file, static_cast<int *>(arg));
> >  		break;
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > index fccec241879d..81ef7788e9fe 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -58,6 +58,7 @@ private:
> >  	int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> >  	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >  			 libcamera::MutexLocker *locker);
> > +	int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
> >  	int vidioc_streamon(V4L2CameraFile *file, int *arg);
> >  	int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> >
Vedant Paranjape Nov. 28, 2021, 7:06 p.m. UTC | #3
Hello Laurent,
Thanks for the review.

On Sun, Nov 28, 2021 at 7:02 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Vedant,
>
> Thank you for the patch.
>
> On Sun, Nov 28, 2021 at 11:45:56AM +0530, Vedant Paranjape wrote:
> > To support DMABUF as one of the memory buffer, we need to implement
> > EXPBUF in the v4l2 compat layer.
> >
> > This patch implements vidioc_expbuf as one of the supported ioctls.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 37 ++++++++++++++++++++++++++++++++++
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 3610e63cade3..cb2f93631ab9 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -624,6 +624,39 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >       return 0;
> >  }
> >
> > +int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
> > +{
> > +     LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
> > +
> > +     if (!hasOwnership(file))
> > +             return -EBUSY;
> > +
> > +     /* \todo Verify that the memory type is MMAP when adding DMABUF support */
> > +     if (!validateBufferType(arg->type))
> > +             return -EINVAL;
> > +
> > +     if (arg->index >= bufferCount_)
> > +             return -EINVAL;
> > +
> > +     if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
> > +             return -EINVAL;
> > +
> > +     LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";
>
> I'd either drop this, or combine it with the message at the top of the
> function.
>
> > +
> > +     if (file->priority() < maxPriority())
> > +             return -EBUSY;
>
> Why is this chech needed ?
>
> > +
> > +     memset(arg->reserved, 0, sizeof(arg->reserved));
> > +
> > +     arg->fd = fcntl(buffers_[arg->index].m.fd,
> > +                     arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);
> > +     arg->fd = fcntl(arg->fd, F_SETFL, 0);
>
> 0 isn't the right value.

I am not sure what will come here, the Docs are not super clear about it.
Is this right instead ?

> arg->fd = fcntl(arg->fd, F_SETFL);

>
> > +
> > +     LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;
>
> I'd drop this too.
>
> > +
> > +     return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
> >  {
> >       LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
> > @@ -685,6 +718,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> >       VIDIOC_QUERYBUF,
> >       VIDIOC_QBUF,
> >       VIDIOC_DQBUF,
> > +     VIDIOC_EXPBUF,
> >       VIDIOC_STREAMON,
> >       VIDIOC_STREAMOFF,
> >  };
> > @@ -755,6 +789,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> >       case VIDIOC_DQBUF:
> >               ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
> >               break;
> > +     case VIDIOC_EXPBUF:
> > +             ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
> > +             break;
> >       case VIDIOC_STREAMON:
> >               ret = vidioc_streamon(file, static_cast<int *>(arg));
> >               break;
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > index fccec241879d..81ef7788e9fe 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -58,6 +58,7 @@ private:
> >       int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> >       int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >                        libcamera::MutexLocker *locker);
> > +     int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
> >       int vidioc_streamon(V4L2CameraFile *file, int *arg);
> >       int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> >
>
> --
> Regards,
>
> Laurent Pinchart
Vedant Paranjape Nov. 28, 2021, 7:10 p.m. UTC | #4
From the docs
> * @fd: for non-multiplanar buffers with memory == V4L2_MEMORY_DMABUF;
> * a userspace file descriptor associated with this buffer

My guess is since memory is not equal to V4L2_MEMORY_DMABUF yet, this
won't work.

On Sun, Nov 28, 2021 at 10:07 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Sun, Nov 28, 2021 at 03:31:40PM +0200, Laurent Pinchart wrote:
> > Hi Vedant,
> >
> > Thank you for the patch.
> >
> > On Sun, Nov 28, 2021 at 11:45:56AM +0530, Vedant Paranjape wrote:
> > > To support DMABUF as one of the memory buffer, we need to implement
> > > EXPBUF in the v4l2 compat layer.
> > >
> > > This patch implements vidioc_expbuf as one of the supported ioctls.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> > >
> > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > > ---
> > >  src/v4l2/v4l2_camera_proxy.cpp | 37 ++++++++++++++++++++++++++++++++++
> > >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> > >  2 files changed, 38 insertions(+)
> > >
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index 3610e63cade3..cb2f93631ab9 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -624,6 +624,39 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> > >     return 0;
> > >  }
> > >
> > > +int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
> > > +{
> > > +   LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
> > > +
> > > +   if (!hasOwnership(file))
> > > +           return -EBUSY;
> > > +
> > > +   /* \todo Verify that the memory type is MMAP when adding DMABUF support */
> > > +   if (!validateBufferType(arg->type))
> > > +           return -EINVAL;
> > > +
> > > +   if (arg->index >= bufferCount_)
> > > +           return -EINVAL;
> > > +
> > > +   if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
> > > +           return -EINVAL;
> > > +
> > > +   LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";
> >
> > I'd either drop this, or combine it with the message at the top of the
> > function.
> >
> > > +
> > > +   if (file->priority() < maxPriority())
> > > +           return -EBUSY;
> >
> > Why is this chech needed ?
> >
> > > +
> > > +   memset(arg->reserved, 0, sizeof(arg->reserved));
> > > +
> > > +   arg->fd = fcntl(buffers_[arg->index].m.fd,
> > > +                   arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);
>
> Actually I don't think this is correct. .m.fd is for importing dmabufs,
> it's always 0 at the moment.
>
> If this has passed v4l2-compliance, it means EXPBUF isn't properly
> tested by it.
>
> > > +   arg->fd = fcntl(arg->fd, F_SETFL, 0);
> >
> > 0 isn't the right value.
> >
> > > +
> > > +   LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;
> >
> > I'd drop this too.
> >
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >  int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
> > >  {
> > >     LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
> > > @@ -685,6 +718,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> > >     VIDIOC_QUERYBUF,
> > >     VIDIOC_QBUF,
> > >     VIDIOC_DQBUF,
> > > +   VIDIOC_EXPBUF,
> > >     VIDIOC_STREAMON,
> > >     VIDIOC_STREAMOFF,
> > >  };
> > > @@ -755,6 +789,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> > >     case VIDIOC_DQBUF:
> > >             ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
> > >             break;
> > > +   case VIDIOC_EXPBUF:
> > > +           ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
> > > +           break;
> > >     case VIDIOC_STREAMON:
> > >             ret = vidioc_streamon(file, static_cast<int *>(arg));
> > >             break;
> > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > > index fccec241879d..81ef7788e9fe 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.h
> > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > @@ -58,6 +58,7 @@ private:
> > >     int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> > >     int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> > >                      libcamera::MutexLocker *locker);
> > > +   int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
> > >     int vidioc_streamon(V4L2CameraFile *file, int *arg);
> > >     int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> > >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Nov. 28, 2021, 10:49 p.m. UTC | #5
Hi Vedant,

On Mon, Nov 29, 2021 at 12:36:50AM +0530, Vedant Paranjape wrote:
> On Sun, Nov 28, 2021 at 7:02 PM Laurent Pinchart wrote:
> > On Sun, Nov 28, 2021 at 11:45:56AM +0530, Vedant Paranjape wrote:
> > > To support DMABUF as one of the memory buffer, we need to implement
> > > EXPBUF in the v4l2 compat layer.
> > >
> > > This patch implements vidioc_expbuf as one of the supported ioctls.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> > >
> > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > > ---
> > >  src/v4l2/v4l2_camera_proxy.cpp | 37 ++++++++++++++++++++++++++++++++++
> > >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> > >  2 files changed, 38 insertions(+)
> > >
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index 3610e63cade3..cb2f93631ab9 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -624,6 +624,39 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> > >       return 0;
> > >  }
> > >
> > > +int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
> > > +{
> > > +     LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
> > > +
> > > +     if (!hasOwnership(file))
> > > +             return -EBUSY;
> > > +
> > > +     /* \todo Verify that the memory type is MMAP when adding DMABUF support */
> > > +     if (!validateBufferType(arg->type))
> > > +             return -EINVAL;
> > > +
> > > +     if (arg->index >= bufferCount_)
> > > +             return -EINVAL;
> > > +
> > > +     if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
> > > +             return -EINVAL;
> > > +
> > > +     LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";
> >
> > I'd either drop this, or combine it with the message at the top of the
> > function.
> >
> > > +
> > > +     if (file->priority() < maxPriority())
> > > +             return -EBUSY;
> >
> > Why is this chech needed ?
> >
> > > +
> > > +     memset(arg->reserved, 0, sizeof(arg->reserved));
> > > +
> > > +     arg->fd = fcntl(buffers_[arg->index].m.fd,
> > > +                     arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);
> > > +     arg->fd = fcntl(arg->fd, F_SETFL, 0);
> >
> > 0 isn't the right value.
> 
> I am not sure what will come here, the Docs are not super clear about it.
> Is this right instead ?
> 
> > arg->fd = fcntl(arg->fd, F_SETFL);

No, F_SETFL takes an int argument:

       F_SETFL (int)
	      Set the file status flags to the value specified by arg.
	      File access mode (O_RDONLY, O_WRONLY, O_RDWR)  and  file
	      creation  flags  (i.e.,  O_CREAT,  O_EXCL,  O_NOCTTY,
	      O_TRUNC)  in arg are ignored.  On Linux, this command can
	      change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME,
	      and O_NONBLOCK flags.  It is not possible to change the
	      O_DSYNC and O_SYNC flags; see BUGS, below.

And now that I read this more carefully, it's clear that it's not the
right command, as it ignores the file access mode.

It seems fcntl() can't change the access mode. The question is thus how
to honour the O_ACCMODE flags passed to this function, or possibly, how
to emulate them. That doesn't seem straightforward, intercepting mmap()
on the exported dmabufs could be the only way. I don't think that needs
to be implemented here, but a \todo would be good.

> > > +
> > > +     LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;
> >
> > I'd drop this too.
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
> > >  {
> > >       LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
> > > @@ -685,6 +718,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> > >       VIDIOC_QUERYBUF,
> > >       VIDIOC_QBUF,
> > >       VIDIOC_DQBUF,
> > > +     VIDIOC_EXPBUF,
> > >       VIDIOC_STREAMON,
> > >       VIDIOC_STREAMOFF,
> > >  };
> > > @@ -755,6 +789,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> > >       case VIDIOC_DQBUF:
> > >               ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
> > >               break;
> > > +     case VIDIOC_EXPBUF:
> > > +             ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
> > > +             break;
> > >       case VIDIOC_STREAMON:
> > >               ret = vidioc_streamon(file, static_cast<int *>(arg));
> > >               break;
> > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > > index fccec241879d..81ef7788e9fe 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.h
> > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > @@ -58,6 +58,7 @@ private:
> > >       int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> > >       int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> > >                        libcamera::MutexLocker *locker);
> > > +     int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
> > >       int vidioc_streamon(V4L2CameraFile *file, int *arg);
> > >       int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> > >

Patch
diff mbox series

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index 3610e63cade3..cb2f93631ab9 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -624,6 +624,39 @@  int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
 	return 0;
 }
 
+int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
+{
+	LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
+
+	if (!hasOwnership(file))
+		return -EBUSY;
+
+	/* \todo Verify that the memory type is MMAP when adding DMABUF support */
+	if (!validateBufferType(arg->type))
+		return -EINVAL;
+
+	if (arg->index >= bufferCount_)
+		return -EINVAL;
+
+	if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
+		return -EINVAL;
+
+	LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";
+
+	if (file->priority() < maxPriority())
+		return -EBUSY;
+
+	memset(arg->reserved, 0, sizeof(arg->reserved));
+
+	arg->fd = fcntl(buffers_[arg->index].m.fd,
+			arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);
+	arg->fd = fcntl(arg->fd, F_SETFL, 0);
+
+	LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;
+
+	return 0;
+}
+
 int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
 {
 	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
@@ -685,6 +718,7 @@  const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
 	VIDIOC_QUERYBUF,
 	VIDIOC_QBUF,
 	VIDIOC_DQBUF,
+	VIDIOC_EXPBUF,
 	VIDIOC_STREAMON,
 	VIDIOC_STREAMOFF,
 };
@@ -755,6 +789,9 @@  int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
 	case VIDIOC_DQBUF:
 		ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
 		break;
+	case VIDIOC_EXPBUF:
+		ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
+		break;
 	case VIDIOC_STREAMON:
 		ret = vidioc_streamon(file, static_cast<int *>(arg));
 		break;
diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
index fccec241879d..81ef7788e9fe 100644
--- a/src/v4l2/v4l2_camera_proxy.h
+++ b/src/v4l2/v4l2_camera_proxy.h
@@ -58,6 +58,7 @@  private:
 	int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
 	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
 			 libcamera::MutexLocker *locker);
+	int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
 	int vidioc_streamon(V4L2CameraFile *file, int *arg);
 	int vidioc_streamoff(V4L2CameraFile *file, int *arg);