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

[Laurent Pinchart]

Hi Dafna,

On Mon, Mar 01, 2021 at 08:13:13AM +0100, Dafna Hirschfeld wrote:
> On 28.02.21 16:10, Laurent Pinchart wrote:
> > 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.
> 
> I see that in other pipeline-handlers, the topology entities are all members of
> CameraData. In rkisp1, the sensor, mainpath and selfpath are members of RkISP1CameraData
> while the isp, params, stats, are members of PipelineHandlerRkISP1.
> I can't see why it is done like that.

There's no hard rule here. What we usually do is that resources that are
shared by all cameras are stored in the pipeline handler, while
resources specific to one camera are stored in the camera data. Another
option is to store all resources in the pipeline handler, and store
pointers to the camera-specific resources in each camera data instance.
By storing resources I mean for example an instance of a V4L2Subdevice,
usually stored in a unique_ptr, while pointers to resources are normal
pointers.

> It seems that for consistency with other pipline-handler we can move
> all entities to be unique_pointers in RkISP1CameraData

There's a single ISP instance, so there should be a single corresponding
V4L2Subdevice instance, and it thus has to be stored in the pipeline
handler. The camera data instances can point to it for convenience.

> > 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