[{"id":5295,"web_url":"https://patchwork.libcamera.org/comment/5295/","msgid":"<20200620014230.GS5823@pendragon.ideasonboard.com>","date":"2020-06-20T01:42:30","subject":"Re: [libcamera-devel] [PATCH v2 05/17] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_G/S_PRIORITY","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jun 19, 2020 at 02:41:11PM +0900, Paul Elder wrote:\n> Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what\n> v4l2-compliance wants.\n\nHow about mentioning the kernel implementation instead of\nv4l2-compliance ? Something along the lines of\n\nImplement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY. The behaviour\ndocumented in the V4L2 specification doesn't match the implementation in\nthe Linux kernel, implement the latter.\n\n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - use V4L2CameraFile instead of fd and priorities map\n> ---\n> src/v4l2/v4l2_camera_proxy.cpp | 42 +++++++++++++++++++++++++++++++++-\n> src/v4l2/v4l2_camera_proxy.h | 3 +++\n> 2 files changed, 44 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 117d7ff..3e95645 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -37,7 +37,8 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,\n> \t\t\t\t std::shared_ptr camera)\n> \t: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),\n> \t vcam_(std::make_unique(camera)), efd_(-1),\n> -\t acquiredCf_(nullptr), initialized_(false)\n> +\t v4l2RecordPriorityCf_(nullptr), acquiredCf_(nullptr),\n> +\t initialized_(false)\n> {\n> \tquerycap(camera);\n> }\n> @@ -344,6 +345,37 @@ int V4L2CameraProxy::vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)\n> \treturn 0;\n> }\n> \n> +int V4L2CameraProxy::vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_priority fd = \" << cf->efd();\n> +\n> +\tif (v4l2RecordPriorityCf_)\n> +\t\t*arg = V4L2_PRIORITY_RECORD;\n> +\telse\n> +\t\t*arg = cf->priority_;\n\nThis will please v4l2-compliance, but won't correctly support\nV4L2_PRIORITY_INTERACTIVE and V4L2_PRIORITY_BACKGROUND. Is that why you\nmentioned v4l2-compliance and not the kernel in the commit message ? Do\nyou think we should implement the full behaviour ? It shouldn't be too\ndifficult, but would require retrieving the max priority. You could do\nso by storing a list of V4L2CameraFile in V4L2CameraProxy, or an array\nof integers containing with the number of V4L2CameraFile for a given\npriority.\n\nTo really implement priority support you would need to check if the file\nhas a high enough priority in all the ioctls that require doing so. For\nthe ioctls we support, that would be\n\n\tVIDIOC_S_FMT\n\tVIDIOC_S_PRIORITY\n\tVIDIOC_S_INPUT\n\tVIDIOC_REQBUFS\n\tVIDIOC_STREAMON\n\tVIDIOC_STREAMOFF\n\nThe check would simply verify that the file priority is >= max priority,\nimplemented in a helper function. I think that would also simplify the\nimplementation of vidioc_s_priority.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug)\n> +\t\t<< \"Servicing vidioc_s_priority fd = \" << cf->efd()\n> +\t\t<< \" prio = \" << (int)*arg;\n\nstatic_cast\n\nShould you check that the value of *arg is a valid priority ?\n\n> +\n> +\tif (v4l2RecordPriorityCf_ && v4l2RecordPriorityCf_ != cf)\n> +\t\treturn -EBUSY;\n> +\n> +\tif (*arg == V4L2_PRIORITY_RECORD)\n> +\t\tv4l2RecordPriorityCf_ = cf;\n> +\telse if (v4l2RecordPriorityCf_ == cf)\n> +\t\tv4l2RecordPriorityCf_ = nullptr;\n> +\n> +\tcf->priority_ = *arg;\n> +\n> +\treturn 0;\n> +}\n> +\n> int V4L2CameraProxy::freeBuffers()\n> {\n> \tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> @@ -560,6 +592,8 @@ std::set V4L2CameraProxy::supportedIoctls_ = {\n\nI forgot to mention in the review of the patch that introduced this, I\nwonder if std::unordered_set would be more efficient.\n\n> \tVIDIOC_G_FMT,\n> \tVIDIOC_S_FMT,\n> \tVIDIOC_TRY_FMT,\n> +\tVIDIOC_G_PRIORITY,\n> +\tVIDIOC_S_PRIORITY,\n> \tVIDIOC_REQBUFS,\n> \tVIDIOC_QUERYBUF,\n> \tVIDIOC_QBUF,\n> @@ -597,6 +631,12 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)\n> \tcase VIDIOC_TRY_FMT:\n> \t\tret = vidioc_try_fmt(cf, static_cast(arg));\n> \t\tbreak;\n> +\tcase VIDIOC_G_PRIORITY:\n> +\t\tret = vidioc_g_priority(cf, static_cast(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_S_PRIORITY:\n> +\t\tret = vidioc_s_priority(cf, static_cast(arg));\n> +\t\tbreak;\n> \tcase VIDIOC_REQBUFS:\n> \t\tret = vidioc_reqbufs(cf, static_cast(arg));\n> \t\tbreak;\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index dd7e793..3260eec 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -51,6 +51,8 @@ private:\n> \tint vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);\n> \tint vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);\n> \tint vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg);\n> +\tint vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg);\n> +\tint vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg);\n> \tint vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg);\n> \tint vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);\n> \tint vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);\n> @@ -87,6 +89,7 @@ private:\n> \n> \tint efd_;\n> \n> +\tV4L2CameraFile *v4l2RecordPriorityCf_;\n> \t/*\n> \t * This is an exclusive lock on the camera. When this is not taken,\n> \t * anybody can call any ioctl before reqbufs. reqbufs with count > 0","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66D3560710\n\tfor ;\n\tSat, 20 Jun 2020 03:42:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D09DC552;\n\tSat, 20 Jun 2020 03:42:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YbdZFlie\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592617374;\n\tbh=uzv3M58ZA4AMo5w/H3AlUCYPtuFsiVHSjzN5QIa1D1o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YbdZFliem4QZoZ9XsD6sy3469h6CmonMof1Zl79XnzaIoDkqq9sp15PspCqeiobA6\n\tkLJkNR7NJRAKjYea3E74AyqEYuvRvAWb9WmpFyY5IvTcCfZUI5/Wl/Vfb1Pas6CJhh\n\t3mzipG2e33asr4a7BnCTENuJm+7NSlPU0I7qnhG0=","Date":"Sat, 20 Jun 2020 04:42:30 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200620014230.GS5823@pendragon.ideasonboard.com>","References":"<20200619054123.19052-1-paul.elder@ideasonboard.com>\n\t<20200619054123.19052-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200619054123.19052-6-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 05/17] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_G/S_PRIORITY","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 20 Jun 2020 01:42:54 -0000"}}]