[libcamera-devel] [PATCH v2 05/17] v4l2: v4l2_camera_proxy: Implement VIDIOC_G/S_PRIORITY

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 19, 2020 at 02:41:11PM +0900, Paul Elder wrote:
> Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what
> v4l2-compliance wants.

How about mentioning the kernel implementation instead of
v4l2-compliance ? Something along the lines of

Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY. The behaviour
documented in the V4L2 specification doesn't match the implementation in
the Linux kernel, implement the latter.

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Changes in v2:
> - use V4L2CameraFile instead of fd and priorities map
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 42 +++++++++++++++++++++++++++++++++-
>  src/v4l2/v4l2_camera_proxy.h   |  3 +++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 117d7ff..3e95645 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -37,7 +37,8 @@ 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),
> -	  acquiredCf_(nullptr), initialized_(false)
> +	  v4l2RecordPriorityCf_(nullptr), acquiredCf_(nullptr),
> +	  initialized_(false)
>  {
>  	querycap(camera);
>  }
> @@ -344,6 +345,37 @@ int V4L2CameraProxy::vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)
>  	return 0;
>  }
>  
> +int V4L2CameraProxy::vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg)
> +{
> +	LOG(V4L2Compat, Debug) << "Servicing vidioc_g_priority fd = " << cf->efd();
> +
> +	if (v4l2RecordPriorityCf_)
> +		*arg = V4L2_PRIORITY_RECORD;
> +	else
> +		*arg = cf->priority_;

This will please v4l2-compliance, but won't correctly support
V4L2_PRIORITY_INTERACTIVE and V4L2_PRIORITY_BACKGROUND. Is that why you
mentioned v4l2-compliance and not the kernel in the commit message ? Do
you think we should implement the full behaviour ? It shouldn't be too
difficult, but would require retrieving the max priority. You could do
so by storing a list of V4L2CameraFile in V4L2CameraProxy, or an array
of integers containing with the number of V4L2CameraFile for a given
priority.

To really implement priority support you would need to check if the file
has a high enough priority in all the ioctls that require doing so. For
the ioctls we support, that would be

	VIDIOC_S_FMT
	VIDIOC_S_PRIORITY
	VIDIOC_S_INPUT
	VIDIOC_REQBUFS
	VIDIOC_STREAMON
	VIDIOC_STREAMOFF

The check would simply verify that the file priority is >= max priority,
implemented in a helper function. I think that would also simplify the
implementation of vidioc_s_priority.

> +
> +	return 0;
> +}
> +
> +int V4L2CameraProxy::vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg)
> +{
> +	LOG(V4L2Compat, Debug)
> +		<< "Servicing vidioc_s_priority fd = " << cf->efd()
> +		<< " prio =  " << (int)*arg;

static_cast<int>

Should you check that the value of *arg is a valid priority ?

> +
> +	if (v4l2RecordPriorityCf_ && v4l2RecordPriorityCf_ != cf)
> +		return -EBUSY;
> +
> +	if (*arg == V4L2_PRIORITY_RECORD)
> +		v4l2RecordPriorityCf_ = cf;
> +	else if (v4l2RecordPriorityCf_ == cf)
> +		v4l2RecordPriorityCf_ = nullptr;
> +
> +	cf->priority_ = *arg;
> +
> +	return 0;
> +}
> +
>  int V4L2CameraProxy::freeBuffers()
>  {
>  	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> @@ -560,6 +592,8 @@ std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {

I forgot to mention in the review of the patch that introduced this, I
wonder if std::unordered_set would be more efficient.

>  	VIDIOC_G_FMT,
>  	VIDIOC_S_FMT,
>  	VIDIOC_TRY_FMT,
> +	VIDIOC_G_PRIORITY,
> +	VIDIOC_S_PRIORITY,
>  	VIDIOC_REQBUFS,
>  	VIDIOC_QUERYBUF,
>  	VIDIOC_QBUF,
> @@ -597,6 +631,12 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)
>  	case VIDIOC_TRY_FMT:
>  		ret = vidioc_try_fmt(cf, static_cast<struct v4l2_format *>(arg));
>  		break;
> +	case VIDIOC_G_PRIORITY:
> +		ret = vidioc_g_priority(cf, static_cast<enum v4l2_priority *>(arg));
> +		break;
> +	case VIDIOC_S_PRIORITY:
> +		ret = vidioc_s_priority(cf, static_cast<enum v4l2_priority *>(arg));
> +		break;
>  	case VIDIOC_REQBUFS:
>  		ret = vidioc_reqbufs(cf, static_cast<struct v4l2_requestbuffers *>(arg));
>  		break;
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index dd7e793..3260eec 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -51,6 +51,8 @@ private:
>  	int vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);
>  	int vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);
>  	int vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);
> +	int vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg);
> +	int vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg);
>  	int vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg);
>  	int vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);
>  	int vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);
> @@ -87,6 +89,7 @@ private:
>  
>  	int efd_;
>  
> +	V4L2CameraFile *v4l2RecordPriorityCf_;
>  	/*
>  	 * 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