[libcamera-devel,04/15] v4l2: v4l2_camera_proxy: Implement VIDIOC_G/S_PRIORITY
diff mbox series

Message ID 20200616131244.70308-5-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Support v4l2-compliance
Related show

Commit Message

Paul Elder June 16, 2020, 1:12 p.m. UTC 
Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what
v4l2-compliance wants.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/v4l2/v4l2_camera_proxy.cpp | 48 +++++++++++++++++++++++++++++++++-
 src/v4l2/v4l2_camera_proxy.h   |  4 +++
 2 files changed, 51 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi June 17, 2020, 1 p.m. UTC | #1 
Hi Paul,

On Tue, Jun 16, 2020 at 10:12:33PM +0900, Paul Elder wrote:
> Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what
> v4l2-compliance wants.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 48 +++++++++++++++++++++++++++++++++-
>  src/v4l2/v4l2_camera_proxy.h   |  4 +++
>  2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index d899853..f268f7a 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -34,7 +34,7 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
>  				 std::shared_ptr<Camera> camera)
>  	: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),
>  	  vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1),
> -	  acquiredFd_(-1), initialized_(false)
> +	  v4l2RecordPriorityFd_(-1), acquiredFd_(-1), initialized_(false)
>  {
>  	querycap(camera);
>  }
> @@ -44,6 +44,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)
>  	LOG(V4L2Compat, Debug) << "Servicing open fd = " << fd;
>
>  	nonBlockingFds_[fd] = nonBlocking;
> +	priorities_[fd] = V4L2_PRIORITY_DEFAULT;
>
>  	refcount_++;
>
> @@ -77,6 +78,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)
>  void V4L2CameraProxy::dup(int oldfd, int newfd)
>  {
>  	nonBlockingFds_[newfd] = nonBlockingFds_[oldfd];
> +	priorities_[newfd] = V4L2_PRIORITY_DEFAULT;

Honest question, should this copy the priority of the duplicated fd,
or set it to default ? I guess copying it would creat issues with
PRIORITY_RECORD, but the V4L2 documentation does not specify this

>  	refcount_++;
>  }
>
> @@ -85,6 +87,7 @@ void V4L2CameraProxy::close(int fd)
>  	LOG(V4L2Compat, Debug) << "Servicing close fd = " << fd;
>
>  	nonBlockingFds_.erase(fd);
> +	priorities_.erase(fd);
>
>  	if (acquiredFd_ == fd)
>  		acquiredFd_ = -1;
> @@ -357,6 +360,43 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg)
>  	return 0;
>  }
>
> +int V4L2CameraProxy::vidioc_g_priority(int fd, enum v4l2_priority *arg)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing vidioc_g_priority fd = " << fd;
> +
> +	if (arg == nullptr)

As a general note, !arg is preferred instead of (arg == nullptr).
This seems to apply to the whole patchset

> +		return -EFAULT;
> +
> +	if (v4l2RecordPriorityFd_ != -1)
> +		*arg = V4L2_PRIORITY_RECORD;
> +	else
> +		*arg = priorities_[fd];
> +
> +	return 0;
> +}
> +
> +int V4L2CameraProxy::vidioc_s_priority(int fd, enum v4l2_priority *arg)
> +{
> +	LOG(V4L2Compat, Debug)
> +		<< "Servicing vidioc_s_priority fd = " << fd
> +		<< " prio =  " << ((arg == nullptr) ? -1 : (int)*arg);
> +
> +	if (arg == nullptr)
> +		return -EFAULT;
> +
> +	if (v4l2RecordPriorityFd_ != -1 && v4l2RecordPriorityFd_ != fd)
> +		return -EBUSY;
> +
> +	if (*arg == V4L2_PRIORITY_RECORD)
> +		v4l2RecordPriorityFd_ = fd;
> +	else if (v4l2RecordPriorityFd_ == fd)
> +		v4l2RecordPriorityFd_ = -1;
> +
> +	priorities_[fd] = *arg;
> +
> +	return 0;
> +}
> +

