[libcamera-devel] [PATCH v2 1/3] libcamera: raspberrypi: dmaheaps: Improve device open

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Aug 28, 2020 at 05:51:34PM +0200, Jacopo Mondi wrote:
> Improve the device opening at class construction time by testing all
> the available device paths and printout the appropriate error message.
> 
> While at it, initialize dmaHeapHandle_ to -1 and check for it's value

s/it's/its/

> at destruction time.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  .../pipeline/raspberrypi/dma_heaps.cpp        | 28 +++++++++++++------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> index 6769c04640f1..4d5dd6cb87cc 100644
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> @@ -7,6 +7,7 @@
>  
>  #include "dma_heaps.h"
>  
> +#include <array>
>  #include <fcntl.h>
>  #include <linux/dma-buf.h>
>  #include <linux/dma-heap.h>
> @@ -22,8 +23,10 @@
>   * Annoyingly, should the cma heap size be specified on the kernel command line
>   * instead of DT, the heap gets named "reserved" instead.
>   */
> -#define DMA_HEAP_CMA_NAME "/dev/dma_heap/linux,cma"
> -#define DMA_HEAP_CMA_ALT_NAME "/dev/dma_heap/reserved"
> +static constexpr std::array<const char *, 2> heapNames = {
> +	"/dev/dma_heap/linux,cma",
> +	"/dev/dma_heap/reserved"
> +};
>  
>  namespace libcamera {
>  
> @@ -32,19 +35,28 @@ LOG_DECLARE_CATEGORY(RPI)
>  namespace RPi {
>  
>  DmaHeap::DmaHeap()
> +	: dmaHeapHandle_(-1)
>  {
> -	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";
> +	for (const char *name : heapNames) {
> +		int ret = ::open(name, O_RDWR, 0);

I wonder if we should set O_CLOEXEC. That's out of scope for this patch
anyway.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		if (ret < 0) {
> +			ret = errno;
> +			LOG(RPI, Debug) << "Failed to open " << name << ": "
> +					<< strerror(ret);
> +			continue;
>  		}
> +
> +		dmaHeapHandle_ = ret;
> +		break;
>  	}
> +
> +	if (dmaHeapHandle_ < 0)
> +		LOG(RPI, Error) << "Could not open any dmaHeap device";
>  }
>  
>  DmaHeap::~DmaHeap()
>  {
> -	if (dmaHeapHandle_)
> +	if (dmaHeapHandle_ > -1)
>  		::close(dmaHeapHandle_);
>  }

-- 
Regards,

Laurent Pinchart