[libcamera-devel,v4] android: mm: generic: use GRALLOC_HARDWARE_MODULE_ID
diff mbox series

Message ID 20230227-android-gralloc-v4-1-8cd3a44c5b70@baylibre.com
State Accepted
Headers show
Series
  • [libcamera-devel,v4] android: mm: generic: use GRALLOC_HARDWARE_MODULE_ID
Related show

Commit Message

Mattijs Korpershoek May 29, 2023, 1:30 p.m. UTC 
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@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(-)


---
base-commit: 76e1cb9f7176b4e7a935295d4e633ee24a0fef67
change-id: 20230227-android-gralloc-04e31d5edc68

Best regards,

Comments

Laurent Pinchart May 29, 2023, 3:58 p.m. UTC | #1 
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@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_);
>  		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@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
Kieran Bingham May 30, 2023, 12:18 p.m. UTC | #2 
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@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@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@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
Mattijs Korpershoek May 30, 2023, 3:26 p.m. UTC | #3 
Hi Kieran,

On mar., mai 30, 2023 at 13:18, Kieran Bingham <kieran.bingham@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@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@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@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

Patch
diff mbox series 

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..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')]
 elif platform == 'cros'
     android_hal_sources += files(['cros_camera_buffer.cpp',
                                   'cros_frame_buffer_allocator.cpp'])