[{"id":5292,"web_url":"https://patchwork.libcamera.org/comment/5292/","msgid":"<20200620010224.GP5823@pendragon.ideasonboard.com>","date":"2020-06-20T01:02:24","subject":"Re: [libcamera-devel] [PATCH v2 02/17] 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 Fri, Jun 19, 2020 at 02:41:08PM +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 an exclusive lock on the V4L2Camera. All vidioc ioctls prior to\n> reqbufs > 0 (except for s_fmt) are able to access the camera without\n> this exclusive lock. A call to reqbufs > 0 (and s_fmt) will take the\n> lock, and the lock will be released at reqbufs = 0.\n\nSo far so good.\n\n> While the lock is\n> taken, the eventfd that should be signaled (and cleared) by V4L2Camera\n> and V4L2CameraProxy is set to the fd that has taken the lock, and is\n> cleared when the lock is released. In case close() is called without a\n> reqbufs = 0 first, the lock is also released on close().\n\nIn V4L2 poll() signalling is performed at the video device (== inode)\nlevel for buffers, and at the file level for V4L2 events. This would be\ntricky to emulate, as we would have to signal all fds, but there would\nbe a single DQBUF call that would acquire the semaphore. All the other\nfds would then return immediately from a poll() call, without a wait to\nreset the semaphore.\n\nGiven that poll()ing from a different fd (pointing to the same file or\neven a different file) and the one that locks the device is really a\ncorner case, I don't think we need to address this issue until an\napplication sadly forces us to, but recording the explanation somewhere\nwould be useful, either in a comment in the code, or in the commit\nmessage.\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. We also handle dup in V4L2CompatManager instead, since that is\n> just another int fd pointing to the same V4L2CameraFile instance.\n> V4L2CompatManager is also modified to access the V4L2CameraProxy through\n> the V4L2CameraFile.\n\nThe patch is fairly large, and contains multiple changes, I think I\nwould have split it (for instance passing V4L2CameraFile to the various\nV4L2CameraProxy functions could have gone to a separate preparatory\npatch). It would make review a bit easier. There's no need to resubmit\njust for this.\n\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> \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_file.cpp    |   4 +-\n>  src/v4l2/v4l2_camera_proxy.cpp   | 201 ++++++++++++++++++++++---------\n>  src/v4l2/v4l2_camera_proxy.h     |  47 +++++---\n>  src/v4l2/v4l2_compat_manager.cpp |  71 +++++------\n>  src/v4l2/v4l2_compat_manager.h   |   4 +-\n>  7 files changed, 215 insertions(+), 122 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_file.cpp b/src/v4l2/v4l2_camera_file.cpp\n> index 8916729..d07b936 100644\n> --- a/src/v4l2/v4l2_camera_file.cpp\n> +++ b/src/v4l2/v4l2_camera_file.cpp\n> @@ -21,12 +21,12 @@ V4L2CameraFile::V4L2CameraFile(int efd, bool nonBlocking, V4L2CameraProxy *proxy\n>  \t: priority_(V4L2_PRIORITY_DEFAULT), proxy_(proxy),\n>  \t  nonBlocking_(nonBlocking), efd_(efd)\n>  {\n> -\tproxy_->open(nonBlocking);\n> +\tproxy_->open(this);\n>  }\n>  \n>  V4L2CameraFile::~V4L2CameraFile()\n>  {\n> -\tproxy_->close();\n> +\tproxy_->close(this);\n>  }\n>  \n>  V4L2CameraProxy *V4L2CameraFile::proxy()\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index bf47aa7..f06f58d 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -23,6 +23,7 @@\n>  #include \"libcamera/internal/utils.h\"\n>  \n>  #include \"v4l2_camera.h\"\n> +#include \"v4l2_camera_file.h\"\n>  #include \"v4l2_compat_manager.h\"\n>  \n>  #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))\n> @@ -34,45 +35,57 @@ 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)), efd_(-1),\n> +\t  acquiredCf_(nullptr), initialized_(false)\n\nHow about s/acquiredCf_/owner_/ ?\n\n>  {\n>  \tquerycap(camera);\n>  }\n>  \n> -int V4L2CameraProxy::open(bool nonBlocking)\n> +int V4L2CameraProxy::open(V4L2CameraFile *cf)\n\ns/cf/file/\n\nand below too. cf isn't very explicit.\n\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing open\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing open fd = \" << cf->efd();\n>  \n> -\tint ret = vcam_->open();\n> -\tif (ret < 0) {\n> -\t\terrno = -ret;\n> -\t\treturn -1;\n> -\t}\n> +\trefcount_++;\n> +\n> +\tif (initialized_)\n> +\t\treturn 0;\n>  \n> -\tnonBlocking_ = nonBlocking;\n> +\t/*\n> +\t * We will open the camera here, once, and keep it open until the last\n> +\t * V4L2CameraFile is closed. Before any reqbufs with count > 0 is\n> +\t * called, anybody can call any ioctl. Once reqbufs is called with\n> +\t * count > 0, the * exclusive lock will be assigned to that\n\ns/the \\*/the/\n\n> +\t * V4L2CameraFile, in acquiredCf_. At this point, no other fd can call\n\ns/fd/file/\n\n> +\t * any ioctl (except for querycap, try_fmt, g/s_priority, and\n> +\t * enum/g/s_input), and they will return -EBUSY. After reqbufs is\n> +\t * called with count = 0, the exclusive lock will be released.\n> +\t */\n\nI think it would be better to talk about owner than lock. That's the\nconcept used in V4L2.\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 owner by any\n\t * file. The first file that calls reqbufs with count > 0 will become\n\t * the owner, and no other file will be allowed to call buffer-related\n\t * ioctls (except querybuf), set the format or start or stop the\n\t * stream until ownership is released with a call to reqbufs with\n\t * count = 0.\n\t */\n\nOne reason is that I believe we'll have to add locking in the future to\nmake the V4L2 compat layer thread-safe, and confusion between the owner\nlock and the thread lock would then be likely.\n\n> +\n> +\tinitialized_ = true;\n> +\n\nShould you increase refcount_ and set initialized_ after calling\nvcam_->open() ?\n\n> +\tint ret = vcam_->open();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tvcam_->getStreamConfig(&streamConfig_);\n>  \tsetFmtFromConfig(streamConfig_);\n>  \tsizeimage_ = calculateSizeImage(streamConfig_);\n>  \n> -\trefcount_++;\n> -\n>  \treturn 0;\n>  }\n>  \n> -void V4L2CameraProxy::dup()\n> +void V4L2CameraProxy::close(V4L2CameraFile *cf)\n>  {\n> -\trefcount_++;\n> -}\n> +\tLOG(V4L2Compat, Debug) << \"Servicing close fd = \" << cf->efd();\n>  \n> -void V4L2CameraProxy::close()\n> -{\n> -\tLOG(V4L2Compat, Debug) << \"Servicing close\";\n> +\tunlock(cf);\n>  \n>  \tif (--refcount_ > 0)\n>  \t\treturn;\n>  \n>  \tvcam_->close();\n> +\n> +\tinitialized_ = false;\n\ninitialized_ seems to duplicated refcount_. How about dropping it, and\nchanging open() as follows ? You can then ignore my comment about\nincreasing refcount_ after vcam_->open(), but you will need to decrease\nit in the error path.\n\n-\trefcount_++;\n-\n-\tif (initialized_)\n-\t\treturn 0;\n+\tif (refcount_++)\n+\t\treturn 0;\n\n>  }\n>  \n>  void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,\n> @@ -221,9 +234,10 @@ int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)\n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)\n> +int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_enum_fmt fd = \" << cf->efd();\n> +\n>  \n>  \tif (!validateBufferType(arg->type) ||\n>  \t    arg->index >= streamConfig_.formats().pixelformats().size())\n> @@ -237,9 +251,10 @@ int V4L2CameraProxy::vidioc_enum_fmt(struct v4l2_fmtdesc *arg)\n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg)\n> +int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_fmt\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_g_fmt fd = \" << cf->efd();\n> +\n>  \n>  \tif (!validateBufferType(arg->type))\n>  \t\treturn -EINVAL;\n> @@ -275,9 +290,13 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg)\n>  \targ->fmt.pix.colorspace   = V4L2_COLORSPACE_SRGB;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)\n> +int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_s_fmt\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_s_fmt fd = \" << cf->efd();\n> +\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n\nShould this be moved after validateBufferType() ?\n\n>  \n>  \tif (!validateBufferType(arg->type))\n>  \t\treturn -EINVAL;\n> @@ -285,9 +304,9 @@ int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)\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> @@ -302,9 +321,9 @@ int V4L2CameraProxy::vidioc_s_fmt(struct v4l2_format *arg)\n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_try_fmt(struct v4l2_format *arg)\n> +int V4L2CameraProxy::vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_try_fmt\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_try_fmt fd = \" << cf->efd();\n>  \n>  \tif (!validateBufferType(arg->type))\n>  \t\treturn -EINVAL;\n> @@ -329,11 +348,13 @@ int V4L2CameraProxy::freeBuffers()\n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n> +int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg)\n>  {\n> -\tint ret;\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_reqbufs fd = \" << cf->efd();\n>  \n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_reqbufs\";\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (!validateBufferType(arg->type) ||\n>  \t    !validateMemoryType(arg->memory))\n> @@ -342,9 +363,21 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n>  \tLOG(V4L2Compat, Debug) << arg->count << \" buffers requested \";\n>  \n>  \targ->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;\n> +\tmemset(arg->reserved, 0, sizeof(arg->reserved));\n>  \n> -\tif (arg->count == 0)\n> +\tif (arg->count == 0) {\n> +\t\tunlock(cf);\n>  \t\treturn freeBuffers();\n> +\t}\n> +\n> +\tif (bufferCount_ > 0) {\n> +\t\tret = freeBuffers();\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(V4L2Compat, Error)\n> +\t\t\t\t<< \"Failed to free libcamera buffers for re-reqbufs\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n\nThe bufferCount_ > 0 part seems unrelated to $subject, should it be\nmoved to a different patch ?\n\n>  \n>  \tSize size(curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height);\n>  \tret = vcam_->configure(&streamConfig_, size,\n> @@ -387,6 +420,7 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n>  \t\tbuf.memory = V4L2_MEMORY_MMAP;\n>  \t\tbuf.m.offset = i * curV4L2Format_.fmt.pix.sizeimage;\n>  \t\tbuf.index = i;\n> +\t\tbuf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;\n\nSame here.\n\n>  \n>  \t\tbuffers_[i] = buf;\n>  \t}\n> @@ -396,9 +430,13 @@ int V4L2CameraProxy::vidioc_reqbufs(struct v4l2_requestbuffers *arg)\n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)\n> +int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querybuf\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_querybuf fd = \" << cf->efd();\n> +\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n\nquerybuf should be callable from any file, not just the owner. Looks\nlike a missing test in v4l2-compliance ? :-)\n\n>  \n>  \tif (!validateBufferType(arg->type) ||\n>  \t    arg->index >= bufferCount_)\n> @@ -411,17 +449,21 @@ int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)\n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)\n> +int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)\n>  {\n>  \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_qbuf, index = \"\n> -\t\t\t       << arg->index;\n> +\t\t\t       << arg->index << \" fd = \" << cf->efd();\n> +\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n\nI wonder if a hasOwnership() function would make sense, to write\n\n\tif (!hasOwnership(cf))\n\t\treturn -EBUSY;\n\nlock()/acquire() would then only be called in reqbufs and s_fmt, and we\ncould call them at the end of the function, to avoid taking ownership in\ncase of failure. The function could actually be called setOwner(), and\ncover both lock() and unlock() (when called with a null owner). Maybe\nI'm too influenced by the implementation of vb2, but keeping a similar\nscheme could help achieving more similar behaviours. What do you think ?\n\n>  \n>  \tif (!validateBufferType(arg->type) ||\n>  \t    !validateMemoryType(arg->memory) ||\n>  \t    arg->index >= bufferCount_)\n>  \t\treturn -EINVAL;\n>  \n> -\tint ret = vcam_->qbuf(arg->index);\n> +\tret = vcam_->qbuf(arg->index);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> @@ -431,15 +473,19 @@ int V4L2CameraProxy::vidioc_qbuf(struct v4l2_buffer *arg)\n>  \treturn ret;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)\n> +int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_dqbuf\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_dqbuf fd = \" << cf->efd();\n> +\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (!validateBufferType(arg->type) ||\n>  \t    !validateMemoryType(arg->memory))\n>  \t\treturn -EINVAL;\n>  \n> -\tif (!nonBlocking_)\n> +\tif (!(cf->nonBlocking()))\n>  \t\tvcam_->bufferSema_.acquire();\n>  \telse if (!vcam_->bufferSema_.tryAcquire())\n>  \t\treturn -EAGAIN;\n> @@ -455,16 +501,20 @@ int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)\n>  \tcurrentBuf_ = (currentBuf_ + 1) % bufferCount_;\n>  \n>  \tuint64_t data;\n> -\tint ret = ::read(efd_, &data, sizeof(data));\n> +\tret = ::read(efd_, &data, sizeof(data));\n>  \tif (ret != sizeof(data))\n>  \t\tLOG(V4L2Compat, Error) << \"Failed to clear eventfd POLLIN\";\n>  \n>  \treturn 0;\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_streamon(int *arg)\n> +int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *cf, int *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamon\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamon fd = \" << cf->efd();\n> +\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (!validateBufferType(*arg))\n>  \t\treturn -EINVAL;\n> @@ -474,14 +524,18 @@ int V4L2CameraProxy::vidioc_streamon(int *arg)\n>  \treturn vcam_->streamOn();\n>  }\n>  \n> -int V4L2CameraProxy::vidioc_streamoff(int *arg)\n> +int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg)\n>  {\n> -\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamoff\";\n> +\tLOG(V4L2Compat, Debug) << \"Servicing vidioc_streamoff fd = \" << cf->efd();\n> +\n> +\tint ret = lock(cf);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (!validateBufferType(*arg))\n>  \t\treturn -EINVAL;\n>  \n> -\tint ret = vcam_->streamOff();\n> +\tret = vcam_->streamOff();\n>  \n>  \tfor (struct v4l2_buffer &buf : buffers_)\n>  \t\tbuf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);\n> @@ -489,7 +543,7 @@ int V4L2CameraProxy::vidioc_streamoff(int *arg)\n>  \treturn ret;\n>  }\n>  \n> -int V4L2CameraProxy::ioctl(unsigned long request, void *arg)\n> +int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)\n>  {\n>  \tint ret;\n>  \tswitch (request) {\n> @@ -497,34 +551,34 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg)\n>  \t\tret = vidioc_querycap(static_cast<struct v4l2_capability *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_ENUM_FMT:\n> -\t\tret = vidioc_enum_fmt(static_cast<struct v4l2_fmtdesc *>(arg));\n> +\t\tret = vidioc_enum_fmt(cf, static_cast<struct v4l2_fmtdesc *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_G_FMT:\n> -\t\tret = vidioc_g_fmt(static_cast<struct v4l2_format *>(arg));\n> +\t\tret = vidioc_g_fmt(cf, static_cast<struct v4l2_format *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_S_FMT:\n> -\t\tret = vidioc_s_fmt(static_cast<struct v4l2_format *>(arg));\n> +\t\tret = vidioc_s_fmt(cf, static_cast<struct v4l2_format *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_TRY_FMT:\n> -\t\tret = vidioc_try_fmt(static_cast<struct v4l2_format *>(arg));\n> +\t\tret = vidioc_try_fmt(cf, static_cast<struct v4l2_format *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_REQBUFS:\n> -\t\tret = vidioc_reqbufs(static_cast<struct v4l2_requestbuffers *>(arg));\n> +\t\tret = vidioc_reqbufs(cf, static_cast<struct v4l2_requestbuffers *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_QUERYBUF:\n> -\t\tret = vidioc_querybuf(static_cast<struct v4l2_buffer *>(arg));\n> +\t\tret = vidioc_querybuf(cf, static_cast<struct v4l2_buffer *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_QBUF:\n> -\t\tret = vidioc_qbuf(static_cast<struct v4l2_buffer *>(arg));\n> +\t\tret = vidioc_qbuf(cf, static_cast<struct v4l2_buffer *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_DQBUF:\n> -\t\tret = vidioc_dqbuf(static_cast<struct v4l2_buffer *>(arg));\n> +\t\tret = vidioc_dqbuf(cf, static_cast<struct v4l2_buffer *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_STREAMON:\n> -\t\tret = vidioc_streamon(static_cast<int *>(arg));\n> +\t\tret = vidioc_streamon(cf, static_cast<int *>(arg));\n>  \t\tbreak;\n>  \tcase VIDIOC_STREAMOFF:\n> -\t\tret = vidioc_streamoff(static_cast<int *>(arg));\n> +\t\tret = vidioc_streamoff(cf, static_cast<int *>(arg));\n>  \t\tbreak;\n>  \tdefault:\n>  \t\tret = -ENOTTY;\n> @@ -539,10 +593,37 @@ int V4L2CameraProxy::ioctl(unsigned long request, void *arg)\n>  \treturn ret;\n>  }\n>  \n> -void V4L2CameraProxy::bind(int fd)\n> +/**\n> + * \\brief Acquire an exclusive lock on the V4L2Camera\n> + *\n> + * \\return Zero on success or if already acquired, and negative error on\n> + * failure.\n> + */\n> +int V4L2CameraProxy::lock(V4L2CameraFile *cf)\n\nIf we go for the concept of owner, I would call this acquire() and the\nnext function release(). The comment above should then state \"Acquire\nexclusive ownership of the V4L2Camera\" or something similar.\n\n> +{\n> +\tif (acquiredCf_ == cf)\n> +\t\treturn 0;\n> +\n> +\tif (acquiredCf_)\n> +\t\treturn -EBUSY;\n> +\n> +\tefd_ = cf->efd();\n\nDo we still need to store the fd in efd_ ? It duplicates acquiredCf_,\ncan't we access the fd from acquiredCf_ when needed ?\n\n> +\tvcam_->bind(efd_);\n> +\n> +\tacquiredCf_ = cf;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void V4L2CameraProxy::unlock(V4L2CameraFile *cf)\n>  {\n> -\tefd_ = fd;\n> -\tvcam_->bind(fd);\n> +\tif (acquiredCf_ != cf)\n> +\t\treturn;\n> +\n> +\tefd_ = -1;\n> +\tvcam_->unbind();\n> +\n> +\tacquiredCf_ = 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 27d3e50..43290ca 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -21,20 +21,19 @@\n>  \n>  using namespace libcamera;\n>  \n> +class V4L2CameraFile;\n> +\n>  class V4L2CameraProxy\n>  {\n>  public:\n>  \tV4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera);\n>  \n> -\tint open(bool nonBlocking);\n> -\tvoid dup();\n> -\tvoid close();\n> +\tint open(V4L2CameraFile *cfile);\n> +\tvoid close(V4L2CameraFile *cf);\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> -\tint ioctl(unsigned long request, void *arg);\n> -\n> -\tvoid bind(int fd);\n> +\tint ioctl(V4L2CameraFile *cf, unsigned long request, void *arg);\n>  \n>  private:\n>  \tbool validateBufferType(uint32_t type);\n> @@ -47,16 +46,19 @@ private:\n>  \tint freeBuffers();\n>  \n>  \tint vidioc_querycap(struct v4l2_capability *arg);\n> -\tint vidioc_enum_fmt(struct v4l2_fmtdesc *arg);\n> -\tint vidioc_g_fmt(struct v4l2_format *arg);\n> -\tint vidioc_s_fmt(struct v4l2_format *arg);\n> -\tint vidioc_try_fmt(struct v4l2_format *arg);\n> -\tint vidioc_reqbufs(struct v4l2_requestbuffers *arg);\n> -\tint vidioc_querybuf(struct v4l2_buffer *arg);\n> -\tint vidioc_qbuf(struct v4l2_buffer *arg);\n> -\tint vidioc_dqbuf(struct v4l2_buffer *arg);\n> -\tint vidioc_streamon(int *arg);\n> -\tint vidioc_streamoff(int *arg);\n> +\tint vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *arg);\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_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> +\tint vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg);\n> +\tint vidioc_streamon(V4L2CameraFile *cf, int *arg);\n> +\tint vidioc_streamoff(V4L2CameraFile *cf, int *arg);\n> +\n> +\tint lock(V4L2CameraFile *cf);\n> +\tvoid unlock(V4L2CameraFile *cf);\n>  \n>  \tstatic unsigned int bplMultiplier(uint32_t format);\n>  \tstatic unsigned int imageSize(uint32_t format, unsigned int width,\n> @@ -67,7 +69,6 @@ private:\n>  \n>  \tunsigned int refcount_;\n>  \tunsigned int index_;\n> -\tbool nonBlocking_;\n>  \n>  \tstruct v4l2_format curV4L2Format_;\n>  \tStreamConfiguration streamConfig_;\n> @@ -82,6 +83,18 @@ private:\n>  \tstd::unique_ptr<V4L2Camera> vcam_;\n>  \n>  \tint efd_;\n> +\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> +\t * will claim this lock, and reqbufs with count = 0 will release it.\n> +\t * Any ioctl is called while the lock is taken will return -EBUSY\n> +\t * (if applicable, eg. try_fmt, querycap, g/s_priority, and\n> +\t * enum/g/s_input are exempt).\n\nThis could would also need to be updated if we use the concept of\nownership.\n\n> +\t */\n> +\tV4L2CameraFile *acquiredCf_;\n> +\n> +\tbool initialized_;\n>  };\n>  \n>  #endif /* __V4L2_CAMERA_PROXY_H__ */\n> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp\n> index 8da3316..9cede76 100644\n> --- a/src/v4l2/v4l2_compat_manager.cpp\n> +++ b/src/v4l2/v4l2_compat_manager.cpp\n> @@ -24,6 +24,8 @@\n>  \n>  #include \"libcamera/internal/log.h\"\n>  \n> +#include \"v4l2_camera_file.h\"\n> +\n>  using namespace libcamera;\n>  \n>  LOG_DEFINE_CATEGORY(V4L2Compat)\n> @@ -49,7 +51,7 @@ V4L2CompatManager::V4L2CompatManager()\n>  \n>  V4L2CompatManager::~V4L2CompatManager()\n>  {\n> -\tdevices_.clear();\n> +\tcameraFiles_.clear();\n\nHow about naming this just files_, and the various cameraFile variables\nfile ? All the files are instances of V4L2CameraFile, I don't think a\ncamera prefix clears up any possible confusion.\n\n>  \tmmaps_.clear();\n>  \n>  \tif (cm_) {\n> @@ -95,13 +97,13 @@ V4L2CompatManager *V4L2CompatManager::instance()\n>  \treturn &instance;\n>  }\n>  \n> -V4L2CameraProxy *V4L2CompatManager::getProxy(int fd)\n> +std::shared_ptr<V4L2CameraFile> V4L2CompatManager::getCameraFile(int fd)\n>  {\n> -\tauto device = devices_.find(fd);\n> -\tif (device == devices_.end())\n> -\t\treturn nullptr;\n> +\tauto cameraFile = cameraFiles_.find(fd);\n> +\tif (cameraFile == cameraFiles_.end())\n> +\t\treturn std::shared_ptr<V4L2CameraFile>();\n>  \n> -\treturn device->second;\n> +\treturn cameraFile->second;\n>  }\n>  \n>  int V4L2CompatManager::getCameraIndex(int fd)\n> @@ -148,25 +150,17 @@ int V4L2CompatManager::openat(int dirfd, const char *path, int oflag, mode_t mod\n>  \n>  \tfops_.close(fd);\n>  \n> -\tunsigned int camera_index = static_cast<unsigned int>(ret);\n> -\n> -\tV4L2CameraProxy *proxy = proxies_[camera_index].get();\n> -\tret = proxy->open(oflag & O_NONBLOCK);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n>  \tint efd = eventfd(0, EFD_SEMAPHORE |\n>  \t\t\t     ((oflag & O_CLOEXEC) ? EFD_CLOEXEC : 0) |\n>  \t\t\t     ((oflag & O_NONBLOCK) ? EFD_NONBLOCK : 0));\n> -\tif (efd < 0) {\n> -\t\tint err = errno;\n> -\t\tproxy->close();\n> -\t\terrno = err;\n> +\tif (efd < 0)\n>  \t\treturn efd;\n> -\t}\n>  \n> -\tproxy->bind(efd);\n> -\tdevices_.emplace(efd, proxy);\n> +\tunsigned int camera_index = static_cast<unsigned int>(ret);\n> +\n> +\tV4L2CameraProxy *proxy = proxies_[camera_index].get();\n\nI think you can skip the local camera_index variable and use ret\ndirectly. If you prefer keeping it, I'd name it index, and remove the\nblank line between the two.\n\n> +\n\nAnd this blank line too :-)\n\n> +\tcameraFiles_.emplace(efd, std::make_shared<V4L2CameraFile>(efd, oflag & O_NONBLOCK, proxy));\n\nI wonder if V4L2CameraFile could later need to support more flags, in\nwhich case passing it oflag may be useful already. the nonBlocking()\nmethod would return flags_ & O_NONBLOCK. It would also shorten this line\n(OK, it's cheating ;-)).\n\n>  \n>  \treturn efd;\n>  }\n> @@ -177,40 +171,39 @@ int V4L2CompatManager::dup(int oldfd)\n>  \tif (newfd < 0)\n>  \t\treturn newfd;\n>  \n> -\tauto device = devices_.find(oldfd);\n> -\tif (device != devices_.end()) {\n> -\t\tV4L2CameraProxy *proxy = device->second;\n> -\t\tdevices_[newfd] = proxy;\n> -\t\tproxy->dup();\n> -\t}\n> +\tauto cameraFile = cameraFiles_.find(oldfd);\n> +\tif (cameraFile != cameraFiles_.end())\n> +\t\tcameraFiles_[newfd] = cameraFile->second;\n>  \n>  \treturn newfd;\n>  }\n>  \n>  int V4L2CompatManager::close(int fd)\n>  {\n> -\tV4L2CameraProxy *proxy = getProxy(fd);\n> -\tif (proxy) {\n> -\t\tproxy->close();\n> -\t\tdevices_.erase(fd);\n> -\t\treturn 0;\n> -\t}\n> +\tstd::shared_ptr<V4L2CameraFile> cameraFile = getCameraFile(fd);\n> +\tif (cameraFile)\n> +\t\tcameraFiles_.erase(fd);\n\n\tauto file = files_.find(fd);\n\tif (file != files_.end())\n\t\tfiles_.erase(file);\n\nThat will avoid a double lookup.\n\n>  \n> +\t/* We still need to close the eventfd. */\n>  \treturn fops_.close(fd);\n>  }\n>  \n>  void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,\n>  \t\t\t      int fd, off64_t offset)\n>  {\n> -\tV4L2CameraProxy *proxy = getProxy(fd);\n> -\tif (!proxy)\n> +\tstd::shared_ptr<V4L2CameraFile> cameraFile = getCameraFile(fd);\n> +\tif (!cameraFile)\n>  \t\treturn fops_.mmap(addr, length, prot, flags, fd, offset);\n>  \n> -\tvoid *map = proxy->mmap(addr, length, prot, flags, offset);\n> +\tvoid *map = cameraFile->proxy()->mmap(addr, length, prot, flags, offset);\n>  \tif (map == MAP_FAILED)\n>  \t\treturn map;\n>  \n> -\tmmaps_[map] = proxy;\n> +\t/*\n> +\t * Map to V4L2CameraProxy directly to prevent adding more references\n> +\t * to V4L2CameraFile.\n> +\t */\n> +\tmmaps_[map] = cameraFile->proxy();\n>  \treturn map;\n>  }\n>  \n> @@ -233,9 +226,9 @@ int V4L2CompatManager::munmap(void *addr, size_t length)\n>  \n>  int V4L2CompatManager::ioctl(int fd, unsigned long request, void *arg)\n>  {\n> -\tV4L2CameraProxy *proxy = getProxy(fd);\n> -\tif (!proxy)\n> +\tstd::shared_ptr<V4L2CameraFile> cameraFile = getCameraFile(fd);\n> +\tif (!cameraFile)\n>  \t\treturn fops_.ioctl(fd, request, arg);\n>  \n> -\treturn proxy->ioctl(request, arg);\n> +\treturn cameraFile->proxy()->ioctl(cameraFile.get(), request, arg);\n>  }\n> diff --git a/src/v4l2/v4l2_compat_manager.h b/src/v4l2/v4l2_compat_manager.h\n> index 3d4e512..b92f147 100644\n> --- a/src/v4l2/v4l2_compat_manager.h\n> +++ b/src/v4l2/v4l2_compat_manager.h\n> @@ -44,7 +44,7 @@ public:\n>  \n>  \tstatic V4L2CompatManager *instance();\n>  \n> -\tV4L2CameraProxy *getProxy(int fd);\n> +\tstd::shared_ptr<V4L2CameraFile> getCameraFile(int fd);\n\nWhile at it, I would rename this function to cameraFile() as we don't\nprefix getters with get. It could even become just file().\n\n>  \tconst FileOperations &fops() const { return fops_; }\n>  \n>  \tint openat(int dirfd, const char *path, int oflag, mode_t mode);\n> @@ -68,7 +68,7 @@ private:\n>  \tCameraManager *cm_;\n>  \n>  \tstd::vector<std::unique_ptr<V4L2CameraProxy>> proxies_;\n> -\tstd::map<int, V4L2CameraProxy *> devices_;\n> +\tstd::map<int, std::shared_ptr<V4L2CameraFile>> cameraFiles_;\n>  \tstd::map<void *, V4L2CameraProxy *> mmaps_;\n>  };\n>","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 6521360710\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 20 Jun 2020 03:02:49 +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 B55B8552;\n\tSat, 20 Jun 2020 03:02:48 +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=\"lHDfwsxK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592614969;\n\tbh=sylwI02XyVnaYJaxkgnip7bGr9luWc3LgMnUZBo5dC4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lHDfwsxKcclldXwbjmsA7evwR4+Wr9g8M6bm7FFRgAF9z9G/FsliFkagqKlXPHy0t\n\tLw+PTE9PH/acEZuSYLIYLfBbRNGensQPsUNx/LY3LnOOfxRU7hL1UxcyhrFLdBXAxo\n\tgV0i8qmvkicFD33gIrCqPA5PmLNk90DqQvvsqUn0=","Date":"Sat, 20 Jun 2020 04:02:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200620010224.GP5823@pendragon.ideasonboard.com>","References":"<20200619054123.19052-1-paul.elder@ideasonboard.com>\n\t<20200619054123.19052-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200619054123.19052-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/17] 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":"Sat, 20 Jun 2020 01:02:49 -0000"}}]