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

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

Commit Message

Mattijs Korpershoek March 2, 2023, 8:27 a.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 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,

Comments

Mattijs Korpershoek March 21, 2023, 2:03 p.m. UTC | #1 
On jeu., mars 02, 2023 at 09:27, Mattijs Korpershoek <mkorpershoek@baylibre.com> 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]
>
> [1] https://android.googlesource.com/platform/hardware/libhardware
>
> Fixes: c58662c5770e ("android: Introduce PlatformFrameBufferAllocator")
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Hi, gentle ping on this patch. Is there anything missing besides some
time to review?

Sorry if I have pinged too soon on this.

Regards
Mattijs

> ---
> * 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'])
>
> ---
> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca
> change-id: 20230227-android-gralloc-04e31d5edc68
>
> Best regards,
> -- 
> Mattijs Korpershoek <mkorpershoek@baylibre.com>

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..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'])