[libcamera-devel] [PATCH 04/15] v4l2: v4l2_camera_proxy: Implement VIDIOC_G/S_PRIORITY

[Jacopo Mondi]

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 at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel