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

[Jacopo Mondi]

Hi Kieran,

On Thu, Aug 27, 2020 at 12:45:17PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 27/08/2020 12:46, Jacopo Mondi wrote:
> > 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
>
> Oh, that's not how I interpret the comment up at the #defines.
>
> It says if the cma heap size is specified on the kernel command line,
> then the heap gets named as reserved instead.
>
> That therefore implies to me that with your patch, when the user
> specifies a cma heap size on the kernel, they will always be presented
> with an error - in a situation which is not an error if I understand it
> correctly.
>

 * Annoyingly, should the cma heap size be specified on the kernel command line
 * instead of DT, the heap gets named "reserved" instead.

You're right indeed, even if that's just a printout, it should be
avoided.

I'll re-work this one!

Thanks
   j

>
>
> >>
> >>>  		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
>
> --
> Regards
> --
> Kieran