[{"id":5240,"web_url":"https://patchwork.libcamera.org/comment/5240/","msgid":"<20200617130023.ksmt3ciwpg3ulhbr@uno.localdomain>","date":"2020-06-17T13:00:23","subject":"Re: [libcamera-devel] [PATCH 04/15] v4l2: v4l2_camera_proxy:\n\tImplement VIDIOC_G/S_PRIORITY","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Jun 16, 2020 at 10:12:33PM +0900, Paul Elder wrote:\n> Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what\n> v4l2-compliance wants.\n>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  src/v4l2/v4l2_camera_proxy.cpp | 48 +++++++++++++++++++++++++++++++++-\n>  src/v4l2/v4l2_camera_proxy.h   |  4 +++\n>  2 files changed, 51 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index d899853..f268f7a 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -34,7 +34,7 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,\n>  \t\t\t\t std::shared_ptr<Camera> camera)\n>  \t: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),\n>  \t  vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1),\n> -\t  acquiredFd_(-1), initialized_(false)\n> +\t  v4l2RecordPriorityFd_(-1), acquiredFd_(-1), initialized_(false)\n>  {\n>  \tquerycap(camera);\n>  }\n> @@ -44,6 +44,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)\n>  \tLOG(V4L2Compat, Debug) << \"Servicing open fd = \" << fd;\n>\n>  \tnonBlockingFds_[fd] = nonBlocking;\n> +\tpriorities_[fd] = V4L2_PRIORITY_DEFAULT;\n>\n>  \trefcount_++;\n>\n> @@ -77,6 +78,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)\n>  void V4L2CameraProxy::dup(int oldfd, int newfd)\n>  {\n>  \tnonBlockingFds_[newfd] = nonBlockingFds_[oldfd];\n> +\tpriorities_[newfd] = V4L2_PRIORITY_DEFAULT;\n\nHonest question, should this copy the priority of the duplicated fd,\nor set it to default ? I guess copying it would creat issues with\nPRIORITY_RECORD, but the V4L2 documentation does not specify this\n\n>  \trefcount_++;\n>  }\n>\n> @@ -85,6 +87,7 @@ void V4L2CameraProxy::close(int fd)\n>  \tLOG(V4L2Compat, Debug) << \"Servicing close fd = \" << fd;\n>\n>  \tnonBlockingFds_.erase(fd);\n> +\tpriorities_.erase(fd);\n>\n>  \tif (acquiredFd_ == fd)\n>  \t\tacquiredFd_ = -1;\n> @@ -357,6 +360,43 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg)\n>  \treturn 0;\n>  }\n>\n> +int V4L2CameraProxy::vidioc_g_priority(int fd, enum v4l2_priority *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_priority fd = \" << fd;\n> +\n> +\tif (arg == nullptr)\n\nAs a general note, !arg is preferred instead of (arg == nullptr).\nThis seems to apply to the whole patchset\n\n> +\t\treturn -EFAULT;\n> +\n> +\tif (v4l2RecordPriorityFd_ != -1)\n> +\t\t*arg = V4L2_PRIORITY_RECORD;\n> +\telse\n> +\t\t*arg = priorities_[fd];\n> +\n> +\treturn 0;\n> +}\n> +\n> +int V4L2CameraProxy::vidioc_s_priority(int fd, enum v4l2_priority *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug)\n> +\t\t<< \"Servicing vidioc_s_priority fd = \" << fd\n> +\t\t<< \" prio =  \" << ((arg == nullptr) ? -1 : (int)*arg);\n> +\n> +\tif (arg == nullptr)\n> +\t\treturn -EFAULT;\n> +\n> +\tif (v4l2RecordPriorityFd_ != -1 && v4l2RecordPriorityFd_ != fd)\n> +\t\treturn -EBUSY;\n> +\n> +\tif (*arg == V4L2_PRIORITY_RECORD)\n> +\t\tv4l2RecordPriorityFd_ = fd;\n> +\telse if (v4l2RecordPriorityFd_ == fd)\n> +\t\tv4l2RecordPriorityFd_ = -1;\n> +\n> +\tpriorities_[fd] = *arg;\n> +\n> +\treturn 0;\n> +}\n> +\n\nThe patch looks good to me, minors apart\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n  j\n\n>  int V4L2CameraProxy::freeBuffers()\n>  {\n>  \tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> @@ -603,6 +643,12 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)\n>  \tcase VIDIOC_TRY_FMT:\n>  \t\tret = vidioc_try_fmt(fd, static_cast<struct v4l2_format *>(arg));\n>  \t\tbreak;\n> +\tcase VIDIOC_G_PRIORITY:\n> +\t\tret = vidioc_g_priority(fd, static_cast<enum v4l2_priority *>(arg));\n> +\t\tbreak;\n> +\tcase VIDIOC_S_PRIORITY:\n> +\t\tret = vidioc_s_priority(fd, static_cast<enum v4l2_priority *>(arg));\n> +\t\tbreak;\n>  \tcase VIDIOC_REQBUFS:\n>  \t\tret = vidioc_reqbufs(fd, static_cast<struct v4l2_requestbuffers *>(arg));\n>  \t\tbreak;\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index 90e518c..a0c373d 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -49,6 +49,8 @@ private:\n>  \tint vidioc_g_fmt(int fd, struct v4l2_format *arg);\n>  \tint vidioc_s_fmt(int fd, struct v4l2_format *arg);\n>  \tint vidioc_try_fmt(int fd, struct v4l2_format *arg);\n> +\tint vidioc_g_priority(int fd, enum v4l2_priority *arg);\n> +\tint vidioc_s_priority(int fd, enum v4l2_priority *arg);\n>  \tint vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg);\n>  \tint vidioc_querybuf(int fd, struct v4l2_buffer *arg);\n>  \tint vidioc_qbuf(int fd, struct v4l2_buffer *arg);\n> @@ -84,6 +86,8 @@ private:\n>  \tint efd_;\n>\n>  \tstd::map<int, bool> nonBlockingFds_;\n> +\tstd::map<int, enum v4l2_priority> priorities_;\n> +\tint v4l2RecordPriorityFd_;\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\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBDAF603BF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 17 Jun 2020 14:56:58 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 66F6A240010;\n\tWed, 17 Jun 2020 12:56:58 +0000 (UTC)"],"Date":"Wed, 17 Jun 2020 15:00:23 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200617130023.ksmt3ciwpg3ulhbr@uno.localdomain>","References":"<20200616131244.70308-1-paul.elder@ideasonboard.com>\n\t<20200616131244.70308-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200616131244.70308-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 04/15] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 17 Jun 2020 12:56:59 -0000"}},{"id":5250,"web_url":"https://patchwork.libcamera.org/comment/5250/","msgid":"<20200617153315.GH5838@pendragon.ideasonboard.com>","date":"2020-06-17T15:33:15","subject":"Re: [libcamera-devel] [PATCH 04/15] 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 Jacopo,\n\nOn Wed, Jun 17, 2020 at 03:00:23PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 16, 2020 at 10:12:33PM +0900, Paul Elder wrote:\n> > Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what\n> > v4l2-compliance wants.\n> >\n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > ---\n> >  src/v4l2/v4l2_camera_proxy.cpp | 48 +++++++++++++++++++++++++++++++++-\n> >  src/v4l2/v4l2_camera_proxy.h   |  4 +++\n> >  2 files changed, 51 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index d899853..f268f7a 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -34,7 +34,7 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,\n> >  \t\t\t\t std::shared_ptr<Camera> camera)\n> >  \t: refcount_(0), index_(index), bufferCount_(0), currentBuf_(0),\n> >  \t  vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1),\n> > -\t  acquiredFd_(-1), initialized_(false)\n> > +\t  v4l2RecordPriorityFd_(-1), acquiredFd_(-1), initialized_(false)\n> >  {\n> >  \tquerycap(camera);\n> >  }\n> > @@ -44,6 +44,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)\n> >  \tLOG(V4L2Compat, Debug) << \"Servicing open fd = \" << fd;\n> >\n> >  \tnonBlockingFds_[fd] = nonBlocking;\n> > +\tpriorities_[fd] = V4L2_PRIORITY_DEFAULT;\n> >\n> >  \trefcount_++;\n> >\n> > @@ -77,6 +78,7 @@ int V4L2CameraProxy::open(int fd, bool nonBlocking)\n> >  void V4L2CameraProxy::dup(int oldfd, int newfd)\n> >  {\n> >  \tnonBlockingFds_[newfd] = nonBlockingFds_[oldfd];\n> > +\tpriorities_[newfd] = V4L2_PRIORITY_DEFAULT;\n> \n> Honest question, should this copy the priority of the duplicated fd,\n> or set it to default ? I guess copying it would creat issues with\n> PRIORITY_RECORD, but the V4L2 documentation does not specify this\n\nYou're right, the priority is a property of the file, not the the fd.\n>From a V4L2 point of view, dup() is invisible and the dup'ed fd refers\nto the same file object. I believe Paul is reworking the series to\nintroduce a class to model the file object.\n\n> >  \trefcount_++;\n> >  }\n> >\n> > @@ -85,6 +87,7 @@ void V4L2CameraProxy::close(int fd)\n> >  \tLOG(V4L2Compat, Debug) << \"Servicing close fd = \" << fd;\n> >\n> >  \tnonBlockingFds_.erase(fd);\n> > +\tpriorities_.erase(fd);\n> >\n> >  \tif (acquiredFd_ == fd)\n> >  \t\tacquiredFd_ = -1;\n> > @@ -357,6 +360,43 @@ int V4L2CameraProxy::vidioc_try_fmt(int fd, struct v4l2_format *arg)\n> >  \treturn 0;\n> >  }\n> >\n> > +int V4L2CameraProxy::vidioc_g_priority(int fd, enum v4l2_priority *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_priority fd = \" << fd;\n> > +\n> > +\tif (arg == nullptr)\n> \n> As a general note, !arg is preferred instead of (arg == nullptr).\n> This seems to apply to the whole patchset\n> \n> > +\t\treturn -EFAULT;\n> > +\n> > +\tif (v4l2RecordPriorityFd_ != -1)\n> > +\t\t*arg = V4L2_PRIORITY_RECORD;\n> > +\telse\n> > +\t\t*arg = priorities_[fd];\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int V4L2CameraProxy::vidioc_s_priority(int fd, enum v4l2_priority *arg)\n> > +{\n> > +\tLOG(V4L2Compat, Debug)\n> > +\t\t<< \"Servicing vidioc_s_priority fd = \" << fd\n> > +\t\t<< \" prio =  \" << ((arg == nullptr) ? -1 : (int)*arg);\n> > +\n> > +\tif (arg == nullptr)\n> > +\t\treturn -EFAULT;\n> > +\n> > +\tif (v4l2RecordPriorityFd_ != -1 && v4l2RecordPriorityFd_ != fd)\n> > +\t\treturn -EBUSY;\n> > +\n> > +\tif (*arg == V4L2_PRIORITY_RECORD)\n> > +\t\tv4l2RecordPriorityFd_ = fd;\n> > +\telse if (v4l2RecordPriorityFd_ == fd)\n> > +\t\tv4l2RecordPriorityFd_ = -1;\n> > +\n> > +\tpriorities_[fd] = *arg;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> \n> The patch looks good to me, minors apart\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> >  int V4L2CameraProxy::freeBuffers()\n> >  {\n> >  \tLOG(V4L2Compat, Debug) << \"Freeing libcamera bufs\";\n> > @@ -603,6 +643,12 @@ int V4L2CameraProxy::ioctl(int fd, unsigned long request, void *arg)\n> >  \tcase VIDIOC_TRY_FMT:\n> >  \t\tret = vidioc_try_fmt(fd, static_cast<struct v4l2_format *>(arg));\n> >  \t\tbreak;\n> > +\tcase VIDIOC_G_PRIORITY:\n> > +\t\tret = vidioc_g_priority(fd, static_cast<enum v4l2_priority *>(arg));\n> > +\t\tbreak;\n> > +\tcase VIDIOC_S_PRIORITY:\n> > +\t\tret = vidioc_s_priority(fd, static_cast<enum v4l2_priority *>(arg));\n> > +\t\tbreak;\n> >  \tcase VIDIOC_REQBUFS:\n> >  \t\tret = vidioc_reqbufs(fd, static_cast<struct v4l2_requestbuffers *>(arg));\n> >  \t\tbreak;\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index 90e518c..a0c373d 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -49,6 +49,8 @@ private:\n> >  \tint vidioc_g_fmt(int fd, struct v4l2_format *arg);\n> >  \tint vidioc_s_fmt(int fd, struct v4l2_format *arg);\n> >  \tint vidioc_try_fmt(int fd, struct v4l2_format *arg);\n> > +\tint vidioc_g_priority(int fd, enum v4l2_priority *arg);\n> > +\tint vidioc_s_priority(int fd, enum v4l2_priority *arg);\n> >  \tint vidioc_reqbufs(int fd, struct v4l2_requestbuffers *arg);\n> >  \tint vidioc_querybuf(int fd, struct v4l2_buffer *arg);\n> >  \tint vidioc_qbuf(int fd, struct v4l2_buffer *arg);\n> > @@ -84,6 +86,8 @@ private:\n> >  \tint efd_;\n> >\n> >  \tstd::map<int, bool> nonBlockingFds_;\n> > +\tstd::map<int, enum v4l2_priority> priorities_;\n> > +\tint v4l2RecordPriorityFd_;\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":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 537F9603BF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 17 Jun 2020 17:33:38 +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 D90C7F9;\n\tWed, 17 Jun 2020 17:33:37 +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=\"n2CI//aY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592408018;\n\tbh=lY7zIBB3u77mr7uikKb9XCSI4PH1A61IcOZu6ZatARI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n2CI//aYTtD/tv6MCAjENUYxMUlsPvumR7LC6sLFdjIziue6YAHDzJeE3NiSFyJc/\n\tVW+VwTsnHGeEHiHR94tgpQiaUp+N9OFRsIPSLciD42V1pqPquvzjyCQHA0vBnV1wF2\n\tuRv4/VMo0om3YVkku9iM071H0mo5S1IEO3IzcK30=","Date":"Wed, 17 Jun 2020 18:33:15 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"Paul Elder <paul.elder@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200617153315.GH5838@pendragon.ideasonboard.com>","References":"<20200616131244.70308-1-paul.elder@ideasonboard.com>\n\t<20200616131244.70308-5-paul.elder@ideasonboard.com>\n\t<20200617130023.ksmt3ciwpg3ulhbr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200617130023.ksmt3ciwpg3ulhbr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 04/15] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 17 Jun 2020 15:33:38 -0000"}}]