[libcamera-devel] [PATCH 2/3] libcamera: raspberrypi: dma_heaps: Be verbose on errors

[Jacopo Mondi]

On Thu, Aug 27, 2020 at 12:21:24PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 27/08/2020 09:20, Jacopo Mondi wrote:
> > Be a tad more verbose on failures in opening the dma_heaps allocator
> > devices.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > index 739f05d3d4d8..500f1eac2eb8 100644
> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > @@ -45,9 +45,14 @@ int DmaHeap::open()
> >  {
> >  	dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);
> >  	if (dmaHeapHandle_ == -1) {
> > +		LOG(RPI, Error) << "Could not open dmaHeap device "
> > +				<< DMA_HEAP_CMA_NAME << ": "
> > +				<< strerror(errno);
>
> This will report an error, even if the ALT_NAME is successful...
>

True, but I think that's ok as the DMA_HEAP_CMA_ALT_NAME device is
just a fallback and there shouldn't be a need to poke it

>
> >  		dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);
> >  		if (dmaHeapHandle_ == -1) {
> > -			LOG(RPI, Error) << "Could not open dmaHeap device";
> > +			LOG(RPI, Error) << "Could not open dmaHeap device "
> > +					<< DMA_HEAP_CMA_ALT_NAME << ": "
> > +					<< strerror(errno);
> >  			return dmaHeapHandle_;
> >  		}
> >  	}
> >
>
> I wonder if this is a good opportunity to refactor this code:
>
> (untested/uncompiled psuedo code to follow)
>
>
> std::array<const char *, 2> heap_names {
> 	"/dev/dma_heap/linux,cma",
> 	"/dev/dma_heap/reserved",
> }
>
> int DmaHeap::open()
> {
> 	for (const char *heap : heap_names) {
> 		if (!libcamera::File::exists(heap))
> 			continue;
>
> 		dmaHeapHandle_ = ::open(heap, O_RDWR, 0);
>
> 		if (dmaHeapHandle_ != -1) /* Success! */
> 			return 0;
>
> 		LOG(RPI, Warning)
> 			<< "Could not open dmaHeap "
> 			<< heap << ": "	<< strerror(errno);
> 	}
>
> 	/* No DMA Heap was available */
> 	LOG(RPI, Error) << "Could not open any dmaHeap device";
>
> 	return -ENODEV;


We could work on something similar indeed if erroring on the first
failure is not welcome.

Thanks
  j

> }
>
> --
> Regards
> --
> Kieran