[libcamera-devel] [PATCH v4] android: mm: generic: use GRALLOC_HARDWARE_MODULE_ID
Mattijs Korpershoek
mkorpershoek at baylibre.com
Tue May 30 17:26:22 CEST 2023
Hi Kieran,
On mar., mai 30, 2023 at 13:18, Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
> 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.
Thank you for the quick review. I agree that breaking the meson build
for -Dandroid=enabled -Dandroid_platform=generic is preferable.
>
> 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'm not super fluent with meson, but I see that Laurent already
implemented your suggestion here:
https://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037975.html
>
> 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).
That's understandable. I hope that both patches can be merged together.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Thanks again for the help to both of you.
Regards
Mattijs
>
>> > 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
More information about the libcamera-devel
mailing list