[libcamera-devel,11/12] android: Introduce cros_cbm_buffer

11404 diff mbox series

Introduce the CameraBuffer backend for the ChromeOS operating system.

The cros_cbm CameraBuffer implementation uses the ChromeOS
CameraBufferManager class to perform mapping of 1 plane and multiplane
buffers.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/mm/cros_cbm.cpp | 167 ++++++++++++++++++++++++++++++++++++
 src/android/mm/meson.build  |   3 +
 2 files changed, 170 insertions(+)
 create mode 100644 src/android/mm/cros_cbm.cpp

Hi Jacopo,

Thank you for the patch.

On Fri, Feb 26, 2021 at 02:29:31PM +0100, Jacopo Mondi wrote:
> Introduce the CameraBuffer backend for the ChromeOS operating system.
> 
> The cros_cbm CameraBuffer implementation uses the ChromeOS
> CameraBufferManager class to perform mapping of 1 plane and multiplane
> buffers.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/mm/cros_cbm.cpp | 167 ++++++++++++++++++++++++++++++++++++
>  src/android/mm/meson.build  |   3 +
>  2 files changed, 170 insertions(+)
>  create mode 100644 src/android/mm/cros_cbm.cpp
> 
> diff --git a/src/android/mm/cros_cbm.cpp b/src/android/mm/cros_cbm.cpp
> new file mode 100644
> index 000000000000..6c931c99d2d0
> --- /dev/null
> +++ b/src/android/mm/cros_cbm.cpp
> @@ -0,0 +1,167 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * cros_cbm.cpp - ChromiumOS libcbm frame buffer backend

s/ChromiumOS/Chromium OS/

cbm stands for CameraBufferManager, which you use below. Maybe there
used to be a cbm library, but the class is now part of libcros_camera.
There's still libgbm, upon which the cross::CameraBufferManager is
implemented, but that's an implementation detail. I'd thus write

 * cros_cbm.cpp - Chromium OS CameraBufferManager frame buffer backend

or maybe

 * cros_cbm.cpp - Chromium OS frame buffer backend using CameraBufferManager

> + */
> +
> +#include "../camera_buffer.h"
> +
> +#include "libcamera/internal/log.h"
> +
> +#include "cros-camera/camera_buffer_manager.h"
> +
> +using namespace libcamera;
> +
> +LOG_DECLARE_CATEGORY(HAL)
> +
> +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer
> +{
> +public:
> +	CameraBufferImpl(buffer_handle_t camera3Buffer, int flags);
> +	~CameraBufferImpl();
> +
> +	bool isValid() const { return valid_; }
> +
> +	unsigned int numPlanes() const;
> +	ssize_t planeSize(unsigned int plane) const;
> +
> +	const uint8_t *plane(unsigned int plane) const;
> +	uint8_t *plane(unsigned int plane);
> +
> +private:
> +	cros::CameraBufferManager *bufferManager_;
> +	buffer_handle_t handle_;
> +	unsigned int numPlanes_;
> +	bool valid_;
> +	union {
> +		void *addr;
> +		android_ycbcr ycbcr;
> +	} mem;
> +};
> +
> +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,
> +						 int flags)
> +	: handle_(camera3Buffer), numPlanes_(0), valid_(false)
> +{
> +	bufferManager_ = cros::CameraBufferManager::GetInstance();
> +
> +	bufferManager_->Register(camera3Buffer);
> +
> +	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> +	switch (numPlanes_) {
> +	case 1: {
> +		int 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: {

That's a weird placement for the brace, does it work if you move it
after case 3 ?

> +	case 3:
> +		int 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;
> +	}

I'm tempted to delay the mapping to the first plane() call, in order to
wrap all buffers we get from the camera service in the CameraBuffer
class and use it through the HAL implementation. That's for later
though, not this series.

> +
> +	valid_ = true;
> +}
> +
> +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()
> +{
> +	bufferManager_->Unlock(handle_);
> +	bufferManager_->Deregister(handle_);
> +}
> +
> +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const
> +{
> +	return bufferManager_->GetNumPlanes(handle_);
> +}
> +
> +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const
> +{
> +	if (plane >= numPlanes())
> +		return -EINVAL;
> +
> +	return bufferManager_->GetPlaneSize(handle_, plane);
> +}
> +
> +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const
> +{
> +	if (plane >= numPlanes())
> +		return nullptr;
> +

You can drop the check, it's in the next function.

> +	return const_cast<const uint8_t *>(this->plane(plane));

I think this will cause an infinite loop as you're calling the same
function.

	CameraBuffer::CameraBufferImpl *self =
		const_cast<CameraBuffer::CameraBufferImpl *>(this);
	return const_cast<const uint8_t *>(self->plane(plane));

> +}
> +
> +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)
> +{
> +	if (plane >= numPlanes())
> +		return nullptr;
> +
> +	void *addr;
> +	switch (numPlanes()) {
> +	case 1:
> +		addr = mem.addr;
> +		break;
> +	default:
> +		switch (plane) {
> +		case 1:
> +			addr = mem.ycbcr.y;
> +			break;
> +		case 2:
> +			addr = mem.ycbcr.cb;
> +			break;
> +		case 3:
> +			addr = mem.ycbcr.cr;
> +			break;
> +		}
> +	}
> +
> +	return static_cast<uint8_t *>(addr);
> +}
> +
> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> +	: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))
> +{
> +}
> +
> +CameraBuffer::~CameraBuffer()
> +{
> +	delete impl_;
> +}
> +
> +bool CameraBuffer::isValid() const
> +{
> +	return impl_->isValid();
> +}
> +
> +unsigned int CameraBuffer::numPlanes() const
> +{
> +	return impl_->numPlanes();
> +}
> +
> +ssize_t CameraBuffer::planeSize(unsigned int plane) const
> +{
> +	return impl_->planeSize(plane);
> +}
> +
> +const uint8_t *CameraBuffer::plane(unsigned int plane) const
> +{
> +	return impl_->plane(plane);
> +}
> +
> +uint8_t *CameraBuffer::plane(unsigned int plane)
> +{
> +	return impl_->plane(plane);
> +}
> +

The blank line isn't needed.

> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> index 39be8fec8567..a801c54ff4f4 100644
> --- a/src/android/mm/meson.build
> +++ b/src/android/mm/meson.build
> @@ -3,4 +3,7 @@
>  memory_backend = get_option('android_memory_backend')
>  if memory_backend == 'android_generic'
>      android_hal_sources += files(['android_generic_buffer.cpp'])
> +elif memory_backend == 'cros_cbm'
> +    android_hal_sources += files(['cros_cbm.cpp'])
> +    android_deps += [ dependency('libcros_camera') ]

No spaces within [ ].

>  endif