[libcamera-devel] [PATCH v2] cros: Support the new cros camera API with set_up and tear_down

[Jacopo Mondi]

Hi Laurent,

On Wed, Mar 03, 2021 at 10:37:22AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Mar 03, 2021 at 09:28:06AM +0100, Jacopo Mondi wrote:
> > On Wed, Mar 03, 2021 at 08:43:16AM +0200, Laurent Pinchart wrote:
> > > On Wed, Mar 03, 2021 at 01:03:12PM +0900, Paul Elder wrote:
> > > > Implement and expose the symbol and functions that the new cros camera
> > > > API requires. Since we don't actually need them, leave them empty.
> > > > Update meson accordingly.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > >
> > > > ---
> > > > This patch depends on v4 of the series "android: Supports memory
> > > > backends"
> > > >
> > > > To use this, one needs to emerge chromeos-base/cros-camera-libs first.
> > > > Also obviously the android_platform meson option needs to be set to
> > > > cros. I'm not sure where best to put this information.
> > > >
> > > > Changes in v2:
> > > > - simplified the whole thing a ton
> > > > - use the android_platform meson option
> > > > ---
> > > >  src/android/meson.build   |  5 +++++
> > > >  src/cros/camera3_hal.cpp  | 21 +++++++++++++++++++++
> > > >  src/cros/meson.build      | 17 +++++++++++++++++
> > >
> > > I expect more interactions between the HAL implementation and the
> > > CrOS-specific code in the future when we'll implement support for the
> > > CrOS JPEG encoder. Could we move this to src/android/cros/ to group all
> > > the HAL-related code together ?
> > >
> > > >  src/libcamera/meson.build |  6 ++++++
> > > >  src/meson.build           |  1 +
> > > >  5 files changed, 50 insertions(+)
> > > >  create mode 100644 src/cros/camera3_hal.cpp
> > > >  create mode 100644 src/cros/meson.build
> > > >
> > > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > > index 7004d32d..16d95616 100644
> > > > --- a/src/android/meson.build
> > > > +++ b/src/android/meson.build
> > > > @@ -6,6 +6,7 @@ android_deps = [
> > > >  ]
> > > >
> > > >  android_enabled = true
> > > > +cros_enabled = false
> > > >
> > > >  foreach dep : android_deps
> > > >      if not dep.found()
> > > > @@ -37,6 +38,10 @@ if android_enabled
> > > >      android_deps += [libyuv_dep]
> > > >  endif
> > > >
> > > > +if get_option('android_platform') == 'cros'
> > > > +    cros_enabled = true
> > > > +endif
> > > > +
> > > >  android_hal_sources = files([
> > > >      'camera3_hal.cpp',
> > > >      'camera_hal_manager.cpp',
> > > > diff --git a/src/cros/camera3_hal.cpp b/src/cros/camera3_hal.cpp
> > > > new file mode 100644
> > > > index 00000000..31ad36ac
> > > > --- /dev/null
> > > > +++ b/src/cros/camera3_hal.cpp
> > > > @@ -0,0 +1,21 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * camera3_hal.cpp - cros-specific components of Android Camera HALv3 module
> > > > + */
> > > > +
> > > > +#include <cros-camera/cros_camera_hal.h>
> > > > +
> > > > +static void set_up(cros::CameraMojoChannelManagerToken *token)
> > > > +{
> > > > +}
> > > > +
> > > > +static void tear_down()
> > > > +{
> > > > +}
> > > > +
> > > > +cros::cros_camera_hal_t CROS_CAMERA_EXPORT CROS_CAMERA_HAL_INFO_SYM = {
> > > > +	.set_up = set_up,
> > > > +	.tear_down = tear_down
> > > > +};
> > > > diff --git a/src/cros/meson.build b/src/cros/meson.build
> > > > new file mode 100644
> > > > index 00000000..6fb1b5c0
> > > > --- /dev/null
> > > > +++ b/src/cros/meson.build
> > > > @@ -0,0 +1,17 @@
> > > > +# SPDX-License-Identifier: CC0-1.0
> > > > +
> > > > +if not cros_enabled
> > > > +   subdir_done()
> > > > +endif
> > > > +
> > > > +cros_hal_info_sources = files([
> > > > +    'camera3_hal.cpp',
> > > > +])
> > > > +
> > > > +cros_hal_info = static_library('cros_hal_info',
> > > > +                               cros_hal_info_sources,
> > > > +                               dependencies : dependency('libcros_camera'),
> > > > +                               c_args : '-Wno-shadow',
> > > > +                               include_directories : android_includes)
> > > > +
> > > > +cros_hal_info_obj = cros_hal_info.extract_objects('camera3_hal.cpp')
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index 4b5e33ce..c40eb41f 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -139,6 +139,11 @@ if android_enabled
> > > >      libcamera_deps += android_deps
> > > >  endif
> > > >
> > > > +libcamera_objects = []
> > >
> > > How about moving this to src/
> > >
> > > > +if cros_enabled
> > > > +    libcamera_objects += cros_hal_info_obj
> > > > +endif
> > >
> > > And this to src/android/cros/ ? You could then drop the cros_enabled
> > > variable and use get_option('android_platform') == 'cros' directly as it
> > > will only be used once.
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > > +
> > > >  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> > > >  # The build_rpath is stripped at install time by meson, so we determine at
> > > >  # runtime if the library is running from an installed location by checking
> > > > @@ -149,6 +154,7 @@ libcamera = shared_library('camera',
> > > >                             install : true,
> > > >                             link_with : libcamera_link_with,
> > > >                             include_directories : includes,
> > > > +                           objects : libcamera_objects,
> >
> > What is the difference in using 'objects' instead of adding the
> > 'cros_hal_info' symbol to the 'libcamera_link_with' as we do for the
> > camera metadata library ?
>
> With link_with, I think it will only pull symbols that are used. With
> objects, it will pull everything in. As CROS_CAMERA_HAL_INFO_SYM is a
> symbol we export but don't use internally, I think it won't work with
> link_with. I haven't tested this though, I may thus be mistaken.
>

Oh I see. I bet there's a way to force a symbol to be exported
unconditionally, but this is fine the way it is

> > I don't mind what you have here though, with Laurent's comments
> > addressed:
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > >                             build_rpath : '/',
> > > >                             dependencies : libcamera_deps)
> > > >
> > > > diff --git a/src/meson.build b/src/meson.build
> > > > index 0b26ca70..318b7212 100644
> > > > --- a/src/meson.build
> > > > +++ b/src/meson.build
> > > > @@ -14,6 +14,7 @@ endif
> > > >  # The 'android' subdir must be processed first, and the build targets
> > > >  # are included directly into the libcamera library when this is enabled.
> > > >  subdir('android')
> > > > +subdir('cros')
> > > >
> > > >  subdir('libcamera')
> > > >  subdir('ipa')
>
> --
> Regards,
>
> Laurent Pinchart