| Message ID | 20210415135213.94511-5-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Open the HAL configuration file in the Camera HAL manager and get > the camera properties for each created CameraDevice and initialize it > with them. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> When would you merge this series? -Hiro > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 56 +++++++++++++++++++++++++----- > src/android/camera_device.h | 3 +- > src/android/camera_hal_manager.cpp | 33 +++++++++++++++++- > src/android/camera_hal_manager.h | 3 ++ > 4 files changed, 85 insertions(+), 10 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 89044efa7ebe..50809c6ffbdc 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -6,6 +6,7 @@ > */ > > #include "camera_device.h" > +#include "camera_hal_config.h" > #include "camera_ops.h" > #include "post_processor.h" > > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> > CameraDevice::create(unsigned int id, > } > > /* > - * Initialize the camera static information. > + * Initialize the camera static information retrieved from the > + * Camera::properties or from the cameraConfigData. > + * > + * cameraConfigData is optional for external camera devices and can be > + * nullptr. > + * > * This method is called before the camera device is opened. > */ > -int CameraDevice::initialize() > +int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > { > - /* Initialize orientation and facing side of the camera. */ > + /* > + * Initialize orientation and facing side of the camera. > + * > + * If the libcamera::Camera provides those information as retrieved > + * from firmware use them, otherwise fallback to values parsed from > + * the configuration file. If the configuration file is not > available > + * the camera is external so its location and rotation can be > safely > + * defaulted. > + */ > const ControlList &properties = camera_->properties(); > > if (properties.contains(properties::Location)) { > @@ -464,12 +478,22 @@ int CameraDevice::initialize() > facing_ = CAMERA_FACING_EXTERNAL; > break; > } > + > + if (cameraConfigData && cameraConfigData->facing != -1 && > + facing_ != cameraConfigData->facing) { > + LOG(HAL, Warning) > + << "Camera location does not match" > + << " configuration file. Using " << > facing_; > + } > + } else if (cameraConfigData) { > + if (cameraConfigData->facing == -1) { > + LOG(HAL, Error) > + << "Camera facing not in configuration > file"; > + return -EINVAL; > + } > + facing_ = cameraConfigData->facing; > } else { > - /* > - * \todo Retrieve the camera location from configuration > file > - * if not available from the library. > - */ > - facing_ = CAMERA_FACING_FRONT; > + facing_ = CAMERA_FACING_EXTERNAL; > } > > /* > @@ -483,8 +507,24 @@ int CameraDevice::initialize() > if (properties.contains(properties::Rotation)) { > int rotation = properties.get(properties::Rotation); > orientation_ = (360 - rotation) % 360; > + if (cameraConfigData && cameraConfigData->rotation != -1 && > + orientation_ != cameraConfigData->rotation) { > + LOG(HAL, Warning) > + << "Camera orientation does not match" > + << " configuration file. Using " << > orientation_; > + } > + } else if (cameraConfigData) { > + if (cameraConfigData->rotation == -1) { > + LOG(HAL, Error) > + << "Camera rotation not in configuration > file"; > + return -EINVAL; > + } > + orientation_ = cameraConfigData->rotation; > + } else { > + orientation_ = 0; > } > > + /* Acquire the camera and initialize available stream > configurations. */ > int ret = camera_->acquire(); > if (ret) { > LOG(HAL, Error) << "Failed to temporarily acquire the > camera"; > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 11bdfec8d587..9cc0d4005242 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -29,6 +29,7 @@ > #include "camera_worker.h" > #include "jpeg/encoder.h" > > +struct CameraConfigData; > class CameraDevice : protected libcamera::Loggable > { > public: > @@ -36,7 +37,7 @@ public: > > std::shared_ptr<libcamera::Camera> cam); > ~CameraDevice(); > > - int initialize(); > + int initialize(const CameraConfigData *cameraConfigData); > > int open(const hw_module_t *hardwareModule); > void close(); > diff --git a/src/android/camera_hal_manager.cpp > b/src/android/camera_hal_manager.cpp > index bf3fcda75237..f5b86974e8e3 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -41,6 +41,15 @@ int CameraHalManager::init() > { > cameraManager_ = std::make_unique<CameraManager>(); > > + /* > + * If the configuration file is not available the HAL only supports > + * external cameras. If it exists but it's not valid then error > out. > + */ > + if (halConfig_.exists() && !halConfig_.isValid()) { > + LOG(HAL, Error) << "HAL configuration file is not valid"; > + return -EINVAL; > + } > + > /* Support camera hotplug. */ > cameraManager_->cameraAdded.connect(this, > &CameraHalManager::cameraAdded); > cameraManager_->cameraRemoved.connect(this, > &CameraHalManager::cameraRemoved); > @@ -100,6 +109,8 @@ void > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > auto iter = cameraIdsMap_.find(cam->id()); > if (iter != cameraIdsMap_.end()) { > id = iter->second; > + if (id >= firstExternalCameraId_) > + isCameraExternal = true; > } else { > isCameraNew = true; > > @@ -117,7 +128,27 @@ void > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, > cam); > - int ret = camera->initialize(); > + > + /* > + * The configuration file must be valid, and contain a > corresponding > + * entry for internal cameras. External cameras can be initialized > + * without configuration file. > + */ > + if (!isCameraExternal && !halConfig_.exists()) { > + LOG(HAL, Error) > + << "HAL configuration file is mandatory for > internal cameras"; > + return; > + } > + > + const CameraConfigData *cameraConfigData = > halConfig_.cameraConfigData(cam->id()); > + if (!isCameraExternal && !cameraConfigData) { > + LOG(HAL, Error) > + << "HAL configuration entry for internal camera " > + << cam->id() << " is missing"; > + return; > + } > + > + int ret = camera->initialize(cameraConfigData); > if (ret) { > LOG(HAL, Error) << "Failed to initialize camera: " << > cam->id(); > return; > diff --git a/src/android/camera_hal_manager.h > b/src/android/camera_hal_manager.h > index d9bf27989965..af1581da6579 100644 > --- a/src/android/camera_hal_manager.h > +++ b/src/android/camera_hal_manager.h > @@ -19,6 +19,8 @@ > > #include <libcamera/camera_manager.h> > > +#include "camera_hal_config.h" > + > class CameraDevice; > > class CameraHalManager > @@ -50,6 +52,7 @@ private: > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > + CameraHalConfig halConfig_; > > const camera_module_callbacks_t *callbacks_; > std::vector<std::unique_ptr<CameraDevice>> cameras_; > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Hiro, On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > Hi Jacopo, > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Open the HAL configuration file in the Camera HAL manager and get > > the camera properties for each created CameraDevice and initialize it > > with them. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > When would you merge this series? My plan was to update my SDK to the latest version which contains the CL to the libcamera and libcamera-configs ebuild, so I could have tested one more time on a fresh image. Am I too paranoid ? Could you or Han-lin tell me which is the id of the first CPFE image that contains: https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 I've tested those patches applied on my rather ancient SDK version, but I would like to try with an out-of-the-box image Thanks j > > -Hiro > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 56 +++++++++++++++++++++++++----- > > src/android/camera_device.h | 3 +- > > src/android/camera_hal_manager.cpp | 33 +++++++++++++++++- > > src/android/camera_hal_manager.h | 3 ++ > > 4 files changed, 85 insertions(+), 10 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 89044efa7ebe..50809c6ffbdc 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -6,6 +6,7 @@ > > */ > > > > #include "camera_device.h" > > +#include "camera_hal_config.h" > > #include "camera_ops.h" > > #include "post_processor.h" > > > > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> > > CameraDevice::create(unsigned int id, > > } > > > > /* > > - * Initialize the camera static information. > > + * Initialize the camera static information retrieved from the > > + * Camera::properties or from the cameraConfigData. > > + * > > + * cameraConfigData is optional for external camera devices and can be > > + * nullptr. > > + * > > * This method is called before the camera device is opened. > > */ > > -int CameraDevice::initialize() > > +int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > > { > > - /* Initialize orientation and facing side of the camera. */ > > + /* > > + * Initialize orientation and facing side of the camera. > > + * > > + * If the libcamera::Camera provides those information as retrieved > > + * from firmware use them, otherwise fallback to values parsed from > > + * the configuration file. If the configuration file is not > > available > > + * the camera is external so its location and rotation can be > > safely > > + * defaulted. > > + */ > > const ControlList &properties = camera_->properties(); > > > > if (properties.contains(properties::Location)) { > > @@ -464,12 +478,22 @@ int CameraDevice::initialize() > > facing_ = CAMERA_FACING_EXTERNAL; > > break; > > } > > + > > + if (cameraConfigData && cameraConfigData->facing != -1 && > > + facing_ != cameraConfigData->facing) { > > + LOG(HAL, Warning) > > + << "Camera location does not match" > > + << " configuration file. Using " << > > facing_; > > + } > > + } else if (cameraConfigData) { > > + if (cameraConfigData->facing == -1) { > > + LOG(HAL, Error) > > + << "Camera facing not in configuration > > file"; > > + return -EINVAL; > > + } > > + facing_ = cameraConfigData->facing; > > } else { > > - /* > > - * \todo Retrieve the camera location from configuration > > file > > - * if not available from the library. > > - */ > > - facing_ = CAMERA_FACING_FRONT; > > + facing_ = CAMERA_FACING_EXTERNAL; > > } > > > > /* > > @@ -483,8 +507,24 @@ int CameraDevice::initialize() > > if (properties.contains(properties::Rotation)) { > > int rotation = properties.get(properties::Rotation); > > orientation_ = (360 - rotation) % 360; > > + if (cameraConfigData && cameraConfigData->rotation != -1 && > > + orientation_ != cameraConfigData->rotation) { > > + LOG(HAL, Warning) > > + << "Camera orientation does not match" > > + << " configuration file. Using " << > > orientation_; > > + } > > + } else if (cameraConfigData) { > > + if (cameraConfigData->rotation == -1) { > > + LOG(HAL, Error) > > + << "Camera rotation not in configuration > > file"; > > + return -EINVAL; > > + } > > + orientation_ = cameraConfigData->rotation; > > + } else { > > + orientation_ = 0; > > } > > > > + /* Acquire the camera and initialize available stream > > configurations. */ > > int ret = camera_->acquire(); > > if (ret) { > > LOG(HAL, Error) << "Failed to temporarily acquire the > > camera"; > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 11bdfec8d587..9cc0d4005242 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -29,6 +29,7 @@ > > #include "camera_worker.h" > > #include "jpeg/encoder.h" > > > > +struct CameraConfigData; > > class CameraDevice : protected libcamera::Loggable > > { > > public: > > @@ -36,7 +37,7 @@ public: > > > > std::shared_ptr<libcamera::Camera> cam); > > ~CameraDevice(); > > > > - int initialize(); > > + int initialize(const CameraConfigData *cameraConfigData); > > > > int open(const hw_module_t *hardwareModule); > > void close(); > > diff --git a/src/android/camera_hal_manager.cpp > > b/src/android/camera_hal_manager.cpp > > index bf3fcda75237..f5b86974e8e3 100644 > > --- a/src/android/camera_hal_manager.cpp > > +++ b/src/android/camera_hal_manager.cpp > > @@ -41,6 +41,15 @@ int CameraHalManager::init() > > { > > cameraManager_ = std::make_unique<CameraManager>(); > > > > + /* > > + * If the configuration file is not available the HAL only supports > > + * external cameras. If it exists but it's not valid then error > > out. > > + */ > > + if (halConfig_.exists() && !halConfig_.isValid()) { > > + LOG(HAL, Error) << "HAL configuration file is not valid"; > > + return -EINVAL; > > + } > > + > > /* Support camera hotplug. */ > > cameraManager_->cameraAdded.connect(this, > > &CameraHalManager::cameraAdded); > > cameraManager_->cameraRemoved.connect(this, > > &CameraHalManager::cameraRemoved); > > @@ -100,6 +109,8 @@ void > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > auto iter = cameraIdsMap_.find(cam->id()); > > if (iter != cameraIdsMap_.end()) { > > id = iter->second; > > + if (id >= firstExternalCameraId_) > > + isCameraExternal = true; > > } else { > > isCameraNew = true; > > > > @@ -117,7 +128,27 @@ void > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, > > cam); > > - int ret = camera->initialize(); > > + > > + /* > > + * The configuration file must be valid, and contain a > > corresponding > > + * entry for internal cameras. External cameras can be initialized > > + * without configuration file. > > + */ > > + if (!isCameraExternal && !halConfig_.exists()) { > > + LOG(HAL, Error) > > + << "HAL configuration file is mandatory for > > internal cameras"; > > + return; > > + } > > + > > + const CameraConfigData *cameraConfigData = > > halConfig_.cameraConfigData(cam->id()); > > + if (!isCameraExternal && !cameraConfigData) { > > + LOG(HAL, Error) > > + << "HAL configuration entry for internal camera " > > + << cam->id() << " is missing"; > > + return; > > + } > > + > > + int ret = camera->initialize(cameraConfigData); > > if (ret) { > > LOG(HAL, Error) << "Failed to initialize camera: " << > > cam->id(); > > return; > > diff --git a/src/android/camera_hal_manager.h > > b/src/android/camera_hal_manager.h > > index d9bf27989965..af1581da6579 100644 > > --- a/src/android/camera_hal_manager.h > > +++ b/src/android/camera_hal_manager.h > > @@ -19,6 +19,8 @@ > > > > #include <libcamera/camera_manager.h> > > > > +#include "camera_hal_config.h" > > + > > class CameraDevice; > > > > class CameraHalManager > > @@ -50,6 +52,7 @@ private: > > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > > + CameraHalConfig halConfig_; > > > > const camera_module_callbacks_t *callbacks_; > > std::vector<std::unique_ptr<CameraDevice>> cameras_; > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Fri, May 21, 2021 at 4:29 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Hiro, > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > the camera properties for each created CameraDevice and initialize it > > > with them. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > When would you merge this series? > > My plan was to update my SDK to the latest version which contains the > CL to the libcamera and libcamera-configs ebuild, so I could have > tested one more time on a fresh image. Am I too paranoid ? > > Could you or Han-lin tell me which is the id of the first CPFE image > that contains: > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 They landed on 13971.0.0. -Hiro > > > I've tested those patches applied on my rather ancient SDK version, > but I would like to try with an out-of-the-box image > > Thanks > j > > > > > -Hiro > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 56 +++++++++++++++++++++++++----- > > > src/android/camera_device.h | 3 +- > > > src/android/camera_hal_manager.cpp | 33 +++++++++++++++++- > > > src/android/camera_hal_manager.h | 3 ++ > > > 4 files changed, 85 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp > b/src/android/camera_device.cpp > > > index 89044efa7ebe..50809c6ffbdc 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -6,6 +6,7 @@ > > > */ > > > > > > #include "camera_device.h" > > > +#include "camera_hal_config.h" > > > #include "camera_ops.h" > > > #include "post_processor.h" > > > > > > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> > > > CameraDevice::create(unsigned int id, > > > } > > > > > > /* > > > - * Initialize the camera static information. > > > + * Initialize the camera static information retrieved from the > > > + * Camera::properties or from the cameraConfigData. > > > + * > > > + * cameraConfigData is optional for external camera devices and can be > > > + * nullptr. > > > + * > > > * This method is called before the camera device is opened. > > > */ > > > -int CameraDevice::initialize() > > > +int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > > > { > > > - /* Initialize orientation and facing side of the camera. */ > > > + /* > > > + * Initialize orientation and facing side of the camera. > > > + * > > > + * If the libcamera::Camera provides those information as > retrieved > > > + * from firmware use them, otherwise fallback to values parsed > from > > > + * the configuration file. If the configuration file is not > > > available > > > + * the camera is external so its location and rotation can be > > > safely > > > + * defaulted. > > > + */ > > > const ControlList &properties = camera_->properties(); > > > > > > if (properties.contains(properties::Location)) { > > > @@ -464,12 +478,22 @@ int CameraDevice::initialize() > > > facing_ = CAMERA_FACING_EXTERNAL; > > > break; > > > } > > > + > > > + if (cameraConfigData && cameraConfigData->facing != -1 > && > > > + facing_ != cameraConfigData->facing) { > > > + LOG(HAL, Warning) > > > + << "Camera location does not match" > > > + << " configuration file. Using " << > > > facing_; > > > + } > > > + } else if (cameraConfigData) { > > > + if (cameraConfigData->facing == -1) { > > > + LOG(HAL, Error) > > > + << "Camera facing not in configuration > > > file"; > > > + return -EINVAL; > > > + } > > > + facing_ = cameraConfigData->facing; > > > } else { > > > - /* > > > - * \todo Retrieve the camera location from > configuration > > > file > > > - * if not available from the library. > > > - */ > > > - facing_ = CAMERA_FACING_FRONT; > > > + facing_ = CAMERA_FACING_EXTERNAL; > > > } > > > > > > /* > > > @@ -483,8 +507,24 @@ int CameraDevice::initialize() > > > if (properties.contains(properties::Rotation)) { > > > int rotation = properties.get(properties::Rotation); > > > orientation_ = (360 - rotation) % 360; > > > + if (cameraConfigData && cameraConfigData->rotation != > -1 && > > > + orientation_ != cameraConfigData->rotation) { > > > + LOG(HAL, Warning) > > > + << "Camera orientation does not match" > > > + << " configuration file. Using " << > > > orientation_; > > > + } > > > + } else if (cameraConfigData) { > > > + if (cameraConfigData->rotation == -1) { > > > + LOG(HAL, Error) > > > + << "Camera rotation not in > configuration > > > file"; > > > + return -EINVAL; > > > + } > > > + orientation_ = cameraConfigData->rotation; > > > + } else { > > > + orientation_ = 0; > > > } > > > > > > + /* Acquire the camera and initialize available stream > > > configurations. */ > > > int ret = camera_->acquire(); > > > if (ret) { > > > LOG(HAL, Error) << "Failed to temporarily acquire the > > > camera"; > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index 11bdfec8d587..9cc0d4005242 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -29,6 +29,7 @@ > > > #include "camera_worker.h" > > > #include "jpeg/encoder.h" > > > > > > +struct CameraConfigData; > > > class CameraDevice : protected libcamera::Loggable > > > { > > > public: > > > @@ -36,7 +37,7 @@ public: > > > > > > std::shared_ptr<libcamera::Camera> cam); > > > ~CameraDevice(); > > > > > > - int initialize(); > > > + int initialize(const CameraConfigData *cameraConfigData); > > > > > > int open(const hw_module_t *hardwareModule); > > > void close(); > > > diff --git a/src/android/camera_hal_manager.cpp > > > b/src/android/camera_hal_manager.cpp > > > index bf3fcda75237..f5b86974e8e3 100644 > > > --- a/src/android/camera_hal_manager.cpp > > > +++ b/src/android/camera_hal_manager.cpp > > > @@ -41,6 +41,15 @@ int CameraHalManager::init() > > > { > > > cameraManager_ = std::make_unique<CameraManager>(); > > > > > > + /* > > > + * If the configuration file is not available the HAL only > supports > > > + * external cameras. If it exists but it's not valid then error > > > out. > > > + */ > > > + if (halConfig_.exists() && !halConfig_.isValid()) { > > > + LOG(HAL, Error) << "HAL configuration file is not > valid"; > > > + return -EINVAL; > > > + } > > > + > > > /* Support camera hotplug. */ > > > cameraManager_->cameraAdded.connect(this, > > > &CameraHalManager::cameraAdded); > > > cameraManager_->cameraRemoved.connect(this, > > > &CameraHalManager::cameraRemoved); > > > @@ -100,6 +109,8 @@ void > > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > auto iter = cameraIdsMap_.find(cam->id()); > > > if (iter != cameraIdsMap_.end()) { > > > id = iter->second; > > > + if (id >= firstExternalCameraId_) > > > + isCameraExternal = true; > > > } else { > > > isCameraNew = true; > > > > > > @@ -117,7 +128,27 @@ void > > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > > > > /* Create a CameraDevice instance to wrap the libcamera > Camera. */ > > > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, > > > cam); > > > - int ret = camera->initialize(); > > > + > > > + /* > > > + * The configuration file must be valid, and contain a > > > corresponding > > > + * entry for internal cameras. External cameras can be > initialized > > > + * without configuration file. > > > + */ > > > + if (!isCameraExternal && !halConfig_.exists()) { > > > + LOG(HAL, Error) > > > + << "HAL configuration file is mandatory for > > > internal cameras"; > > > + return; > > > + } > > > + > > > + const CameraConfigData *cameraConfigData = > > > halConfig_.cameraConfigData(cam->id()); > > > + if (!isCameraExternal && !cameraConfigData) { > > > + LOG(HAL, Error) > > > + << "HAL configuration entry for internal > camera " > > > + << cam->id() << " is missing"; > > > + return; > > > + } > > > + > > > + int ret = camera->initialize(cameraConfigData); > > > if (ret) { > > > LOG(HAL, Error) << "Failed to initialize camera: " << > > > cam->id(); > > > return; > > > diff --git a/src/android/camera_hal_manager.h > > > b/src/android/camera_hal_manager.h > > > index d9bf27989965..af1581da6579 100644 > > > --- a/src/android/camera_hal_manager.h > > > +++ b/src/android/camera_hal_manager.h > > > @@ -19,6 +19,8 @@ > > > > > > #include <libcamera/camera_manager.h> > > > > > > +#include "camera_hal_config.h" > > > + > > > class CameraDevice; > > > > > > class CameraHalManager > > > @@ -50,6 +52,7 @@ private: > > > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > > > > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > > > + CameraHalConfig halConfig_; > > > > > > const camera_module_callbacks_t *callbacks_; > > > std::vector<std::unique_ptr<CameraDevice>> cameras_; > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
Hi Jacopo, Thanks for the efforts. The commits should be contained in 13971 and any later versions. On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > the camera properties for each created CameraDevice and initialize it > > > with them. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > When would you merge this series? > > My plan was to update my SDK to the latest version which contains the > CL to the libcamera and libcamera-configs ebuild, so I could have > tested one more time on a fresh image. Am I too paranoid ? > > Could you or Han-lin tell me which is the id of the first CPFE image > that contains: > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > I've tested those patches applied on my rather ancient SDK version, > but I would like to try with an out-of-the-box image > > Thanks > j > > > > > -Hiro > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 56 +++++++++++++++++++++++++----- > > > src/android/camera_device.h | 3 +- > > > src/android/camera_hal_manager.cpp | 33 +++++++++++++++++- > > > src/android/camera_hal_manager.h | 3 ++ > > > 4 files changed, 85 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 89044efa7ebe..50809c6ffbdc 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -6,6 +6,7 @@ > > > */ > > > > > > #include "camera_device.h" > > > +#include "camera_hal_config.h" > > > #include "camera_ops.h" > > > #include "post_processor.h" > > > > > > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> > > > CameraDevice::create(unsigned int id, > > > } > > > > > > /* > > > - * Initialize the camera static information. > > > + * Initialize the camera static information retrieved from the > > > + * Camera::properties or from the cameraConfigData. > > > + * > > > + * cameraConfigData is optional for external camera devices and can be > > > + * nullptr. > > > + * > > > * This method is called before the camera device is opened. > > > */ > > > -int CameraDevice::initialize() > > > +int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > > > { > > > - /* Initialize orientation and facing side of the camera. */ > > > + /* > > > + * Initialize orientation and facing side of the camera. > > > + * > > > + * If the libcamera::Camera provides those information as retrieved > > > + * from firmware use them, otherwise fallback to values parsed from > > > + * the configuration file. If the configuration file is not > > > available > > > + * the camera is external so its location and rotation can be > > > safely > > > + * defaulted. > > > + */ > > > const ControlList &properties = camera_->properties(); > > > > > > if (properties.contains(properties::Location)) { > > > @@ -464,12 +478,22 @@ int CameraDevice::initialize() > > > facing_ = CAMERA_FACING_EXTERNAL; > > > break; > > > } > > > + > > > + if (cameraConfigData && cameraConfigData->facing != -1 && > > > + facing_ != cameraConfigData->facing) { > > > + LOG(HAL, Warning) > > > + << "Camera location does not match" > > > + << " configuration file. Using " << > > > facing_; > > > + } > > > + } else if (cameraConfigData) { > > > + if (cameraConfigData->facing == -1) { > > > + LOG(HAL, Error) > > > + << "Camera facing not in configuration > > > file"; > > > + return -EINVAL; > > > + } > > > + facing_ = cameraConfigData->facing; > > > } else { > > > - /* > > > - * \todo Retrieve the camera location from configuration > > > file > > > - * if not available from the library. > > > - */ > > > - facing_ = CAMERA_FACING_FRONT; > > > + facing_ = CAMERA_FACING_EXTERNAL; > > > } > > > > > > /* > > > @@ -483,8 +507,24 @@ int CameraDevice::initialize() > > > if (properties.contains(properties::Rotation)) { > > > int rotation = properties.get(properties::Rotation); > > > orientation_ = (360 - rotation) % 360; > > > + if (cameraConfigData && cameraConfigData->rotation != -1 && > > > + orientation_ != cameraConfigData->rotation) { > > > + LOG(HAL, Warning) > > > + << "Camera orientation does not match" > > > + << " configuration file. Using " << > > > orientation_; > > > + } > > > + } else if (cameraConfigData) { > > > + if (cameraConfigData->rotation == -1) { > > > + LOG(HAL, Error) > > > + << "Camera rotation not in configuration > > > file"; > > > + return -EINVAL; > > > + } > > > + orientation_ = cameraConfigData->rotation; > > > + } else { > > > + orientation_ = 0; > > > } > > > > > > + /* Acquire the camera and initialize available stream > > > configurations. */ > > > int ret = camera_->acquire(); > > > if (ret) { > > > LOG(HAL, Error) << "Failed to temporarily acquire the > > > camera"; > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index 11bdfec8d587..9cc0d4005242 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -29,6 +29,7 @@ > > > #include "camera_worker.h" > > > #include "jpeg/encoder.h" > > > > > > +struct CameraConfigData; > > > class CameraDevice : protected libcamera::Loggable > > > { > > > public: > > > @@ -36,7 +37,7 @@ public: > > > > > > std::shared_ptr<libcamera::Camera> cam); > > > ~CameraDevice(); > > > > > > - int initialize(); > > > + int initialize(const CameraConfigData *cameraConfigData); > > > > > > int open(const hw_module_t *hardwareModule); > > > void close(); > > > diff --git a/src/android/camera_hal_manager.cpp > > > b/src/android/camera_hal_manager.cpp > > > index bf3fcda75237..f5b86974e8e3 100644 > > > --- a/src/android/camera_hal_manager.cpp > > > +++ b/src/android/camera_hal_manager.cpp > > > @@ -41,6 +41,15 @@ int CameraHalManager::init() > > > { > > > cameraManager_ = std::make_unique<CameraManager>(); > > > > > > + /* > > > + * If the configuration file is not available the HAL only supports > > > + * external cameras. If it exists but it's not valid then error > > > out. > > > + */ > > > + if (halConfig_.exists() && !halConfig_.isValid()) { > > > + LOG(HAL, Error) << "HAL configuration file is not valid"; > > > + return -EINVAL; > > > + } > > > + > > > /* Support camera hotplug. */ > > > cameraManager_->cameraAdded.connect(this, > > > &CameraHalManager::cameraAdded); > > > cameraManager_->cameraRemoved.connect(this, > > > &CameraHalManager::cameraRemoved); > > > @@ -100,6 +109,8 @@ void > > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > auto iter = cameraIdsMap_.find(cam->id()); > > > if (iter != cameraIdsMap_.end()) { > > > id = iter->second; > > > + if (id >= firstExternalCameraId_) > > > + isCameraExternal = true; > > > } else { > > > isCameraNew = true; > > > > > > @@ -117,7 +128,27 @@ void > > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > > > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, > > > cam); > > > - int ret = camera->initialize(); > > > + > > > + /* > > > + * The configuration file must be valid, and contain a > > > corresponding > > > + * entry for internal cameras. External cameras can be initialized > > > + * without configuration file. > > > + */ > > > + if (!isCameraExternal && !halConfig_.exists()) { > > > + LOG(HAL, Error) > > > + << "HAL configuration file is mandatory for > > > internal cameras"; > > > + return; > > > + } > > > + > > > + const CameraConfigData *cameraConfigData = > > > halConfig_.cameraConfigData(cam->id()); > > > + if (!isCameraExternal && !cameraConfigData) { > > > + LOG(HAL, Error) > > > + << "HAL configuration entry for internal camera " > > > + << cam->id() << " is missing"; > > > + return; > > > + } > > > + > > > + int ret = camera->initialize(cameraConfigData); > > > if (ret) { > > > LOG(HAL, Error) << "Failed to initialize camera: " << > > > cam->id(); > > > return; > > > diff --git a/src/android/camera_hal_manager.h > > > b/src/android/camera_hal_manager.h > > > index d9bf27989965..af1581da6579 100644 > > > --- a/src/android/camera_hal_manager.h > > > +++ b/src/android/camera_hal_manager.h > > > @@ -19,6 +19,8 @@ > > > > > > #include <libcamera/camera_manager.h> > > > > > > +#include "camera_hal_config.h" > > > + > > > class CameraDevice; > > > > > > class CameraHalManager > > > @@ -50,6 +52,7 @@ private: > > > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > > > > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > > > + CameraHalConfig halConfig_; > > > > > > const camera_module_callbacks_t *callbacks_; > > > std::vector<std::unique_ptr<CameraDevice>> cameras_; > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > >
Hi Hiro and Han-lin, On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > Hi Jacopo, > Thanks for the efforts. The commits should be contained in 13971 and > any later versions. Great, thanks, I have now verified my series works on an out-of-the-box image. However the manifests for the SDK in manifest-version have not yet catch up with the images and are stuck to R91. We'll have to wait before merging the series for the right manifest to land, so we can have both the image and the SDK at the same version. Thanks j > > On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Hiro, > > > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > > Hi Jacopo, > > > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > > the camera properties for each created CameraDevice and initialize it > > > > with them. > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > When would you merge this series? > > > > My plan was to update my SDK to the latest version which contains the > > CL to the libcamera and libcamera-configs ebuild, so I could have > > tested one more time on a fresh image. Am I too paranoid ? > > > > Could you or Han-lin tell me which is the id of the first CPFE image > > that contains: > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > > > I've tested those patches applied on my rather ancient SDK version, > > but I would like to try with an out-of-the-box image > > > > Thanks > > j > > > > > > > > -Hiro > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/android/camera_device.cpp | 56 +++++++++++++++++++++++++----- > > > > src/android/camera_device.h | 3 +- > > > > src/android/camera_hal_manager.cpp | 33 +++++++++++++++++- > > > > src/android/camera_hal_manager.h | 3 ++ > > > > 4 files changed, 85 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 89044efa7ebe..50809c6ffbdc 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -6,6 +6,7 @@ > > > > */ > > > > > > > > #include "camera_device.h" > > > > +#include "camera_hal_config.h" > > > > #include "camera_ops.h" > > > > #include "post_processor.h" > > > > > > > > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> > > > > CameraDevice::create(unsigned int id, > > > > } > > > > > > > > /* > > > > - * Initialize the camera static information. > > > > + * Initialize the camera static information retrieved from the > > > > + * Camera::properties or from the cameraConfigData. > > > > + * > > > > + * cameraConfigData is optional for external camera devices and can be > > > > + * nullptr. > > > > + * > > > > * This method is called before the camera device is opened. > > > > */ > > > > -int CameraDevice::initialize() > > > > +int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > > > > { > > > > - /* Initialize orientation and facing side of the camera. */ > > > > + /* > > > > + * Initialize orientation and facing side of the camera. > > > > + * > > > > + * If the libcamera::Camera provides those information as retrieved > > > > + * from firmware use them, otherwise fallback to values parsed from > > > > + * the configuration file. If the configuration file is not > > > > available > > > > + * the camera is external so its location and rotation can be > > > > safely > > > > + * defaulted. > > > > + */ > > > > const ControlList &properties = camera_->properties(); > > > > > > > > if (properties.contains(properties::Location)) { > > > > @@ -464,12 +478,22 @@ int CameraDevice::initialize() > > > > facing_ = CAMERA_FACING_EXTERNAL; > > > > break; > > > > } > > > > + > > > > + if (cameraConfigData && cameraConfigData->facing != -1 && > > > > + facing_ != cameraConfigData->facing) { > > > > + LOG(HAL, Warning) > > > > + << "Camera location does not match" > > > > + << " configuration file. Using " << > > > > facing_; > > > > + } > > > > + } else if (cameraConfigData) { > > > > + if (cameraConfigData->facing == -1) { > > > > + LOG(HAL, Error) > > > > + << "Camera facing not in configuration > > > > file"; > > > > + return -EINVAL; > > > > + } > > > > + facing_ = cameraConfigData->facing; > > > > } else { > > > > - /* > > > > - * \todo Retrieve the camera location from configuration > > > > file > > > > - * if not available from the library. > > > > - */ > > > > - facing_ = CAMERA_FACING_FRONT; > > > > + facing_ = CAMERA_FACING_EXTERNAL; > > > > } > > > > > > > > /* > > > > @@ -483,8 +507,24 @@ int CameraDevice::initialize() > > > > if (properties.contains(properties::Rotation)) { > > > > int rotation = properties.get(properties::Rotation); > > > > orientation_ = (360 - rotation) % 360; > > > > + if (cameraConfigData && cameraConfigData->rotation != -1 && > > > > + orientation_ != cameraConfigData->rotation) { > > > > + LOG(HAL, Warning) > > > > + << "Camera orientation does not match" > > > > + << " configuration file. Using " << > > > > orientation_; > > > > + } > > > > + } else if (cameraConfigData) { > > > > + if (cameraConfigData->rotation == -1) { > > > > + LOG(HAL, Error) > > > > + << "Camera rotation not in configuration > > > > file"; > > > > + return -EINVAL; > > > > + } > > > > + orientation_ = cameraConfigData->rotation; > > > > + } else { > > > > + orientation_ = 0; > > > > } > > > > > > > > + /* Acquire the camera and initialize available stream > > > > configurations. */ > > > > int ret = camera_->acquire(); > > > > if (ret) { > > > > LOG(HAL, Error) << "Failed to temporarily acquire the > > > > camera"; > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > > index 11bdfec8d587..9cc0d4005242 100644 > > > > --- a/src/android/camera_device.h > > > > +++ b/src/android/camera_device.h > > > > @@ -29,6 +29,7 @@ > > > > #include "camera_worker.h" > > > > #include "jpeg/encoder.h" > > > > > > > > +struct CameraConfigData; > > > > class CameraDevice : protected libcamera::Loggable > > > > { > > > > public: > > > > @@ -36,7 +37,7 @@ public: > > > > > > > > std::shared_ptr<libcamera::Camera> cam); > > > > ~CameraDevice(); > > > > > > > > - int initialize(); > > > > + int initialize(const CameraConfigData *cameraConfigData); > > > > > > > > int open(const hw_module_t *hardwareModule); > > > > void close(); > > > > diff --git a/src/android/camera_hal_manager.cpp > > > > b/src/android/camera_hal_manager.cpp > > > > index bf3fcda75237..f5b86974e8e3 100644 > > > > --- a/src/android/camera_hal_manager.cpp > > > > +++ b/src/android/camera_hal_manager.cpp > > > > @@ -41,6 +41,15 @@ int CameraHalManager::init() > > > > { > > > > cameraManager_ = std::make_unique<CameraManager>(); > > > > > > > > + /* > > > > + * If the configuration file is not available the HAL only supports > > > > + * external cameras. If it exists but it's not valid then error > > > > out. > > > > + */ > > > > + if (halConfig_.exists() && !halConfig_.isValid()) { > > > > + LOG(HAL, Error) << "HAL configuration file is not valid"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > /* Support camera hotplug. */ > > > > cameraManager_->cameraAdded.connect(this, > > > > &CameraHalManager::cameraAdded); > > > > cameraManager_->cameraRemoved.connect(this, > > > > &CameraHalManager::cameraRemoved); > > > > @@ -100,6 +109,8 @@ void > > > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > > auto iter = cameraIdsMap_.find(cam->id()); > > > > if (iter != cameraIdsMap_.end()) { > > > > id = iter->second; > > > > + if (id >= firstExternalCameraId_) > > > > + isCameraExternal = true; > > > > } else { > > > > isCameraNew = true; > > > > > > > > @@ -117,7 +128,27 @@ void > > > > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > > > > > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */ > > > > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, > > > > cam); > > > > - int ret = camera->initialize(); > > > > + > > > > + /* > > > > + * The configuration file must be valid, and contain a > > > > corresponding > > > > + * entry for internal cameras. External cameras can be initialized > > > > + * without configuration file. > > > > + */ > > > > + if (!isCameraExternal && !halConfig_.exists()) { > > > > + LOG(HAL, Error) > > > > + << "HAL configuration file is mandatory for > > > > internal cameras"; > > > > + return; > > > > + } > > > > + > > > > + const CameraConfigData *cameraConfigData = > > > > halConfig_.cameraConfigData(cam->id()); > > > > + if (!isCameraExternal && !cameraConfigData) { > > > > + LOG(HAL, Error) > > > > + << "HAL configuration entry for internal camera " > > > > + << cam->id() << " is missing"; > > > > + return; > > > > + } > > > > + > > > > + int ret = camera->initialize(cameraConfigData); > > > > if (ret) { > > > > LOG(HAL, Error) << "Failed to initialize camera: " << > > > > cam->id(); > > > > return; > > > > diff --git a/src/android/camera_hal_manager.h > > > > b/src/android/camera_hal_manager.h > > > > index d9bf27989965..af1581da6579 100644 > > > > --- a/src/android/camera_hal_manager.h > > > > +++ b/src/android/camera_hal_manager.h > > > > @@ -19,6 +19,8 @@ > > > > > > > > #include <libcamera/camera_manager.h> > > > > > > > > +#include "camera_hal_config.h" > > > > + > > > > class CameraDevice; > > > > > > > > class CameraHalManager > > > > @@ -50,6 +52,7 @@ private: > > > > CameraDevice *cameraDeviceFromHalId(unsigned int id); > > > > > > > > std::unique_ptr<libcamera::CameraManager> cameraManager_; > > > > + CameraHalConfig halConfig_; > > > > > > > > const camera_module_callbacks_t *callbacks_; > > > > std::vector<std::unique_ptr<CameraDevice>> cameras_; > > > > -- > > > > 2.31.1 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > -- > Cheers. > Hanlin Chen
Hi Jacopo, On Fri, May 21, 2021 at 05:56:41PM +0200, Jacopo Mondi wrote: > On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > > Hi Jacopo, > > Thanks for the efforts. The commits should be contained in 13971 and > > any later versions. > > Great, thanks, I have now verified my series works on an > out-of-the-box image. > > However the manifests for the SDK in manifest-version have not yet > catch up with the images and are stuck to R91. We'll have to wait > before merging the series for the right manifest to land, so we can > have both the image and the SDK at the same version. Seems like it's available now. I'll kick off a build with full/buildspecs/92/13971.0.0-rc1.xml and report the results. > > On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi wrote: > > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > > > the camera properties for each created CameraDevice and initialize it > > > > > with them. > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > When would you merge this series? > > > > > > My plan was to update my SDK to the latest version which contains the > > > CL to the libcamera and libcamera-configs ebuild, so I could have > > > tested one more time on a fresh image. Am I too paranoid ? > > > > > > Could you or Han-lin tell me which is the id of the first CPFE image > > > that contains: > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > > > > > I've tested those patches applied on my rather ancient SDK version, > > > but I would like to try with an out-of-the-box image > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
On Sun, May 23, 2021 at 03:32:08AM +0300, Laurent Pinchart wrote: > On Fri, May 21, 2021 at 05:56:41PM +0200, Jacopo Mondi wrote: > > On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > > > Hi Jacopo, > > > Thanks for the efforts. The commits should be contained in 13971 and > > > any later versions. > > > > Great, thanks, I have now verified my series works on an > > out-of-the-box image. > > > > However the manifests for the SDK in manifest-version have not yet > > catch up with the images and are stuck to R91. We'll have to wait > > before merging the series for the right manifest to land, so we can > > have both the image and the SDK at the same version. > > Seems like it's available now. I'll kick off a build with > full/buildspecs/92/13971.0.0-rc1.xml and report the results. I had a build error in the chromeos-base/regions packages: 14:49:49 >>> Compiling source in /build/soraka-libcamera/tmp/portage/chromeos-base/regions-0.0.1-r2020/work/regions-0.0.1/regions ... usage: regions.py [-h] [--format {human-readable,csv,json,yaml}] [--all] [--notes] [--include_pseudolocales] [--output OUTPUT] [--overlay OVERLAY] regions.py: error: unrecognized arguments: This was caused by the ebuild passing an empty argument to the regions.py script (a `print(args)` in regions.py clearly shows this). I've fixed it with the following patch. diff --git a/chromeos-base/regions/regions-0.0.1-r2020.ebuild b/chromeos-base/regions/regions-0.0.1-r2020.ebuild index 22684c2bf7e8..06b1a82f027c 100644 --- a/chromeos-base/regions/regions-0.0.1-r2020.ebuild +++ b/chromeos-base/regions/regions-0.0.1-r2020.ebuild @@ -31,7 +31,7 @@ src_unpack() { } src_compile() { - ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" "$(usex cros-debug "--include_pseudolocales" "")" + ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" $(usex cros-debug "--include_pseudolocales" "") } src_test() { I'm curious, as the offending code was added in commit a05940690157c100c902f41e8118400f2183eb3b Author: Matt Stark <msta@google.com> AuthorDate: Mon Apr 19 11:16:04 2021 +1000 Commit: Commit Bot <commit-bot@chromium.org> CommitDate: Wed May 5 05:06:19 2021 +0000 When creating a chrome OS debug build, add pseudolocales to the build. which is more than two weeks old, and I would have expected the issue to be caught. I can't see any fix in neither the chromeos-overlay (for the build) nor the platform2 (for regions.py) main branches. I can't rule out that it could be specific to my environment, as I haven( recreated the SDK from scratch, I've removed the build directory and updated the chroot with update_chroot. Could anyone on the Chrome OS site which if they can reproduce this issue when the cros-debug use flag isn't set ? I've added a comment to https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2879390. Other than that, the build is still ongoing, I'll report when it completes. > > > On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi wrote: > > > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > > > > the camera properties for each created CameraDevice and initialize it > > > > > > with them. > > > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > > > When would you merge this series? > > > > > > > > My plan was to update my SDK to the latest version which contains the > > > > CL to the libcamera and libcamera-configs ebuild, so I could have > > > > tested one more time on a fresh image. Am I too paranoid ? > > > > > > > > Could you or Han-lin tell me which is the id of the first CPFE image > > > > that contains: > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > > > > > > > I've tested those patches applied on my rather ancient SDK version, > > > > but I would like to try with an out-of-the-box image > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
On Sun, May 23, 2021 at 03:56:54PM +0300, Laurent Pinchart wrote: > On Sun, May 23, 2021 at 03:32:08AM +0300, Laurent Pinchart wrote: > > On Fri, May 21, 2021 at 05:56:41PM +0200, Jacopo Mondi wrote: > > > On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > > > > Hi Jacopo, > > > > Thanks for the efforts. The commits should be contained in 13971 and > > > > any later versions. > > > > > > Great, thanks, I have now verified my series works on an > > > out-of-the-box image. > > > > > > However the manifests for the SDK in manifest-version have not yet > > > catch up with the images and are stuck to R91. We'll have to wait > > > before merging the series for the right manifest to land, so we can > > > have both the image and the SDK at the same version. > > > > Seems like it's available now. I'll kick off a build with > > full/buildspecs/92/13971.0.0-rc1.xml and report the results. > > I had a build error in the chromeos-base/regions packages: > > 14:49:49 >>> Compiling source in /build/soraka-libcamera/tmp/portage/chromeos-base/regions-0.0.1-r2020/work/regions-0.0.1/regions ... > usage: regions.py [-h] [--format {human-readable,csv,json,yaml}] [--all] [--notes] [--include_pseudolocales] [--output OUTPUT] [--overlay OVERLAY] > regions.py: error: unrecognized arguments: > > This was caused by the ebuild passing an empty argument to the > regions.py script (a `print(args)` in regions.py clearly shows this). > I've fixed it with the following patch. > > diff --git a/chromeos-base/regions/regions-0.0.1-r2020.ebuild b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > index 22684c2bf7e8..06b1a82f027c 100644 > --- a/chromeos-base/regions/regions-0.0.1-r2020.ebuild > +++ b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > @@ -31,7 +31,7 @@ src_unpack() { > } > > src_compile() { > - ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" "$(usex cros-debug "--include_pseudolocales" "")" > + ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" $(usex cros-debug "--include_pseudolocales" "") > } > > src_test() { > > I'm curious, as the offending code was added in > > commit a05940690157c100c902f41e8118400f2183eb3b > Author: Matt Stark <msta@google.com> > AuthorDate: Mon Apr 19 11:16:04 2021 +1000 > Commit: Commit Bot <commit-bot@chromium.org> > CommitDate: Wed May 5 05:06:19 2021 +0000 > > When creating a chrome OS debug build, add pseudolocales to the build. > > which is more than two weeks old, and I would have expected the issue to > be caught. I can't see any fix in neither the chromeos-overlay (for the > build) nor the platform2 (for regions.py) main branches. > > I can't rule out that it could be specific to my environment, as I > haven( recreated the SDK from scratch, I've removed the build directory > and updated the chroot with update_chroot. > > Could anyone on the Chrome OS site which if they can reproduce this > issue when the cros-debug use flag isn't set ? I've added a comment to > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2879390. > > Other than that, the build is still ongoing, I'll report when it > completes. Or when it fails again :-S The next issue is related to cryptohome. cryptohome-0.0.1-r3658: 17:36:31 >>> Install chromeos-base/cryptohome-0.0.1-r3658 into /build/soraka-libcamera/tmp/portage/chromeos-base/cryptohome-0.0.1-r3658/image cryptohome-0.0.1-r3658: * ERROR: chromeos-base/cryptohome-0.0.1-r3658::chromiumos failed (install phase): cryptohome-0.0.1-r3658: * direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled! cryptohome-0.0.1-r3658: * cryptohome-0.0.1-r3658: * Call stack: cryptohome-0.0.1-r3658: * ebuild.sh, line 125: Called src_install cryptohome-0.0.1-r3658: * environment, line 4321: Called die cryptohome-0.0.1-r3658: * The specific snippet of code: cryptohome-0.0.1-r3658: * die "direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled!"; The error check was added in the ebuild in commit c9cbca71d43158c5dc1bd366224087e34e7905b7 Author: Daniil Lunev <dlunev@chromium.org> AuthorDate: Fri Feb 12 10:17:01 2021 +1100 Commit: Commit Bot <commit-bot@chromium.org> CommitDate: Mon Mar 8 23:30:29 2021 +0000 cryptohome: rollout control for fscrypt_v2 The uprev-4-to-5 USE flag got enabled for Soraka (and Nautilus) in commit 14bb32f0477d5956f86802f80e8e4d0ab8c7b9bc Author: Daniil Lunev <dlunev@chromium.org> AuthorDate: Wed Mar 10 09:22:06 2021 +1100 Commit: Commit Bot <commit-bot@chromium.org> CommitDate: Tue Mar 9 23:56:23 2021 +0000 overlays: add a special tag for 4->5 uprevs The direncription_allow_v2 USE flag got enabled for soraka-libcamera in commit 6b846af8ee5752878fc2e125c612719b3467cc3b Author: Daniil Lunev <dlunev@chromium.org> AuthorDate: Mon Mar 1 09:28:11 2021 +1100 Commit: Commit Bot <commit-bot@chromium.org> CommitDate: Tue Mar 2 06:14:31 2021 +0000 overlays: enable fscrypt v2 attempt on post 5.4 boards Disabling direncription_allow_v2 manually allows the build to continue, but I don't think that's a very good idea. > > > > On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi wrote: > > > > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > > > > > the camera properties for each created CameraDevice and initialize it > > > > > > > with them. > > > > > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > > > > > When would you merge this series? > > > > > > > > > > My plan was to update my SDK to the latest version which contains the > > > > > CL to the libcamera and libcamera-configs ebuild, so I could have > > > > > tested one more time on a fresh image. Am I too paranoid ? > > > > > > > > > > Could you or Han-lin tell me which is the id of the first CPFE image > > > > > that contains: > > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > > > > > > > > > I've tested those patches applied on my rather ancient SDK version, > > > > > but I would like to try with an out-of-the-box image > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
On Sun, May 23, 2021 at 06:21:19PM +0300, Laurent Pinchart wrote: > On Sun, May 23, 2021 at 03:56:54PM +0300, Laurent Pinchart wrote: > > On Sun, May 23, 2021 at 03:32:08AM +0300, Laurent Pinchart wrote: > > > On Fri, May 21, 2021 at 05:56:41PM +0200, Jacopo Mondi wrote: > > > > On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > > > > > Hi Jacopo, > > > > > Thanks for the efforts. The commits should be contained in 13971 and > > > > > any later versions. > > > > > > > > Great, thanks, I have now verified my series works on an > > > > out-of-the-box image. > > > > > > > > However the manifests for the SDK in manifest-version have not yet > > > > catch up with the images and are stuck to R91. We'll have to wait > > > > before merging the series for the right manifest to land, so we can > > > > have both the image and the SDK at the same version. > > > > > > Seems like it's available now. I'll kick off a build with > > > full/buildspecs/92/13971.0.0-rc1.xml and report the results. > > > > I had a build error in the chromeos-base/regions packages: > > > > 14:49:49 >>> Compiling source in /build/soraka-libcamera/tmp/portage/chromeos-base/regions-0.0.1-r2020/work/regions-0.0.1/regions ... > > usage: regions.py [-h] [--format {human-readable,csv,json,yaml}] [--all] [--notes] [--include_pseudolocales] [--output OUTPUT] [--overlay OVERLAY] > > regions.py: error: unrecognized arguments: > > > > This was caused by the ebuild passing an empty argument to the > > regions.py script (a `print(args)` in regions.py clearly shows this). > > I've fixed it with the following patch. > > > > diff --git a/chromeos-base/regions/regions-0.0.1-r2020.ebuild b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > index 22684c2bf7e8..06b1a82f027c 100644 > > --- a/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > +++ b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > @@ -31,7 +31,7 @@ src_unpack() { > > } > > > > src_compile() { > > - ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" "$(usex cros-debug "--include_pseudolocales" "")" > > + ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" $(usex cros-debug "--include_pseudolocales" "") > > } > > > > src_test() { > > > > I'm curious, as the offending code was added in > > > > commit a05940690157c100c902f41e8118400f2183eb3b > > Author: Matt Stark <msta@google.com> > > AuthorDate: Mon Apr 19 11:16:04 2021 +1000 > > Commit: Commit Bot <commit-bot@chromium.org> > > CommitDate: Wed May 5 05:06:19 2021 +0000 > > > > When creating a chrome OS debug build, add pseudolocales to the build. > > > > which is more than two weeks old, and I would have expected the issue to > > be caught. I can't see any fix in neither the chromeos-overlay (for the > > build) nor the platform2 (for regions.py) main branches. > > > > I can't rule out that it could be specific to my environment, as I > > haven( recreated the SDK from scratch, I've removed the build directory > > and updated the chroot with update_chroot. > > > > Could anyone on the Chrome OS site which if they can reproduce this > > issue when the cros-debug use flag isn't set ? I've added a comment to > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2879390. > > > > Other than that, the build is still ongoing, I'll report when it > > completes. > > Or when it fails again :-S > > The next issue is related to cryptohome. > > cryptohome-0.0.1-r3658: 17:36:31 >>> Install chromeos-base/cryptohome-0.0.1-r3658 into /build/soraka-libcamera/tmp/portage/chromeos-base/cryptohome-0.0.1-r3658/image > cryptohome-0.0.1-r3658: * ERROR: chromeos-base/cryptohome-0.0.1-r3658::chromiumos failed (install phase): > cryptohome-0.0.1-r3658: * direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled! > cryptohome-0.0.1-r3658: * > cryptohome-0.0.1-r3658: * Call stack: > cryptohome-0.0.1-r3658: * ebuild.sh, line 125: Called src_install > cryptohome-0.0.1-r3658: * environment, line 4321: Called die > cryptohome-0.0.1-r3658: * The specific snippet of code: > cryptohome-0.0.1-r3658: * die "direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled!"; > > > The error check was added in the ebuild in > > commit c9cbca71d43158c5dc1bd366224087e34e7905b7 > Author: Daniil Lunev <dlunev@chromium.org> > AuthorDate: Fri Feb 12 10:17:01 2021 +1100 > Commit: Commit Bot <commit-bot@chromium.org> > CommitDate: Mon Mar 8 23:30:29 2021 +0000 > > cryptohome: rollout control for fscrypt_v2 > > > The uprev-4-to-5 USE flag got enabled for Soraka (and Nautilus) in > > commit 14bb32f0477d5956f86802f80e8e4d0ab8c7b9bc > Author: Daniil Lunev <dlunev@chromium.org> > AuthorDate: Wed Mar 10 09:22:06 2021 +1100 > Commit: Commit Bot <commit-bot@chromium.org> > CommitDate: Tue Mar 9 23:56:23 2021 +0000 > > overlays: add a special tag for 4->5 uprevs > > > The direncription_allow_v2 USE flag got enabled for soraka-libcamera in > > commit 6b846af8ee5752878fc2e125c612719b3467cc3b > Author: Daniil Lunev <dlunev@chromium.org> > AuthorDate: Mon Mar 1 09:28:11 2021 +1100 > Commit: Commit Bot <commit-bot@chromium.org> > CommitDate: Tue Mar 2 06:14:31 2021 +0000 > > overlays: enable fscrypt v2 attempt on post 5.4 boards > > > Disabling direncription_allow_v2 manually allows the build to continue, > but I don't think that's a very good idea. After fixing (or working around) those two issues, the build completes. I'll now try to build the latest version (R93-13987), to see if some of the problems have been fixed. > > > > > On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi wrote: > > > > > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > > > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > > > > > > the camera properties for each created CameraDevice and initialize it > > > > > > > > with them. > > > > > > > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > > > > > > > When would you merge this series? > > > > > > > > > > > > My plan was to update my SDK to the latest version which contains the > > > > > > CL to the libcamera and libcamera-configs ebuild, so I could have > > > > > > tested one more time on a fresh image. Am I too paranoid ? > > > > > > > > > > > > Could you or Han-lin tell me which is the id of the first CPFE image > > > > > > that contains: > > > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > > > > > > > > > > > I've tested those patches applied on my rather ancient SDK version, > > > > > > but I would like to try with an out-of-the-box image > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
On Sun, May 23, 2021 at 07:44:45PM +0300, Laurent Pinchart wrote: > On Sun, May 23, 2021 at 06:21:19PM +0300, Laurent Pinchart wrote: > > On Sun, May 23, 2021 at 03:56:54PM +0300, Laurent Pinchart wrote: > > > On Sun, May 23, 2021 at 03:32:08AM +0300, Laurent Pinchart wrote: > > > > On Fri, May 21, 2021 at 05:56:41PM +0200, Jacopo Mondi wrote: > > > > > On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > > > > > > Hi Jacopo, > > > > > > Thanks for the efforts. The commits should be contained in 13971 and > > > > > > any later versions. > > > > > > > > > > Great, thanks, I have now verified my series works on an > > > > > out-of-the-box image. > > > > > > > > > > However the manifests for the SDK in manifest-version have not yet > > > > > catch up with the images and are stuck to R91. We'll have to wait > > > > > before merging the series for the right manifest to land, so we can > > > > > have both the image and the SDK at the same version. > > > > > > > > Seems like it's available now. I'll kick off a build with > > > > full/buildspecs/92/13971.0.0-rc1.xml and report the results. > > > > > > I had a build error in the chromeos-base/regions packages: > > > > > > 14:49:49 >>> Compiling source in /build/soraka-libcamera/tmp/portage/chromeos-base/regions-0.0.1-r2020/work/regions-0.0.1/regions ... > > > usage: regions.py [-h] [--format {human-readable,csv,json,yaml}] [--all] [--notes] [--include_pseudolocales] [--output OUTPUT] [--overlay OVERLAY] > > > regions.py: error: unrecognized arguments: > > > > > > This was caused by the ebuild passing an empty argument to the > > > regions.py script (a `print(args)` in regions.py clearly shows this). > > > I've fixed it with the following patch. > > > > > > diff --git a/chromeos-base/regions/regions-0.0.1-r2020.ebuild b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > > index 22684c2bf7e8..06b1a82f027c 100644 > > > --- a/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > > +++ b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > > @@ -31,7 +31,7 @@ src_unpack() { > > > } > > > > > > src_compile() { > > > - ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" "$(usex cros-debug "--include_pseudolocales" "")" > > > + ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" $(usex cros-debug "--include_pseudolocales" "") > > > } > > > > > > src_test() { > > > > > > I'm curious, as the offending code was added in > > > > > > commit a05940690157c100c902f41e8118400f2183eb3b > > > Author: Matt Stark <msta@google.com> > > > AuthorDate: Mon Apr 19 11:16:04 2021 +1000 > > > Commit: Commit Bot <commit-bot@chromium.org> > > > CommitDate: Wed May 5 05:06:19 2021 +0000 > > > > > > When creating a chrome OS debug build, add pseudolocales to the build. > > > > > > which is more than two weeks old, and I would have expected the issue to > > > be caught. I can't see any fix in neither the chromeos-overlay (for the > > > build) nor the platform2 (for regions.py) main branches. > > > > > > I can't rule out that it could be specific to my environment, as I > > > haven( recreated the SDK from scratch, I've removed the build directory > > > and updated the chroot with update_chroot. > > > > > > Could anyone on the Chrome OS site which if they can reproduce this > > > issue when the cros-debug use flag isn't set ? I've added a comment to > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2879390. > > > > > > Other than that, the build is still ongoing, I'll report when it > > > completes. > > > > Or when it fails again :-S > > > > The next issue is related to cryptohome. > > > > cryptohome-0.0.1-r3658: 17:36:31 >>> Install chromeos-base/cryptohome-0.0.1-r3658 into /build/soraka-libcamera/tmp/portage/chromeos-base/cryptohome-0.0.1-r3658/image > > cryptohome-0.0.1-r3658: * ERROR: chromeos-base/cryptohome-0.0.1-r3658::chromiumos failed (install phase): > > cryptohome-0.0.1-r3658: * direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled! > > cryptohome-0.0.1-r3658: * > > cryptohome-0.0.1-r3658: * Call stack: > > cryptohome-0.0.1-r3658: * ebuild.sh, line 125: Called src_install > > cryptohome-0.0.1-r3658: * environment, line 4321: Called die > > cryptohome-0.0.1-r3658: * The specific snippet of code: > > cryptohome-0.0.1-r3658: * die "direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled!"; > > > > > > The error check was added in the ebuild in > > > > commit c9cbca71d43158c5dc1bd366224087e34e7905b7 > > Author: Daniil Lunev <dlunev@chromium.org> > > AuthorDate: Fri Feb 12 10:17:01 2021 +1100 > > Commit: Commit Bot <commit-bot@chromium.org> > > CommitDate: Mon Mar 8 23:30:29 2021 +0000 > > > > cryptohome: rollout control for fscrypt_v2 > > > > > > The uprev-4-to-5 USE flag got enabled for Soraka (and Nautilus) in > > > > commit 14bb32f0477d5956f86802f80e8e4d0ab8c7b9bc > > Author: Daniil Lunev <dlunev@chromium.org> > > AuthorDate: Wed Mar 10 09:22:06 2021 +1100 > > Commit: Commit Bot <commit-bot@chromium.org> > > CommitDate: Tue Mar 9 23:56:23 2021 +0000 > > > > overlays: add a special tag for 4->5 uprevs > > > > > > The direncription_allow_v2 USE flag got enabled for soraka-libcamera in > > > > commit 6b846af8ee5752878fc2e125c612719b3467cc3b > > Author: Daniil Lunev <dlunev@chromium.org> > > AuthorDate: Mon Mar 1 09:28:11 2021 +1100 > > Commit: Commit Bot <commit-bot@chromium.org> > > CommitDate: Tue Mar 2 06:14:31 2021 +0000 > > > > overlays: enable fscrypt v2 attempt on post 5.4 boards > > > > > > Disabling direncription_allow_v2 manually allows the build to continue, > > but I don't think that's a very good idea. > > After fixing (or working around) those two issues, the build completes. > I'll now try to build the latest version (R93-13987), to see if some of > the problems have been fixed. Exact same issues with R93-13987. Other than that, it compiles fine. I also confirm that Soraka finally produces sound :-) > > > > > > On Fri, May 21, 2021 at 3:29 PM Jacopo Mondi wrote: > > > > > > > On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote: > > > > > > > > On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > > > > > > > > Open the HAL configuration file in the Camera HAL manager and get > > > > > > > > > the camera properties for each created CameraDevice and initialize it > > > > > > > > > with them. > > > > > > > > > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > > > > > > > > > When would you merge this series? > > > > > > > > > > > > > > My plan was to update my SDK to the latest version which contains the > > > > > > > CL to the libcamera and libcamera-configs ebuild, so I could have > > > > > > > tested one more time on a fresh image. Am I too paranoid ? > > > > > > > > > > > > > > Could you or Han-lin tell me which is the id of the first CPFE image > > > > > > > that contains: > > > > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093 > > > > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535 > > > > > > > > > > > > > > I've tested those patches applied on my rather ancient SDK version, > > > > > > > but I would like to try with an out-of-the-box image > > > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Hi Laurent, all On Mon, May 24, 2021 at 01:08:53AM +0300, Laurent Pinchart wrote: > On Sun, May 23, 2021 at 07:44:45PM +0300, Laurent Pinchart wrote: > > On Sun, May 23, 2021 at 06:21:19PM +0300, Laurent Pinchart wrote: > > > On Sun, May 23, 2021 at 03:56:54PM +0300, Laurent Pinchart wrote: > > > > On Sun, May 23, 2021 at 03:32:08AM +0300, Laurent Pinchart wrote: > > > > > On Fri, May 21, 2021 at 05:56:41PM +0200, Jacopo Mondi wrote: > > > > > > On Fri, May 21, 2021 at 03:41:10PM +0800, Han-lin Chen wrote: > > > > > > > Hi Jacopo, > > > > > > > Thanks for the efforts. The commits should be contained in 13971 and > > > > > > > any later versions. > > > > > > > > > > > > Great, thanks, I have now verified my series works on an > > > > > > out-of-the-box image. > > > > > > > > > > > > However the manifests for the SDK in manifest-version have not yet > > > > > > catch up with the images and are stuck to R91. We'll have to wait > > > > > > before merging the series for the right manifest to land, so we can > > > > > > have both the image and the SDK at the same version. > > > > > > > > > > Seems like it's available now. I'll kick off a build with > > > > > full/buildspecs/92/13971.0.0-rc1.xml and report the results. > > > > > > > > I had a build error in the chromeos-base/regions packages: > > > > > > > > 14:49:49 >>> Compiling source in /build/soraka-libcamera/tmp/portage/chromeos-base/regions-0.0.1-r2020/work/regions-0.0.1/regions ... > > > > usage: regions.py [-h] [--format {human-readable,csv,json,yaml}] [--all] [--notes] [--include_pseudolocales] [--output OUTPUT] [--overlay OVERLAY] > > > > regions.py: error: unrecognized arguments: > > > > > > > > This was caused by the ebuild passing an empty argument to the > > > > regions.py script (a `print(args)` in regions.py clearly shows this). > > > > I've fixed it with the following patch. > > > > > > > > diff --git a/chromeos-base/regions/regions-0.0.1-r2020.ebuild b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > > > index 22684c2bf7e8..06b1a82f027c 100644 > > > > --- a/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > > > +++ b/chromeos-base/regions/regions-0.0.1-r2020.ebuild > > > > @@ -31,7 +31,7 @@ src_unpack() { > > > > } > > > > > > > > src_compile() { > > > > - ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" "$(usex cros-debug "--include_pseudolocales" "")" > > > > + ./regions.py --format=json --output "${WORKDIR}/cros-regions.json" $(usex cros-debug "--include_pseudolocales" "") > > > > } > > > > > > > > src_test() { > > > > > > > > I'm curious, as the offending code was added in > > > > > > > > commit a05940690157c100c902f41e8118400f2183eb3b > > > > Author: Matt Stark <msta@google.com> > > > > AuthorDate: Mon Apr 19 11:16:04 2021 +1000 > > > > Commit: Commit Bot <commit-bot@chromium.org> > > > > CommitDate: Wed May 5 05:06:19 2021 +0000 > > > > > > > > When creating a chrome OS debug build, add pseudolocales to the build. > > > > > > > > which is more than two weeks old, and I would have expected the issue to > > > > be caught. I can't see any fix in neither the chromeos-overlay (for the > > > > build) nor the platform2 (for regions.py) main branches. > > > > > > > > I can't rule out that it could be specific to my environment, as I > > > > haven( recreated the SDK from scratch, I've removed the build directory > > > > and updated the chroot with update_chroot. > > > > > > > > Could anyone on the Chrome OS site which if they can reproduce this > > > > issue when the cros-debug use flag isn't set ? I've added a comment to > > > > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2879390. > > > > > > > > Other than that, the build is still ongoing, I'll report when it > > > > completes. > > > > > > Or when it fails again :-S > > > > > > The next issue is related to cryptohome. > > > > > > cryptohome-0.0.1-r3658: 17:36:31 >>> Install chromeos-base/cryptohome-0.0.1-r3658 into /build/soraka-libcamera/tmp/portage/chromeos-base/cryptohome-0.0.1-r3658/image > > > cryptohome-0.0.1-r3658: * ERROR: chromeos-base/cryptohome-0.0.1-r3658::chromiumos failed (install phase): > > > cryptohome-0.0.1-r3658: * direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled! > > > cryptohome-0.0.1-r3658: * > > > cryptohome-0.0.1-r3658: * Call stack: > > > cryptohome-0.0.1-r3658: * ebuild.sh, line 125: Called src_install > > > cryptohome-0.0.1-r3658: * environment, line 4321: Called die > > > cryptohome-0.0.1-r3658: * The specific snippet of code: > > > cryptohome-0.0.1-r3658: * die "direncription_allow_v2 is enabled where it shouldn't be. Do you need to change the board overlay? Note, uprev boards should have it disabled!"; > > > > > > > > > The error check was added in the ebuild in > > > > > > commit c9cbca71d43158c5dc1bd366224087e34e7905b7 > > > Author: Daniil Lunev <dlunev@chromium.org> > > > AuthorDate: Fri Feb 12 10:17:01 2021 +1100 > > > Commit: Commit Bot <commit-bot@chromium.org> > > > CommitDate: Mon Mar 8 23:30:29 2021 +0000 > > > > > > cryptohome: rollout control for fscrypt_v2 > > > > > > > > > The uprev-4-to-5 USE flag got enabled for Soraka (and Nautilus) in > > > > > > commit 14bb32f0477d5956f86802f80e8e4d0ab8c7b9bc > > > Author: Daniil Lunev <dlunev@chromium.org> > > > AuthorDate: Wed Mar 10 09:22:06 2021 +1100 > > > Commit: Commit Bot <commit-bot@chromium.org> > > > CommitDate: Tue Mar 9 23:56:23 2021 +0000 > > > > > > overlays: add a special tag for 4->5 uprevs > > > > > > > > > The direncription_allow_v2 USE flag got enabled for soraka-libcamera in > > > > > > commit 6b846af8ee5752878fc2e125c612719b3467cc3b > > > Author: Daniil Lunev <dlunev@chromium.org> > > > AuthorDate: Mon Mar 1 09:28:11 2021 +1100 > > > Commit: Commit Bot <commit-bot@chromium.org> > > > CommitDate: Tue Mar 2 06:14:31 2021 +0000 > > > > > > overlays: enable fscrypt v2 attempt on post 5.4 boards > > > > > > > > > Disabling direncription_allow_v2 manually allows the build to continue, > > > but I don't think that's a very good idea. > > > > After fixing (or working around) those two issues, the build completes. > > I'll now try to build the latest version (R93-13987), to see if some of > > the problems have been fixed. > > Exact same issues with R93-13987. Other than that, it compiles fine. I > also confirm that Soraka finally produces sound :-) > I've now built R93-13987 with the fixes suggested by Laurent and tested the configuration file series there and can confirm it works Once we got all team with an updated SDK we'll finally merge the series. Thanks j
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 89044efa7ebe..50809c6ffbdc 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -6,6 +6,7 @@ */ #include "camera_device.h" +#include "camera_hal_config.h" #include "camera_ops.h" #include "post_processor.h" @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id, } /* - * Initialize the camera static information. + * Initialize the camera static information retrieved from the + * Camera::properties or from the cameraConfigData. + * + * cameraConfigData is optional for external camera devices and can be + * nullptr. + * * This method is called before the camera device is opened. */ -int CameraDevice::initialize() +int CameraDevice::initialize(const CameraConfigData *cameraConfigData) { - /* Initialize orientation and facing side of the camera. */ + /* + * Initialize orientation and facing side of the camera. + * + * If the libcamera::Camera provides those information as retrieved + * from firmware use them, otherwise fallback to values parsed from + * the configuration file. If the configuration file is not available + * the camera is external so its location and rotation can be safely + * defaulted. + */ const ControlList &properties = camera_->properties(); if (properties.contains(properties::Location)) { @@ -464,12 +478,22 @@ int CameraDevice::initialize() facing_ = CAMERA_FACING_EXTERNAL; break; } + + if (cameraConfigData && cameraConfigData->facing != -1 && + facing_ != cameraConfigData->facing) { + LOG(HAL, Warning) + << "Camera location does not match" + << " configuration file. Using " << facing_; + } + } else if (cameraConfigData) { + if (cameraConfigData->facing == -1) { + LOG(HAL, Error) + << "Camera facing not in configuration file"; + return -EINVAL; + } + facing_ = cameraConfigData->facing; } else { - /* - * \todo Retrieve the camera location from configuration file - * if not available from the library. - */ - facing_ = CAMERA_FACING_FRONT; + facing_ = CAMERA_FACING_EXTERNAL; } /* @@ -483,8 +507,24 @@ int CameraDevice::initialize() if (properties.contains(properties::Rotation)) { int rotation = properties.get(properties::Rotation); orientation_ = (360 - rotation) % 360; + if (cameraConfigData && cameraConfigData->rotation != -1 && + orientation_ != cameraConfigData->rotation) { + LOG(HAL, Warning) + << "Camera orientation does not match" + << " configuration file. Using " << orientation_; + } + } else if (cameraConfigData) { + if (cameraConfigData->rotation == -1) { + LOG(HAL, Error) + << "Camera rotation not in configuration file"; + return -EINVAL; + } + orientation_ = cameraConfigData->rotation; + } else { + orientation_ = 0; } + /* Acquire the camera and initialize available stream configurations. */ int ret = camera_->acquire(); if (ret) { LOG(HAL, Error) << "Failed to temporarily acquire the camera"; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 11bdfec8d587..9cc0d4005242 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -29,6 +29,7 @@ #include "camera_worker.h" #include "jpeg/encoder.h" +struct CameraConfigData; class CameraDevice : protected libcamera::Loggable { public: @@ -36,7 +37,7 @@ public: std::shared_ptr<libcamera::Camera> cam); ~CameraDevice(); - int initialize(); + int initialize(const CameraConfigData *cameraConfigData); int open(const hw_module_t *hardwareModule); void close(); diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index bf3fcda75237..f5b86974e8e3 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -41,6 +41,15 @@ int CameraHalManager::init() { cameraManager_ = std::make_unique<CameraManager>(); + /* + * If the configuration file is not available the HAL only supports + * external cameras. If it exists but it's not valid then error out. + */ + if (halConfig_.exists() && !halConfig_.isValid()) { + LOG(HAL, Error) << "HAL configuration file is not valid"; + return -EINVAL; + } + /* Support camera hotplug. */ cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); @@ -100,6 +109,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) auto iter = cameraIdsMap_.find(cam->id()); if (iter != cameraIdsMap_.end()) { id = iter->second; + if (id >= firstExternalCameraId_) + isCameraExternal = true; } else { isCameraNew = true; @@ -117,7 +128,27 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) /* Create a CameraDevice instance to wrap the libcamera Camera. */ std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, cam); - int ret = camera->initialize(); + + /* + * The configuration file must be valid, and contain a corresponding + * entry for internal cameras. External cameras can be initialized + * without configuration file. + */ + if (!isCameraExternal && !halConfig_.exists()) { + LOG(HAL, Error) + << "HAL configuration file is mandatory for internal cameras"; + return; + } + + const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id()); + if (!isCameraExternal && !cameraConfigData) { + LOG(HAL, Error) + << "HAL configuration entry for internal camera " + << cam->id() << " is missing"; + return; + } + + int ret = camera->initialize(cameraConfigData); if (ret) { LOG(HAL, Error) << "Failed to initialize camera: " << cam->id(); return; diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h index d9bf27989965..af1581da6579 100644 --- a/src/android/camera_hal_manager.h +++ b/src/android/camera_hal_manager.h @@ -19,6 +19,8 @@ #include <libcamera/camera_manager.h> +#include "camera_hal_config.h" + class CameraDevice; class CameraHalManager @@ -50,6 +52,7 @@ private: CameraDevice *cameraDeviceFromHalId(unsigned int id); std::unique_ptr<libcamera::CameraManager> cameraManager_; + CameraHalConfig halConfig_; const camera_module_callbacks_t *callbacks_; std::vector<std::unique_ptr<CameraDevice>> cameras_;