[{"id":3578,"web_url":"https://patchwork.libcamera.org/comment/3578/","msgid":"<20200122163614.GX1124294@oden.dyn.berto.se>","date":"2020-01-22T16:36:14","subject":"Re: [libcamera-devel] [PATCH 16/19] libcamera: camera: Implement\n\tthe threading model","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2020-01-20 02:24:34 +0200, Laurent Pinchart wrote:\n> Document the threading model of the Camera class and implement it.\n> Selected functions become thread-safe, and require a few functions of\n> the PipelineHandler class to be called through cross-thread invocation\n> as the pipeline handlers live in the camera manager thread, while the\n> Camera class is mostly accessed from the application thread. The\n> PipelineHandler is made to inherit from the Object class to support\n> this.\n> \n> Disconnection is currently left out as it is not implemented in pipeline\n> handlers, and isn't fully supported in the Camera class either. This\n> will be revisited when implementing proper hotplug support.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  src/libcamera/camera.cpp                 | 101 +++++++++++++++++------\n>  src/libcamera/include/pipeline_handler.h |   4 +-\n>  2 files changed, 78 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 83c2249b8897..f281a4cd346b 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -8,6 +8,7 @@\n>  #include <libcamera/camera.h>\n>  \n>  #include <array>\n> +#include <atomic>\n>  #include <iomanip>\n>  \n>  #include <libcamera/framebuffer_allocator.h>\n> @@ -282,7 +283,7 @@ public:\n>  \n>  private:\n>  \tbool disconnected_;\n> -\tState state_;\n> +\tstd::atomic<State> state_;\n>  };\n>  \n>  Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n> @@ -294,7 +295,7 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n>  \n>  Camera::Private::~Private()\n>  {\n> -\tif (state_ != Private::CameraAvailable)\n> +\tif (state_.load(std::memory_order_acquire) != Private::CameraAvailable)\n>  \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n>  }\n>  \n> @@ -310,12 +311,13 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const\n>  \tif (!allowDisconnected && disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (state_ == state)\n> +\tState currentState = state_.load(std::memory_order_acquire);\n> +\tif (currentState == state)\n>  \t\treturn 0;\n>  \n>  \tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n>  \n> -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> +\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[currentState]\n>  \t\t\t   << \" state trying operation requiring state \"\n>  \t\t\t   << camera_state_names[state];\n>  \n> @@ -328,13 +330,14 @@ int Camera::Private::isAccessAllowed(State low, State high,\n>  \tif (!allowDisconnected && disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (state_ >= low && state_ <= high)\n> +\tState currentState = state_.load(std::memory_order_acquire);\n> +\tif (currentState >= low && currentState <= high)\n>  \t\treturn 0;\n>  \n>  \tASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&\n>  \t       static_cast<unsigned int>(high) < camera_state_names.size());\n>  \n> -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> +\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[currentState]\n>  \t\t\t   << \" state trying operation requiring state between \"\n>  \t\t\t   << camera_state_names[low] << \" and \"\n>  \t\t\t   << camera_state_names[high];\n> @@ -349,15 +352,15 @@ void Camera::Private::disconnect()\n>  \t * state to Configured state to allow applications to free resources\n>  \t * and call release() before deleting the camera.\n>  \t */\n> -\tif (state_ == Private::CameraRunning)\n> -\t\tstate_ = Private::CameraConfigured;\n> +\tif (state_.load(std::memory_order_acquire) == Private::CameraRunning)\n> +\t\tstate_.store(Private::CameraConfigured, std::memory_order_release);\n>  \n>  \tdisconnected_ = true;\n>  }\n>  \n>  void Camera::Private::setState(State state)\n>  {\n> -\tstate_ = state;\n> +\tstate_.store(state, std::memory_order_release);\n>  }\n>  \n>  /**\n> @@ -389,12 +392,17 @@ void Camera::Private::setState(State state)\n>   * An application may start and stop a camera multiple times as long as it is\n>   * not released. The camera may also be reconfigured.\n>   *\n> + * Functions that affect the camera state as defined below are generally not\n> + * synchronized with each other by the Camera class. The caller is responsible\n> + * for ensuring their synchronization if necessary.\n> + *\n>   * \\subsection Camera States\n>   *\n>   * To help manage the sequence of operations needed to control the camera a set\n>   * of states are defined. Each state describes which operations may be performed\n> - * on the camera. Operations not listed in the state diagram are allowed in all\n> - * states.\n> + * on the camera. Performing an operation not allowed in the camera state\n> + * results in undefined behaviour. Operations not listed at all in the state\n> + * diagram are allowed in all states.\n>   *\n>   * \\dot\n>   * digraph camera_state_machine {\n> @@ -467,6 +475,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n>  \n>  /**\n>   * \\brief Retrieve the name of the camera\n> + * \\context This function is \\threadsafe.\n>   * \\return Name of the camera device\n>   */\n>  const std::string &Camera::name() const\n> @@ -540,7 +549,9 @@ int Camera::exportFrameBuffers(Stream *stream,\n>  \tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end())\n>  \t\treturn -EINVAL;\n>  \n> -\treturn p_->pipe_->exportFrameBuffers(this, stream, buffers);\n> +\treturn p_->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> +\t\t\t\t       ConnectionTypeBlocking, this, stream,\n> +\t\t\t\t       buffers);\n>  }\n>  \n>  int Camera::freeFrameBuffers(Stream *stream)\n> @@ -549,7 +560,8 @@ int Camera::freeFrameBuffers(Stream *stream)\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> -\tp_->pipe_->freeFrameBuffers(this, stream);\n> +\tp_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,\n> +\t\t\t\tConnectionTypeBlocking, this, stream);\n>  \n>  \treturn 0;\n>  }\n> @@ -571,7 +583,8 @@ int Camera::freeFrameBuffers(Stream *stream)\n>   * Once exclusive access isn't needed anymore, the device should be released\n>   * with a call to the release() function.\n>   *\n> - * This function affects the state of the camera, see \\ref camera_operation.\n> + * \\context This function is \\threadsafe. It may only be called when the camera\n> + * is in the Available state as defined in \\ref camera_operation.\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -ENODEV The camera has been disconnected from the system\n> @@ -579,6 +592,10 @@ int Camera::freeFrameBuffers(Stream *stream)\n>   */\n>  int Camera::acquire()\n>  {\n> +\t/*\n> +\t * No manual locking is required as PipelineHandler::lock() is\n> +\t * thread-safe.\n> +\t */\n>  \tint ret = p_->isAccessAllowed(Private::CameraAvailable);\n>  \tif (ret < 0)\n>  \t\treturn ret == -EACCES ? -EBUSY : ret;\n> @@ -600,7 +617,10 @@ int Camera::acquire()\n>   * Releasing the camera device allows other users to acquire exclusive access\n>   * with the acquire() function.\n>   *\n> - * This function affects the state of the camera, see \\ref camera_operation.\n> + * \\context This function may only be called when the camera is in the\n> + * Available or Configured state as defined in \\ref camera_operation, and shall\n> + * be synchronized by the caller with other functions that affect the camera\n> + * state.\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -EBUSY The camera is running and can't be released\n> @@ -634,6 +654,8 @@ int Camera::release()\n>   *\n>   * Camera controls remain constant through the lifetime of the camera.\n>   *\n> + * \\context This function is \\threadsafe.\n> + *\n>   * \\return A ControlInfoMap listing the controls supported by the camera\n>   */\n>  const ControlInfoMap &Camera::controls()\n> @@ -648,6 +670,8 @@ const ControlInfoMap &Camera::controls()\n>   * information describes among other things how many streams the camera\n>   * supports and the capabilities of each stream.\n>   *\n> + * \\context This function is \\threadsafe.\n> + *\n>   * \\return An array of all the camera's streams.\n>   */\n>  const std::set<Stream *> &Camera::streams() const\n> @@ -665,6 +689,8 @@ const std::set<Stream *> &Camera::streams() const\n>   * empty list of roles is valid, and will generate an empty configuration that\n>   * can be filled by the caller.\n>   *\n> + * \\context This function is \\threadsafe.\n> + *\n>   * \\return A CameraConfiguration if the requested roles can be satisfied, or a\n>   * null pointer otherwise. The ownership of the returned configuration is\n>   * passed to the caller.\n> @@ -715,7 +741,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR\n>   * Exclusive access to the camera shall be ensured by a call to acquire() prior\n>   * to calling this function, otherwise an -EACCES error will be returned.\n>   *\n> - * This function affects the state of the camera, see \\ref camera_operation.\n> + * \\context This function may only be called when the camera is in the Acquired\n> + * or Configured state as defined in \\ref camera_operation, and shall be\n> + * synchronized by the caller with other functions that affect the camera\n> + * state.\n>   *\n>   * Upon return the StreamConfiguration entries in \\a config are associated with\n>   * Stream instances which can be retrieved with StreamConfiguration::stream().\n> @@ -754,7 +783,8 @@ int Camera::configure(CameraConfiguration *config)\n>  \n>  \tLOG(Camera, Info) << msg.str();\n>  \n> -\tret = p_->pipe_->configure(this, config);\n> +\tret = p_->pipe_->invokeMethod(&PipelineHandler::configure,\n> +\t\t\t\t      ConnectionTypeBlocking, this, config);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> @@ -789,8 +819,8 @@ int Camera::configure(CameraConfiguration *config)\n>   * The ownership of the returned request is passed to the caller, which is\n>   * responsible for either queueing the request or deleting it.\n>   *\n> - * This function shall only be called when the camera is in the Configured\n> - * or Running state, see \\ref camera_operation.\n> + * \\context This function is \\threadsafe. It may only be called when the camera\n> + * is in the Configured or Running state as defined in \\ref camera_operation.\n>   *\n>   * \\return A pointer to the newly created request, or nullptr on error\n>   */\n> @@ -820,6 +850,9 @@ Request *Camera::createRequest(uint64_t cookie)\n>   * Ownership of the request is transferred to the camera. It will be deleted\n>   * automatically after it completes.\n>   *\n> + * \\context This function is \\threadsafe. It may only be called when the camera\n> + * is in the Running state as defined in \\ref camera_operation.\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -ENODEV The camera has been disconnected from the system\n>   * \\retval -EACCES The camera is not running so requests can't be queued\n> @@ -832,6 +865,12 @@ int Camera::queueRequest(Request *request)\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> +\t/*\n> +\t * The camera state may chance until the end of the function. No locking\n> +\t * is however needed as PipelineHandler::queueRequest() will handle\n> +\t * this.\n> +\t */\n> +\n>  \tif (request->buffers().empty()) {\n>  \t\tLOG(Camera, Error) << \"Request contains no buffers\";\n>  \t\treturn -EINVAL;\n> @@ -846,7 +885,8 @@ int Camera::queueRequest(Request *request)\n>  \t\t}\n>  \t}\n>  \n> -\treturn p_->pipe_->queueRequest(this, request);\n> +\treturn p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> +\t\t\t\t       ConnectionTypeQueued, this, request);\n>  }\n>  \n>  /**\n> @@ -856,7 +896,10 @@ int Camera::queueRequest(Request *request)\n>   * can queue requests to the camera to process and return to the application\n>   * until the capture session is terminated with \\a stop().\n>   *\n> - * This function affects the state of the camera, see \\ref camera_operation.\n> + * \\context This function may only be called when the camera is in the\n> + * Configured state as defined in \\ref camera_operation, and shall be\n> + * synchronized by the caller with other functions that affect the camera\n> + * state.\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -ENODEV The camera has been disconnected from the system\n> @@ -874,10 +917,12 @@ int Camera::start()\n>  \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n>  \t\t\tcontinue;\n>  \n> -\t\tp_->pipe_->importFrameBuffers(this, stream);\n> +\t\tp_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers,\n> +\t\t\t\t\tConnectionTypeDirect, this, stream);\n>  \t}\n>  \n> -\tret = p_->pipe_->start(this);\n> +\tret = p_->pipe_->invokeMethod(&PipelineHandler::start,\n> +\t\t\t\t      ConnectionTypeBlocking, this);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> @@ -892,7 +937,9 @@ int Camera::start()\n>   * This method stops capturing and processing requests immediately. All pending\n>   * requests are cancelled and complete synchronously in an error state.\n>   *\n> - * This function affects the state of the camera, see \\ref camera_operation.\n> + * \\context This function may only be called when the camera is in the Running\n> + * state as defined in \\ref camera_operation, and shall be synchronized by the\n> + * caller with other functions that affect the camera state.\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -ENODEV The camera has been disconnected from the system\n> @@ -908,13 +955,15 @@ int Camera::stop()\n>  \n>  \tp_->setState(Private::CameraConfigured);\n>  \n> -\tp_->pipe_->stop(this);\n> +\tp_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> +\t\t\t\tthis);\n>  \n>  \tfor (Stream *stream : p_->activeStreams_) {\n>  \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n>  \t\t\tcontinue;\n>  \n> -\t\tp_->pipe_->freeFrameBuffers(this, stream);\n> +\t\tp_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,\n> +\t\t\t\t\tConnectionTypeBlocking, this, stream);\n>  \t}\n>  \n>  \treturn 0;\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index a6c1e1fbae38..db0ec6ac00d7 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -17,6 +17,7 @@\n>  \n>  #include <ipa/ipa_interface.h>\n>  #include <libcamera/controls.h>\n> +#include <libcamera/object.h>\n>  #include <libcamera/stream.h>\n>  \n>  namespace libcamera {\n> @@ -51,7 +52,8 @@ private:\n>  \tCameraData &operator=(const CameraData &) = delete;\n>  };\n>  \n> -class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>\n> +class PipelineHandler : public std::enable_shared_from_this<PipelineHandler>,\n> +\t\t\tpublic Object\n>  {\n>  public:\n>  \tPipelineHandler(CameraManager *manager);\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1358660804\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 17:36:16 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id j1so7566384lja.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 08:36:16 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tj19sm24619952lfb.90.2020.01.22.08.36.14\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 22 Jan 2020 08:36:14 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=49ekluvtSmuQF1yOeQLhu7tx5VxDGzXm66oS23BXQ4Y=;\n\tb=N0rGK+WggyOmq10u+zq/SXoe3notB0nl1j2ovYJNgJ7u2qPgDZy3COWqNbZ96nCWJ9\n\t0h0qgbepiR8gq9KIWwbSBRfw5V5/RT1rM75OMucLrbfsbFgTeqD9R+VOa1M0MR8u2BVs\n\t5xAqvKIBtcqn91ScujuwaMU2VG8DQ2zrwcUd1aA4oxQY1O6Nt53/BPZOCqjGWHtL+06O\n\t/c+IapN0QW1yeWe/pBVhFnfAgZu5jL4txN1pqFgwtDxeztNIUpsVlLV2eHl5nbDvBnLB\n\tPs4zHMJOCf9ME8TBrR92SXkHjJJffWoJVMOyAjUgPHSmjEop83Zvx+TUocFhRr6zmJiT\n\tETqA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=49ekluvtSmuQF1yOeQLhu7tx5VxDGzXm66oS23BXQ4Y=;\n\tb=NWJMRlJGoOa3R74lRt+JoTUZP0GBCdeQE5Ocb2WiiZ+33zRkavjaGLkHE8izPVxoBQ\n\tL04WeXEVmL8YO8mHuce6S5lOMIRHy6lwbihYvo4cj8pGAvmZe7DXPj6zGsI5h7ldPtPH\n\tI+i/5joMXvZ/qvrzvbyh9PtX+sRRRSzarf00QjJnEgZgX0OilDPsi3n5Mwh0aEO7z1hP\n\tUGQ86D4e0kVRueM273SFoH2XE2yXJb+yONrxR0xZ89ALuw64JaONNZvTM9gRnJGhHZ9Z\n\tS3mw+29AePlaNhoLoCMeuShrtnCtTKKZlWUBHk1BD3ebwhXZrIv9Tay9KfTGFi/BKXuM\n\t9k9A==","X-Gm-Message-State":"APjAAAWhZsuSfO8rig1mmYY8OxwGhnhuk/cR8H4G6XmvIaa/5RTwgcVq\n\tktKFw3TMulqkNleQb6Zjta3+mQ==","X-Google-Smtp-Source":"APXvYqybU+nD9FLu/OHsSv4Uya/iKvGtib84DMG6PWtdvmBS33/4Vo8hWCDFISLqctRTJDqYxVsSfQ==","X-Received":"by 2002:a2e:556:: with SMTP id 83mr19876392ljf.127.1579710975197;\n\tWed, 22 Jan 2020 08:36:15 -0800 (PST)","Date":"Wed, 22 Jan 2020 17:36:14 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200122163614.GX1124294@oden.dyn.berto.se>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-17-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200120002437.6633-17-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 16/19] libcamera: camera: Implement\n\tthe threading model","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, 22 Jan 2020 16:36:16 -0000"}}]