The patch looks good to me, minors apart
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  int V4L2CameraProxy::freeBuffers()
>  {
>  	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> @@ -603,6 +643,12 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)
>  	case VIDIOC_TRY_FMT:
>  		ret = vidioc_try_fmt(fd, static_cast<struct v4l2_format *>(arg));
>  		break;
> +	case VIDIOC_G_PRIORITY:
> +		ret = vidioc_g_priority(fd, static_cast<enum v4l2_priority *>(arg));
> +		break;
> +	case VIDIOC_S_PRIORITY:
> +		ret = vidioc_s_priority(fd, static_cast<enum v4l2_priority *>(arg));
> +		break;
>  	case VIDIOC_REQBUFS:
>  		ret = vidioc_reqbufs(fd, static_cast<struct v4l2_requestbuffers *>(arg));
>  		break;
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 90e518c..a0c373d 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -49,6 +49,8 @@ private:
>  	int vidioc_g_fmt(int fd, struct v4l2_format *arg);
>  	int vidioc_s_fmt(int fd, struct v4l2_format *arg);
>  	int vidioc_try_fmt(int fd, struct v4l2_format *arg);
> +	int vidioc_g_priority(int fd, enum v4l2_priority *arg);
> +	int vidioc_s_priority(int fd, enum v4l2_priority *arg);
>  	int vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg);
>  	int vidioc_querybuf(int fd, struct v4l2_buffer *arg);
>  	int vidioc_qbuf(int fd, struct v4l2_buffer *arg);
> @@ -84,6 +86,8 @@ private:
>  	int efd_;
>
>  	std::map<int, bool> nonBlockingFds_;
> +	std::map<int, enum v4l2_priority> priorities_;
> +	int v4l2RecordPriorityFd_;
>  	/*
>  	 * This is an exclusive lock on the camera. When this is not taken,
>  	 * anybody can call any ioctl before reqbufs. reqbufs with count > 0
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 17, 2020, 3:33 p.m. UTC | #2 
Hi Jacopo,

On Wed, Jun 17, 2020 at 03:00:23PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 16, 2020 at 10:12:33PM +0900, Paul Elder wrote:
> > Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what
> > v4l2-compliance wants.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 48 +++++++++++++++++++++++++++++++++-
> >  src/v4l2/v4l2_camera_proxy.h   |  4 +++
> >  2 files changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index d899853..f268f7a 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -34,7 +34,7 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
> >  				 std::shared_ptr<Camera> camera)
> >  	: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),
> >  	  vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1),
> > -	  acquiredFd_(-1), initialized_(false)
> > +	  v4l2RecordPriorityFd_(-1), acquiredFd_(-1), initialized_(false)
> >  {
> >  	querycap(camera);
> >  }
> > @@ -44,6 +44,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)
> >  	LOG(V4L2Compat, Debug) << "Servicing open fd = " << fd;
> >
> >  	nonBlockingFds_[fd] = nonBlocking;
> > +	priorities_[fd] = V4L2_PRIORITY_DEFAULT;
> >
> >  	refcount_++;
> >
> > @@ -77,6 +78,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)
> >  void V4L2CameraProxy::dup(int oldfd, int newfd)
> >  {
> >  	nonBlockingFds_[newfd] = nonBlockingFds_[oldfd];
> > +	priorities_[newfd] = V4L2_PRIORITY_DEFAULT;
> 
> Honest question, should this copy the priority of the duplicated fd,
> or set it to default ? I guess copying it would creat issues with
> PRIORITY_RECORD, but the V4L2 documentation does not specify this

You're right, the priority is a property of the file, not the the fd.
>From a V4L2 point of view, dup() is invisible and the dup'ed fd refers
to the same file object. I believe Paul is reworking the series to
introduce a class to model the file object.

> >  	refcount_++;
> >  }
> >
> > @@ -85,6 +87,7 @@ void V4L2CameraProxy::close(int fd)
> >  	LOG(V4L2Compat, Debug) << "Servicing close fd = " << fd;
> >
> >  	nonBlockingFds_.erase(fd);
> > +	priorities_.erase(fd);
> >
> >  	if (acquiredFd_ == fd)
> >  		acquiredFd_ = -1;
> > @@ -357,6 +360,43 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg)
> >  	return 0;
> >  }
> >
> > +int V4L2CameraProxy::vidioc_g_priority(int fd, enum v4l2_priority *arg)
> > +{
> > +	LOG(V4L2Compat, Debug) << "Servicing vidioc_g_priority fd = " << fd;
> > +
> > +	if (arg == nullptr)
> 
> As a general note, !arg is preferred instead of (arg == nullptr).
> This seems to apply to the whole patchset
> 
> > +		return -EFAULT;
> > +
> > +	if (v4l2RecordPriorityFd_ != -1)
> > +		*arg = V4L2_PRIORITY_RECORD;
> > +	else
> > +		*arg = priorities_[fd];
> > +
> > +	return 0;
> > +}
> > +
> > +int V4L2CameraProxy::vidioc_s_priority(int fd, enum v4l2_priority *arg)
> > +{
> > +	LOG(V4L2Compat, Debug)
> > +		<< "Servicing vidioc_s_priority fd = " << fd
> > +		<< " prio =  " << ((arg == nullptr) ? -1 : (int)*arg);
> > +
> > +	if (arg == nullptr)
> > +		return -EFAULT;
> > +
> > +	if (v4l2RecordPriorityFd_ != -1 && v4l2RecordPriorityFd_ != fd)
> > +		return -EBUSY;
> > +
> > +	if (*arg == V4L2_PRIORITY_RECORD)
> > +		v4l2RecordPriorityFd_ = fd;
> > +	else if (v4l2RecordPriorityFd_ == fd)
> > +		v4l2RecordPriorityFd_ = -1;
> > +
> > +	priorities_[fd] = *arg;
> > +
> > +	return 0;
> > +}
> > +
> 
> The patch looks good to me, minors apart
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  int V4L2CameraProxy::freeBuffers()
> >  {
> >  	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> > @@ -603,6 +643,12 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)
> >  	case VIDIOC_TRY_FMT:
> >  		ret = vidioc_try_fmt(fd, static_cast<struct v4l2_format *>(arg));
> >  		break;
> > +	case VIDIOC_G_PRIORITY:
> > +		ret = vidioc_g_priority(fd, static_cast<enum v4l2_priority *>(arg));
> > +		break;
> > +	case VIDIOC_S_PRIORITY:
> > +		ret = vidioc_s_priority(fd, static_cast<enum v4l2_priority *>(arg));
> > +		break;
> >  	case VIDIOC_REQBUFS:
> >  		ret = vidioc_reqbufs(fd, static_cast<struct v4l2_requestbuffers *>(arg));
> >  		break;
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > index 90e518c..a0c373d 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -49,6 +49,8 @@ private:
> >  	int vidioc_g_fmt(int fd, struct v4l2_format *arg);
> >  	int vidioc_s_fmt(int fd, struct v4l2_format *arg);
> >  	int vidioc_try_fmt(int fd, struct v4l2_format *arg);
> > +	int vidioc_g_priority(int fd, enum v4l2_priority *arg);
> > +	int vidioc_s_priority(int fd, enum v4l2_priority *arg);
> >  	int vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg);
> >  	int vidioc_querybuf(int fd, struct v4l2_buffer *arg);
> >  	int vidioc_qbuf(int fd, struct v4l2_buffer *arg);
> > @@ -84,6 +86,8 @@ private:
> >  	int efd_;
> >
> >  	std::map<int, bool> nonBlockingFds_;
> > +	std::map<int, enum v4l2_priority> priorities_;
> > +	int v4l2RecordPriorityFd_;
> >  	/*
> >  	 * This is an exclusive lock on the camera. When this is not taken,
> >  	 * anybody can call any ioctl before reqbufs. reqbufs with count > 0

Patch
diff mbox series 

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index d899853..f268f7a 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -34,7 +34,7 @@  V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
 				 std::shared_ptr<Camera> camera)
 	: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),
 	  vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1),
-	  acquiredFd_(-1), initialized_(false)
+	  v4l2RecordPriorityFd_(-1), acquiredFd_(-1), initialized_(false)
 {
 	querycap(camera);
 }
@@ -44,6 +44,7 @@  int V4L2CameraProxy::open(int fd, bool nonBlocking)
 	LOG(V4L2Compat, Debug) << "Servicing open fd = " << fd;
 
 	nonBlockingFds_[fd] = nonBlocking;
+	priorities_[fd] = V4L2_PRIORITY_DEFAULT;
 
 	refcount_++;
 
@@ -77,6 +78,7 @@  int V4L2CameraProxy::open(int fd, bool nonBlocking)
 void V4L2CameraProxy::dup(int oldfd, int newfd)
 {
 	nonBlockingFds_[newfd] = nonBlockingFds_[oldfd];
+	priorities_[newfd] = V4L2_PRIORITY_DEFAULT;
 	refcount_++;
 }
 
@@ -85,6 +87,7 @@  void V4L2CameraProxy::close(int fd)
 	LOG(V4L2Compat, Debug) << "Servicing close fd = " << fd;
 
 	nonBlockingFds_.erase(fd);
+	priorities_.erase(fd);
 
 	if (acquiredFd_ == fd)
 		acquiredFd_ = -1;
@@ -357,6 +360,43 @@  int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg)
 	return 0;
 }
 
+int V4L2CameraProxy::vidioc_g_priority(int fd, enum v4l2_priority *arg)
+{
+	LOG(V4L2Compat, Debug) << "Servicing vidioc_g_priority fd = " << fd;
+
+	if (arg == nullptr)
+		return -EFAULT;
+
+	if (v4l2RecordPriorityFd_ != -1)
+		*arg = V4L2_PRIORITY_RECORD;
+	else
+		*arg = priorities_[fd];
+
+	return 0;
+}
+
+int V4L2CameraProxy::vidioc_s_priority(int fd, enum v4l2_priority *arg)
+{
+	LOG(V4L2Compat, Debug)
+		<< "Servicing vidioc_s_priority fd = " << fd
+		<< " prio =  " << ((arg == nullptr) ? -1 : (int)*arg);
+
+	if (arg == nullptr)
+		return -EFAULT;
+
+	if (v4l2RecordPriorityFd_ != -1 && v4l2RecordPriorityFd_ != fd)
+		return -EBUSY;
+
+	if (*arg == V4L2_PRIORITY_RECORD)
+		v4l2RecordPriorityFd_ = fd;
+	else if (v4l2RecordPriorityFd_ == fd)
+		v4l2RecordPriorityFd_ = -1;
+
+	priorities_[fd] = *arg;
+
+	return 0;
+}
+
 int V4L2CameraProxy::freeBuffers()
 {
 	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
@@ -603,6 +643,12 @@  int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)
 	case VIDIOC_TRY_FMT:
 		ret = vidioc_try_fmt(fd, static_cast<struct v4l2_format *>(arg));
 		break;
+	case VIDIOC_G_PRIORITY:
+		ret = vidioc_g_priority(fd, static_cast<enum v4l2_priority *>(arg));
+		break;
+	case VIDIOC_S_PRIORITY:
+		ret = vidioc_s_priority(fd, static_cast<enum v4l2_priority *>(arg));
+		break;
 	case VIDIOC_REQBUFS:
 		ret = vidioc_reqbufs(fd, static_cast<struct v4l2_requestbuffers *>(arg));
 		break;
diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
index 90e518c..a0c373d 100644
--- a/src/v4l2/v4l2_camera_proxy.h
+++ b/src/v4l2/v4l2_camera_proxy.h
@@ -49,6 +49,8 @@  private:
 	int vidioc_g_fmt(int fd, struct v4l2_format *arg);
 	int vidioc_s_fmt(int fd, struct v4l2_format *arg);
 	int vidioc_try_fmt(int fd, struct v4l2_format *arg);
+	int vidioc_g_priority(int fd, enum v4l2_priority *arg);
+	int vidioc_s_priority(int fd, enum v4l2_priority *arg);
 	int vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg);
 	int vidioc_querybuf(int fd, struct v4l2_buffer *arg);
 	int vidioc_qbuf(int fd, struct v4l2_buffer *arg);
@@ -84,6 +86,8 @@  private:
 	int efd_;
 
 	std::map<int, bool> nonBlockingFds_;
+	std::map<int, enum v4l2_priority> priorities_;
+	int v4l2RecordPriorityFd_;
 	/*
 	 * This is an exclusive lock on the camera. When this is not taken,
 	 * anybody can call any ioctl before reqbufs. reqbufs with count > 0