[libcamera-devel,2/2] libcamera: pipeline: raspberrypi: Free buffers when a camera is released

17788 diff mbox series

Implement the PipelineHandlerRPi::releaseDevice method which allows
us to free any allocated buffers when a camera is released.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

Quoting David Plowman via libcamera-devel (2022-11-11 13:30:25)
> Implement the PipelineHandlerRPi::releaseDevice method which allows
> us to free any allocated buffers when a camera is released.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f15fa28b..785eddf9 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -337,6 +337,8 @@ public:
>  
>         bool match(DeviceEnumerator *enumerator) override;
>  
> +       void releaseDevice(Camera *camera) override;
> +
>  private:
>         RPiCameraData *cameraData(Camera *camera)
>         {
> @@ -1193,6 +1195,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>         return !!numCameras;
>  }
>  
> +void PipelineHandlerRPi::releaseDevice(Camera *camera)
> +{
> +       RPiCameraData *data = cameraData(camera);
> +       data->freeBuffers();
> +}
> +
>  int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity)
>  {
>         std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
> -- 
> 2.30.2
>

Hi David,

Thank you for fixing this!


On Fri, 11 Nov 2022 at 13:30, David Plowman via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> Implement the PipelineHandlerRPi::releaseDevice method which allows
> us to free any allocated buffers when a camera is released.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f15fa28b..785eddf9 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -337,6 +337,8 @@ public:
>
>         bool match(DeviceEnumerator *enumerator) override;
>
> +       void releaseDevice(Camera *camera) override;
> +
>  private:
>         RPiCameraData *cameraData(Camera *camera)
>         {
> @@ -1193,6 +1195,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator
> *enumerator)
>         return !!numCameras;
>  }
>
> +void PipelineHandlerRPi::releaseDevice(Camera *camera)
> +{
> +       RPiCameraData *data = cameraData(camera);
> +       data->freeBuffers();
> +}
>

My head's spinning with all these related changes now :-)

Presumably for this to work correctly, RPiCameraData::freeBuffers() must
not do the
if (!buffersAllocated_) test, correct? So this change relies on [1] to
behave correctly?

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

[1] https://patchwork.libcamera.org/patch/17759/


+
>  int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice
> *isp, MediaEntity *sensorEntity)
>  {
>         std::unique_ptr<RPiCameraData> data =
> std::make_unique<RPiCameraData>(this);
> --
> 2.30.2
>
>

Hi Naush

Thanks for the review.

On Fri, 11 Nov 2022 at 14:19, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for fixing this!
>
>
> On Fri, 11 Nov 2022 at 13:30, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
>>
>> Implement the PipelineHandlerRPi::releaseDevice method which allows
>> us to free any allocated buffers when a camera is released.
>>
>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>> ---
>>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index f15fa28b..785eddf9 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -337,6 +337,8 @@ public:
>>
>>         bool match(DeviceEnumerator *enumerator) override;
>>
>> +       void releaseDevice(Camera *camera) override;
>> +
>>  private:
>>         RPiCameraData *cameraData(Camera *camera)
>>         {
>> @@ -1193,6 +1195,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>>         return !!numCameras;
>>  }
>>
>> +void PipelineHandlerRPi::releaseDevice(Camera *camera)
>> +{
>> +       RPiCameraData *data = cameraData(camera);
>> +       data->freeBuffers();
>> +}
>
>
> My head's spinning with all these related changes now :-)
>
> Presumably for this to work correctly, RPiCameraData::freeBuffers() must not do the
> if (!buffersAllocated_) test, correct? So this change relies on [1] to behave correctly?

Well, I think this is only a problem when the camera was started (and
stopped and released), otherwise we never allocate buffers in the
first place. So I think it's good even regardless of the
buffersAllocated_ test. But yeah, my head is spinning too. Just merge
everything and it'll be fine!!

David

>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
> [1] https://patchwork.libcamera.org/patch/17759/
>
>
>> +
>>  int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity)
>>  {
>>         std::unique_ptr<RPiCameraData> data = std::make_unique<RPiCameraData>(this);
>> --
>> 2.30.2
>>