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

[Laurent Pinchart]

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.

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

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

> 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