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

[Kieran Bingham]

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.



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