| Message ID | 20230227-android-gralloc-v1-1-3360fcfa7c0d@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Mattijs, On Mon, Feb 27, 2023 at 09:54:02AM +0100, 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. Good catch ! > 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> > --- > RFC as this adds an additional link-time dependency on an AOSP project > (libhardware) and I'm unsure how to handle this. > > I thought of using the subprojects structure as done for libyuv but > since libhardware has other AOSP deps (like libvnksupport) I'm not sure > this is appropriate. > > Any thoughts on this? We can require linking against AOSP libraries when compiling this code, that's not an issue. A subproject isn't needed (and wouldn't be practical anyway). How did you test-link this patch ? > --- > src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++-- > 1 file changed, 3 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..58e6c68c4998 100644 > --- a/src/android/mm/generic_frame_buffer_allocator.cpp > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > @@ -72,9 +72,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_); hw_get_module(), if my understanding is correct, ends up dlopen()ing the module. Is there anything we need to do in the destructor to close the module ? > ASSERT(hardwareModule_); > } > > @@ -85,7 +86,7 @@ public: > > private: > const CameraDevice *const cameraDevice_; > - struct hw_module_t *const hardwareModule_; > + const struct hw_module_t *hardwareModule_; > struct alloc_device_t *allocDevice_; > }; > > > --- > base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca > change-id: 20230227-android-gralloc-04e31d5edc68
Hi Laurent, Thank you for your review. On lun., févr. 27, 2023 at 18:52, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mattijs, > > On Mon, Feb 27, 2023 at 09:54:02AM +0100, 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. > > Good catch ! > >> 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> >> --- >> RFC as this adds an additional link-time dependency on an AOSP project >> (libhardware) and I'm unsure how to handle this. >> >> I thought of using the subprojects structure as done for libyuv but >> since libhardware has other AOSP deps (like libvnksupport) I'm not sure >> this is appropriate. >> >> Any thoughts on this? > > We can require linking against AOSP libraries when compiling this code, > that's not an issue. A subproject isn't needed (and wouldn't be > practical anyway). Thank you, that is helpful. Does that mean that the change can be kept as-is, and it's acceptable to have link errors when building with -Dandroid=enabled -Dandroid_platform=generic ? > > How did you test-link this patch ? I have link-tested this patch in an Android 12 AOSP-based environment. I also build-tested using the meson build system, noticing the link error. I work in a BayLibre specific integration, where have generated Android.bp files with a python script based on the meson files. It parses the meson.build files to generate Android.bp files. We use this to build libcamera, the Android Camera HAL and the cam app. As I'm not sure sure the libcamera wants to maintain this, I did not submit it. If this seems useful for libcamera, I can polish this up and submit it. > >> --- >> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++-- >> 1 file changed, 3 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..58e6c68c4998 100644 >> --- a/src/android/mm/generic_frame_buffer_allocator.cpp >> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp >> @@ -72,9 +72,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_); > > hw_get_module(), if my understanding is correct, ends up dlopen()ing the > module. Is there anything we need to do in the destructor to close the > module ? Your understanding is correct, hw_get_module() ends up doing dlopen(). AFAIK, there is no equivalent of release the hardwareModule_. There is no hw_put_module(). I've skimmed through some other implementations of HALs (such as gatekeeper) and they indeed call dclose(). Thanks a lot for bringing this up. I will send a v2. > >> ASSERT(hardwareModule_); >> } >> >> @@ -85,7 +86,7 @@ public: >> >> private: >> const CameraDevice *const cameraDevice_; >> - struct hw_module_t *const hardwareModule_; >> + const struct hw_module_t *hardwareModule_; >> struct alloc_device_t *allocDevice_; >> }; >> >> >> --- >> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca >> change-id: 20230227-android-gralloc-04e31d5edc68 > > -- > Regards, > > Laurent Pinchart
Hi Mattijs, On Tue, Feb 28, 2023 at 02:49:27PM +0100, Mattijs Korpershoek wrote: > Hi Laurent, > > Thank you for your review. > > On lun., févr. 27, 2023 at 18:52, Laurent Pinchart wrote: > > > Hi Mattijs, > > > > On Mon, Feb 27, 2023 at 09:54:02AM +0100, 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. > > > > Good catch ! > > > >> 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> > >> --- > >> RFC as this adds an additional link-time dependency on an AOSP project > >> (libhardware) and I'm unsure how to handle this. > >> > >> I thought of using the subprojects structure as done for libyuv but > >> since libhardware has other AOSP deps (like libvnksupport) I'm not sure > >> this is appropriate. > >> > >> Any thoughts on this? > > > > We can require linking against AOSP libraries when compiling this code, > > that's not an issue. A subproject isn't needed (and wouldn't be > > practical anyway). > > Thank you, that is helpful. > > Does that mean that the change can be kept as-is, and it's acceptable to > have link errors when building with > -Dandroid=enabled -Dandroid_platform=generic ? It should fail at meson setup time, when creating the libhardware dependency. > > How did you test-link this patch ? > > I have link-tested this patch in an Android 12 AOSP-based environment. > > I also build-tested using the meson build system, noticing the link error. > > I work in a BayLibre specific integration, where have generated > Android.bp files with a python script based on the meson files. It > parses the meson.build files to generate Android.bp files. > > We use this to build libcamera, the Android Camera HAL and the cam app. > > As I'm not sure sure the libcamera wants to maintain this, I did not > submit it. For now, our plan is to support building against the NDK, but a script that generates Android.bp files is something that could be interesting. We wouldn't commit the generated files to the repository, but we could host the script until a better option is found. I wonder if it could be implemented as a meson backend... > If this seems useful for libcamera, I can polish this up and submit it. > > >> --- > >> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++-- > >> 1 file changed, 3 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..58e6c68c4998 100644 > >> --- a/src/android/mm/generic_frame_buffer_allocator.cpp > >> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp > >> @@ -72,9 +72,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_); > > > > hw_get_module(), if my understanding is correct, ends up dlopen()ing the > > module. Is there anything we need to do in the destructor to close the > > module ? > > Your understanding is correct, hw_get_module() ends up doing dlopen(). > AFAIK, there is no equivalent of release the hardwareModule_. There is > no hw_put_module(). > > I've skimmed through some other implementations of HALs (such as > gatekeeper) and they indeed call dclose(). Lovely API design... *sigh* > Thanks a lot for bringing this up. I will send a v2. > > >> ASSERT(hardwareModule_); > >> } > >> > >> @@ -85,7 +86,7 @@ public: > >> > >> private: > >> const CameraDevice *const cameraDevice_; > >> - struct hw_module_t *const hardwareModule_; > >> + const struct hw_module_t *hardwareModule_; > >> struct alloc_device_t *allocDevice_; > >> }; > >> > >> > >> --- > >> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca > >> change-id: 20230227-android-gralloc-04e31d5edc68
On mer., mars 01, 2023 at 01:20, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mattijs, > > On Tue, Feb 28, 2023 at 02:49:27PM +0100, Mattijs Korpershoek wrote: >> Hi Laurent, >> >> Thank you for your review. >> >> On lun., févr. 27, 2023 at 18:52, Laurent Pinchart wrote: >> >> > Hi Mattijs, >> > >> > On Mon, Feb 27, 2023 at 09:54:02AM +0100, 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. >> > >> > Good catch ! >> > >> >> 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> >> >> --- >> >> RFC as this adds an additional link-time dependency on an AOSP project >> >> (libhardware) and I'm unsure how to handle this. >> >> >> >> I thought of using the subprojects structure as done for libyuv but >> >> since libhardware has other AOSP deps (like libvnksupport) I'm not sure >> >> this is appropriate. >> >> >> >> Any thoughts on this? >> > >> > We can require linking against AOSP libraries when compiling this code, >> > that's not an issue. A subproject isn't needed (and wouldn't be >> > practical anyway). >> >> Thank you, that is helpful. >> >> Does that mean that the change can be kept as-is, and it's acceptable to >> have link errors when building with >> -Dandroid=enabled -Dandroid_platform=generic ? > > It should fail at meson setup time, when creating the libhardware > dependency. Indeed. I forgot about adding the dependency in a meson file for v2. sorry for the noise. > >> > How did you test-link this patch ? >> >> I have link-tested this patch in an Android 12 AOSP-based environment. >> >> I also build-tested using the meson build system, noticing the link error. >> >> I work in a BayLibre specific integration, where have generated >> Android.bp files with a python script based on the meson files. It >> parses the meson.build files to generate Android.bp files. >> >> We use this to build libcamera, the Android Camera HAL and the cam app. >> >> As I'm not sure sure the libcamera wants to maintain this, I did not >> submit it. > > For now, our plan is to support building against the NDK, but a script > that generates Android.bp files is something that could be interesting. I think supporting building against the NDK is the way to go as well. The instructions from mesa (https://docs.mesa3d.org/android.html) look promising. > We wouldn't commit the generated files to the repository, but we could > host the script until a better option is found. I wonder if it could be > implemented as a meson backend... Agreed that the generated .bp files won't be commited, but having the scripts might help the android integrators like myself. If I get some time during the coming weeks I will clean them up and submit them. I don't know meson very well but I will look into that at the same time. > >> If this seems useful for libcamera, I can polish this up and submit it. >> >> >> --- >> >> src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++-- >> >> 1 file changed, 3 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..58e6c68c4998 100644 >> >> --- a/src/android/mm/generic_frame_buffer_allocator.cpp >> >> +++ b/src/android/mm/generic_frame_buffer_allocator.cpp >> >> @@ -72,9 +72,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_); >> > >> > hw_get_module(), if my understanding is correct, ends up dlopen()ing the >> > module. Is there anything we need to do in the destructor to close the >> > module ? >> >> Your understanding is correct, hw_get_module() ends up doing dlopen(). >> AFAIK, there is no equivalent of release the hardwareModule_. There is >> no hw_put_module(). >> >> I've skimmed through some other implementations of HALs (such as >> gatekeeper) and they indeed call dclose(). > > Lovely API design... *sigh* Yeah, and It does not seem to be documented in hardware.h :( To be faire, hardware.h is very, very old and we should probably move this to using hidl interfaces instead, but that's another topic. I still think that this bugfix is valid as-is. > >> Thanks a lot for bringing this up. I will send a v2. >> >> >> ASSERT(hardwareModule_); >> >> } >> >> >> >> @@ -85,7 +86,7 @@ public: >> >> >> >> private: >> >> const CameraDevice *const cameraDevice_; >> >> - struct hw_module_t *const hardwareModule_; >> >> + const struct hw_module_t *hardwareModule_; >> >> struct alloc_device_t *allocDevice_; >> >> }; >> >> >> >> >> >> --- >> >> base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca >> >> change-id: 20230227-android-gralloc-04e31d5edc68 > > -- > 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..58e6c68c4998 100644 --- a/src/android/mm/generic_frame_buffer_allocator.cpp +++ b/src/android/mm/generic_frame_buffer_allocator.cpp @@ -72,9 +72,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 +86,7 @@ public: private: const CameraDevice *const cameraDevice_; - struct hw_module_t *const hardwareModule_; + const struct hw_module_t *hardwareModule_; struct alloc_device_t *allocDevice_; };
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> --- RFC as this adds an additional link-time dependency on an AOSP project (libhardware) and I'm unsure how to handle this. I thought of using the subprojects structure as done for libyuv but since libhardware has other AOSP deps (like libvnksupport) I'm not sure this is appropriate. Any thoughts on this? --- src/android/mm/generic_frame_buffer_allocator.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) --- base-commit: 58e0b6e18c425072a47594f42fc0b61801403aca change-id: 20230227-android-gralloc-04e31d5edc68 Best regards,