| Message ID | 20230227-android-gralloc-v3-1-babbdf049259@baylibre.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Mattijs, Thank you for the patch, and sorry for not reviewing the last version. 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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > [1] https://android.googlesource.com/platform/hardware/libhardware > > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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); > } > > 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']) >
Hi Mattijs, I spoke a bit too fast. 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. > > 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. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > [1] https://android.googlesource.com/platform/hardware/libhardware > > > > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") > > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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 ? > > } > > > > 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']) > >
Hi Laurent, On lun., mai 29, 2023 at 13:09, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mattijs, > > I spoke a bit too fast. > > 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@ideasonboard.com> >> >> > [1] https://android.googlesource.com/platform/hardware/libhardware >> > >> > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") >> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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. 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. Thanks, Mattijs > >> > } >> > >> > 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
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@ideasonboard.com> > >> > >> > [1] https://android.googlesource.com/platform/hardware/libhardware > >> > > >> > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") > >> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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 :-) > 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']) > >> >
Hi Laurent, On lun., mai 29, 2023 at 15:33, Laurent Pinchart <laurent.pinchart@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@ideasonboard.com> >> >> >> >> > [1] https://android.googlesource.com/platform/hardware/libhardware >> >> > >> >> > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") >> >> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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
Hi Mattijs, On Mon, May 29, 2023 at 03:25:40PM +0200, Mattijs Korpershoek wrote: > On lun., mai 29, 2023 at 15:33, Laurent Pinchart wrote: > > 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@ideasonboard.com> > >> >> > >> >> > [1] https://android.googlesource.com/platform/hardware/libhardware > >> >> > > >> >> > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") > >> >> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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). Just to make sure, did that test include linking against libdl ? > I have also functionally tested this: we can open the gralloc hw module. > > Will send a v4 shortly. Thank you. > >> 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']) > >> >> >
On lun., mai 29, 2023 at 16:29, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mattijs, > > On Mon, May 29, 2023 at 03:25:40PM +0200, Mattijs Korpershoek wrote: >> On lun., mai 29, 2023 at 15:33, Laurent Pinchart wrote: >> > 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@ideasonboard.com> >> >> >> >> >> >> > [1] https://android.googlesource.com/platform/hardware/libhardware >> >> >> > >> >> >> > Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") >> >> >> > Signed-off-by: Mattijs Korpershoek <mkorpershoek@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). > > Just to make sure, did that test include linking against libdl ? Yes. Sorry that was not clear. Here is the associated Android.bp file which contains "libdl" in the shared_libs section: https://gitlab.baylibre.com/baylibre/ti/android/aosp/external/libcamera/-/blob/ti-android-12/src/android/Android.bp#L42 Android has libdl implemented in bionic. > >> I have also functionally tested this: we can open the gralloc hw module. >> >> Will send a v4 shortly. > > Thank you. > >> >> 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
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); } 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'])
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] [1] https://android.googlesource.com/platform/hardware/libhardware Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator") Signed-off-by: Mattijs Korpershoek <mkorpershoek@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(-) --- base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca change-id: 20230227-android-gralloc-04e31d5edc68 Best regards,