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

[Laurent Pinchart]

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 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

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 at 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

-- 
Regards,

Laurent Pinchart