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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Aug 27, 2020 at 01:59:45PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 27, 2020 at 12:45:17PM +0100, Kieran Bingham wrote:
> > On 27/08/2020 12:46, Jacopo Mondi wrote:
> > > On Thu, Aug 27, 2020 at 12:21:24PM +0100, Kieran Bingham wrote:
> > >> 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.

Agreed. Let's avoid printing any non-debug message for the case where we
fall back to the alternative name. An error message at the end to report
that no heap could be found should be enough.

> I'll re-work this one!
> 
> > >>
> > >>>  		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.

-- 
Regards,

Laurent Pinchart