| Message ID | 20200427213236.333777-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-04-27 23:32:35 +0200, Jacopo Mondi wrote: > Add method to retrieve the CameraSensorInfo to the CameraSensor class. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera_sensor.cpp | 83 +++++++++++++++++++++++++++ > src/libcamera/include/camera_sensor.h | 1 + > 2 files changed, 84 insertions(+) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 4fd1ee84eb47..8b37f63dc04e 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls) > return subdev_->setControls(ctrls); > } > > +/** > + * \brief Assemble and return the camera sensor info > + * \param[out] info The camera sensor info that reports the sensor information > + * > + * The CameraSensorInfo content is assembled by inspecting the currently > + * applied sensor configuration and the sensor static properties. > + * > + * \return 0 on success, a negative error code otherwise > + */ > +int CameraSensor::sensorInfo(CameraSensorInfo *info) const > +{ > + /* > + * Construct the camera sensor name using the media entity name. > + * > + * \todo There is no standardized naming scheme for sensor entities > + * in the Linux kernel at the moment. The most common naming scheme > + * is the one obtained by associating the sensor name and its I2C > + * device and bus addresses in the form of: "devname i2c-adapt:i2c-addr" > + * Assume this is the standard naming scheme and parse the first part > + * of the entity name to obtain "devname". > + */ > + std::string entityName = subdev_->entity()->name(); > + info->model = entityName.substr(0, entityName.find(' ')); If we need to keep the model name I like the interface provided by CameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add model() function'. As stated in the patch mention above I have comments on the documentation and it's implementation. > + > + /* > + * Get the active area size from the ActiveAreas property. Do > + * not use the content of properties::ActiveAreas but fetch it from the > + * subdevice as the property registration is optional, but it's > + * mandatory to construct the CameraSensorInfo. I'm sorry but this paragraph confuses me. Get FOO from property. Do not use the content of properties::FOO. > + * > + * \todo The ActiveAreas property aims to describe multiple > + * active area rectangles. At the moment only a single active > + * area rectangle is exposed by the subdevice API. Use that single > + * rectangle width and height to construct the ActiveAreaSize. > + */ > + Rectangle rect = {}; > + int ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect); > + if (ret) { > + LOG(CameraSensor, Error) If the target is mandatory for libcamera shall this be a fatal error ? Here and bellow. > + << "Failed to construct camera sensor info: " > + << "the camera sensor does not report the active area"; > + > + return ret; > + } > + info->activeAreaSize = { rect.width, rect.height }; > + > + /* It's mandatory for the subdevice to report its crop rectangle. */ > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &info->analogCrop); > + if (ret) { > + LOG(CameraSensor, Error) > + << "Failed to construct camera sensor info: " > + << "the camera sensor does not report the crop rectangle"; > + return ret; > + } > + > + /* The bit-depth and image size depend on the currently applied format. */ > + V4L2SubdeviceFormat format{}; > + ret = subdev_->getFormat(0, &format); > + if (ret) > + return ret; > + info->bitsPerPixel = format.bitsPerPixel(); > + info->outputSize = format.size; > + > + /* > + * Retrieve the pixel rate and the line length through V4L2 controls. > + * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is > + * mandatory. > + */ > + ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, > + V4L2_CID_HBLANK }); > + if (ctrls.empty()) { What if only one of the controls are implemented by the driver? Would it make more sens to check ctrls.size() != 2 ? > + LOG(CameraSensor, Error) > + << "Failed to retrieve camera info controls"; > + return -EINVAL; > + } > + > + int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>(); > + info->lineLength = info->outputSize.width + hblank; > + info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>(); > + > + return 0; > +} > + > std::string CameraSensor::logPrefix() const > { > return "'" + subdev_->entity()->name() + "'"; > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > index 28ebfaa30c22..91d270552d3f 100644 > --- a/src/libcamera/include/camera_sensor.h > +++ b/src/libcamera/include/camera_sensor.h > @@ -60,6 +60,7 @@ public: > int setControls(ControlList *ctrls); > > const ControlList &properties() const { return properties_; } > + int sensorInfo(CameraSensorInfo *info) const; > > protected: > std::string logPrefix() const; > -- > 2.26.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas and Jacopo, On Tue, Apr 28, 2020 at 02:28:39AM +0200, Niklas Söderlund wrote: > On 2020-04-27 23:32:35 +0200, Jacopo Mondi wrote: > > Add method to retrieve the CameraSensorInfo to the CameraSensor class. s/method/a method/ > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/camera_sensor.cpp | 83 +++++++++++++++++++++++++++ > > src/libcamera/include/camera_sensor.h | 1 + > > 2 files changed, 84 insertions(+) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 4fd1ee84eb47..8b37f63dc04e 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls) > > return subdev_->setControls(ctrls); > > } > > > > +/** > > + * \brief Assemble and return the camera sensor info > > + * \param[out] info The camera sensor info that reports the sensor information I would s/ that reports.*$// as it's redundant I think, up to you. > > + * > > + * The CameraSensorInfo content is assembled by inspecting the currently > > + * applied sensor configuration and the sensor static properties. > > + * > > + * \return 0 on success, a negative error code otherwise > > + */ > > +int CameraSensor::sensorInfo(CameraSensorInfo *info) const > > +{ > > + /* > > + * Construct the camera sensor name using the media entity name. > > + * > > + * \todo There is no standardized naming scheme for sensor entities > > + * in the Linux kernel at the moment. The most common naming scheme > > + * is the one obtained by associating the sensor name and its I2C > > + * device and bus addresses in the form of: "devname i2c-adapt:i2c-addr" > > + * Assume this is the standard naming scheme and parse the first part > > + * of the entity name to obtain "devname". > > + */ > > + std::string entityName = subdev_->entity()->name(); > > + info->model = entityName.substr(0, entityName.find(' ')); > > If we need to keep the model name I like the interface provided by > CameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add > model() function'. > > As stated in the patch mention above I have comments on the > documentation and it's implementation. I've copied the code for that function from this patch :-) I think we should then replace this by just a call to model(). Let's discuss your comments as part of the other patch. > > + > > + /* > > + * Get the active area size from the ActiveAreas property. Do > > + * not use the content of properties::ActiveAreas but fetch it from the > > + * subdevice as the property registration is optional, but it's > > + * mandatory to construct the CameraSensorInfo. > > I'm sorry but this paragraph confuses me. > > Get FOO from property. Do not use the content of properties::FOO. > > > + * > > + * \todo The ActiveAreas property aims to describe multiple > > + * active area rectangles. At the moment only a single active > > + * area rectangle is exposed by the subdevice API. Use that single > > + * rectangle width and height to construct the ActiveAreaSize. > > + */ I agree with Niklas. I think you can just write /* Get the active area size. */ > > + Rectangle rect = {}; > > + int ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect); > > + if (ret) { > > + LOG(CameraSensor, Error) > > If the target is mandatory for libcamera shall this be a fatal error ? > Here and bellow. I don't think an abort() is required here, returning an error should allow us to handle that gracefully and, for instance, display a message to the user in the GUI instead of killing the process. > > + << "Failed to construct camera sensor info: " > > + << "the camera sensor does not report the active area"; > > + > > + return ret; > > + } > > + info->activeAreaSize = { rect.width, rect.height }; > > + > > + /* It's mandatory for the subdevice to report its crop rectangle. */ > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &info->analogCrop); > > + if (ret) { > > + LOG(CameraSensor, Error) > > + << "Failed to construct camera sensor info: " > > + << "the camera sensor does not report the crop rectangle"; > > + return ret; > > + } > > + > > + /* The bit-depth and image size depend on the currently applied format. */ s/bit-depth/bit depth/ > > + V4L2SubdeviceFormat format{}; > > + ret = subdev_->getFormat(0, &format); > > + if (ret) > > + return ret; > > + info->bitsPerPixel = format.bitsPerPixel(); > > + info->outputSize = format.size; > > + > > + /* > > + * Retrieve the pixel rate and the line length through V4L2 controls. > > + * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is > > + * mandatory. > > + */ > > + ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, > > + V4L2_CID_HBLANK }); > > + if (ctrls.empty()) { > > What if only one of the controls are implemented by the driver? Would it > make more sens to check ctrls.size() != 2 ? If any of the controls is unsupported, getControls() returns an empty list. I'm fine with != 2 too though, up to Jacopo. Assuming we call model() above and thus don't need to discuss how the model is retrieved as part of this patch, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + LOG(CameraSensor, Error) > > + << "Failed to retrieve camera info controls"; > > + return -EINVAL; > > + } > > + > > + int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>(); > > + info->lineLength = info->outputSize.width + hblank; > > + info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>(); > > + > > + return 0; > > +} > > + > > std::string CameraSensor::logPrefix() const > > { > > return "'" + subdev_->entity()->name() + "'"; > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > index 28ebfaa30c22..91d270552d3f 100644 > > --- a/src/libcamera/include/camera_sensor.h > > +++ b/src/libcamera/include/camera_sensor.h > > @@ -60,6 +60,7 @@ public: > > int setControls(ControlList *ctrls); > > > > const ControlList &properties() const { return properties_; } > > + int sensorInfo(CameraSensorInfo *info) const; > > > > protected: > > std::string logPrefix() const;
Hi Niklas, Laurent, On Tue, Apr 28, 2020 at 05:07:53AM +0300, Laurent Pinchart wrote: > Hi Niklas and Jacopo, > > On Tue, Apr 28, 2020 at 02:28:39AM +0200, Niklas Söderlund wrote: > > On 2020-04-27 23:32:35 +0200, Jacopo Mondi wrote: > > > Add method to retrieve the CameraSensorInfo to the CameraSensor class. > > s/method/a method/ > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/camera_sensor.cpp | 83 +++++++++++++++++++++++++++ > > > src/libcamera/include/camera_sensor.h | 1 + > > > 2 files changed, 84 insertions(+) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 4fd1ee84eb47..8b37f63dc04e 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls) > > > return subdev_->setControls(ctrls); > > > } > > > > > > +/** > > > + * \brief Assemble and return the camera sensor info > > > + * \param[out] info The camera sensor info that reports the sensor information > > I would s/ that reports.*$// as it's redundant I think, up to you. > > > > + * > > > + * The CameraSensorInfo content is assembled by inspecting the currently > > > + * applied sensor configuration and the sensor static properties. > > > + * > > > + * \return 0 on success, a negative error code otherwise > > > + */ > > > +int CameraSensor::sensorInfo(CameraSensorInfo *info) const > > > +{ > > > + /* > > > + * Construct the camera sensor name using the media entity name. > > > + * > > > + * \todo There is no standardized naming scheme for sensor entities > > > + * in the Linux kernel at the moment. The most common naming scheme > > > + * is the one obtained by associating the sensor name and its I2C > > > + * device and bus addresses in the form of: "devname i2c-adapt:i2c-addr" > > > + * Assume this is the standard naming scheme and parse the first part > > > + * of the entity name to obtain "devname". > > > + */ > > > + std::string entityName = subdev_->entity()->name(); > > > + info->model = entityName.substr(0, entityName.find(' ')); > > > > If we need to keep the model name I like the interface provided by > > CameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add > > model() function'. There was no such a patch when I sent this out > > > > As stated in the patch mention above I have comments on the > > documentation and it's implementation. > > I've copied the code for that function from this patch :-) I think we > should then replace this by just a call to model(). Let's discuss your > comments as part of the other patch. > Ack > > > + > > > + /* > > > + * Get the active area size from the ActiveAreas property. Do > > > + * not use the content of properties::ActiveAreas but fetch it from the > > > + * subdevice as the property registration is optional, but it's > > > + * mandatory to construct the CameraSensorInfo. > > > > I'm sorry but this paragraph confuses me. > > > > Get FOO from property. Do not use the content of properties::FOO. > > > > > + * > > > + * \todo The ActiveAreas property aims to describe multiple > > > + * active area rectangles. At the moment only a single active > > > + * area rectangle is exposed by the subdevice API. Use that single > > > + * rectangle width and height to construct the ActiveAreaSize. > > > + */ > > I agree with Niklas. I think you can just write > > /* Get the active area size. */ > Yes, leftovers > > > + Rectangle rect = {}; > > > + int ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect); > > > + if (ret) { > > > + LOG(CameraSensor, Error) > > > > If the target is mandatory for libcamera shall this be a fatal error ? > > Here and bellow. > > I don't think an abort() is required here, returning an error should > allow us to handle that gracefully and, for instance, display a message > to the user in the GUI instead of killing the process. I don't see a reason to abort() to be honest. This method is called by pipeline handlers, they should be able to exit gracefully and stop cameras and whatnot. Why would you tear down the whole library here ? > > > > + << "Failed to construct camera sensor info: " > > > + << "the camera sensor does not report the active area"; > > > + > > > + return ret; > > > + } > > > + info->activeAreaSize = { rect.width, rect.height }; > > > + > > > + /* It's mandatory for the subdevice to report its crop rectangle. */ > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &info->analogCrop); > > > + if (ret) { > > > + LOG(CameraSensor, Error) > > > + << "Failed to construct camera sensor info: " > > > + << "the camera sensor does not report the crop rectangle"; > > > + return ret; > > > + } > > > + > > > + /* The bit-depth and image size depend on the currently applied format. */ > > s/bit-depth/bit depth/ > > > > + V4L2SubdeviceFormat format{}; > > > + ret = subdev_->getFormat(0, &format); > > > + if (ret) > > > + return ret; > > > + info->bitsPerPixel = format.bitsPerPixel(); > > > + info->outputSize = format.size; > > > + > > > + /* > > > + * Retrieve the pixel rate and the line length through V4L2 controls. > > > + * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is > > > + * mandatory. > > > + */ > > > + ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, > > > + V4L2_CID_HBLANK }); > > > + if (ctrls.empty()) { > > > > What if only one of the controls are implemented by the driver? Would it > > make more sens to check ctrls.size() != 2 ? We need both controls, there's no use for a single one, so != 2 or empty() lead to the same result. And to get something != empty() on error the getControls() function should be reworked. > > If any of the controls is unsupported, getControls() returns an empty > list. I'm fine with != 2 too though, up to Jacopo. > > Assuming we call model() above and thus don't need to discuss how the > model is retrieved as part of this patch, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks j > > > > + LOG(CameraSensor, Error) > > > + << "Failed to retrieve camera info controls"; > > > + return -EINVAL; > > > + } > > > + > > > + int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>(); > > > + info->lineLength = info->outputSize.width + hblank; > > > + info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>(); > > > + > > > + return 0; > > > +} > > > + > > > std::string CameraSensor::logPrefix() const > > > { > > > return "'" + subdev_->entity()->name() + "'"; > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > index 28ebfaa30c22..91d270552d3f 100644 > > > --- a/src/libcamera/include/camera_sensor.h > > > +++ b/src/libcamera/include/camera_sensor.h > > > @@ -60,6 +60,7 @@ public: > > > int setControls(ControlList *ctrls); > > > > > > const ControlList &properties() const { return properties_; } > > > + int sensorInfo(CameraSensorInfo *info) const; > > > > > > protected: > > > std::string logPrefix() const; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 4fd1ee84eb47..8b37f63dc04e 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls) return subdev_->setControls(ctrls); } +/** + * \brief Assemble and return the camera sensor info + * \param[out] info The camera sensor info that reports the sensor information + * + * The CameraSensorInfo content is assembled by inspecting the currently + * applied sensor configuration and the sensor static properties. + * + * \return 0 on success, a negative error code otherwise + */ +int CameraSensor::sensorInfo(CameraSensorInfo *info) const +{ + /* + * Construct the camera sensor name using the media entity name. + * + * \todo There is no standardized naming scheme for sensor entities + * in the Linux kernel at the moment. The most common naming scheme + * is the one obtained by associating the sensor name and its I2C + * device and bus addresses in the form of: "devname i2c-adapt:i2c-addr" + * Assume this is the standard naming scheme and parse the first part + * of the entity name to obtain "devname". + */ + std::string entityName = subdev_->entity()->name(); + info->model = entityName.substr(0, entityName.find(' ')); + + /* + * Get the active area size from the ActiveAreas property. Do + * not use the content of properties::ActiveAreas but fetch it from the + * subdevice as the property registration is optional, but it's + * mandatory to construct the CameraSensorInfo. + * + * \todo The ActiveAreas property aims to describe multiple + * active area rectangles. At the moment only a single active + * area rectangle is exposed by the subdevice API. Use that single + * rectangle width and height to construct the ActiveAreaSize. + */ + Rectangle rect = {}; + int ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect); + if (ret) { + LOG(CameraSensor, Error) + << "Failed to construct camera sensor info: " + << "the camera sensor does not report the active area"; + + return ret; + } + info->activeAreaSize = { rect.width, rect.height }; + + /* It's mandatory for the subdevice to report its crop rectangle. */ + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &info->analogCrop); + if (ret) { + LOG(CameraSensor, Error) + << "Failed to construct camera sensor info: " + << "the camera sensor does not report the crop rectangle"; + return ret; + } + + /* The bit-depth and image size depend on the currently applied format. */ + V4L2SubdeviceFormat format{}; + ret = subdev_->getFormat(0, &format); + if (ret) + return ret; + info->bitsPerPixel = format.bitsPerPixel(); + info->outputSize = format.size; + + /* + * Retrieve the pixel rate and the line length through V4L2 controls. + * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is + * mandatory. + */ + ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, + V4L2_CID_HBLANK }); + if (ctrls.empty()) { + LOG(CameraSensor, Error) + << "Failed to retrieve camera info controls"; + return -EINVAL; + } + + int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>(); + info->lineLength = info->outputSize.width + hblank; + info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>(); + + return 0; +} + std::string CameraSensor::logPrefix() const { return "'" + subdev_->entity()->name() + "'"; diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h index 28ebfaa30c22..91d270552d3f 100644 --- a/src/libcamera/include/camera_sensor.h +++ b/src/libcamera/include/camera_sensor.h @@ -60,6 +60,7 @@ public: int setControls(ControlList *ctrls); const ControlList &properties() const { return properties_; } + int sensorInfo(CameraSensorInfo *info) const; protected: std::string logPrefix() const;
Add method to retrieve the CameraSensorInfo to the CameraSensor class. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera_sensor.cpp | 83 +++++++++++++++++++++++++++ src/libcamera/include/camera_sensor.h | 1 + 2 files changed, 84 insertions(+)