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

[Dafna Hirschfeld]

Hi,

On 28.02.21 16:10, Laurent Pinchart wrote:
> 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.

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.
It seems that for consistency with other pipline-handler we can move
all entities to be unique_pointers in RkISP1CameraData

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