| Message ID | 20200828155136.541696-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Aug 28, 2020 at 05:51:34PM +0200, Jacopo Mondi wrote: > Improve the device opening at class construction time by testing all > the available device paths and printout the appropriate error message. > > While at it, initialize dmaHeapHandle_ to -1 and check for it's value s/it's/its/ > at destruction time. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../pipeline/raspberrypi/dma_heaps.cpp | 28 +++++++++++++------ > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > index 6769c04640f1..4d5dd6cb87cc 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > @@ -7,6 +7,7 @@ > > #include "dma_heaps.h" > > +#include <array> > #include <fcntl.h> > #include <linux/dma-buf.h> > #include <linux/dma-heap.h> > @@ -22,8 +23,10 @@ > * Annoyingly, should the cma heap size be specified on the kernel command line > * instead of DT, the heap gets named "reserved" instead. > */ > -#define DMA_HEAP_CMA_NAME "/dev/dma_heap/linux,cma" > -#define DMA_HEAP_CMA_ALT_NAME "/dev/dma_heap/reserved" > +static constexpr std::array<const char *, 2> heapNames = { > + "/dev/dma_heap/linux,cma", > + "/dev/dma_heap/reserved" > +}; > > namespace libcamera { > > @@ -32,19 +35,28 @@ LOG_DECLARE_CATEGORY(RPI) > namespace RPi { > > DmaHeap::DmaHeap() > + : dmaHeapHandle_(-1) > { > - dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0); > - if (dmaHeapHandle_ == -1) { > - dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0); > - if (dmaHeapHandle_ == -1) { > - LOG(RPI, Error) << "Could not open dmaHeap device"; > + for (const char *name : heapNames) { > + int ret = ::open(name, O_RDWR, 0); I wonder if we should set O_CLOEXEC. That's out of scope for this patch anyway. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + if (ret < 0) { > + ret = errno; > + LOG(RPI, Debug) << "Failed to open " << name << ": " > + << strerror(ret); > + continue; > } > + > + dmaHeapHandle_ = ret; > + break; > } > + > + if (dmaHeapHandle_ < 0) > + LOG(RPI, Error) << "Could not open any dmaHeap device"; > } > > DmaHeap::~DmaHeap() > { > - if (dmaHeapHandle_) > + if (dmaHeapHandle_ > -1) > ::close(dmaHeapHandle_); > }
Hi Jacopo, On 28/08/2020 16:51, Jacopo Mondi wrote: > Improve the device opening at class construction time by testing all > the available device paths and printout the appropriate error message. > > While at it, initialize dmaHeapHandle_ to -1 and check for it's value > at destruction time. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/dma_heaps.cpp | 28 +++++++++++++------ > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > index 6769c04640f1..4d5dd6cb87cc 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > @@ -7,6 +7,7 @@ > > #include "dma_heaps.h" > > +#include <array> > #include <fcntl.h> > #include <linux/dma-buf.h> > #include <linux/dma-heap.h> > @@ -22,8 +23,10 @@ > * Annoyingly, should the cma heap size be specified on the kernel command line > * instead of DT, the heap gets named "reserved" instead. > */ > -#define DMA_HEAP_CMA_NAME "/dev/dma_heap/linux,cma" > -#define DMA_HEAP_CMA_ALT_NAME "/dev/dma_heap/reserved" > +static constexpr std::array<const char *, 2> heapNames = { > + "/dev/dma_heap/linux,cma", > + "/dev/dma_heap/reserved" > +}; > > namespace libcamera { > > @@ -32,19 +35,28 @@ LOG_DECLARE_CATEGORY(RPI) > namespace RPi { > > DmaHeap::DmaHeap() > + : dmaHeapHandle_(-1) > { > - dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0); > - if (dmaHeapHandle_ == -1) { > - dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0); > - if (dmaHeapHandle_ == -1) { > - LOG(RPI, Error) << "Could not open dmaHeap device"; > + for (const char *name : heapNames) { > + int ret = ::open(name, O_RDWR, 0); > + if (ret < 0) { > + ret = errno; > + LOG(RPI, Debug) << "Failed to open " << name << ": " > + << strerror(ret); > + continue; > } > + > + dmaHeapHandle_ = ret; > + break; > } > + > + if (dmaHeapHandle_ < 0) > + LOG(RPI, Error) << "Could not open any dmaHeap device"; > } > > DmaHeap::~DmaHeap() > { > - if (dmaHeapHandle_) > + if (dmaHeapHandle_ > -1) > ::close(dmaHeapHandle_); > } > >
diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp index 6769c04640f1..4d5dd6cb87cc 100644 --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp @@ -7,6 +7,7 @@ #include "dma_heaps.h" +#include <array> #include <fcntl.h> #include <linux/dma-buf.h> #include <linux/dma-heap.h> @@ -22,8 +23,10 @@ * Annoyingly, should the cma heap size be specified on the kernel command line * instead of DT, the heap gets named "reserved" instead. */ -#define DMA_HEAP_CMA_NAME "/dev/dma_heap/linux,cma" -#define DMA_HEAP_CMA_ALT_NAME "/dev/dma_heap/reserved" +static constexpr std::array<const char *, 2> heapNames = { + "/dev/dma_heap/linux,cma", + "/dev/dma_heap/reserved" +}; namespace libcamera { @@ -32,19 +35,28 @@ LOG_DECLARE_CATEGORY(RPI) namespace RPi { DmaHeap::DmaHeap() + : dmaHeapHandle_(-1) { - dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0); - if (dmaHeapHandle_ == -1) { - dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0); - if (dmaHeapHandle_ == -1) { - LOG(RPI, Error) << "Could not open dmaHeap device"; + for (const char *name : heapNames) { + int ret = ::open(name, O_RDWR, 0); + if (ret < 0) { + ret = errno; + LOG(RPI, Debug) << "Failed to open " << name << ": " + << strerror(ret); + continue; } + + dmaHeapHandle_ = ret; + break; } + + if (dmaHeapHandle_ < 0) + LOG(RPI, Error) << "Could not open any dmaHeap device"; } DmaHeap::~DmaHeap() { - if (dmaHeapHandle_) + if (dmaHeapHandle_ > -1) ::close(dmaHeapHandle_); }
Improve the device opening at class construction time by testing all the available device paths and printout the appropriate error message. While at it, initialize dmaHeapHandle_ to -1 and check for it's value at destruction time. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../pipeline/raspberrypi/dma_heaps.cpp | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-)