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

[Mattijs Korpershoek]

On mer., mars 01, 2023 at 01:20, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> Hi Mattijs,
>
> On Tue, Feb 28, 2023 at 02:49:27PM +0100, Mattijs Korpershoek wrote:
>> Hi Laurent,
>> 
>> Thank you for your review.
>> 
>> On lun., févr. 27, 2023 at 18:52, Laurent Pinchart 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 ?
>
> It should fail at meson setup time, when creating the libhardware
> dependency.

Indeed. I forgot about adding the dependency in a meson file for v2.
sorry for the noise.

>
>> > 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.
>
> For now, our plan is to support building against the NDK, but a script
> that generates Android.bp files is something that could be interesting.

I think supporting building against the NDK is the way to go as well.
The instructions from mesa (https://docs.mesa3d.org/android.html) look
promising.

> We wouldn't commit the generated files to the repository, but we could
> host the script until a better option is found. I wonder if it could be
> implemented as a meson backend...

Agreed that the generated .bp files won't be commited, but having the
scripts might help the android integrators like myself.

If I get some time during the coming weeks I will clean them up and
submit them. I don't know meson very well but I will look into that at
the same time.

>
>> 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().
>
> Lovely API design... *sigh*

Yeah, and It does not seem to be documented in hardware.h :(
To be faire, hardware.h is very, very old and we should probably move
this to using hidl interfaces instead, but that's another topic. I still
think that this bugfix is valid as-is.

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