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

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2023-05-29 16:58:18)
> Hi Mattijs,
> 
> Thank you for the patch.
> 
> On Mon, May 29, 2023 at 03:30:17PM +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 adds new dependencies on android's libhardware [1] and on libdl.
> > 
> > [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
> > 
> > (Laurent, I dropped your reviewed-by since there are additional changes)
> > ---
> > Changes in v4:
> > - Add missing meson.build change to link against libdl
> > - Link to v3: https://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037926.html
> > 
> > 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_);

As discussed with Laurent directly, I think keeping compilation
supported for non-aosp builds is helpful to continue maintaining the
Android build with compile tests.

But we can do this easily by having a compilation unit
(libhardware-stubs.cpp perhaps) that gets compiled when libhardware is
not available, to provide linkage to a stubbed hw_get_module().

I don't mind if that's on top as long as all the builds don't stay
broken for long!  (There will still be regular builds for ChromeOS
though so it wouldn't get forgotten, so I'm not too worried).

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> >               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);
> >  }
> >  
> >  std::unique_ptr<HALFrameBuffer>
> > diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build
> > index d40a3b0ba2eb..c92416bd3a6f 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 += [libdl, dependency('libhardware')]
> 
> We usually split arrays with multiple entries on multiple lines:
> 
>     android_deps += [
>         libdl,
>         dependency('libhardware'),
>     ]
> 
> I'll do this locally.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >  elif platform == 'cros'
> >      android_hal_sources += files(['cros_camera_buffer.cpp',
> >                                    'cros_frame_buffer_allocator.cpp'])
> > 
> > ---
> > base-commit: 76e1cb9f7176b4e7a935295d4e633ee24a0fef67
> > change-id: 20230227-android-gralloc-04e31d5edc68
> 
> -- 
> Regards,
> 
> Laurent Pinchart