[{"id":3577,"web_url":"https://patchwork.libcamera.org/comment/3577/","msgid":"<20200122163128.GW1124294@oden.dyn.berto.se>","date":"2020-01-22T16:31:28","subject":"Re: [libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize\n\tstate checks in Private class","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:33 +0200, Laurent Pinchart wrote:\n> Move all accesses to the state_ and disconnected_ members to functions\n> of the Private class. This will make it easier to implement\n> synchronization, and simplifies the Camera class implementation.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nI really like this patch.\n\n> ---\n>  src/libcamera/camera.cpp           | 163 ++++++++++++++++-------------\n>  src/libcamera/pipeline_handler.cpp |   5 +-\n>  2 files changed, 95 insertions(+), 73 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 6fe802f375a3..83c2249b8897 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -266,15 +266,21 @@ public:\n>  \n>  \tPrivate(PipelineHandler *pipe, const std::string &name,\n>  \t\tconst std::set<Stream *> &streams);\n> +\t~Private();\n>  \n> -\tbool stateBetween(State low, State high) const;\n> -\tbool stateIs(State state) const;\n> +\tint isAccessAllowed(State state, bool allowDisconnected = false) const;\n> +\tint isAccessAllowed(State low, State high,\n> +\t\t\t    bool allowDisconnected = false) const;\n> +\n> +\tvoid disconnect();\n> +\tvoid setState(State state);\n>  \n>  \tstd::shared_ptr<PipelineHandler> pipe_;\n>  \tstd::string name_;\n>  \tstd::set<Stream *> streams_;\n>  \tstd::set<Stream *> activeStreams_;\n>  \n> +private:\n>  \tbool disconnected_;\n>  \tState state_;\n>  };\n> @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n>  {\n>  }\n>  \n> +Camera::Private::~Private()\n> +{\n> +\tif (state_ != Private::CameraAvailable)\n> +\t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> +}\n> +\n>  static constexpr std::array<const char *, 4> camera_state_names = {\n>  \t\"Available\",\n>  \t\"Acquired\",\n> @@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {\n>  \t\"Running\",\n>  };\n>  \n> -bool Camera::Private::stateBetween(State low, State high) const\n\nI wonder if we should document the \\retval codes here and then use \n\\copydetails from callers in Camera to make sure -ENODEV and -EACCESS is \nuniformly documented.\n\nOr maybe that is not possible as Doxygen ignores libcamera::*::Private.\n\nWith or without this cherry on top,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const\n>  {\n> +\tif (!allowDisconnected && disconnected_)\n> +\t\treturn -ENODEV;\n> +\n> +\tif (state_ == 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> +\t\t\t   << \" state trying operation requiring state \"\n> +\t\t\t   << camera_state_names[state];\n> +\n> +\treturn -EACCES;\n> +}\n> +\n> +int Camera::Private::isAccessAllowed(State low, State high,\n> +\t\t\t\t     bool allowDisconnected) const\n> +{\n> +\tif (!allowDisconnected && disconnected_)\n> +\t\treturn -ENODEV;\n> +\n>  \tif (state_ >= low && state_ <= high)\n> -\t\treturn true;\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> @@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const\n>  \t\t\t   << camera_state_names[low] << \" and \"\n>  \t\t\t   << camera_state_names[high];\n>  \n> -\treturn false;\n> +\treturn -EACCES;\n>  }\n>  \n> -bool Camera::Private::stateIs(State state) const\n> +void Camera::Private::disconnect()\n>  {\n> -\tif (state_ == state)\n> -\t\treturn true;\n> -\n> -\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n> +\t/*\n> +\t * If the camera was running when the hardware was removed force the\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>  \n> -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> -\t\t\t   << \" state trying operation requiring state \"\n> -\t\t\t   << camera_state_names[state];\n> +\tdisconnected_ = true;\n> +}\n>  \n> -\treturn false;\n> +void Camera::Private::setState(State state)\n> +{\n> +\tstate_ = state;\n>  }\n>  \n>  /**\n> @@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,\n>  \n>  Camera::~Camera()\n>  {\n> -\tif (!p_->stateIs(Private::CameraAvailable))\n> -\t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n>  }\n>  \n>  /**\n> @@ -488,23 +523,16 @@ void Camera::disconnect()\n>  {\n>  \tLOG(Camera, Debug) << \"Disconnecting camera \" << name();\n>  \n> -\t/*\n> -\t * If the camera was running when the hardware was removed force the\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 (p_->state_ == Private::CameraRunning)\n> -\t\tp_->state_ = Private::CameraConfigured;\n> -\n> -\tp_->disconnected_ = true;\n> +\tp_->disconnect();\n>  \tdisconnected.emit(this);\n>  }\n>  \n>  int Camera::exportFrameBuffers(Stream *stream,\n>  \t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n>  {\n> -\tif (!p_->stateIs(Private::CameraConfigured))\n> -\t\treturn -EACCES;\n> +\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (streams().find(stream) == streams().end())\n>  \t\treturn -EINVAL;\n> @@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,\n>  \n>  int Camera::freeFrameBuffers(Stream *stream)\n>  {\n> -\tif (!p_->stateIs(Private::CameraConfigured))\n> -\t\treturn -EACCES;\n> +\tint ret = p_->isAccessAllowed(Private::CameraConfigured, true);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tp_->pipe_->freeFrameBuffers(this, stream);\n>  \n> @@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)\n>   */\n>  int Camera::acquire()\n>  {\n> -\tif (p_->disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!p_->stateIs(Private::CameraAvailable))\n> -\t\treturn -EBUSY;\n> +\tint ret = p_->isAccessAllowed(Private::CameraAvailable);\n> +\tif (ret < 0)\n> +\t\treturn ret == -EACCES ? -EBUSY : ret;\n>  \n>  \tif (!p_->pipe_->lock()) {\n>  \t\tLOG(Camera, Info)\n> @@ -562,7 +589,7 @@ int Camera::acquire()\n>  \t\treturn -EBUSY;\n>  \t}\n>  \n> -\tp_->state_ = Private::CameraAcquired;\n> +\tp_->setState(Private::CameraAcquired);\n>  \n>  \treturn 0;\n>  }\n> @@ -580,9 +607,10 @@ int Camera::acquire()\n>   */\n>  int Camera::release()\n>  {\n> -\tif (!p_->stateBetween(Private::CameraAvailable,\n> -\t\t\t      Private::CameraConfigured))\n> -\t\treturn -EBUSY;\n> +\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\t\t      Private::CameraConfigured, true);\n> +\tif (ret < 0)\n> +\t\treturn ret == -EACCES ? -EBUSY : ret;\n>  \n>  \tif (allocator_) {\n>  \t\t/*\n> @@ -596,7 +624,7 @@ int Camera::release()\n>  \n>  \tp_->pipe_->unlock();\n>  \n> -\tp_->state_ = Private::CameraAvailable;\n> +\tp_->setState(Private::CameraAvailable);\n>  \n>  \treturn 0;\n>  }\n> @@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const\n>   */\n>  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)\n>  {\n> -\tif (p_->disconnected_ || roles.size() > streams().size())\n> +\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\t\t      Private::CameraRunning);\n> +\tif (ret < 0)\n> +\t\treturn nullptr;\n> +\n> +\tif (roles.size() > streams().size())\n>  \t\treturn nullptr;\n>  \n>  \tCameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);\n> @@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR\n>   */\n>  int Camera::configure(CameraConfiguration *config)\n>  {\n> -\tint ret;\n> -\n> -\tif (p_->disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!p_->stateBetween(Private::CameraAcquired,\n> -\t\t\t      Private::CameraConfigured))\n> -\t\treturn -EACCES;\n> +\tint ret = p_->isAccessAllowed(Private::CameraAcquired,\n> +\t\t\t\t      Private::CameraConfigured);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (allocator_ && allocator_->allocated()) {\n>  \t\tLOG(Camera, Error)\n> @@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)\n>  \t\tp_->activeStreams_.insert(stream);\n>  \t}\n>  \n> -\tp_->state_ = Private::CameraConfigured;\n> +\tp_->setState(Private::CameraConfigured);\n>  \n>  \treturn 0;\n>  }\n> @@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)\n>   */\n>  Request *Camera::createRequest(uint64_t cookie)\n>  {\n> -\tif (p_->disconnected_ ||\n> -\t    !p_->stateBetween(Private::CameraConfigured,\n> -\t\t\t      Private::CameraRunning))\n> +\tint ret = p_->isAccessAllowed(Private::CameraConfigured,\n> +\t\t\t\t      Private::CameraRunning);\n> +\tif (ret < 0)\n>  \t\treturn nullptr;\n>  \n>  \treturn new Request(this, cookie);\n> @@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)\n>   */\n>  int Camera::queueRequest(Request *request)\n>  {\n> -\tif (p_->disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!p_->stateIs(Private::CameraRunning))\n> -\t\treturn -EACCES;\n> +\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tif (request->buffers().empty()) {\n>  \t\tLOG(Camera, Error) << \"Request contains no buffers\";\n> @@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)\n>   */\n>  int Camera::start()\n>  {\n> -\tif (p_->disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!p_->stateIs(Private::CameraConfigured))\n> -\t\treturn -EACCES;\n> +\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tLOG(Camera, Debug) << \"Starting capture\";\n>  \n> @@ -852,11 +877,11 @@ int Camera::start()\n>  \t\tp_->pipe_->importFrameBuffers(this, stream);\n>  \t}\n>  \n> -\tint ret = p_->pipe_->start(this);\n> +\tret = p_->pipe_->start(this);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tp_->state_ = Private::CameraRunning;\n> +\tp_->setState(Private::CameraRunning);\n>  \n>  \treturn 0;\n>  }\n> @@ -875,15 +900,13 @@ int Camera::start()\n>   */\n>  int Camera::stop()\n>  {\n> -\tif (p_->disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!p_->stateIs(Private::CameraRunning))\n> -\t\treturn -EACCES;\n> +\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n>  \tLOG(Camera, Debug) << \"Stopping capture\";\n>  \n> -\tp_->state_ = Private::CameraConfigured;\n> +\tp_->setState(Private::CameraConfigured);\n>  \n>  \tp_->pipe_->stop(this);\n>  \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 669097f609ab..3091971d5da0 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n>   * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n>   * any camera configuration.\n>   *\n> - * The only intended caller is the FrameBufferAllocator helper.\n> + * The only intended caller is Camera::exportFrameBuffers().\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> @@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n>   * called only after a successful call to either of these two methods, and only\n>   * once per stream.\n>   *\n> - * The only intended callers are Camera::stop() and the FrameBufferAllocator\n> - * helper.\n> + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().\n>   */\n>  \n>  /**\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 25A5E60804\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 17:31:31 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id a13so7507227ljm.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 08:31:31 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tl21sm21058880lfh.74.2020.01.22.08.31.29\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 22 Jan 2020 08:31:29 -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=d+MAbiXmwLq7Fs8M6qdvfBFRzGviB+7q8N+M40uvoLg=;\n\tb=rrhyWVYFBfCqQgwVjrz05+L2AOk2b6DmIWNI0IM7V+kJgPTHS78aOtfhNswelNvSmA\n\tVjvgOsW2jlqBync8V1iu0Qgccs3Dv7MVgm1H27WbwscrKK1L6q2+4Embv7GtE/uZAG4Q\n\tqiIdXUSXsV9bH0CVZEok61m1FpSTbgELdCzaIn8OpPnjyomuqjymcW5mzSUEfHiPjCcr\n\tMGIgsRE+GHNfUDyifRtgVUUh9QVzg6AB718wJ0BQJzsuJX/oKnNzU7oKoJ8Jpq6uPwmm\n\tiMMmvKRKRvPq+Zzx1Lj4vPyhfHfUnSjdciIZrnbQdXwmwaEIxr3sBVaO+4JMKFJ+sptM\n\tOdFA==","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=d+MAbiXmwLq7Fs8M6qdvfBFRzGviB+7q8N+M40uvoLg=;\n\tb=nyFD1cnv02hVQ1Zpd2373F4Q5gKtuYEr+VdiWKwouyIxe8ojk+Rxzy8fUJWQVQe4Ec\n\tTh0APrYeiZRIIJwBitz9hWfWsD+e+Xq2GaXU5fhoYH+Arvl0Wuac0SDlMCITP3dEorPw\n\tUGOhGu2Ss41Yc8dBEqd/hGEkmKspQ33GW0zJLFrUmObu4AOdF59Wxc+HvRwOdZAuDXKT\n\tiIoY2V9SJFKMMsvZjWgKFOi+iqhnzGl2DHhnRYKHzl2/OfeJb1g1dzEOU44ryIzGpCEk\n\tyk/ZnxPYWocfoBoHLqdwLayh6kXs1jLttE0iGhUF3cvsyfPve29c/8Pe2BjoOEg6lsgZ\n\tJGJg==","X-Gm-Message-State":"APjAAAWuRlRqWwqSKUAK2Dpso5IKdVWXEjV5+HagKcsIs63bGmFHwO37\n\t00mtQ4msZuvz2THKpwMUHdv31A==","X-Google-Smtp-Source":"APXvYqyawjDiRuWeFeluCG19+7n2j6efcJchFSeb7MHNRoDZGjJyKFg4J7G//ZATvCQV2ub8BPtQKQ==","X-Received":"by 2002:a2e:8755:: with SMTP id\n\tq21mr20043199ljj.156.1579710690234; \n\tWed, 22 Jan 2020 08:31:30 -0800 (PST)","Date":"Wed, 22 Jan 2020 17:31:28 +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":"<20200122163128.GW1124294@oden.dyn.berto.se>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-16-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-16-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize\n\tstate checks in Private class","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:31:31 -0000"}},{"id":3584,"web_url":"https://patchwork.libcamera.org/comment/3584/","msgid":"<20200122204929.GG15239@pendragon.ideasonboard.com>","date":"2020-01-22T20:49:29","subject":"Re: [libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize\n\tstate checks in Private class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, Jan 22, 2020 at 05:31:28PM +0100, Niklas Söderlund wrote:\n> On 2020-01-20 02:24:33 +0200, Laurent Pinchart wrote:\n> > Move all accesses to the state_ and disconnected_ members to functions\n> > of the Private class. This will make it easier to implement\n> > synchronization, and simplifies the Camera class implementation.\n> > \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> I really like this patch.\n> \n> > ---\n> >  src/libcamera/camera.cpp           | 163 ++++++++++++++++-------------\n> >  src/libcamera/pipeline_handler.cpp |   5 +-\n> >  2 files changed, 95 insertions(+), 73 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 6fe802f375a3..83c2249b8897 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -266,15 +266,21 @@ public:\n> >  \n> >  \tPrivate(PipelineHandler *pipe, const std::string &name,\n> >  \t\tconst std::set<Stream *> &streams);\n> > +\t~Private();\n> >  \n> > -\tbool stateBetween(State low, State high) const;\n> > -\tbool stateIs(State state) const;\n> > +\tint isAccessAllowed(State state, bool allowDisconnected = false) const;\n> > +\tint isAccessAllowed(State low, State high,\n> > +\t\t\t    bool allowDisconnected = false) const;\n> > +\n> > +\tvoid disconnect();\n> > +\tvoid setState(State state);\n> >  \n> >  \tstd::shared_ptr<PipelineHandler> pipe_;\n> >  \tstd::string name_;\n> >  \tstd::set<Stream *> streams_;\n> >  \tstd::set<Stream *> activeStreams_;\n> >  \n> > +private:\n> >  \tbool disconnected_;\n> >  \tState state_;\n> >  };\n> > @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n> >  {\n> >  }\n> >  \n> > +Camera::Private::~Private()\n> > +{\n> > +\tif (state_ != Private::CameraAvailable)\n> > +\t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > +}\n> > +\n> >  static constexpr std::array<const char *, 4> camera_state_names = {\n> >  \t\"Available\",\n> >  \t\"Acquired\",\n> > @@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {\n> >  \t\"Running\",\n> >  };\n> >  \n> > -bool Camera::Private::stateBetween(State low, State high) const\n> \n> I wonder if we should document the \\retval codes here and then use \n> \\copydetails from callers in Camera to make sure -ENODEV and -EACCESS is \n> uniformly documented.\n> \n> Or maybe that is not possible as Doxygen ignores libcamera::*::Private.\n\nYes, that's the issue :-(\n\n> With or without this cherry on top,\n> \n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> \n> > +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const\n> >  {\n> > +\tif (!allowDisconnected && disconnected_)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tif (state_ == 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> > +\t\t\t   << \" state trying operation requiring state \"\n> > +\t\t\t   << camera_state_names[state];\n> > +\n> > +\treturn -EACCES;\n> > +}\n> > +\n> > +int Camera::Private::isAccessAllowed(State low, State high,\n> > +\t\t\t\t     bool allowDisconnected) const\n> > +{\n> > +\tif (!allowDisconnected && disconnected_)\n> > +\t\treturn -ENODEV;\n> > +\n> >  \tif (state_ >= low && state_ <= high)\n> > -\t\treturn true;\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> > @@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const\n> >  \t\t\t   << camera_state_names[low] << \" and \"\n> >  \t\t\t   << camera_state_names[high];\n> >  \n> > -\treturn false;\n> > +\treturn -EACCES;\n> >  }\n> >  \n> > -bool Camera::Private::stateIs(State state) const\n> > +void Camera::Private::disconnect()\n> >  {\n> > -\tif (state_ == state)\n> > -\t\treturn true;\n> > -\n> > -\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n> > +\t/*\n> > +\t * If the camera was running when the hardware was removed force the\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> >  \n> > -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > -\t\t\t   << \" state trying operation requiring state \"\n> > -\t\t\t   << camera_state_names[state];\n> > +\tdisconnected_ = true;\n> > +}\n> >  \n> > -\treturn false;\n> > +void Camera::Private::setState(State state)\n> > +{\n> > +\tstate_ = state;\n> >  }\n> >  \n> >  /**\n> > @@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,\n> >  \n> >  Camera::~Camera()\n> >  {\n> > -\tif (!p_->stateIs(Private::CameraAvailable))\n> > -\t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> >  }\n> >  \n> >  /**\n> > @@ -488,23 +523,16 @@ void Camera::disconnect()\n> >  {\n> >  \tLOG(Camera, Debug) << \"Disconnecting camera \" << name();\n> >  \n> > -\t/*\n> > -\t * If the camera was running when the hardware was removed force the\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 (p_->state_ == Private::CameraRunning)\n> > -\t\tp_->state_ = Private::CameraConfigured;\n> > -\n> > -\tp_->disconnected_ = true;\n> > +\tp_->disconnect();\n> >  \tdisconnected.emit(this);\n> >  }\n> >  \n> >  int Camera::exportFrameBuffers(Stream *stream,\n> >  \t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> >  {\n> > -\tif (!p_->stateIs(Private::CameraConfigured))\n> > -\t\treturn -EACCES;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> >  \tif (streams().find(stream) == streams().end())\n> >  \t\treturn -EINVAL;\n> > @@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,\n> >  \n> >  int Camera::freeFrameBuffers(Stream *stream)\n> >  {\n> > -\tif (!p_->stateIs(Private::CameraConfigured))\n> > -\t\treturn -EACCES;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraConfigured, true);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> >  \tp_->pipe_->freeFrameBuffers(this, stream);\n> >  \n> > @@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)\n> >   */\n> >  int Camera::acquire()\n> >  {\n> > -\tif (p_->disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!p_->stateIs(Private::CameraAvailable))\n> > -\t\treturn -EBUSY;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraAvailable);\n> > +\tif (ret < 0)\n> > +\t\treturn ret == -EACCES ? -EBUSY : ret;\n> >  \n> >  \tif (!p_->pipe_->lock()) {\n> >  \t\tLOG(Camera, Info)\n> > @@ -562,7 +589,7 @@ int Camera::acquire()\n> >  \t\treturn -EBUSY;\n> >  \t}\n> >  \n> > -\tp_->state_ = Private::CameraAcquired;\n> > +\tp_->setState(Private::CameraAcquired);\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -580,9 +607,10 @@ int Camera::acquire()\n> >   */\n> >  int Camera::release()\n> >  {\n> > -\tif (!p_->stateBetween(Private::CameraAvailable,\n> > -\t\t\t      Private::CameraConfigured))\n> > -\t\treturn -EBUSY;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n> > +\t\t\t\t      Private::CameraConfigured, true);\n> > +\tif (ret < 0)\n> > +\t\treturn ret == -EACCES ? -EBUSY : ret;\n> >  \n> >  \tif (allocator_) {\n> >  \t\t/*\n> > @@ -596,7 +624,7 @@ int Camera::release()\n> >  \n> >  \tp_->pipe_->unlock();\n> >  \n> > -\tp_->state_ = Private::CameraAvailable;\n> > +\tp_->setState(Private::CameraAvailable);\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const\n> >   */\n> >  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)\n> >  {\n> > -\tif (p_->disconnected_ || roles.size() > streams().size())\n> > +\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n> > +\t\t\t\t      Private::CameraRunning);\n> > +\tif (ret < 0)\n> > +\t\treturn nullptr;\n> > +\n> > +\tif (roles.size() > streams().size())\n> >  \t\treturn nullptr;\n> >  \n> >  \tCameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);\n> > @@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR\n> >   */\n> >  int Camera::configure(CameraConfiguration *config)\n> >  {\n> > -\tint ret;\n> > -\n> > -\tif (p_->disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!p_->stateBetween(Private::CameraAcquired,\n> > -\t\t\t      Private::CameraConfigured))\n> > -\t\treturn -EACCES;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraAcquired,\n> > +\t\t\t\t      Private::CameraConfigured);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> >  \tif (allocator_ && allocator_->allocated()) {\n> >  \t\tLOG(Camera, Error)\n> > @@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)\n> >  \t\tp_->activeStreams_.insert(stream);\n> >  \t}\n> >  \n> > -\tp_->state_ = Private::CameraConfigured;\n> > +\tp_->setState(Private::CameraConfigured);\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)\n> >   */\n> >  Request *Camera::createRequest(uint64_t cookie)\n> >  {\n> > -\tif (p_->disconnected_ ||\n> > -\t    !p_->stateBetween(Private::CameraConfigured,\n> > -\t\t\t      Private::CameraRunning))\n> > +\tint ret = p_->isAccessAllowed(Private::CameraConfigured,\n> > +\t\t\t\t      Private::CameraRunning);\n> > +\tif (ret < 0)\n> >  \t\treturn nullptr;\n> >  \n> >  \treturn new Request(this, cookie);\n> > @@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)\n> >   */\n> >  int Camera::queueRequest(Request *request)\n> >  {\n> > -\tif (p_->disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!p_->stateIs(Private::CameraRunning))\n> > -\t\treturn -EACCES;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> >  \tif (request->buffers().empty()) {\n> >  \t\tLOG(Camera, Error) << \"Request contains no buffers\";\n> > @@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)\n> >   */\n> >  int Camera::start()\n> >  {\n> > -\tif (p_->disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!p_->stateIs(Private::CameraConfigured))\n> > -\t\treturn -EACCES;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> >  \tLOG(Camera, Debug) << \"Starting capture\";\n> >  \n> > @@ -852,11 +877,11 @@ int Camera::start()\n> >  \t\tp_->pipe_->importFrameBuffers(this, stream);\n> >  \t}\n> >  \n> > -\tint ret = p_->pipe_->start(this);\n> > +\tret = p_->pipe_->start(this);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >  \n> > -\tp_->state_ = Private::CameraRunning;\n> > +\tp_->setState(Private::CameraRunning);\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -875,15 +900,13 @@ int Camera::start()\n> >   */\n> >  int Camera::stop()\n> >  {\n> > -\tif (p_->disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!p_->stateIs(Private::CameraRunning))\n> > -\t\treturn -EACCES;\n> > +\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> >  \tLOG(Camera, Debug) << \"Stopping capture\";\n> >  \n> > -\tp_->state_ = Private::CameraConfigured;\n> > +\tp_->setState(Private::CameraConfigured);\n> >  \n> >  \tp_->pipe_->stop(this);\n> >  \n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 669097f609ab..3091971d5da0 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> >   * exportFrameBuffers() and importFrameBuffers() for the streams contained in\n> >   * any camera configuration.\n> >   *\n> > - * The only intended caller is the FrameBufferAllocator helper.\n> > + * The only intended caller is Camera::exportFrameBuffers().\n> >   *\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> > @@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n> >   * called only after a successful call to either of these two methods, and only\n> >   * once per stream.\n> >   *\n> > - * The only intended callers are Camera::stop() and the FrameBufferAllocator\n> > - * helper.\n> > + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().\n> >   */\n> >  \n> >  /**","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","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 46D9D60804\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 21:49:45 +0100 (CET)","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 C1EF82E5;\n\tWed, 22 Jan 2020 21:49:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579726184;\n\tbh=HweMKqNWBUD7qDIjPYplOlCe94jsMbD1OVVrkNuJ000=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q38iKRwK6QisUTVtOpaBV7ltTcDqzuxObUxZ5np85pXMeT0O+RKU+/uhZv1r42gHl\n\tRT+VDkNxMeAg2+LvK53ZXa4DiyMnOnIfyJo3vQPnYx3u2xDroINgOvqx4d2I0iVZsx\n\teaUAlEy6KoIfg7WAd+Co0TBOwbMs/C33ws5QGQxk=","Date":"Wed, 22 Jan 2020 22:49:29 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200122204929.GG15239@pendragon.ideasonboard.com>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-16-laurent.pinchart@ideasonboard.com>\n\t<20200122163128.GW1124294@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200122163128.GW1124294@oden.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize\n\tstate checks in Private class","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 20:49:45 -0000"}}]