[libcamera-devel] [PATCH RFC] android: mm: generic: use GRALLOC_HARDWARE_MODULE_ID

[Laurent Pinchart]

Hi Mattijs,

On Mon, Feb 27, 2023 at 09:54:02AM +0100, Mattijs Korpershoek via libcamera-devel wrote:
> PlatformFrameBufferAllocator is an abstraction over gralloc.
> Right now hardwareModule_ points towards a CAMERA_HARDWARE_MODULE_ID.
> 
> When gralloc_open() is called we observe:
> libcamera: DEBUG HAL camera3_hal.cpp:75 Open camera gpu0
> libcamera: ERROR Camera camera.cpp:524 Camera in Configured state trying acquire() requiring state Available
> 01-23 14:14:04.742   370   416 E libcamera: FATAL HAL generic_frame_buffer_allocator.cpp:105 gralloc_open() failed: -87
> 
> Which is wrong, gralloc_open() is attempting to re-open the camera HAL,
> instead of the gralloc HAL.
> 
> Point to a GRALLOC_HARDWARE_MODULE_ID instead so that we can request
> buffers from gralloc in android.

Good catch !

> Note: this add a new dependency on android's libhardware [1]
> 
> [1] https://android.googlesource.com/platform/hardware/libhardware
> 
> Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator")
> Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> ---
> RFC as this adds an additional link-time dependency on an AOSP project
> (libhardware) and I'm unsure how to handle this.
> 
> I thought of using the subprojects structure as done for libyuv but
> since libhardware has other AOSP deps (like libvnksupport) I'm not sure
> this is appropriate.
> 
> Any thoughts on this?

We can require linking against AOSP libraries when compiling this code,
that's not an issue. A subproject isn't needed (and wouldn't be
practical anyway).

How did you test-link this patch ?

> ---
>  src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> index 3750e1bf52a9..58e6c68c4998 100644
> --- a/src/android/mm/generic_frame_buffer_allocator.cpp
> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> @@ -72,9 +72,10 @@ class PlatformFrameBufferAllocator::Private : public Extensible::Private
>  public:
>  	Private(CameraDevice *const cameraDevice)
>  		: cameraDevice_(cameraDevice),
> -		  hardwareModule_(cameraDevice->camera3Device()->common.module),
> +		  hardwareModule_(nullptr),
>  		  allocDevice_(nullptr)
>  	{
> +		hw_get_module(GRALLOC_HARDWARE_MODULE_ID, &hardwareModule_);

hw_get_module(), if my understanding is correct, ends up dlopen()ing the
module. Is there anything we need to do in the destructor to close the
module ?

>  		ASSERT(hardwareModule_);
>  	}
>  
> @@ -85,7 +86,7 @@ public:
>  
>  private:
>  	const CameraDevice *const cameraDevice_;
> -	struct hw_module_t *const hardwareModule_;
> +	const struct hw_module_t *hardwareModule_;
>  	struct alloc_device_t *allocDevice_;
>  };
>  
> 
> ---
> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca
> change-id: 20230227-android-gralloc-04e31d5edc68

-- 
Regards,

Laurent Pinchart