[libcamera-devel] [PATCH 1/3] libcamera: raspberrypi: dma_heaps: Add open/close

[Laurent Pinchart]

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 at jmondi.org>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at 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:

-- 
Regards,

Laurent Pinchart