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

[Dafna Hirschfeld]

On 02.03.21 03:35, Laurent Pinchart wrote:
> 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.

ok, I see it now, thanks for clarifying.

Dafna

> 
>>> 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();
>>>>    
>>>
>