| Message ID | 20200827082038.40758-2-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, I've got my eye on this class, and thinking we should try to move it to core libcamera too, as I already want to be able to allocate intermediate buffers for the Android HAL. But that's separate, so improving the usage in this series seems like a good thing to do: On 27/08/2020 09:20, Jacopo Mondi wrote: > The dma_heaps device is opened in the class constructor, making > it impossible (or rather ugly) to catch errors before the allocator gets > actually used. > > Provide an open() and a close() method to allow users to catch errors > earlier. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/dma_heaps.cpp | 22 ++++++++++++++++--- > .../pipeline/raspberrypi/dma_heaps.h | 2 ++ > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > index 6769c04640f1..739f05d3d4d8 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI) > namespace RPi { > > DmaHeap::DmaHeap() > + : dmaHeapHandle_(-1) > +{ > +} > + > +DmaHeap::~DmaHeap() > +{ > + close(); > +} > + > +int DmaHeap::open() > { > 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"; > + return dmaHeapHandle_; > } > } > + > + return 0; > } > > -DmaHeap::~DmaHeap() > +void DmaHeap::close() > { > - if (dmaHeapHandle_) > - ::close(dmaHeapHandle_); > + if (dmaHeapHandle_ < 0) > + return; > + > + ::close(dmaHeapHandle_); > + dmaHeapHandle_ = -1; > } > > FileDescriptor DmaHeap::alloc(const char *name, std::size_t size) > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > index ae6be1135f17..3e17993097ef 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > @@ -18,6 +18,8 @@ class DmaHeap > public: > DmaHeap(); > ~DmaHeap(); > + int open(); > + void close(); > FileDescriptor alloc(const char *name, std::size_t size); > > private: >
Hi Jacopo, Thank you for the patch. On Thu, Aug 27, 2020 at 10:20:36AM +0200, Jacopo Mondi wrote: > The dma_heaps device is opened in the class constructor, making > it impossible (or rather ugly) to catch errors before the allocator gets > actually used. > > Provide an open() and a close() method to allow users to catch errors > earlier. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../pipeline/raspberrypi/dma_heaps.cpp | 22 ++++++++++++++++--- > .../pipeline/raspberrypi/dma_heaps.h | 2 ++ > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > index 6769c04640f1..739f05d3d4d8 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI) > namespace RPi { > > DmaHeap::DmaHeap() > + : dmaHeapHandle_(-1) > +{ > +} > + > +DmaHeap::~DmaHeap() > +{ > + close(); > +} > + > +int DmaHeap::open() > { > 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"; > + return dmaHeapHandle_; > } > } > + > + return 0; > } Doesn't this break bisection ? Please also note that you could alternatively add a bool isValid() const (or similar) function to query whether the open succeeded. I think that would be less intrusive in this case. Otherwise you should guard about double-open for instance. > > -DmaHeap::~DmaHeap() > +void DmaHeap::close() > { > - if (dmaHeapHandle_) > - ::close(dmaHeapHandle_); > + if (dmaHeapHandle_ < 0) > + return; > + > + ::close(dmaHeapHandle_); > + dmaHeapHandle_ = -1; > } > > FileDescriptor DmaHeap::alloc(const char *name, std::size_t size) > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > index ae6be1135f17..3e17993097ef 100644 > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > @@ -18,6 +18,8 @@ class DmaHeap > public: > DmaHeap(); > ~DmaHeap(); > + int open(); > + void close(); > FileDescriptor alloc(const char *name, std::size_t size); > > private:
Hi Kieran, On Thu, Aug 27, 2020 at 12:02:19PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > I've got my eye on this class, and thinking we should try to move it to > core libcamera too, as I already want to be able to allocate > intermediate buffers for the Android HAL. Fully agreed, I think this will graduate to a core helper once we need it in other places. What kind of intermediate buffers do you need to allocate for the HAL though ? Buffers for YUV frames when all that the camera service requests is JPEG ? That's something we probably want to handle with a FramebufferAllocator. Down the line FramebufferAllocator should gain dmabuf heaps support, but that will require more work (in particular given that the dmabuf heaps API is still fairly limited in mainline, without support to handle device-specific constraints). > But that's separate, so improving the usage in this series seems like a > good thing to do: > > On 27/08/2020 09:20, Jacopo Mondi wrote: > > The dma_heaps device is opened in the class constructor, making > > it impossible (or rather ugly) to catch errors before the allocator gets > > actually used. > > > > Provide an open() and a close() method to allow users to catch errors > > earlier. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > .../pipeline/raspberrypi/dma_heaps.cpp | 22 ++++++++++++++++--- > > .../pipeline/raspberrypi/dma_heaps.h | 2 ++ > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > > index 6769c04640f1..739f05d3d4d8 100644 > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp > > @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI) > > namespace RPi { > > > > DmaHeap::DmaHeap() > > + : dmaHeapHandle_(-1) > > +{ > > +} > > + > > +DmaHeap::~DmaHeap() > > +{ > > + close(); > > +} > > + > > +int DmaHeap::open() > > { > > 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"; > > + return dmaHeapHandle_; > > } > > } > > + > > + return 0; > > } > > > > -DmaHeap::~DmaHeap() > > +void DmaHeap::close() > > { > > - if (dmaHeapHandle_) > > - ::close(dmaHeapHandle_); > > + if (dmaHeapHandle_ < 0) > > + return; > > + > > + ::close(dmaHeapHandle_); > > + dmaHeapHandle_ = -1; > > } > > > > FileDescriptor DmaHeap::alloc(const char *name, std::size_t size) > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > > index ae6be1135f17..3e17993097ef 100644 > > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h > > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h > > @@ -18,6 +18,8 @@ class DmaHeap > > public: > > DmaHeap(); > > ~DmaHeap(); > > + int open(); > > + void close(); > > FileDescriptor alloc(const char *name, std::size_t size); > > > > private:
diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp index 6769c04640f1..739f05d3d4d8 100644 --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI) namespace RPi { DmaHeap::DmaHeap() + : dmaHeapHandle_(-1) +{ +} + +DmaHeap::~DmaHeap() +{ + close(); +} + +int DmaHeap::open() { 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"; + return dmaHeapHandle_; } } + + return 0; } -DmaHeap::~DmaHeap() +void DmaHeap::close() { - if (dmaHeapHandle_) - ::close(dmaHeapHandle_); + if (dmaHeapHandle_ < 0) + return; + + ::close(dmaHeapHandle_); + dmaHeapHandle_ = -1; } FileDescriptor DmaHeap::alloc(const char *name, std::size_t size) diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h index ae6be1135f17..3e17993097ef 100644 --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h @@ -18,6 +18,8 @@ class DmaHeap public: DmaHeap(); ~DmaHeap(); + int open(); + void close(); FileDescriptor alloc(const char *name, std::size_t size); private:
The dma_heaps device is opened in the class constructor, making it impossible (or rather ugly) to catch errors before the allocator gets actually used. Provide an open() and a close() method to allow users to catch errors earlier. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- .../pipeline/raspberrypi/dma_heaps.cpp | 22 ++++++++++++++++--- .../pipeline/raspberrypi/dma_heaps.h | 2 ++ 2 files changed, 21 insertions(+), 3 deletions(-)