[libcamera-devel] [PATCH v3 10/11] libcamera: pipeline_handler: Add functions to lock a whole pipeline

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, May 11, 2019 at 11:19:06AM +0200, Niklas Söderlund wrote:
> Add lock() and unlock() which are backed by the MediaDevice
> implementation and will lock all media devices claimed by a pipeline

s/will lock/lock/

> handler instance.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  3 ++
>  src/libcamera/pipeline_handler.cpp       | 38 ++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 4c13496246c2251c..019c8f4751a8c2b2 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -57,6 +57,9 @@ public:
>  	MediaDevice *tryAcquire(DeviceEnumerator *enumerator,
>  				const DeviceMatch &dm);
>  
> +	bool lock();
> +	void unlock();
> +
>  	virtual CameraConfiguration
>  	streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
>  	virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 8f50ef51f0c23301..84e40e4672518d1f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -182,6 +182,44 @@ out:
>  	return media.get();
>  }
>  
> +/**
> + * \brief Lock all media devices acquired by the pipeline
> + *
> + * This method shall not be called from a pipeline handler implementation
> + * directly, as the Camera handles this on the behalf of the specified
> + * implementation.

I'm not sure what you mean by "on the behalf of the specified
implementation". How about just "This method shall not be called from
pipeline handler implementation, as the Camera class handles locking
directly." ?

> + *
> + * \return True if the devices could be locked, false otherwise
> + * \sa unlock()
> + * \sa MediaDevice::lock()
> + */
> +bool PipelineHandler::lock()
> +{
> +	for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> +		if (!media->lock()) {
> +			unlock();
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * \brief Unlock all media devices acquired by the pipeline
> + *
> + * This method shall not be called from a pipeline handler implementation
> + * directly, as the Camera handles this on the behalf of the specified
> + * implementation.

Same comment here. Apart from this,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> + *
> + * \sa lock()
> + */
> +void PipelineHandler::unlock()
> +{
> +	for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> +		media->unlock();
> +}
> +
>  /**
>   * \fn PipelineHandler::streamConfiguration()
>   * \brief Retrieve a group of stream configurations for a specified camera

-- 
Regards,

Laurent Pinchart