<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 21, 2021 at 4:29 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Fri, May 21, 2021 at 02:15:51PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo,<br>
><br>
> On Thu, Apr 15, 2021 at 10:51 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > Open the HAL configuration file in the Camera HAL manager and get<br>
> > the camera properties for each created CameraDevice and initialize it<br>
> > with them.<br>
> ><br>
> > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> ><br>
><br>
> Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
><br>
> When would you merge this series?<br>
<br>
My plan was to update my SDK to the latest version which contains the<br>
CL to the libcamera and libcamera-configs ebuild, so I could have<br>
tested one more time on a fresh image. Am I too paranoid ?<br>
<br>
Could you or Han-lin tell me which is the id of the first CPFE image<br>
that contains:<br>
<a href="https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2887093</a><br>
<a href="https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2886535</a></blockquote><div><br></div><div>They landed on 13971.0.0.</div><div> </div><div>-Hiro</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
I've tested those patches applied on my rather ancient SDK version,<br>
but I would like to try with an out-of-the-box image<br>
<br>
Thanks<br>
j<br>
<br>
><br>
> -Hiro<br>
><br>
> > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > ---<br>
> > src/android/camera_device.cpp | 56 +++++++++++++++++++++++++-----<br>
> > src/android/camera_device.h | 3 +-<br>
> > src/android/camera_hal_manager.cpp | 33 +++++++++++++++++-<br>
> > src/android/camera_hal_manager.h | 3 ++<br>
> > 4 files changed, 85 insertions(+), 10 deletions(-)<br>
> ><br>
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> > index 89044efa7ebe..50809c6ffbdc 100644<br>
> > --- a/src/android/camera_device.cpp<br>
> > +++ b/src/android/camera_device.cpp<br>
> > @@ -6,6 +6,7 @@<br>
> > */<br>
> ><br>
> > #include "camera_device.h"<br>
> > +#include "camera_hal_config.h"<br>
> > #include "camera_ops.h"<br>
> > #include "post_processor.h"<br>
> ><br>
> > @@ -443,12 +444,25 @@ std::unique_ptr<CameraDevice><br>
> > CameraDevice::create(unsigned int id,<br>
> > }<br>
> ><br>
> > /*<br>
> > - * Initialize the camera static information.<br>
> > + * Initialize the camera static information retrieved from the<br>
> > + * Camera::properties or from the cameraConfigData.<br>
> > + *<br>
> > + * cameraConfigData is optional for external camera devices and can be<br>
> > + * nullptr.<br>
> > + *<br>
> > * This method is called before the camera device is opened.<br>
> > */<br>
> > -int CameraDevice::initialize()<br>
> > +int CameraDevice::initialize(const CameraConfigData *cameraConfigData)<br>
> > {<br>
> > - /* Initialize orientation and facing side of the camera. */<br>
> > + /*<br>
> > + * Initialize orientation and facing side of the camera.<br>
> > + *<br>
> > + * If the libcamera::Camera provides those information as retrieved<br>
> > + * from firmware use them, otherwise fallback to values parsed from<br>
> > + * the configuration file. If the configuration file is not<br>
> > available<br>
> > + * the camera is external so its location and rotation can be<br>
> > safely<br>
> > + * defaulted.<br>
> > + */<br>
> > const ControlList &properties = camera_->properties();<br>
> ><br>
> > if (properties.contains(properties::Location)) {<br>
> > @@ -464,12 +478,22 @@ int CameraDevice::initialize()<br>
> > facing_ = CAMERA_FACING_EXTERNAL;<br>
> > break;<br>
> > }<br>
> > +<br>
> > + if (cameraConfigData && cameraConfigData->facing != -1 &&<br>
> > + facing_ != cameraConfigData->facing) {<br>
> > + LOG(HAL, Warning)<br>
> > + << "Camera location does not match"<br>
> > + << " configuration file. Using " <<<br>
> > facing_;<br>
> > + }<br>
> > + } else if (cameraConfigData) {<br>
> > + if (cameraConfigData->facing == -1) {<br>
> > + LOG(HAL, Error)<br>
> > + << "Camera facing not in configuration<br>
> > file";<br>
> > + return -EINVAL;<br>
> > + }<br>
> > + facing_ = cameraConfigData->facing;<br>
> > } else {<br>
> > - /*<br>
> > - * \todo Retrieve the camera location from configuration<br>
> > file<br>
> > - * if not available from the library.<br>
> > - */<br>
> > - facing_ = CAMERA_FACING_FRONT;<br>
> > + facing_ = CAMERA_FACING_EXTERNAL;<br>
> > }<br>
> ><br>
> > /*<br>
> > @@ -483,8 +507,24 @@ int CameraDevice::initialize()<br>
> > if (properties.contains(properties::Rotation)) {<br>
> > int rotation = properties.get(properties::Rotation);<br>
> > orientation_ = (360 - rotation) % 360;<br>
> > + if (cameraConfigData && cameraConfigData->rotation != -1 &&<br>
> > + orientation_ != cameraConfigData->rotation) {<br>
> > + LOG(HAL, Warning)<br>
> > + << "Camera orientation does not match"<br>
> > + << " configuration file. Using " <<<br>
> > orientation_;<br>
> > + }<br>
> > + } else if (cameraConfigData) {<br>
> > + if (cameraConfigData->rotation == -1) {<br>
> > + LOG(HAL, Error)<br>
> > + << "Camera rotation not in configuration<br>
> > file";<br>
> > + return -EINVAL;<br>
> > + }<br>
> > + orientation_ = cameraConfigData->rotation;<br>
> > + } else {<br>
> > + orientation_ = 0;<br>
> > }<br>
> ><br>
> > + /* Acquire the camera and initialize available stream<br>
> > configurations. */<br>
> > int ret = camera_->acquire();<br>
> > if (ret) {<br>
> > LOG(HAL, Error) << "Failed to temporarily acquire the<br>
> > camera";<br>
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
> > index 11bdfec8d587..9cc0d4005242 100644<br>
> > --- a/src/android/camera_device.h<br>
> > +++ b/src/android/camera_device.h<br>
> > @@ -29,6 +29,7 @@<br>
> > #include "camera_worker.h"<br>
> > #include "jpeg/encoder.h"<br>
> ><br>
> > +struct CameraConfigData;<br>
> > class CameraDevice : protected libcamera::Loggable<br>
> > {<br>
> > public:<br>
> > @@ -36,7 +37,7 @@ public:<br>
> ><br>
> > std::shared_ptr<libcamera::Camera> cam);<br>
> > ~CameraDevice();<br>
> ><br>
> > - int initialize();<br>
> > + int initialize(const CameraConfigData *cameraConfigData);<br>
> ><br>
> > int open(const hw_module_t *hardwareModule);<br>
> > void close();<br>
> > diff --git a/src/android/camera_hal_manager.cpp<br>
> > b/src/android/camera_hal_manager.cpp<br>
> > index bf3fcda75237..f5b86974e8e3 100644<br>
> > --- a/src/android/camera_hal_manager.cpp<br>
> > +++ b/src/android/camera_hal_manager.cpp<br>
> > @@ -41,6 +41,15 @@ int CameraHalManager::init()<br>
> > {<br>
> > cameraManager_ = std::make_unique<CameraManager>();<br>
> ><br>
> > + /*<br>
> > + * If the configuration file is not available the HAL only supports<br>
> > + * external cameras. If it exists but it's not valid then error<br>
> > out.<br>
> > + */<br>
> > + if (halConfig_.exists() && !halConfig_.isValid()) {<br>
> > + LOG(HAL, Error) << "HAL configuration file is not valid";<br>
> > + return -EINVAL;<br>
> > + }<br>
> > +<br>
> > /* Support camera hotplug. */<br>
> > cameraManager_->cameraAdded.connect(this,<br>
> > &CameraHalManager::cameraAdded);<br>
> > cameraManager_->cameraRemoved.connect(this,<br>
> > &CameraHalManager::cameraRemoved);<br>
> > @@ -100,6 +109,8 @@ void<br>
> > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)<br>
> > auto iter = cameraIdsMap_.find(cam->id());<br>
> > if (iter != cameraIdsMap_.end()) {<br>
> > id = iter->second;<br>
> > + if (id >= firstExternalCameraId_)<br>
> > + isCameraExternal = true;<br>
> > } else {<br>
> > isCameraNew = true;<br>
> ><br>
> > @@ -117,7 +128,27 @@ void<br>
> > CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)<br>
> ><br>
> > /* Create a CameraDevice instance to wrap the libcamera Camera. */<br>
> > std::unique_ptr<CameraDevice> camera = CameraDevice::create(id,<br>
> > cam);<br>
> > - int ret = camera->initialize();<br>
> > +<br>
> > + /*<br>
> > + * The configuration file must be valid, and contain a<br>
> > corresponding<br>
> > + * entry for internal cameras. External cameras can be initialized<br>
> > + * without configuration file.<br>
> > + */<br>
> > + if (!isCameraExternal && !halConfig_.exists()) {<br>
> > + LOG(HAL, Error)<br>
> > + << "HAL configuration file is mandatory for<br>
> > internal cameras";<br>
> > + return;<br>
> > + }<br>
> > +<br>
> > + const CameraConfigData *cameraConfigData =<br>
> > halConfig_.cameraConfigData(cam->id());<br>
> > + if (!isCameraExternal && !cameraConfigData) {<br>
> > + LOG(HAL, Error)<br>
> > + << "HAL configuration entry for internal camera "<br>
> > + << cam->id() << " is missing";<br>
> > + return;<br>
> > + }<br>
> > +<br>
> > + int ret = camera->initialize(cameraConfigData);<br>
> > if (ret) {<br>
> > LOG(HAL, Error) << "Failed to initialize camera: " <<<br>
> > cam->id();<br>
> > return;<br>
> > diff --git a/src/android/camera_hal_manager.h<br>
> > b/src/android/camera_hal_manager.h<br>
> > index d9bf27989965..af1581da6579 100644<br>
> > --- a/src/android/camera_hal_manager.h<br>
> > +++ b/src/android/camera_hal_manager.h<br>
> > @@ -19,6 +19,8 @@<br>
> ><br>
> > #include <libcamera/camera_manager.h><br>
> ><br>
> > +#include "camera_hal_config.h"<br>
> > +<br>
> > class CameraDevice;<br>
> ><br>
> > class CameraHalManager<br>
> > @@ -50,6 +52,7 @@ private:<br>
> > CameraDevice *cameraDeviceFromHalId(unsigned int id);<br>
> ><br>
> > std::unique_ptr<libcamera::CameraManager> cameraManager_;<br>
> > + CameraHalConfig halConfig_;<br>
> ><br>
> > const camera_module_callbacks_t *callbacks_;<br>
> > std::vector<std::unique_ptr<CameraDevice>> cameras_;<br>
> > --<br>
> > 2.31.1<br>
> ><br>
> > _______________________________________________<br>
> > libcamera-devel mailing list<br>
> > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div>