[libcamera-devel] [PATCH v5 1/2] pipeline: rkisp1: Share the ISP subdevice

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Sat, Feb 27, 2021 at 07:01:25PM +0100, Sebastian Fricke wrote:
> Share the ISP subdevice between the RkISP1CameraData and the
> PipelineHandlerRkISP1 class. This enables other classes like
> RkISP1CameraConfiguration to get access to the device.

While the code should work, I don't think it convey the right meaning.

std::shared_ptr<> and std::unique_ptr<> are used to denote ownership
rules. The former is used for objects that have multiple owners, while
the later is for single-owner objects. See
Documentation/coding-style.rst for more information.

There's one ISP at the hardware level (well, there are actually two
instances, but that's a separate question, the rkisp1 pipeline handler
only uses once instance). There should thus be a single instance of the
corresponding V4L2Subdevice, and that instance should belong to the
PipelineHandlerRkISP1 class. Its ownership doesn't need to be shared
between multiple classes.

This doesn't preclude storing references to the ISP in other classes.
Those are called borrowed references. They require the code to carefully
ensure that the object will not be destroyed before all borrowed
references are released.

> Signed-off-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 538c0139..50eaa6a4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -90,6 +90,7 @@ public:
>  	Stream mainPathStream_;
>  	Stream selfPathStream_;
>  	std::unique_ptr<CameraSensor> sensor_;
> +	std::shared_ptr<V4L2Subdevice> isp_;

	V4L2Subdevice *isp_;

>  	std::unique_ptr<DelayedControls> delayedCtrls_;
>  	unsigned int frame_;
>  	std::vector<IPABuffer> ipaBuffers_;
> @@ -172,7 +173,7 @@ private:
>  	int freeBuffers(Camera *camera);
>  
>  	MediaDevice *media_;
> -	std::unique_ptr<V4L2Subdevice> isp_;
> +	std::shared_ptr<V4L2Subdevice> isp_;
>  	std::unique_ptr<V4L2VideoDevice> param_;
>  	std::unique_ptr<V4L2VideoDevice> stat_;
>  
> @@ -930,6 +931,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	if (ret)
>  		return ret;
>  
> +	data->isp_ = isp_;
> +

	data->isp_ = isp_.get();

As RkISP1CameraData will be destroyed before PipelineHandlerRkISP1, this
will not cause any issue.

>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  

-- 
Regards,

Laurent Pinchart