| Message ID | 20210227180126.37591-2-sebastian.fricke@posteo.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
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@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(); >
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@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(); >> >
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@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(); > >> > >
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@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(); >>>> >>> >
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_; 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_; + /* Initialize the camera properties. */ data->properties_ = data->sensor_->properties();
Share the ISP subdevice between the RkISP1CameraData and the PipelineHandlerRkISP1 class. This enables other classes like RkISP1CameraConfiguration to get access to the device. Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)