android: camera_buffer: Map buffer in the first plane() call

CameraBuffer implementation maps a given buffer_handle_t in
constructor. Mapping is redundant to only know the plane info like
stride and offset. Mapping should be executed later in the first
plane() call.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

src/android/mm/cros_camera_buffer.cpp

@@ -25,7 +25,7 @@ public:
 		int flags);
 	~Private();
 
-	bool isValid() const { return valid_; }
+	bool isValid() const { return registered_; }
 
 	unsigned int numPlanes() const;
 
@@ -34,10 +34,12 @@ public:
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 
 private:
+	void map();
+
 	cros::CameraBufferManager *bufferManager_;
 	buffer_handle_t handle_;
 	unsigned int numPlanes_;
-	bool valid_;
+	bool mapped_;
 	bool registered_;
 	union {
 		void *addr;
@@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
 			       [[maybe_unused]] const libcamera::Size &size,
 			       [[maybe_unused]] int flags)
-	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
+	: handle_(camera3Buffer), numPlanes_(0), mapped_(false),
 	  registered_(false)
 {
 	bufferManager_ = cros::CameraBufferManager::GetInstance();
@@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 
 	registered_ = true;
 	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
-	switch (numPlanes_) {
-	case 1: {
-		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
-		if (ret) {
-			LOG(HAL, Error) << "Single plane buffer mapping failed";
-			return;
-		}
-		break;
-	}
-	case 2:
-	case 3: {
-		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
-						&mem.ycbcr);
-		if (ret) {
-			LOG(HAL, Error) << "YCbCr buffer mapping failed";
-			return;
-		}
-		break;
-	}
-	default:
-		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
-		return;
-	}
-
-	valid_ = true;
 }
 
 CameraBuffer::Private::~Private()
 {
-	if (valid_)
+	if (mapped_)
 		bufferManager_->Unlock(handle_);
 	if (registered_)
 		bufferManager_->Deregister(handle_);
@@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
 
 Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 {
+	if (!mapped_)
+		map();
+	if (!mapped_)
+		return {};
+
 	void *addr;
 
 	switch (numPlanes()) {
@@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
 	return bufferManager_->GetPlaneSize(handle_, 0);
 }
 
+void CameraBuffer::Private::map()
+{
+	int ret;
+	switch (numPlanes_) {
+	case 1: {
+		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
+		if (ret) {
+			LOG(HAL, Error) << "Single plane buffer mapping failed";
+			return;
+		}
+		break;
+	}
+	case 2:
+	case 3: {
+		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
+						&mem.ycbcr);
+		if (ret) {
+			LOG(HAL, Error) << "YCbCr buffer mapping failed";
+			return;
+		}
+		break;
+	}
+	default:
+		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
+		return;
+	}
+
+	mapped_ = true;
+	return;
+}
+
 PUBLIC_CAMERA_BUFFER_IMPLEMENTATION

src/android/mm/generic_camera_buffer.cpp

@@ -37,6 +37,18 @@ public:
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 
 private:
+	struct PlaneInfo {
+		unsigned int offset;
+		unsigned int size;
+	};
+
+	void map();
+
+	int fd_;
+	int flags_;
+	off_t bufferLength_;
+	bool mapped_;
+	std::vector<PlaneInfo> planeInfo_;
 	/* \todo Remove planes_ when it will be added to MappedBuffer */
 	std::vector<Span<uint8_t>> planes_;
 };
@@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 			       buffer_handle_t camera3Buffer,
 			       libcamera::PixelFormat pixelFormat,
 			       const libcamera::Size &size, int flags)
+	: fd_(-1), flags_(flags), bufferLength_(0), mapped_(false)
 {
 	error_ = 0;
 
@@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 	 * now that the buffer is backed by a single dmabuf, with planes being
 	 * stored contiguously.
 	 */
-	int fd = -1;
 	for (int i = 0; i < camera3Buffer->numFds; i++) {
-		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
+		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
 			continue;
 
-		if (fd != -1) {
+		if (fd_ != -1) {
 			error_ = -EINVAL;
 			LOG(HAL, Error) << "Discontiguous planes are not supported";
 			return;
 		}
 
-		fd = camera3Buffer->data[i];
+		fd_ = camera3Buffer->data[i];
 	}
 
-	if (fd == -1) {
+	if (fd_ == -1) {
 		error_ = -EINVAL;
 		LOG(HAL, Error) << "No valid file descriptor";
 		return;
 	}
 
-	off_t bufferLength = lseek(fd, 0, SEEK_END);
+	bufferLength_ = lseek(fd_, 0, SEEK_END);
-	if (bufferLength < 0) {
+	if (bufferLength_ < 0) {
 		error_ = -errno;
 		LOG(HAL, Error) << "Failed to get buffer length";
 		return;
 	}
 
-	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
-	if (address == MAP_FAILED) {
-		error_ = -errno;
-		LOG(HAL, Error) << "Failed to mmap plane";
-		return;
-	}
-	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
-
 	const unsigned int numPlanes = info.numPlanes();
-	planes_.resize(numPlanes);
+	planeInfo_.resize(numPlanes);
+
 	unsigned int offset = 0;
 	for (unsigned int i = 0; i < numPlanes; ++i) {
 		/*
@@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 		const unsigned int planeSize =
 			stride * ((size.height + vertSubSample - 1) / vertSubSample);
 
-		planes_[i] = libcamera::Span<uint8_t>(
+		planeInfo_[i].offset = offset;
-			static_cast<uint8_t *>(address) + offset, planeSize);
+		planeInfo_[i].size = planeSize;
 
-		if (bufferLength < offset + planeSize) {
+		if (bufferLength_ < offset + planeSize) {
-			error_ = -EINVAL;
+			LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
-			LOG(HAL, Error) << "Plane " << i << " is out of buffer"
+					<< " plane offset=" << offset
-					<< ", buffer length=" << bufferLength
+					<< ", plane size=" << planeSize
-					<< ", offset=" << offset
+					<< ", buffer length=" << bufferLength_;
-					<< ", size=" << planeSize;
 			return;
 		}
+
 		offset += planeSize;
 	}
 }
@@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()
 
 unsigned int CameraBuffer::Private::numPlanes() const
 {
-	return planes_.size();
+	return planeInfo_.size();
 }
 
 Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 {
-	if (plane >= planes_.size())
+	if (!mapped_)
+		map();
+	if (!mapped_)
 		return {};
 
 	return planes_[plane];
@@ -143,8 +150,31 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 
 size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
 {
-	return std::min<unsigned int>(maps_[0].size(),
+	ASSERT(bufferLength_ >= 0);
-				      maxJpegBufferSize);
+
+	return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
+}
+
+void CameraBuffer::Private::map()
+{
+	ASSERT(fd_ != -1);
+	ASSERT(bufferLength_ >= 0);
+
+	void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
+	if (address == MAP_FAILED) {
+		error_ = -errno;
+		LOG(HAL, Error) << "Failed to mmap plane";
+		return;
+	}
+	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
+
+	planes_.reserve(planeInfo_.size());
+	for (const auto &info : planeInfo_) {
+		planes_.emplace_back(
+			static_cast<uint8_t *>(address) + info.offset, info.size);
+	}
+
+	mapped_ = true;
 }
 
 PUBLIC_CAMERA_BUFFER_IMPLEMENTATION