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

[Mattijs Korpershoek]

Hi Laurent,

Thank you for your review.

On lun., févr. 27, 2023 at 18:52, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> 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).

Thank you, that is helpful.

Does that mean that the change can be kept as-is, and it's acceptable to
have link errors when building with
-Dandroid=enabled -Dandroid_platform=generic ?

>
> How did you test-link this patch ?

I have link-tested this patch in an Android 12 AOSP-based environment.

I also build-tested using the meson build system, noticing the link error.

I work in a BayLibre specific integration, where have generated
Android.bp files with a python script based on the meson files. It
parses the meson.build files to generate Android.bp files.

We use this to build libcamera, the Android Camera HAL and the cam app.

As I'm not sure sure the libcamera wants to maintain this, I did not
submit it.
If this seems useful for libcamera, I can polish this up and submit it.

>
>> ---
>>  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 ?

Your understanding is correct, hw_get_module() ends up doing dlopen().
AFAIK, there is no equivalent of release the hardwareModule_. There is
no hw_put_module().

I've skimmed through some other implementations of HALs (such as
gatekeeper) and they indeed call dclose().

Thanks a lot for bringing this up. I will send a v2.


>
>>  		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