[libcamera-devel,15/19] libcamera: camera: Centralize state checks in Private class
diff mbox series

Message ID 20200120002437.6633-16-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Initial libcamera threading model
Related show

Commit Message

Laurent Pinchart Jan. 20, 2020, 12:24 a.m. UTC 
Move all accesses to the state_ and disconnected_ members to functions
of the Private class. This will make it easier to implement
synchronization, and simplifies the Camera class implementation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/camera.cpp           | 163 ++++++++++++++++-------------
 src/libcamera/pipeline_handler.cpp |   5 +-
 2 files changed, 95 insertions(+), 73 deletions(-)

Comments

Niklas Söderlund Jan. 22, 2020, 4:31 p.m. UTC | #1 
Hi Laurent,

Thanks for your work.

On 2020-01-20 02:24:33 +0200, Laurent Pinchart wrote:
> Move all accesses to the state_ and disconnected_ members to functions
> of the Private class. This will make it easier to implement
> synchronization, and simplifies the Camera class implementation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I really like this patch.

> ---
>  src/libcamera/camera.cpp           | 163 ++++++++++++++++-------------
>  src/libcamera/pipeline_handler.cpp |   5 +-
>  2 files changed, 95 insertions(+), 73 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 6fe802f375a3..83c2249b8897 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -266,15 +266,21 @@ public:
>  
>  	Private(PipelineHandler *pipe, const std::string &name,
>  		const std::set<Stream *> &streams);
> +	~Private();
>  
> -	bool stateBetween(State low, State high) const;
> -	bool stateIs(State state) const;
> +	int isAccessAllowed(State state, bool allowDisconnected = false) const;
> +	int isAccessAllowed(State low, State high,
> +			    bool allowDisconnected = false) const;
> +
> +	void disconnect();
> +	void setState(State state);
>  
>  	std::shared_ptr<PipelineHandler> pipe_;
>  	std::string name_;
>  	std::set<Stream *> streams_;
>  	std::set<Stream *> activeStreams_;
>  
> +private:
>  	bool disconnected_;
>  	State state_;
>  };
> @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
>  {
>  }
>  
> +Camera::Private::~Private()
> +{
> +	if (state_ != Private::CameraAvailable)
> +		LOG(Camera, Error) << "Removing camera while still in use";
> +}
> +
>  static constexpr std::array<const char *, 4> camera_state_names = {
>  	"Available",
>  	"Acquired",
> @@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {
>  	"Running",
>  };
>  
> -bool Camera::Private::stateBetween(State low, State high) const

I wonder if we should document the \retval codes here and then use 
\copydetails from callers in Camera to make sure -ENODEV and -EACCESS is 
uniformly documented.

Or maybe that is not possible as Doxygen ignores libcamera::*::Private.

With or without this cherry on top,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
>  {
> +	if (!allowDisconnected && disconnected_)
> +		return -ENODEV;
> +
> +	if (state_ == state)
> +		return 0;
> +
> +	ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> +
> +	LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> +			   << " state trying operation requiring state "
> +			   << camera_state_names[state];
> +
> +	return -EACCES;
> +}
> +
> +int Camera::Private::isAccessAllowed(State low, State high,
> +				     bool allowDisconnected) const
> +{
> +	if (!allowDisconnected && disconnected_)
> +		return -ENODEV;
> +
>  	if (state_ >= low && state_ <= high)
> -		return true;
> +		return 0;
>  
>  	ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
>  	       static_cast<unsigned int>(high) < camera_state_names.size());
> @@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const
>  			   << camera_state_names[low] << " and "
>  			   << camera_state_names[high];
>  
> -	return false;
> +	return -EACCES;
>  }
>  
> -bool Camera::Private::stateIs(State state) const
> +void Camera::Private::disconnect()
>  {
> -	if (state_ == state)
> -		return true;
> -
> -	ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> +	/*
> +	 * If the camera was running when the hardware was removed force the
> +	 * state to Configured state to allow applications to free resources
> +	 * and call release() before deleting the camera.
> +	 */
> +	if (state_ == Private::CameraRunning)
> +		state_ = Private::CameraConfigured;
>  
> -	LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> -			   << " state trying operation requiring state "
> -			   << camera_state_names[state];
> +	disconnected_ = true;
> +}
>  
> -	return false;
> +void Camera::Private::setState(State state)
> +{
> +	state_ = state;
>  }
>  
>  /**
> @@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,
>  
>  Camera::~Camera()
>  {
> -	if (!p_->stateIs(Private::CameraAvailable))
> -		LOG(Camera, Error) << "Removing camera while still in use";
>  }
>  
>  /**
> @@ -488,23 +523,16 @@ void Camera::disconnect()
>  {
>  	LOG(Camera, Debug) << "Disconnecting camera " << name();
>  
> -	/*
> -	 * If the camera was running when the hardware was removed force the
> -	 * state to Configured state to allow applications to free resources
> -	 * and call release() before deleting the camera.
> -	 */
> -	if (p_->state_ == Private::CameraRunning)
> -		p_->state_ = Private::CameraConfigured;
> -
> -	p_->disconnected_ = true;
> +	p_->disconnect();
>  	disconnected.emit(this);
>  }
>  
>  int Camera::exportFrameBuffers(Stream *stream,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	if (!p_->stateIs(Private::CameraConfigured))
> -		return -EACCES;
> +	int ret = p_->isAccessAllowed(Private::CameraConfigured);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (streams().find(stream) == streams().end())
>  		return -EINVAL;
> @@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,
>  
>  int Camera::freeFrameBuffers(Stream *stream)
>  {
> -	if (!p_->stateIs(Private::CameraConfigured))
> -		return -EACCES;
> +	int ret = p_->isAccessAllowed(Private::CameraConfigured, true);
> +	if (ret < 0)
> +		return ret;
>  
>  	p_->pipe_->freeFrameBuffers(this, stream);
>  
> @@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)
>   */
>  int Camera::acquire()
>  {
> -	if (p_->disconnected_)
> -		return -ENODEV;
> -
> -	if (!p_->stateIs(Private::CameraAvailable))
> -		return -EBUSY;
> +	int ret = p_->isAccessAllowed(Private::CameraAvailable);
> +	if (ret < 0)
> +		return ret == -EACCES ? -EBUSY : ret;
>  
>  	if (!p_->pipe_->lock()) {
>  		LOG(Camera, Info)
> @@ -562,7 +589,7 @@ int Camera::acquire()
>  		return -EBUSY;
>  	}
>  
> -	p_->state_ = Private::CameraAcquired;
> +	p_->setState(Private::CameraAcquired);
>  
>  	return 0;
>  }
> @@ -580,9 +607,10 @@ int Camera::acquire()
>   */
>  int Camera::release()
>  {
> -	if (!p_->stateBetween(Private::CameraAvailable,
> -			      Private::CameraConfigured))
> -		return -EBUSY;
> +	int ret = p_->isAccessAllowed(Private::CameraAvailable,
> +				      Private::CameraConfigured, true);
> +	if (ret < 0)
> +		return ret == -EACCES ? -EBUSY : ret;
>  
>  	if (allocator_) {
>  		/*
> @@ -596,7 +624,7 @@ int Camera::release()
>  
>  	p_->pipe_->unlock();
>  
> -	p_->state_ = Private::CameraAvailable;
> +	p_->setState(Private::CameraAvailable);
>  
>  	return 0;
>  }
> @@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const
>   */
>  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
>  {
> -	if (p_->disconnected_ || roles.size() > streams().size())
> +	int ret = p_->isAccessAllowed(Private::CameraAvailable,
> +				      Private::CameraRunning);
> +	if (ret < 0)
> +		return nullptr;
> +
> +	if (roles.size() > streams().size())
>  		return nullptr;
>  
>  	CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);
> @@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
>   */
>  int Camera::configure(CameraConfiguration *config)
>  {
> -	int ret;
> -
> -	if (p_->disconnected_)
> -		return -ENODEV;
> -
> -	if (!p_->stateBetween(Private::CameraAcquired,
> -			      Private::CameraConfigured))
> -		return -EACCES;
> +	int ret = p_->isAccessAllowed(Private::CameraAcquired,
> +				      Private::CameraConfigured);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (allocator_ && allocator_->allocated()) {
>  		LOG(Camera, Error)
> @@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)
>  		p_->activeStreams_.insert(stream);
>  	}
>  
> -	p_->state_ = Private::CameraConfigured;
> +	p_->setState(Private::CameraConfigured);
>  
>  	return 0;
>  }
> @@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)
>   */
>  Request *Camera::createRequest(uint64_t cookie)
>  {
> -	if (p_->disconnected_ ||
> -	    !p_->stateBetween(Private::CameraConfigured,
> -			      Private::CameraRunning))
> +	int ret = p_->isAccessAllowed(Private::CameraConfigured,
> +				      Private::CameraRunning);
> +	if (ret < 0)
>  		return nullptr;
>  
>  	return new Request(this, cookie);
> @@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)
>   */
>  int Camera::queueRequest(Request *request)
>  {
> -	if (p_->disconnected_)
> -		return -ENODEV;
> -
> -	if (!p_->stateIs(Private::CameraRunning))
> -		return -EACCES;
> +	int ret = p_->isAccessAllowed(Private::CameraRunning);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (request->buffers().empty()) {
>  		LOG(Camera, Error) << "Request contains no buffers";
> @@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)
>   */
>  int Camera::start()
>  {
> -	if (p_->disconnected_)
> -		return -ENODEV;
> -
> -	if (!p_->stateIs(Private::CameraConfigured))
> -		return -EACCES;
> +	int ret = p_->isAccessAllowed(Private::CameraConfigured);
> +	if (ret < 0)
> +		return ret;
>  
>  	LOG(Camera, Debug) << "Starting capture";
>  
> @@ -852,11 +877,11 @@ int Camera::start()
>  		p_->pipe_->importFrameBuffers(this, stream);
>  	}
>  
> -	int ret = p_->pipe_->start(this);
> +	ret = p_->pipe_->start(this);
>  	if (ret)
>  		return ret;
>  
> -	p_->state_ = Private::CameraRunning;
> +	p_->setState(Private::CameraRunning);
>  
>  	return 0;
>  }
> @@ -875,15 +900,13 @@ int Camera::start()
>   */
>  int Camera::stop()
>  {
> -	if (p_->disconnected_)
> -		return -ENODEV;
> -
> -	if (!p_->stateIs(Private::CameraRunning))
> -		return -EACCES;
> +	int ret = p_->isAccessAllowed(Private::CameraRunning);
> +	if (ret < 0)
> +		return ret;
>  
>  	LOG(Camera, Debug) << "Stopping capture";
>  
> -	p_->state_ = Private::CameraConfigured;
> +	p_->setState(Private::CameraConfigured);
>  
>  	p_->pipe_->stop(this);
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 669097f609ab..3091971d5da0 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
>   * exportFrameBuffers() and importFrameBuffers() for the streams contained in
>   * any camera configuration.
>   *
> - * The only intended caller is the FrameBufferAllocator helper.
> + * The only intended caller is Camera::exportFrameBuffers().
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> @@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
>   * called only after a successful call to either of these two methods, and only
>   * once per stream.
>   *
> - * The only intended callers are Camera::stop() and the FrameBufferAllocator
> - * helper.
> + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().
>   */
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 22, 2020, 8:49 p.m. UTC | #2 
Hi Niklas,

On Wed, Jan 22, 2020 at 05:31:28PM +0100, Niklas Söderlund wrote:
> On 2020-01-20 02:24:33 +0200, Laurent Pinchart wrote:
> > Move all accesses to the state_ and disconnected_ members to functions
> > of the Private class. This will make it easier to implement
> > synchronization, and simplifies the Camera class implementation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I really like this patch.
> 
> > ---
> >  src/libcamera/camera.cpp           | 163 ++++++++++++++++-------------
> >  src/libcamera/pipeline_handler.cpp |   5 +-
> >  2 files changed, 95 insertions(+), 73 deletions(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 6fe802f375a3..83c2249b8897 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -266,15 +266,21 @@ public:
> >  
> >  	Private(PipelineHandler *pipe, const std::string &name,
> >  		const std::set<Stream *> &streams);
> > +	~Private();
> >  
> > -	bool stateBetween(State low, State high) const;
> > -	bool stateIs(State state) const;
> > +	int isAccessAllowed(State state, bool allowDisconnected = false) const;
> > +	int isAccessAllowed(State low, State high,
> > +			    bool allowDisconnected = false) const;
> > +
> > +	void disconnect();
> > +	void setState(State state);
> >  
> >  	std::shared_ptr<PipelineHandler> pipe_;
> >  	std::string name_;
> >  	std::set<Stream *> streams_;
> >  	std::set<Stream *> activeStreams_;
> >  
> > +private:
> >  	bool disconnected_;
> >  	State state_;
> >  };
> > @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
> >  {
> >  }
> >  
> > +Camera::Private::~Private()
> > +{
> > +	if (state_ != Private::CameraAvailable)
> > +		LOG(Camera, Error) << "Removing camera while still in use";
> > +}
> > +
> >  static constexpr std::array<const char *, 4> camera_state_names = {
> >  	"Available",
> >  	"Acquired",
> > @@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {
> >  	"Running",
> >  };
> >  
> > -bool Camera::Private::stateBetween(State low, State high) const
> 
> I wonder if we should document the \retval codes here and then use 
> \copydetails from callers in Camera to make sure -ENODEV and -EACCESS is 
> uniformly documented.
> 
> Or maybe that is not possible as Doxygen ignores libcamera::*::Private.

Yes, that's the issue :-(

> With or without this cherry on top,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> > +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
> >  {
> > +	if (!allowDisconnected && disconnected_)
> > +		return -ENODEV;
> > +
> > +	if (state_ == state)
> > +		return 0;
> > +
> > +	ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> > +
> > +	LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> > +			   << " state trying operation requiring state "
> > +			   << camera_state_names[state];
> > +
> > +	return -EACCES;
> > +}
> > +
> > +int Camera::Private::isAccessAllowed(State low, State high,
> > +				     bool allowDisconnected) const
> > +{
> > +	if (!allowDisconnected && disconnected_)
> > +		return -ENODEV;
> > +
> >  	if (state_ >= low && state_ <= high)
> > -		return true;
> > +		return 0;
> >  
> >  	ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
> >  	       static_cast<unsigned int>(high) < camera_state_names.size());
> > @@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const
> >  			   << camera_state_names[low] << " and "
> >  			   << camera_state_names[high];
> >  
> > -	return false;
> > +	return -EACCES;
> >  }
> >  
> > -bool Camera::Private::stateIs(State state) const
> > +void Camera::Private::disconnect()
> >  {
> > -	if (state_ == state)
> > -		return true;
> > -
> > -	ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> > +	/*
> > +	 * If the camera was running when the hardware was removed force the
> > +	 * state to Configured state to allow applications to free resources
> > +	 * and call release() before deleting the camera.
> > +	 */
> > +	if (state_ == Private::CameraRunning)
> > +		state_ = Private::CameraConfigured;
> >  
> > -	LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> > -			   << " state trying operation requiring state "
> > -			   << camera_state_names[state];
> > +	disconnected_ = true;
> > +}
> >  
> > -	return false;
> > +void Camera::Private::setState(State state)
> > +{
> > +	state_ = state;
> >  }
> >  
> >  /**
> > @@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,
> >  
> >  Camera::~Camera()
> >  {
> > -	if (!p_->stateIs(Private::CameraAvailable))
> > -		LOG(Camera, Error) << "Removing camera while still in use";
> >  }
> >  
> >  /**
> > @@ -488,23 +523,16 @@ void Camera::disconnect()
> >  {
> >  	LOG(Camera, Debug) << "Disconnecting camera " << name();
> >  
> > -	/*
> > -	 * If the camera was running when the hardware was removed force the
> > -	 * state to Configured state to allow applications to free resources
> > -	 * and call release() before deleting the camera.
> > -	 */
> > -	if (p_->state_ == Private::CameraRunning)
> > -		p_->state_ = Private::CameraConfigured;
> > -
> > -	p_->disconnected_ = true;
> > +	p_->disconnect();
> >  	disconnected.emit(this);
> >  }
> >  
> >  int Camera::exportFrameBuffers(Stream *stream,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> > -	if (!p_->stateIs(Private::CameraConfigured))
> > -		return -EACCES;
> > +	int ret = p_->isAccessAllowed(Private::CameraConfigured);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	if (streams().find(stream) == streams().end())
> >  		return -EINVAL;
> > @@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,
> >  
> >  int Camera::freeFrameBuffers(Stream *stream)
> >  {
> > -	if (!p_->stateIs(Private::CameraConfigured))
> > -		return -EACCES;
> > +	int ret = p_->isAccessAllowed(Private::CameraConfigured, true);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	p_->pipe_->freeFrameBuffers(this, stream);
> >  
> > @@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)
> >   */
> >  int Camera::acquire()
> >  {
> > -	if (p_->disconnected_)
> > -		return -ENODEV;
> > -
> > -	if (!p_->stateIs(Private::CameraAvailable))
> > -		return -EBUSY;
> > +	int ret = p_->isAccessAllowed(Private::CameraAvailable);
> > +	if (ret < 0)
> > +		return ret == -EACCES ? -EBUSY : ret;
> >  
> >  	if (!p_->pipe_->lock()) {
> >  		LOG(Camera, Info)
> > @@ -562,7 +589,7 @@ int Camera::acquire()
> >  		return -EBUSY;
> >  	}
> >  
> > -	p_->state_ = Private::CameraAcquired;
> > +	p_->setState(Private::CameraAcquired);
> >  
> >  	return 0;
> >  }
> > @@ -580,9 +607,10 @@ int Camera::acquire()
> >   */
> >  int Camera::release()
> >  {
> > -	if (!p_->stateBetween(Private::CameraAvailable,
> > -			      Private::CameraConfigured))
> > -		return -EBUSY;
> > +	int ret = p_->isAccessAllowed(Private::CameraAvailable,
> > +				      Private::CameraConfigured, true);
> > +	if (ret < 0)
> > +		return ret == -EACCES ? -EBUSY : ret;
> >  
> >  	if (allocator_) {
> >  		/*
> > @@ -596,7 +624,7 @@ int Camera::release()
> >  
> >  	p_->pipe_->unlock();
> >  
> > -	p_->state_ = Private::CameraAvailable;
> > +	p_->setState(Private::CameraAvailable);
> >  
> >  	return 0;
> >  }
> > @@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const
> >   */
> >  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> >  {
> > -	if (p_->disconnected_ || roles.size() > streams().size())
> > +	int ret = p_->isAccessAllowed(Private::CameraAvailable,
> > +				      Private::CameraRunning);
> > +	if (ret < 0)
> > +		return nullptr;
> > +
> > +	if (roles.size() > streams().size())
> >  		return nullptr;
> >  
> >  	CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);
> > @@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> >   */
> >  int Camera::configure(CameraConfiguration *config)
> >  {
> > -	int ret;
> > -
> > -	if (p_->disconnected_)
> > -		return -ENODEV;
> > -
> > -	if (!p_->stateBetween(Private::CameraAcquired,
> > -			      Private::CameraConfigured))
> > -		return -EACCES;
> > +	int ret = p_->isAccessAllowed(Private::CameraAcquired,
> > +				      Private::CameraConfigured);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	if (allocator_ && allocator_->allocated()) {
> >  		LOG(Camera, Error)
> > @@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)
> >  		p_->activeStreams_.insert(stream);
> >  	}
> >  
> > -	p_->state_ = Private::CameraConfigured;
> > +	p_->setState(Private::CameraConfigured);
> >  
> >  	return 0;
> >  }
> > @@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)
> >   */
> >  Request *Camera::createRequest(uint64_t cookie)
> >  {
> > -	if (p_->disconnected_ ||
> > -	    !p_->stateBetween(Private::CameraConfigured,
> > -			      Private::CameraRunning))
> > +	int ret = p_->isAccessAllowed(Private::CameraConfigured,
> > +				      Private::CameraRunning);
> > +	if (ret < 0)
> >  		return nullptr;
> >  
> >  	return new Request(this, cookie);
> > @@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)
> >   */
> >  int Camera::queueRequest(Request *request)
> >  {
> > -	if (p_->disconnected_)
> > -		return -ENODEV;
> > -
> > -	if (!p_->stateIs(Private::CameraRunning))
> > -		return -EACCES;
> > +	int ret = p_->isAccessAllowed(Private::CameraRunning);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	if (request->buffers().empty()) {
> >  		LOG(Camera, Error) << "Request contains no buffers";
> > @@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)
> >   */
> >  int Camera::start()
> >  {
> > -	if (p_->disconnected_)
> > -		return -ENODEV;
> > -
> > -	if (!p_->stateIs(Private::CameraConfigured))
> > -		return -EACCES;
> > +	int ret = p_->isAccessAllowed(Private::CameraConfigured);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	LOG(Camera, Debug) << "Starting capture";
> >  
> > @@ -852,11 +877,11 @@ int Camera::start()
> >  		p_->pipe_->importFrameBuffers(this, stream);
> >  	}
> >  
> > -	int ret = p_->pipe_->start(this);
> > +	ret = p_->pipe_->start(this);
> >  	if (ret)
> >  		return ret;
> >  
> > -	p_->state_ = Private::CameraRunning;
> > +	p_->setState(Private::CameraRunning);
> >  
> >  	return 0;
> >  }
> > @@ -875,15 +900,13 @@ int Camera::start()
> >   */
> >  int Camera::stop()
> >  {
> > -	if (p_->disconnected_)
> > -		return -ENODEV;
> > -
> > -	if (!p_->stateIs(Private::CameraRunning))
> > -		return -EACCES;
> > +	int ret = p_->isAccessAllowed(Private::CameraRunning);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	LOG(Camera, Debug) << "Stopping capture";
> >  
> > -	p_->state_ = Private::CameraConfigured;
> > +	p_->setState(Private::CameraConfigured);
> >  
> >  	p_->pipe_->stop(this);
> >  
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 669097f609ab..3091971d5da0 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> >   * exportFrameBuffers() and importFrameBuffers() for the streams contained in
> >   * any camera configuration.
> >   *
> > - * The only intended caller is the FrameBufferAllocator helper.
> > + * The only intended caller is Camera::exportFrameBuffers().
> >   *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > @@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> >   * called only after a successful call to either of these two methods, and only
> >   * once per stream.
> >   *
> > - * The only intended callers are Camera::stop() and the FrameBufferAllocator
> > - * helper.
> > + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().
> >   */
> >  
> >  /**

Patch
diff mbox series 

diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 6fe802f375a3..83c2249b8897 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -266,15 +266,21 @@  public:
 
 	Private(PipelineHandler *pipe, const std::string &name,
 		const std::set<Stream *> &streams);
+	~Private();
 
-	bool stateBetween(State low, State high) const;
-	bool stateIs(State state) const;
+	int isAccessAllowed(State state, bool allowDisconnected = false) const;
+	int isAccessAllowed(State low, State high,
+			    bool allowDisconnected = false) const;
+
+	void disconnect();
+	void setState(State state);
 
 	std::shared_ptr<PipelineHandler> pipe_;
 	std::string name_;
 	std::set<Stream *> streams_;
 	std::set<Stream *> activeStreams_;
 
+private:
 	bool disconnected_;
 	State state_;
 };
@@ -286,6 +292,12 @@  Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
 {
 }
 
+Camera::Private::~Private()
+{
+	if (state_ != Private::CameraAvailable)
+		LOG(Camera, Error) << "Removing camera while still in use";
+}
+
 static constexpr std::array<const char *, 4> camera_state_names = {
 	"Available",
 	"Acquired",
@@ -293,10 +305,31 @@  static constexpr std::array<const char *, 4> camera_state_names = {
 	"Running",
 };
 
-bool Camera::Private::stateBetween(State low, State high) const
+int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
 {
+	if (!allowDisconnected && disconnected_)
+		return -ENODEV;
+
+	if (state_ == state)
+		return 0;
+
+	ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
+
+	LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
+			   << " state trying operation requiring state "
+			   << camera_state_names[state];
+
+	return -EACCES;
+}
+
+int Camera::Private::isAccessAllowed(State low, State high,
+				     bool allowDisconnected) const
+{
+	if (!allowDisconnected && disconnected_)
+		return -ENODEV;
+
 	if (state_ >= low && state_ <= high)
-		return true;
+		return 0;
 
 	ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
 	       static_cast<unsigned int>(high) < camera_state_names.size());
@@ -306,21 +339,25 @@  bool Camera::Private::stateBetween(State low, State high) const
 			   << camera_state_names[low] << " and "
 			   << camera_state_names[high];
 
-	return false;
+	return -EACCES;
 }
 
-bool Camera::Private::stateIs(State state) const
+void Camera::Private::disconnect()
 {
-	if (state_ == state)
-		return true;
-
-	ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
+	/*
+	 * If the camera was running when the hardware was removed force the
+	 * state to Configured state to allow applications to free resources
+	 * and call release() before deleting the camera.
+	 */
+	if (state_ == Private::CameraRunning)
+		state_ = Private::CameraConfigured;
 
-	LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
-			   << " state trying operation requiring state "
-			   << camera_state_names[state];
+	disconnected_ = true;
+}
 
-	return false;
+void Camera::Private::setState(State state)
+{
+	state_ = state;
 }
 
 /**
@@ -468,8 +505,6 @@  Camera::Camera(PipelineHandler *pipe, const std::string &name,
 
 Camera::~Camera()
 {
-	if (!p_->stateIs(Private::CameraAvailable))
-		LOG(Camera, Error) << "Removing camera while still in use";
 }
 
 /**
@@ -488,23 +523,16 @@  void Camera::disconnect()
 {
 	LOG(Camera, Debug) << "Disconnecting camera " << name();
 
-	/*
-	 * If the camera was running when the hardware was removed force the
-	 * state to Configured state to allow applications to free resources
-	 * and call release() before deleting the camera.
-	 */
-	if (p_->state_ == Private::CameraRunning)
-		p_->state_ = Private::CameraConfigured;
-
-	p_->disconnected_ = true;
+	p_->disconnect();
 	disconnected.emit(this);
 }
 
 int Camera::exportFrameBuffers(Stream *stream,
 			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
 {
-	if (!p_->stateIs(Private::CameraConfigured))
-		return -EACCES;
+	int ret = p_->isAccessAllowed(Private::CameraConfigured);
+	if (ret < 0)
+		return ret;
 
 	if (streams().find(stream) == streams().end())
 		return -EINVAL;
@@ -517,8 +545,9 @@  int Camera::exportFrameBuffers(Stream *stream,
 
 int Camera::freeFrameBuffers(Stream *stream)
 {
-	if (!p_->stateIs(Private::CameraConfigured))
-		return -EACCES;
+	int ret = p_->isAccessAllowed(Private::CameraConfigured, true);
+	if (ret < 0)
+		return ret;
 
 	p_->pipe_->freeFrameBuffers(this, stream);
 
@@ -550,11 +579,9 @@  int Camera::freeFrameBuffers(Stream *stream)
  */
 int Camera::acquire()
 {
-	if (p_->disconnected_)
-		return -ENODEV;
-
-	if (!p_->stateIs(Private::CameraAvailable))
-		return -EBUSY;
+	int ret = p_->isAccessAllowed(Private::CameraAvailable);
+	if (ret < 0)
+		return ret == -EACCES ? -EBUSY : ret;
 
 	if (!p_->pipe_->lock()) {
 		LOG(Camera, Info)
@@ -562,7 +589,7 @@  int Camera::acquire()
 		return -EBUSY;
 	}
 
-	p_->state_ = Private::CameraAcquired;
+	p_->setState(Private::CameraAcquired);
 
 	return 0;
 }
@@ -580,9 +607,10 @@  int Camera::acquire()
  */
 int Camera::release()
 {
-	if (!p_->stateBetween(Private::CameraAvailable,
-			      Private::CameraConfigured))
-		return -EBUSY;
+	int ret = p_->isAccessAllowed(Private::CameraAvailable,
+				      Private::CameraConfigured, true);
+	if (ret < 0)
+		return ret == -EACCES ? -EBUSY : ret;
 
 	if (allocator_) {
 		/*
@@ -596,7 +624,7 @@  int Camera::release()
 
 	p_->pipe_->unlock();
 
-	p_->state_ = Private::CameraAvailable;
+	p_->setState(Private::CameraAvailable);
 
 	return 0;
 }
@@ -643,7 +671,12 @@  const std::set<Stream *> &Camera::streams() const
  */
 std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
 {
-	if (p_->disconnected_ || roles.size() > streams().size())
+	int ret = p_->isAccessAllowed(Private::CameraAvailable,
+				      Private::CameraRunning);
+	if (ret < 0)
+		return nullptr;
+
+	if (roles.size() > streams().size())
 		return nullptr;
 
 	CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);
@@ -694,14 +727,10 @@  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
  */
 int Camera::configure(CameraConfiguration *config)
 {
-	int ret;
-
-	if (p_->disconnected_)
-		return -ENODEV;
-
-	if (!p_->stateBetween(Private::CameraAcquired,
-			      Private::CameraConfigured))
-		return -EACCES;
+	int ret = p_->isAccessAllowed(Private::CameraAcquired,
+				      Private::CameraConfigured);
+	if (ret < 0)
+		return ret;
 
 	if (allocator_ && allocator_->allocated()) {
 		LOG(Camera, Error)
@@ -740,7 +769,7 @@  int Camera::configure(CameraConfiguration *config)
 		p_->activeStreams_.insert(stream);
 	}
 
-	p_->state_ = Private::CameraConfigured;
+	p_->setState(Private::CameraConfigured);
 
 	return 0;
 }
@@ -767,9 +796,9 @@  int Camera::configure(CameraConfiguration *config)
  */
 Request *Camera::createRequest(uint64_t cookie)
 {
-	if (p_->disconnected_ ||
-	    !p_->stateBetween(Private::CameraConfigured,
-			      Private::CameraRunning))
+	int ret = p_->isAccessAllowed(Private::CameraConfigured,
+				      Private::CameraRunning);
+	if (ret < 0)
 		return nullptr;
 
 	return new Request(this, cookie);
@@ -799,11 +828,9 @@  Request *Camera::createRequest(uint64_t cookie)
  */
 int Camera::queueRequest(Request *request)
 {
-	if (p_->disconnected_)
-		return -ENODEV;
-
-	if (!p_->stateIs(Private::CameraRunning))
-		return -EACCES;
+	int ret = p_->isAccessAllowed(Private::CameraRunning);
+	if (ret < 0)
+		return ret;
 
 	if (request->buffers().empty()) {
 		LOG(Camera, Error) << "Request contains no buffers";
@@ -837,11 +864,9 @@  int Camera::queueRequest(Request *request)
  */
 int Camera::start()
 {
-	if (p_->disconnected_)
-		return -ENODEV;
-
-	if (!p_->stateIs(Private::CameraConfigured))
-		return -EACCES;
+	int ret = p_->isAccessAllowed(Private::CameraConfigured);
+	if (ret < 0)
+		return ret;
 
 	LOG(Camera, Debug) << "Starting capture";
 
@@ -852,11 +877,11 @@  int Camera::start()
 		p_->pipe_->importFrameBuffers(this, stream);
 	}
 
-	int ret = p_->pipe_->start(this);
+	ret = p_->pipe_->start(this);
 	if (ret)
 		return ret;
 
-	p_->state_ = Private::CameraRunning;
+	p_->setState(Private::CameraRunning);
 
 	return 0;
 }
@@ -875,15 +900,13 @@  int Camera::start()
  */
 int Camera::stop()
 {
-	if (p_->disconnected_)
-		return -ENODEV;
-
-	if (!p_->stateIs(Private::CameraRunning))
-		return -EACCES;
+	int ret = p_->isAccessAllowed(Private::CameraRunning);
+	if (ret < 0)
+		return ret;
 
 	LOG(Camera, Debug) << "Stopping capture";
 
-	p_->state_ = Private::CameraConfigured;
+	p_->setState(Private::CameraConfigured);
 
 	p_->pipe_->stop(this);
 
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 669097f609ab..3091971d5da0 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -314,7 +314,7 @@  const ControlInfoMap &PipelineHandler::controls(Camera *camera)
  * exportFrameBuffers() and importFrameBuffers() for the streams contained in
  * any camera configuration.
  *
- * The only intended caller is the FrameBufferAllocator helper.
+ * The only intended caller is Camera::exportFrameBuffers().
  *
  * \return 0 on success or a negative error code otherwise
  */
@@ -357,8 +357,7 @@  const ControlInfoMap &PipelineHandler::controls(Camera *camera)
  * called only after a successful call to either of these two methods, and only
  * once per stream.
  *
- * The only intended callers are Camera::stop() and the FrameBufferAllocator
- * helper.
+ * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().
  */
 
 /**