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

[Mattijs Korpershoek]

Hi Laurent,

On lun., mai 29, 2023 at 15:33, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> Hi Mattijs,
>
> On Mon, May 29, 2023 at 02:30:51PM +0200, Mattijs Korpershoek wrote:
>> On lun., mai 29, 2023 at 13:09, Laurent Pinchart wrote:
>> > On Mon, May 29, 2023 at 12:42:55PM +0300, Laurent Pinchart via libcamera-devel wrote:
>> >> Hi Mattijs,
>> >> 
>> >> Thank you for the patch, and sorry for not reviewing the last version.
>> 
>> Thank you for the review. No worries, patches can split through some time.
>> 
>> >> 
>> >> On Sun, May 28, 2023 at 06:07:00PM +0200, 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.
>> >> > 
>> >> > Note: this add a new dependency on android's libhardware [1]
>> >> 
>> >> s/add/adds/
>> >> 
>> >> I'll fix this locally.
>> 
>> Will fix in v4, since I have to respin this anyways for libdl.
>> 
>> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> >> 
>> >> > [1] https://android.googlesource.com/platform/hardware/libhardware
>> >> > 
>> >> > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator")
>> >> > Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>> >> > ---
>> >> > * Tested on an integration branch in Android 12 (Using android.bp)
>> >> > * Build tested with meson setup build -Dandroid=enabled -Dandroid_platform=generic
>> >> > ---
>> >> > Changes in v3:
>> >> > - Add missing meson.build change to define libhardware dependency
>> >> > - Link to v2: https://lists.libcamera.org/pipermail/libcamera-devel/2023-February/036884.html
>> >> > 
>> >> > Changes in v2:
>> >> > - Add dlclose() in PlatformFrameBufferAllocator destructor (Laurent)
>> >> > - Removed RFC, as linking against an AOSP lib for android is OK.
>> >> > - Link to v1: https://lists.libcamera.org/pipermail/libcamera-devel/2023-February/036857.html
>> >> > ---
>> >> >  src/android/mm/generic_frame_buffer_allocator.cpp | 7 +++++--
>> >> >  src/android/mm/meson.build                        | 1 +
>> >> >  2 files changed, 6 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..7ecef2c669df 100644
>> >> > --- a/src/android/mm/generic_frame_buffer_allocator.cpp
>> >> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
>> >> > @@ -5,6 +5,7 @@
>> >> >   * generic_camera_buffer.cpp - Allocate FrameBuffer using gralloc API
>> >> >   */
>> >> >  
>> >> > +#include <dlfcn.h>
>> >> >  #include <memory>
>> >> >  #include <vector>
>> >> >  
>> >> > @@ -72,9 +73,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_);
>> >> >  		ASSERT(hardwareModule_);
>> >> >  	}
>> >> >  
>> >> > @@ -85,7 +87,7 @@ public:
>> >> >  
>> >> >  private:
>> >> >  	const CameraDevice *const cameraDevice_;
>> >> > -	struct hw_module_t *const hardwareModule_;
>> >> > +	const struct hw_module_t *hardwareModule_;
>> >> >  	struct alloc_device_t *allocDevice_;
>> >> >  };
>> >> >  
>> >> > @@ -93,6 +95,7 @@ PlatformFrameBufferAllocator::Private::~Private()
>> >> >  {
>> >> >  	if (allocDevice_)
>> >> >  		gralloc_close(allocDevice_);
>> >> > +	dlclose(hardwareModule_->dso);
>> >
>> > This requires linking with libdl. src/libcamera/meson.build creates a
>> > libdl variable, which I think you should add to the android_deps below.
>> > Could you check if that compiles fine ?
>> 
>> Yes, I can check. I suspect it won't link in any case due to missing libhardware
>> because we don't have access to libhardware when building on linux:
>> 
>> src/android/mm/meson.build:7:4: ERROR: Dependency "libhardware" not found, tried pkgconfig and cmake
>> libcamera/build/../src/android/mm/generic_frame_buffer_allocator.cpp:79: undefined reference to `hw_get_module'
>> 
>> Per my understanding from
>> https://patchwork.libcamera.org/patch/18309/#26503, it was acceptable
>> this way.
>
> Yes, that's fine. What I would like you to do is test linking against
> libdl in a real Android build, either with AOSP or the Android NDK :-)

I confirm I have build and link-tested this using AOSP (with
hand-written Android.bp files).

I have also functionally tested this: we can open the gralloc hw module.

Will send a v4 shortly.

>
>> When commenting out hw_get_module() and the libhardware dependency in
>> build.meson, it already compiled fine.
>> 
>> I will add libdl to the dependency list in v4.
>> 
>> >> >  }
>> >> >  
>> >> >  std::unique_ptr<HALFrameBuffer>
>> >> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
>> >> > index d40a3b0ba2eb..42eeab3bcfa9 100644
>> >> > --- a/src/android/mm/meson.build
>> >> > +++ b/src/android/mm/meson.build
>> >> > @@ -4,6 +4,7 @@ platform = get_option('android_platform')
>> >> >  if platform == 'generic'
>> >> >      android_hal_sources += files(['generic_camera_buffer.cpp',
>> >> >                                    'generic_frame_buffer_allocator.cpp'])
>> >> > +    android_deps += [dependency('libhardware')]
>> >> >  elif platform == 'cros'
>> >> >      android_hal_sources += files(['cros_camera_buffer.cpp',
>> >> >                                    'cros_frame_buffer_allocator.cpp'])
>> >> > 
>
> -- 
> Regards,
>
> Laurent Pinchart