| Message ID | 20200827082038.40758-3-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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@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... > 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; }
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@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
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@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
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@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
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@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.
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); 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_; } }
Be a tad more verbose on failures in opening the dma_heaps allocator devices. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/raspberrypi/dma_heaps.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)