[{"id":5357,"web_url":"https://patchwork.libcamera.org/comment/5357/","msgid":"<20200623222332.GG5870@pendragon.ideasonboard.com>","date":"2020-06-23T22:23:32","subject":"Re: [libcamera-devel] [PATCH v3 03/22] v4l2: v4l2_compat: Support\n\tmultiple open","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 Wed, Jun 24, 2020 at 04:08:17AM +0900, Paul Elder wrote:\n> Previously, since we acquired the libcamera camera upon open(), it was\n> impossible to support multiple open, as any subsequent opens would\n> return error because the camera would already be acquired.\n> \n> To fix this, we first initialize the camera in the first call to\n> V4L2CameraProxy::open(), just to heat up the stream format cache. We\n> then add ownership by a V4L2CameraFile of a V4L2Camera via the\n> V4L2CameraProxy. All vidioc ioctls prior to reqbufs > 0 (except for\n> s_fmt) are able to access the camera without ownership. A call to\n> reqbufs > 0 (and s_fmt) will take ownership, and the ownership will be\n> released at reqbufs = 0. While ownership is assigned, the eventfd that\n> should be signaled (and cleared) by V4L2Camera and V4L2CameraProxy is\n> set to the V4L2CameraFile that has ownership, and is cleared when the\n> ownership is released. In case close() is called without a\n> reqbufs = 0 first, the ownership is also released on close().\n> \n> We also use the V4L2CameraFile to contain all the information specific\n> to an open instance of the file. This removes the need to keep track of\n> such information within V4L2CameraProxy via multiple maps from int fd to\n> info.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n> ---\n> Changes in v3:\n> - split V4L2CompatManager stuff into separate patch\n> - split changing the fd argument to camera file argument in all proxy\n>   methods into the separate patch\n> - remove V4L2CameraProxy::efd_ (it's duplicated from\n>   V4L2CameraFile::efd())\n> - rename cf in all the V4L2CameraProxy methods to file\n> - rename V4L2CameraProxy::acquiredCf_ to V4L2CameraProxy::owner_\n> - remove V4L2CameraProxy::initialized_; just use V4L2CameraProxy::refcount_\n> - move bufferCount_ > 0 in reqbufs to different patch\n> - move TIMESTAMP_MONOTONIC to different patch\n> - don't take the lock for querybuf\n> - add V4L2CameraProxy::hasOwnership\n> - rename lock() to acquire() and unlock() to release()\n> - only lock on s_fmt and reqbufs > 0, in other ioctls only check for\n>   ownership\n> - add explanation about not supporting polling for events from an fd\n>   different than the one that owns the device for polling for bufs\n> \n> Changes in v2:\n> - use V4L2CameraFile in V4L2CompatManager to deal with dup, and in\n>   V4L2CameraProxy to deal with information specific to the open file\n>   (nonblock and priority)\n> - unlock exclusive camera lock on close()\n> ---\n>  src/v4l2/v4l2_camera.cpp       |  9 +++-\n>  src/v4l2/v4l2_camera.h         |  1 +\n>  src/v4l2/v4l2_camera_proxy.cpp | 99 ++++++++++++++++++++++++++++------\n>  src/v4l2/v4l2_camera_proxy.h   | 16 +++++-\n>  4 files changed, 106 insertions(+), 19 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 9a1ebc8..2557320 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -17,7 +17,8 @@ using namespace libcamera;\n>  LOG_DECLARE_CATEGORY(V4L2Compat);\n>  \n>  V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)\n> -\t: camera_(camera), isRunning_(false), bufferAllocator_(nullptr)\n> +\t: camera_(camera), isRunning_(false), bufferAllocator_(nullptr),\n> +\t  efd_(-1)\n>  {\n>  \tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n>  }\n> @@ -29,7 +30,6 @@ V4L2Camera::~V4L2Camera()\n>  \n>  int V4L2Camera::open()\n>  {\n> -\t/* \\todo Support multiple open. */\n>  \tif (camera_->acquire() < 0) {\n>  \t\tLOG(V4L2Compat, Error) << \"Failed to acquire camera\";\n>  \t\treturn -EINVAL;\n> @@ -59,6 +59,11 @@ void V4L2Camera::bind(int efd)\n>  \tefd_ = efd;\n>  }\n>  \n> +void V4L2Camera::unbind()\n> +{\n> +\tefd_ = -1;\n> +}\n> +\n>  void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)\n>  {\n>  \t*streamConfig = config_->at(0);\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index 33f5eb0..30114ed 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -40,6 +40,7 @@ public:\n>  \tint open();\n>  \tvoid close();\n>  \tvoid bind(int efd);\n> +\tvoid unbind();\n>  \tvoid getStreamConfig(StreamConfiguration *streamConfig);\n>  \tstd::vector<Buffer> completedBuffers();\n>  \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index c1ee1be..eb59d49 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -35,7 +35,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat);\n>  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))\n> +\t  vcam_(std::make_unique<V4L2Camera>(camera)), owner_(nullptr)\n>  {\n>  \tquerycap(camera);\n>  }\n> @@ -44,18 +44,29 @@ int V4L2CameraProxy::open(V4L2CameraFile *file)\n>  {\n>  \tLOG(V4L2Compat, Debug) << \"Servicing open fd = \" << file->efd();\n>  \n> +\tif (refcount_++)\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * We open the camera here, once, and keep it open until the last\n> +\t * V4L2CameraFile is closed. The proxy is initially not owned by any\n> +\t * file. The first file that calls reqbufs with count > 0 will become\n\n... with count > 0 or s_fmt will ...\n\n> +\t * the owner, and no other file will be allowed to call buffer-related\n> +\t * ioctls (except querycap, querybuf, try_fmt, g/s_priority,\n> +\t * enum/g/s_input), set the format or start or stop the stream until\n\nIn that list only querybuf is a buffer-related ioctl. I'd just write\n\"(except querybuf), set the format ...\".\n\n> +\t * ownership is released with a call to reqbufs with count = 0.\n> +\t */\n> +\n>  \tint ret = vcam_->open();\n>  \tif (ret < 0) {\n> -\t\terrno = -ret;\n> -\t\treturn -1;\n> +\t\trefcount_--;\n> +\t\treturn ret;\n>  \t}\n>  \n>  \tvcam_->getStreamConfig(&streamConfig_);\n>  \tsetFmtFromConfig(streamConfig_);\n>  \tsizeimage_ = calculateSizeImage(streamConfig_);\n>  \n> -\trefcount_++;\n> -\n>  \treturn 0;\n>  }\n>  \n> @@ -63,6 +74,7 @@ void V4L2CameraProxy::close(V4L2CameraFile *file)\n>  {\n>  \tLOG(V4L2Compat, Debug) << \"Servicing close fd = \" << file->efd();\n>  \n> +\trelease(file);\n>  \n>  \tif (--refcount_ > 0)\n>  \t\treturn;\n> @@ -277,12 +289,16 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)\n>  \tif (!validateBufferType(arg->type))\n>  \t\treturn -EINVAL;\n>  \n> +\tint ret = acquire(file);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n>  \ttryFormat(arg);\n>  \n>  \tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> -\tint ret = vcam_->configure(&streamConfig_, size,\n> -\t\t\t\t   v4l2ToDrm(arg->fmt.pix.pixelformat),\n> -\t\t\t\t   bufferCount_);\n> +\tret = vcam_->configure(&streamConfig_, size,\n> +\t\t\t       v4l2ToDrm(arg->fmt.pix.pixelformat),\n> +\t\t\t       bufferCount_);\n>  \tif (ret < 0)\n>  \t\treturn -EINVAL;\n>  \n> @@ -334,15 +350,21 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf\n>  \n>  \tLOG(V4L2Compat, Debug) << arg->count << \" buffers requested \";\n>  \n> +\tint ret = acquire(file);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n>  \targ->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;\n>  \n> -\tif (arg->count == 0)\n> +\tif (arg->count == 0) {\n> +\t\trelease(file);\n>  \t\treturn freeBuffers();\n> +\t}\n>  \n>  \tSize size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);\n> -\tint ret = vcam_->configure(&streamConfig_, size,\n> -\t\t\t\t   v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),\n> -\t\t\t\t   arg->count);\n> +\tret = vcam_->configure(&streamConfig_, size,\n> +\t\t\t       v4l2ToDrm(curV4L2Format_.fmt.pix.pixelformat),\n> +\t\t\t       arg->count);\n>  \tif (ret < 0)\n\nDon't you need to call release(file) here too ?\n\n>  \t\treturn -EINVAL;\n>  \n> @@ -409,6 +431,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n>  \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_qbuf, index = \"\n>  \t\t\t       << arg->index << \" fd = \" << file->efd();\n>  \n> +\tif (!hasOwnership(file))\n> +\t\treturn -EBUSY;\n> +\n>  \tif (!validateBufferType(arg->type) ||\n>  \t    !validateMemoryType(arg->memory) ||\n>  \t    arg->index >= bufferCount_)\n> @@ -428,6 +453,9 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n>  {\n>  \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_dqbuf fd = \" << file->efd();\n>  \n> +\tif (!hasOwnership(file))\n> +\t\treturn -EBUSY;\n> +\n>  \tif (!validateBufferType(arg->type) ||\n>  \t    !validateMemoryType(arg->memory))\n>  \t\treturn -EINVAL;\n> @@ -462,6 +490,9 @@ int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)\n>  \tif (!validateBufferType(*arg))\n>  \t\treturn -EINVAL;\n>  \n> +\tif (!hasOwnership(file))\n> +\t\treturn -EBUSY;\n> +\n>  \tcurrentBuf_ = 0;\n>  \n>  \treturn vcam_->streamOn();\n> @@ -474,6 +505,9 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)\n>  \tif (!validateBufferType(*arg))\n>  \t\treturn -EINVAL;\n>  \n> +\tif (!hasOwnership(file) && owner_)\n> +\t\treturn -EBUSY;\n> +\n>  \tint ret = vcam_->streamOff();\n>  \n>  \tfor (struct v4l2_buffer &buf : buffers_)\n> @@ -532,10 +566,45 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar\n>  \treturn ret;\n>  }\n>  \n> -void V4L2CameraProxy::bind(int fd)\n> +bool V4L2CameraProxy::hasOwnership(V4L2CameraFile *file)\n> +{\n> +\treturn owner_ == file;\n> +}\n> +\n> +/**\n> + * \\brief Acquire exclusive ownership of the V4L2Camera\n> + *\n> + * \\return Zero on success or if already acquired, and negative error on\n> + * failure.\n> + *\n> + * This is sufficient for poll()ing for buffers. Events, however, are signaled\n> + * on the file level, so all fds must be signaled. poll()ing from a different\n> + * fd than the one that locks the device is a corner case, and is currently not\n> + * supported.\n> + */\n> +int V4L2CameraProxy::acquire(V4L2CameraFile *file)\n>  {\n> -\tefd_ = fd;\n> -\tvcam_->bind(fd);\n> +\tif (owner_ == file)\n> +\t\treturn 0;\n> +\n> +\tif (owner_)\n> +\t\treturn -EBUSY;\n> +\n> +\tvcam_->bind(file->efd());\n> +\n> +\towner_ = file;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void V4L2CameraProxy::release(V4L2CameraFile *file)\n> +{\n> +\tif (owner_ != file)\n> +\t\treturn;\n> +\n> +\tvcam_->unbind();\n> +\n> +\towner_ = nullptr;\n>  }\n>  \n>  struct PixelFormatPlaneInfo {\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index b2197ef..2be46e6 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -33,7 +33,6 @@ public:\n>  \tvoid *mmap(void *addr, size_t length, int prot, int flags, off64_t offset);\n>  \tint munmap(void *addr, size_t length);\n>  \n> -\tvoid bind(int fd);\n>  \tint ioctl(V4L2CameraFile *file, unsigned long request, void *arg);\n>  \n>  private:\n> @@ -58,6 +57,10 @@ private:\n>  \tint vidioc_streamon(V4L2CameraFile *file, int *arg);\n>  \tint vidioc_streamoff(V4L2CameraFile *file, int *arg);\n>  \n> +\tbool hasOwnership(V4L2CameraFile *file);\n> +\tint acquire(V4L2CameraFile *file);\n> +\tvoid release(V4L2CameraFile *file);\n> +\n>  \tstatic unsigned int bplMultiplier(uint32_t format);\n>  \tstatic unsigned int imageSize(uint32_t format, unsigned int width,\n>  \t\t\t\t      unsigned int height);\n> @@ -80,7 +83,16 @@ private:\n>  \n>  \tstd::unique_ptr<V4L2Camera> vcam_;\n>  \n> -\tint efd_;\n> +\t/*\n> +\t * This is the exclusive owner of this V4L2CameraProxy instance.\n> +\t * When there is no owner, anybody can call any ioctl before reqbufs.\n> +\t * The first file to call reqbufs with count > 0 will become the owner,\n\n... count > 0 or s_fmt will ...\n\n> +\t * and when the owner calls reqbufs with count = 0 it will release\n> +\t * ownership. Any ioctl that is call by a non-owner while there exists\n> +\t * and owner will return -EBUSY (except querycap, querybuf, try_fmt,\n> +\t * g/s_priority, enum/g/s_input\n\nAnd to match the previous comment, \"Any buffer-related ioctl (except\nquerybuf) or s_fmt call by a non-owner ... will return -EBUSY.\" ?\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\t */\n> +\tV4L2CameraFile *owner_;\n>  };\n>  \n>  #endif /* __V4L2_CAMERA_PROXY_H__ */","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 3BD75603B9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Jun 2020 00:24:00 +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 809132A9;\n\tWed, 24 Jun 2020 00:23:59 +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=\"v4BPR7Yr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592951039;\n\tbh=jyuZ61RMrSC4ly87yffZt9WY6oqlDGMpokGmTARNSds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v4BPR7YryTdzgulIYxyLmiAuzgQTA3a8Oj2XU1uax2ArUW4wRuUyXhO5aBs8C3sDu\n\tKotagadfG3GNiTEzUJ7dGiZfJerfKFKJoxgE4gt+4M1hPSAWijpLbzoRIHuUxYbEdY\n\tthJ1wu/Zu8InMzsf1IqidpLLsKjWrwWWquTrD1Cc=","Date":"Wed, 24 Jun 2020 01:23:32 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200623222332.GG5870@pendragon.ideasonboard.com>","References":"<20200623190836.53446-1-paul.elder@ideasonboard.com>\n\t<20200623190836.53446-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200623190836.53446-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/22] v4l2: v4l2_compat: Support\n\tmultiple open","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":"Tue, 23 Jun 2020 22:24:00 -0000"}}]