[libcamera-devel] [PATCH 1/2] android: CameraBuffer: Add option of not mapping a buffer

[Laurent Pinchart]

Hello,

On Tue, Apr 06, 2021 at 02:47:34PM +0200, Jacopo Mondi wrote:
> On Mon, Apr 05, 2021 at 01:04:23PM +0900, Hirokazu Honda wrote:
> > CameraBuffer always maps a buffer in constructor. This adds an
> > option to not map a buffer within CameraBuffer. The option is
> > useful for a caller to only validate a buffer and not have to
> > map the buffer.
> 
> Although this requires CameraDevice to create a CameraBuffer in
> processCaptureRequest() only to validate. Would a static method work
> better ? After all if the validation shall happen before mapping, it
> means it is based on static attribute of buffer_handle_t
> 
> Also, I don't really like the 'map' flag, as it seems to me if only
> construction is required, but not mapping, we should rather split the
> two operations in two methods (ie adding an explicit
> CameraBuffer::map() method)

I agree, I think that the constructor should not map the buffer, this
should instead be done in plane() the first time it's called.

> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/android/camera_buffer.h              |  7 ++++---
> >  src/android/mm/cros_camera_buffer.cpp    | 17 +++++++++++++----
> >  src/android/mm/generic_camera_buffer.cpp | 20 ++++++++++++--------
> >  3 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > index 7e8970b4..1fdb76a6 100644
> > --- a/src/android/camera_buffer.h
> > +++ b/src/android/camera_buffer.h
> > @@ -17,7 +17,7 @@ class CameraBuffer final : public libcamera::Extensible
> >  	LIBCAMERA_DECLARE_PRIVATE(CameraBuffer)
> >
> >  public:
> > -	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> > +	CameraBuffer(buffer_handle_t camera3Buffer, int flags, bool map = true);
> >  	~CameraBuffer();
> >
> >  	bool isValid() const;
> > @@ -31,8 +31,9 @@ public:
> >  };
> >
> >  #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION				\
> > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
> > -	: Extensible(new Private(this, camera3Buffer, flags))		\
> > +	CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags, \
> > +				   bool map)				\
> > +		: Extensible(new Private(this, camera3Buffer, flags, map))\
> >  {									\
> >  }									\
> >  CameraBuffer::~CameraBuffer()						\
> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > index 1a4fd5d1..71e03766 100644
> > --- a/src/android/mm/cros_camera_buffer.cpp
> > +++ b/src/android/mm/cros_camera_buffer.cpp
> > @@ -21,7 +21,7 @@ class CameraBuffer::Private : public Extensible::Private
> >
> >  public:
> >  	Private(CameraBuffer *cameraBuffer,
> > -		buffer_handle_t camera3Buffer, int flags);
> > +		buffer_handle_t camera3Buffer, int flags, bool map);
> >  	~Private();
> >
> >  	bool isValid() const { return valid_; }
> > @@ -38,6 +38,7 @@ private:
> >  	unsigned int numPlanes_;
> >  	bool valid_;
> >  	bool registered_;
> > +	bool map_;
> >  	union {
> >  		void *addr;
> >  		android_ycbcr ycbcr;
> > @@ -48,9 +49,10 @@ private:
> >  };
> >
> >  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> > -			       buffer_handle_t camera3Buffer, int flags)
> > +			       buffer_handle_t camera3Buffer, int flags,
> > +			       bool map)
> >  	: Extensible::Private(cameraBuffer), handle_(camera3Buffer),
> > -	  numPlanes_(0), valid_(false), registered_(false)
> > +	  numPlanes_(0), valid_(false), registered_(false), map_(map)
> >  {
> >  	bufferManager_ = cros::CameraBufferManager::GetInstance();
> >
> > @@ -62,6 +64,13 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> >
> >  	registered_ = true;
> >  	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> > +
> > +	memset(&mem, 0, sizeof(mem));
> > +	if (!map_) {
> > +		valid_ = true;
> > +		return;
> > +	}
> > +
> >  	switch (numPlanes_) {
> >  	case 1: {
> >  		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > @@ -91,7 +100,7 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> >
> >  CameraBuffer::Private::~Private()
> >  {
> > -	if (valid_)
> > +	if (map_ && valid_)
> >  		bufferManager_->Unlock(handle_);
> >  	if (registered_)
> >  		bufferManager_->Deregister(handle_);
> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > index 929e078a..8fa79889 100644
> > --- a/src/android/mm/generic_camera_buffer.cpp
> > +++ b/src/android/mm/generic_camera_buffer.cpp
> > @@ -21,7 +21,7 @@ class CameraBuffer::Private : public Extensible::Private,
> >
> >  public:
> >  	Private(CameraBuffer *cameraBuffer,
> > -		buffer_handle_t camera3Buffer, int flags);
> > +		buffer_handle_t camera3Buffer, int flags, bool map);
> >  	~Private();
> >
> >  	unsigned int numPlanes() const;
> > @@ -32,7 +32,8 @@ public:
> >  };
> >
> >  CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> > -			       buffer_handle_t camera3Buffer, int flags)
> > +			       buffer_handle_t camera3Buffer,
> > +			       int flags, bool map)
> >  	: Extensible::Private(cameraBuffer)
> >  {
> >  	maps_.reserve(camera3Buffer->numFds);
> > @@ -49,12 +50,15 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,
> >  			break;
> >  		}
> >
> > -		void *address = mmap(nullptr, length, flags, MAP_SHARED,
> > -				     camera3Buffer->data[i], 0);
> > -		if (address == MAP_FAILED) {
> > -			error_ = -errno;
> > -			LOG(HAL, Error) << "Failed to mmap plane";
> > -			break;
> > +		void *address = nullptr;
> > +		if (map) {
> > +			mmap(nullptr, length, flags, MAP_SHARED,
> > +			     camera3Buffer->data[i], 0);
> > +			if (address == MAP_FAILED) {
> > +				error_ = -errno;
> > +				LOG(HAL, Error) << "Failed to mmap plane";
> > +				break;
> > +			}
> >  		}
> >
> >  		maps_.emplace_back(static_cast<uint8_t *>(address),

-- 
Regards,

Laurent Pinchart