[libcamera-devel] [PATCH 1/3] libcamera: raspberrypi: dma_heaps: Add open/close

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Aug 27, 2020 at 10:20:36AM +0200, Jacopo Mondi wrote:
> The dma_heaps device is opened in the class constructor, making
> it impossible (or rather ugly) to catch errors before the allocator gets
> actually used.
> 
> Provide an open() and a close() method to allow users to catch errors
> earlier.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  .../pipeline/raspberrypi/dma_heaps.cpp        | 22 ++++++++++++++++---
>  .../pipeline/raspberrypi/dma_heaps.h          |  2 ++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> index 6769c04640f1..739f05d3d4d8 100644
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI)
>  namespace RPi {
>  
>  DmaHeap::DmaHeap()
> +	: dmaHeapHandle_(-1)
> +{
> +}
> +
> +DmaHeap::~DmaHeap()
> +{
> +	close();
> +}
> +
> +int DmaHeap::open()
>  {
>  	dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);
>  	if (dmaHeapHandle_ == -1) {
>  		dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);
>  		if (dmaHeapHandle_ == -1) {
>  			LOG(RPI, Error) << "Could not open dmaHeap device";
> +			return dmaHeapHandle_;
>  		}
>  	}
> +
> +	return 0;
>  }

Doesn't this break bisection ?

Please also note that you could alternatively add a bool isValid() const
(or similar) function to query whether the open succeeded. I think that
would be less intrusive in this case. Otherwise you should guard about
double-open for instance.

>  
> -DmaHeap::~DmaHeap()
> +void DmaHeap::close()
>  {
> -	if (dmaHeapHandle_)
> -		::close(dmaHeapHandle_);
> +	if (dmaHeapHandle_ < 0)
> +		return;
> +
> +	::close(dmaHeapHandle_);
> +	dmaHeapHandle_ = -1;
>  }
>  
>  FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)
> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h
> index ae6be1135f17..3e17993097ef 100644
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h
> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h
> @@ -18,6 +18,8 @@ class DmaHeap
>  public:
>  	DmaHeap();
>  	~DmaHeap();
> +	int open();
> +	void close();
>  	FileDescriptor alloc(const char *name, std::size_t size);
>  
>  private:

-- 
Regards,

Laurent Pinchart