| Message ID | 20260313-mali-cru-v4-3-c0d9bc8cd8fa@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 03. 13. 12:33 keltezéssel, Jacopo Mondi írta: > In order to prepare to support memory input cameras, split the handling of > the TPG and Inline camera cases. > > The Mali C55 pipeline handler uses the entity and subdevice stored in the > CameraData to support both the handling of the TPG and of the Inline (CSI-2 > + sensor) use cases. Adding support for memory cameras by using the CRU unit > would add yet-another special case making the code harder to follow and > more prone to errors. > > Split the handling of the TPG and Inline cameras by introducing an > std::variant<> variable and to deflect the functions called on the > camera data to the correct type by using overloaded std::visit<>(). > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 276 ++++++++++++++++----------- > 1 file changed, 167 insertions(+), 109 deletions(-) > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > index c209b0b070b1..6581d13bbc52 100644 > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > @@ -11,12 +11,14 @@ > #include <memory> > #include <set> > #include <string> > +#include <variant> > > #include <linux/mali-c55-config.h> > #include <linux/media-bus-format.h> > #include <linux/media.h> > > #include <libcamera/base/log.h> > +#include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > #include <libcamera/formats.h> > @@ -92,17 +94,90 @@ struct MaliC55FrameInfo { > class MaliC55CameraData : public Camera::Private > { > public: > - MaliC55CameraData(PipelineHandler *pipe, MediaEntity *entity) > - : Camera::Private(pipe), entity_(entity) > + struct Tpg { > + std::vector<Size> sizes(unsigned int mbusCode) const; > + > + Size resolution_; > + std::unique_ptr<V4L2Subdevice> sd_; > + }; > + > + struct Inline { > + std::unique_ptr<V4L2Subdevice> csi2_; > + std::unique_ptr<CameraSensor> sensor_; > + }; > + using CameraType = std::variant<Tpg, Inline>; > + > + MaliC55CameraData(PipelineHandler *pipe) > + : Camera::Private(pipe) > { > } > > - int init(); > int loadIPA(); > > - /* Deflect these functionalities to either TPG or CameraSensor. */ > - std::vector<Size> sizes(unsigned int mbusCode) const; > - Size resolution() const; > + int initTpg(MediaEntity *entity); > + int initInline(MediaEntity *entity); > + > + std::vector<Size> sizes(unsigned int mbusCode) const > + { > + return std::visit(utils::overloaded{ > + [&](const Tpg &tpg) -> std::vector<Size> { > + return tpg.sizes(mbusCode); > + }, > + [&](const Inline &in) -> std::vector<Size> { > + return in.sensor_->sizes(mbusCode); > + } }, > + input_); It's a minor thing, but I personally like the following indentation a lot better: return std::visit(utils::overloaded{ [&](...) { .... }, [&](...) { ... }, }, input_); If checkstyle prefers the current one, then please ignore. > + } > [...] > > void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls) > { > - if (!sensor_) > + if (std::holds_alternative<Tpg>(input_)) > return; > > IPACameraSensorInfo sensorInfo; > - int ret = sensor_->sensorInfo(&sensorInfo); > + int ret = sensor()->sensorInfo(&sensorInfo); For example here, and in some other places it, seems better to me to do something like this: CameraSensor *sensor = data->sensor(); if (!sensor) return; ... sensor->sensorInfo(...); assuming MaliC55CameraData::sensor() is changed to not to use ASSERT(). It would be similar to what the previous version does: `if (!sensor_)`. Generally I think if both work, it's better to use `std::get_if` + checking for nullptr instead of `std::holds_alternative()` + doing something else to get the actual data. > if (ret) { > LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > return; > @@ -379,7 +436,7 @@ int MaliC55CameraData::loadIPA() > int ret; > > /* Do not initialize IPA for TPG. */ > - if (!sensor_) > + if (std::holds_alternative<Tpg>(input_)) > return 0; > > ipa_ = IPAManager::createIPA<ipa::mali_c55::IPAProxyMaliC55>(pipe(), 1, 1); > @@ -388,20 +445,20 @@ int MaliC55CameraData::loadIPA() > > ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls); > > - std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml", > + std::string ipaTuningFile = ipa_->configurationFile(sensor()->model() + ".yaml", > "uncalibrated.yaml"); > > /* We need to inform the IPA of the sensor configuration */ > ipa::mali_c55::IPAConfigInfo ipaConfig{}; > > - ret = sensor_->sensorInfo(&ipaConfig.sensorInfo); > + ret = sensor()->sensorInfo(&ipaConfig.sensorInfo); > if (ret) > return ret; > > - ipaConfig.sensorControls = sensor_->controls(); > + ipaConfig.sensorControls = sensor()->controls(); > > ControlInfoMap ipaControls; > - ret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig, > + ret = ipa_->init({ ipaTuningFile, sensor()->model() }, ipaConfig, > &ipaControls); > if (ret) { > LOG(MaliC55, Error) << "Failed to initialise the Mali-C55 IPA"; > @@ -444,13 +501,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate() > * The TPG doesn't support flips, so we only need to calculate a > * transform if we have a sensor. > */ > - if (data_->sensor_) { > + if (std::holds_alternative<MaliC55CameraData::Tpg>(data_->input_)) { > + combinedTransform_ = Transform::Rot0; > + } else { > Orientation requestedOrientation = orientation; > - combinedTransform_ = data_->sensor_->computeTransform(&orientation); > + combinedTransform_ = data_->sensor()->computeTransform(&orientation); > if (orientation != requestedOrientation) > status = Adjusted; > - } else { > - combinedTransform_ = Transform::Rot0; > } > > /* Only 2 streams available. */ > @@ -927,11 +984,12 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > > /* Link the graph depending if we are operating the TPG or a sensor. */ > MaliC55CameraData *data = cameraData(camera); > - if (data->csi_) { > - const MediaEntity *csiEntity = data->csi_->entity(); > - ret = csiEntity->getPadByIndex(1)->links()[0]->setEnabled(true); > + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { > + const MediaEntity *tpgEntity = data->subdev()->entity(); > + ret = tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true); > } else { > - ret = data->entity_->getPadByIndex(0)->links()[0]->setEnabled(true); > + const MediaEntity *csi2Entity = data->csi2()->entity(); > + ret = csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true); > } I think the above part is definitely a place where std::visit could be used. E.g. (after all patches): ret = std::visit(utils::overloaded { [](MaliC55CameraData::Tpg &x) { const MediaEntity *tpgEntity = x.sd_->entity(); return tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true); }, [](MaliC55CameraData::Inline &x) { const MediaEntity *csi2Entity = x.csi2_->entity(); return csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true); }, [&](MaliC55CameraData::Memory &) { const MediaEntity *ivcEntity = ivcSd_->entity(); return ivcEntity->getPadByIndex(1)->links()[0]->setEnabled(true); }, }, data->input_); > if (ret) > return ret; > @@ -939,26 +997,24 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > MaliC55CameraConfiguration *maliConfig = > static_cast<MaliC55CameraConfiguration *>(config); > V4L2SubdeviceFormat subdevFormat = maliConfig->sensorFormat_; > - ret = data->sd_->getFormat(0, &subdevFormat); > - if (ret) > - return ret; > > - if (data->sensor_) { > - ret = data->sensor_->setFormat(&subdevFormat, > - maliConfig->combinedTransform()); > + /* Apply format to the origin of the pipeline and propagate it. */ > + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { > + ret = data->subdev()->setFormat(0, &subdevFormat); > + } else { > + ret = data->sensor()->setFormat(&subdevFormat, > + maliConfig->combinedTransform()); > if (ret) > return ret; > - } > > - if (data->csi_) { > - ret = data->csi_->setFormat(0, &subdevFormat); > + ret = data->csi2()->setFormat(0, &subdevFormat); > if (ret) > return ret; > > - ret = data->csi_->getFormat(1, &subdevFormat); > - if (ret) > - return ret; > + ret = data->csi2()->getFormat(1, &subdevFormat); > } I have the same impression here as well. > + if (ret) > + return ret; > > V4L2DeviceFormat statsFormat; > ret = stats_->getFormat(&statsFormat); > [...]
Hi Barnabás On Fri, Mar 13, 2026 at 04:19:45PM +0100, Barnabás Pőcze wrote: > Hi > > 2026. 03. 13. 12:33 keltezéssel, Jacopo Mondi írta: > > In order to prepare to support memory input cameras, split the handling of > > the TPG and Inline camera cases. > > > > The Mali C55 pipeline handler uses the entity and subdevice stored in the > > CameraData to support both the handling of the TPG and of the Inline (CSI-2 > > + sensor) use cases. Adding support for memory cameras by using the CRU unit > > would add yet-another special case making the code harder to follow and > > more prone to errors. > > > > Split the handling of the TPG and Inline cameras by introducing an > > std::variant<> variable and to deflect the functions called on the > > camera data to the correct type by using overloaded std::visit<>(). > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 276 ++++++++++++++++----------- > > 1 file changed, 167 insertions(+), 109 deletions(-) > > > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > index c209b0b070b1..6581d13bbc52 100644 > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > @@ -11,12 +11,14 @@ > > #include <memory> > > #include <set> > > #include <string> > > +#include <variant> > > #include <linux/mali-c55-config.h> > > #include <linux/media-bus-format.h> > > #include <linux/media.h> > > #include <libcamera/base/log.h> > > +#include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > > #include <libcamera/formats.h> > > @@ -92,17 +94,90 @@ struct MaliC55FrameInfo { > > class MaliC55CameraData : public Camera::Private > > { > > public: > > - MaliC55CameraData(PipelineHandler *pipe, MediaEntity *entity) > > - : Camera::Private(pipe), entity_(entity) > > + struct Tpg { > > + std::vector<Size> sizes(unsigned int mbusCode) const; > > + > > + Size resolution_; > > + std::unique_ptr<V4L2Subdevice> sd_; > > + }; > > + > > + struct Inline { > > + std::unique_ptr<V4L2Subdevice> csi2_; > > + std::unique_ptr<CameraSensor> sensor_; > > + }; > > + using CameraType = std::variant<Tpg, Inline>; > > + > > + MaliC55CameraData(PipelineHandler *pipe) > > + : Camera::Private(pipe) > > { > > } > > - int init(); > > int loadIPA(); > > - /* Deflect these functionalities to either TPG or CameraSensor. */ > > - std::vector<Size> sizes(unsigned int mbusCode) const; > > - Size resolution() const; > > + int initTpg(MediaEntity *entity); > > + int initInline(MediaEntity *entity); > > + > > + std::vector<Size> sizes(unsigned int mbusCode) const > > + { > > + return std::visit(utils::overloaded{ > > + [&](const Tpg &tpg) -> std::vector<Size> { > > + return tpg.sizes(mbusCode); > > + }, > > + [&](const Inline &in) -> std::vector<Size> { > > + return in.sensor_->sizes(mbusCode); > > + } }, > > + input_); > > It's a minor thing, but I personally like the following indentation a lot better: > personal preferences I would say > return std::visit(utils::overloaded{ > [&](...) { > .... > }, > [&](...) { > ... > }, > }, input_); > > If checkstyle prefers the current one, then please ignore. checkstyle wants it one more indentation level to the right. Anything goes for me, it is a detail. > > > > + } > > [...] > > void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls) > > { > > - if (!sensor_) > > + if (std::holds_alternative<Tpg>(input_)) > > return; > > IPACameraSensorInfo sensorInfo; > > - int ret = sensor_->sensorInfo(&sensorInfo); > > + int ret = sensor()->sensorInfo(&sensorInfo); > > For example here, and in some other places it, seems better to me to do something like this: > > CameraSensor *sensor = data->sensor(); > if (!sensor) > return; > > ... > sensor->sensorInfo(...); > > assuming MaliC55CameraData::sensor() is changed to not to use ASSERT(). It would be similar > to what the previous version does: `if (!sensor_)`. > Generally I think if both work, it's better to use `std::get_if` + checking for nullptr > instead of `std::holds_alternative()` + doing something else to get the actual data. > Why is it better ? Doens't std::variant<> allow you to be explicit in checking the contained value instead of doing the same but through other means ? In this case I think if (std::holds_alternative<Tpg>(input_)) it's more clear than if (!sensor) as I immediately know what I'm checking for, instead of having to dig out in what case !sensor means Tpg, Inline or Memory. > > > if (ret) { > > LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > > return; > > @@ -379,7 +436,7 @@ int MaliC55CameraData::loadIPA() > > int ret; > > /* Do not initialize IPA for TPG. */ > > - if (!sensor_) > > + if (std::holds_alternative<Tpg>(input_)) > > return 0; > > ipa_ = IPAManager::createIPA<ipa::mali_c55::IPAProxyMaliC55>(pipe(), 1, 1); > > @@ -388,20 +445,20 @@ int MaliC55CameraData::loadIPA() > > ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls); > > - std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml", > > + std::string ipaTuningFile = ipa_->configurationFile(sensor()->model() + ".yaml", > > "uncalibrated.yaml"); > > /* We need to inform the IPA of the sensor configuration */ > > ipa::mali_c55::IPAConfigInfo ipaConfig{}; > > - ret = sensor_->sensorInfo(&ipaConfig.sensorInfo); > > + ret = sensor()->sensorInfo(&ipaConfig.sensorInfo); > > if (ret) > > return ret; > > - ipaConfig.sensorControls = sensor_->controls(); > > + ipaConfig.sensorControls = sensor()->controls(); > > ControlInfoMap ipaControls; > > - ret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig, > > + ret = ipa_->init({ ipaTuningFile, sensor()->model() }, ipaConfig, > > &ipaControls); > > if (ret) { > > LOG(MaliC55, Error) << "Failed to initialise the Mali-C55 IPA"; > > @@ -444,13 +501,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate() > > * The TPG doesn't support flips, so we only need to calculate a > > * transform if we have a sensor. > > */ > > - if (data_->sensor_) { > > + if (std::holds_alternative<MaliC55CameraData::Tpg>(data_->input_)) { > > + combinedTransform_ = Transform::Rot0; > > + } else { > > Orientation requestedOrientation = orientation; > > - combinedTransform_ = data_->sensor_->computeTransform(&orientation); > > + combinedTransform_ = data_->sensor()->computeTransform(&orientation); > > if (orientation != requestedOrientation) > > status = Adjusted; > > - } else { > > - combinedTransform_ = Transform::Rot0; > > } > > /* Only 2 streams available. */ > > @@ -927,11 +984,12 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > > /* Link the graph depending if we are operating the TPG or a sensor. */ > > MaliC55CameraData *data = cameraData(camera); > > - if (data->csi_) { > > - const MediaEntity *csiEntity = data->csi_->entity(); > > - ret = csiEntity->getPadByIndex(1)->links()[0]->setEnabled(true); > > + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { > > + const MediaEntity *tpgEntity = data->subdev()->entity(); > > + ret = tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true); > > } else { > > - ret = data->entity_->getPadByIndex(0)->links()[0]->setEnabled(true); > > + const MediaEntity *csi2Entity = data->csi2()->entity(); > > + ret = csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true); > > } > > I think the above part is definitely a place where std::visit could be used. E.g. > (after all patches): > > ret = std::visit(utils::overloaded { > [](MaliC55CameraData::Tpg &x) { > const MediaEntity *tpgEntity = x.sd_->entity(); > return tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true); > }, > [](MaliC55CameraData::Inline &x) { > const MediaEntity *csi2Entity = x.csi2_->entity(); > return csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true); > }, > [&](MaliC55CameraData::Memory &) { > const MediaEntity *ivcEntity = ivcSd_->entity(); > return ivcEntity->getPadByIndex(1)->links()[0]->setEnabled(true); > }, > }, data->input_); > I can do it. I'll ask you to test the series though, I don't have a device right now, I'll be away next week and I don't want this series which has been around for 1 year to be delayed again for minor details. > > > if (ret) > > return ret; > > @@ -939,26 +997,24 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > > MaliC55CameraConfiguration *maliConfig = > > static_cast<MaliC55CameraConfiguration *>(config); > > V4L2SubdeviceFormat subdevFormat = maliConfig->sensorFormat_; > > - ret = data->sd_->getFormat(0, &subdevFormat); > > - if (ret) > > - return ret; > > - if (data->sensor_) { > > - ret = data->sensor_->setFormat(&subdevFormat, > > - maliConfig->combinedTransform()); > > + /* Apply format to the origin of the pipeline and propagate it. */ > > + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { > > + ret = data->subdev()->setFormat(0, &subdevFormat); > > + } else { > > + ret = data->sensor()->setFormat(&subdevFormat, > > + maliConfig->combinedTransform()); > > if (ret) > > return ret; > > - } > > - if (data->csi_) { > > - ret = data->csi_->setFormat(0, &subdevFormat); > > + ret = data->csi2()->setFormat(0, &subdevFormat); > > if (ret) > > return ret; > > - ret = data->csi_->getFormat(1, &subdevFormat); > > - if (ret) > > - return ret; > > + ret = data->csi2()->getFormat(1, &subdevFormat); > > } > > I have the same impression here as well. > ok Thanks j > > > + if (ret) > > + return ret; > > V4L2DeviceFormat statsFormat; > > ret = stats_->getFormat(&statsFormat); > > [...]
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp index c209b0b070b1..6581d13bbc52 100644 --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp @@ -11,12 +11,14 @@ #include <memory> #include <set> #include <string> +#include <variant> #include <linux/mali-c55-config.h> #include <linux/media-bus-format.h> #include <linux/media.h> #include <libcamera/base/log.h> +#include <libcamera/base/utils.h> #include <libcamera/camera.h> #include <libcamera/formats.h> @@ -92,17 +94,90 @@ struct MaliC55FrameInfo { class MaliC55CameraData : public Camera::Private { public: - MaliC55CameraData(PipelineHandler *pipe, MediaEntity *entity) - : Camera::Private(pipe), entity_(entity) + struct Tpg { + std::vector<Size> sizes(unsigned int mbusCode) const; + + Size resolution_; + std::unique_ptr<V4L2Subdevice> sd_; + }; + + struct Inline { + std::unique_ptr<V4L2Subdevice> csi2_; + std::unique_ptr<CameraSensor> sensor_; + }; + using CameraType = std::variant<Tpg, Inline>; + + MaliC55CameraData(PipelineHandler *pipe) + : Camera::Private(pipe) { } - int init(); int loadIPA(); - /* Deflect these functionalities to either TPG or CameraSensor. */ - std::vector<Size> sizes(unsigned int mbusCode) const; - Size resolution() const; + int initTpg(MediaEntity *entity); + int initInline(MediaEntity *entity); + + std::vector<Size> sizes(unsigned int mbusCode) const + { + return std::visit(utils::overloaded{ + [&](const Tpg &tpg) -> std::vector<Size> { + return tpg.sizes(mbusCode); + }, + [&](const Inline &in) -> std::vector<Size> { + return in.sensor_->sizes(mbusCode); + } }, + input_); + } + + V4L2Subdevice *subdev() const + { + return std::visit(utils::overloaded{ + [&](const Tpg &tpg) -> V4L2Subdevice * { + return tpg.sd_.get(); + }, + [&](const Inline &in) -> V4L2Subdevice * { + return in.sensor_->device(); + } }, + input_); + } + + CameraSensor *sensor() const + { + return std::visit(utils::overloaded{ + [&](auto &) -> CameraSensor * { + ASSERT(false); + return nullptr; + }, + [&](const Inline &in) -> CameraSensor * { + return in.sensor_.get(); + } }, + input_); + } + + V4L2Subdevice *csi2() const + { + return std::visit(utils::overloaded{ + [&](auto &) -> V4L2Subdevice * { + ASSERT(false); + return nullptr; + }, + [&](const Inline &in) -> V4L2Subdevice * { + return in.csi2_.get(); + } }, + input_); + } + + Size resolution() const + { + return std::visit(utils::overloaded{ + [&](const Tpg &tpg) -> Size { + return tpg.resolution_; + }, + [&](const Inline &in) -> Size { + return in.sensor_->resolution(); + } }, + input_); + } int pixfmtToMbusCode(const PixelFormat &pixFmt) const; const PixelFormat &bestRawFormat() const; @@ -112,11 +187,6 @@ public: PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const; Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const; - std::unique_ptr<CameraSensor> sensor_; - - MediaEntity *entity_; - std::unique_ptr<V4L2Subdevice> csi_; - std::unique_ptr<V4L2Subdevice> sd_; Stream frStream_; Stream dsStream_; @@ -126,58 +196,28 @@ public: std::unique_ptr<DelayedControls> delayedCtrls_; + CameraType input_; + private: - void initTPGData(); void setSensorControls(const ControlList &sensorControls); - std::string id_; - Size tpgResolution_; }; -int MaliC55CameraData::init() +int MaliC55CameraData::initTpg(MediaEntity *entity) { - int ret; + Tpg tpg; - sd_ = std::make_unique<V4L2Subdevice>(entity_); - ret = sd_->open(); + tpg.sd_ = std::make_unique<V4L2Subdevice>(entity); + int ret = tpg.sd_->open(); if (ret) { - LOG(MaliC55, Error) << "Failed to open sensor subdevice"; + LOG(MaliC55, Error) << "Failed to open TPG subdevice"; return ret; } - /* If this camera is created from TPG, we return here. */ - if (entity_->name() == "mali-c55 tpg") { - initTPGData(); - return 0; - } - - /* - * Register a CameraSensor if we connect to a sensor and create - * an entity for the connected CSI-2 receiver. - */ - sensor_ = CameraSensorFactoryBase::create(entity_); - if (!sensor_) - return -ENODEV; - - const MediaPad *sourcePad = entity_->getPadByIndex(0); - MediaEntity *csiEntity = sourcePad->links()[0]->sink()->entity(); - - csi_ = std::make_unique<V4L2Subdevice>(csiEntity); - ret = csi_->open(); - if (ret) { - LOG(MaliC55, Error) << "Failed to open CSI-2 subdevice"; - return ret; - } - - return 0; -} - -void MaliC55CameraData::initTPGData() -{ /* Replicate the CameraSensor implementation for TPG. */ - V4L2Subdevice::Formats formats = sd_->formats(0); + V4L2Subdevice::Formats formats = tpg.sd_->formats(0); if (formats.empty()) - return; + return -EINVAL; std::vector<Size> tpgSizes; @@ -187,19 +227,39 @@ void MaliC55CameraData::initTPGData() [](const SizeRange &range) { return range.max; }); } - tpgResolution_ = tpgSizes.back(); + tpg.resolution_ = tpgSizes.back(); + + input_.emplace<Tpg>(std::move(tpg)); + + return 0; } -void MaliC55CameraData::setSensorControls(const ControlList &sensorControls) +int MaliC55CameraData::initInline(MediaEntity *sensor) { - delayedCtrls_->push(sensorControls); + Inline in; + + /* Register a CameraSensor and create an entity for the CSI-2 receiver. */ + in.sensor_ = CameraSensorFactoryBase::create(sensor); + if (!in.sensor_) + return -EINVAL; + + const MediaPad *sourcePad = sensor->getPadByIndex(0); + MediaEntity *csiEntity = sourcePad->links()[0]->sink()->entity(); + + in.csi2_ = std::make_unique<V4L2Subdevice>(csiEntity); + int ret = in.csi2_->open(); + if (ret) { + LOG(MaliC55, Error) << "Failed to open CSI-2 subdevice"; + return ret; + } + + input_.emplace<Inline>(std::move(in)); + + return 0; } -std::vector<Size> MaliC55CameraData::sizes(unsigned int mbusCode) const +std::vector<Size> MaliC55CameraData::Tpg::sizes(unsigned int mbusCode) const { - if (sensor_) - return sensor_->sizes(mbusCode); - V4L2Subdevice::Formats formats = sd_->formats(0); if (formats.empty()) return {}; @@ -218,12 +278,9 @@ std::vector<Size> MaliC55CameraData::sizes(unsigned int mbusCode) const return sizes; } -Size MaliC55CameraData::resolution() const +void MaliC55CameraData::setSensorControls(const ControlList &sensorControls) { - if (sensor_) - return sensor_->resolution(); - - return tpgResolution_; + delayedCtrls_->push(sensorControls); } /* @@ -242,7 +299,7 @@ int MaliC55CameraData::pixfmtToMbusCode(const PixelFormat &pixFmt) const if (!bayerFormat.isValid()) return -EINVAL; - V4L2Subdevice::Formats formats = sd_->formats(0); + V4L2Subdevice::Formats formats = subdev()->formats(0); unsigned int sensorMbusCode = 0; unsigned int bitDepth = 0; @@ -280,7 +337,7 @@ const PixelFormat &MaliC55CameraData::bestRawFormat() const { static const PixelFormat invalidPixFmt = {}; - for (const auto &fmt : sd_->formats(0)) { + for (const auto &fmt : subdev()->formats(0)) { BayerFormat sensorBayer = BayerFormat::fromMbusCode(fmt.first); if (!sensorBayer.isValid()) @@ -302,11 +359,11 @@ const PixelFormat &MaliC55CameraData::bestRawFormat() const void MaliC55CameraData::updateControls(const ControlInfoMap &ipaControls) { - if (!sensor_) + if (std::holds_alternative<Tpg>(input_)) return; IPACameraSensorInfo sensorInfo; - int ret = sensor_->sensorInfo(&sensorInfo); + int ret = sensor()->sensorInfo(&sensorInfo); if (ret) { LOG(MaliC55, Error) << "Failed to retrieve sensor info"; return; @@ -379,7 +436,7 @@ int MaliC55CameraData::loadIPA() int ret; /* Do not initialize IPA for TPG. */ - if (!sensor_) + if (std::holds_alternative<Tpg>(input_)) return 0; ipa_ = IPAManager::createIPA<ipa::mali_c55::IPAProxyMaliC55>(pipe(), 1, 1); @@ -388,20 +445,20 @@ int MaliC55CameraData::loadIPA() ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls); - std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml", + std::string ipaTuningFile = ipa_->configurationFile(sensor()->model() + ".yaml", "uncalibrated.yaml"); /* We need to inform the IPA of the sensor configuration */ ipa::mali_c55::IPAConfigInfo ipaConfig{}; - ret = sensor_->sensorInfo(&ipaConfig.sensorInfo); + ret = sensor()->sensorInfo(&ipaConfig.sensorInfo); if (ret) return ret; - ipaConfig.sensorControls = sensor_->controls(); + ipaConfig.sensorControls = sensor()->controls(); ControlInfoMap ipaControls; - ret = ipa_->init({ ipaTuningFile, sensor_->model() }, ipaConfig, + ret = ipa_->init({ ipaTuningFile, sensor()->model() }, ipaConfig, &ipaControls); if (ret) { LOG(MaliC55, Error) << "Failed to initialise the Mali-C55 IPA"; @@ -444,13 +501,13 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate() * The TPG doesn't support flips, so we only need to calculate a * transform if we have a sensor. */ - if (data_->sensor_) { + if (std::holds_alternative<MaliC55CameraData::Tpg>(data_->input_)) { + combinedTransform_ = Transform::Rot0; + } else { Orientation requestedOrientation = orientation; - combinedTransform_ = data_->sensor_->computeTransform(&orientation); + combinedTransform_ = data_->sensor()->computeTransform(&orientation); if (orientation != requestedOrientation) status = Adjusted; - } else { - combinedTransform_ = Transform::Rot0; } /* Only 2 streams available. */ @@ -927,11 +984,12 @@ int PipelineHandlerMaliC55::configure(Camera *camera, /* Link the graph depending if we are operating the TPG or a sensor. */ MaliC55CameraData *data = cameraData(camera); - if (data->csi_) { - const MediaEntity *csiEntity = data->csi_->entity(); - ret = csiEntity->getPadByIndex(1)->links()[0]->setEnabled(true); + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { + const MediaEntity *tpgEntity = data->subdev()->entity(); + ret = tpgEntity->getPadByIndex(0)->links()[0]->setEnabled(true); } else { - ret = data->entity_->getPadByIndex(0)->links()[0]->setEnabled(true); + const MediaEntity *csi2Entity = data->csi2()->entity(); + ret = csi2Entity->getPadByIndex(1)->links()[0]->setEnabled(true); } if (ret) return ret; @@ -939,26 +997,24 @@ int PipelineHandlerMaliC55::configure(Camera *camera, MaliC55CameraConfiguration *maliConfig = static_cast<MaliC55CameraConfiguration *>(config); V4L2SubdeviceFormat subdevFormat = maliConfig->sensorFormat_; - ret = data->sd_->getFormat(0, &subdevFormat); - if (ret) - return ret; - if (data->sensor_) { - ret = data->sensor_->setFormat(&subdevFormat, - maliConfig->combinedTransform()); + /* Apply format to the origin of the pipeline and propagate it. */ + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { + ret = data->subdev()->setFormat(0, &subdevFormat); + } else { + ret = data->sensor()->setFormat(&subdevFormat, + maliConfig->combinedTransform()); if (ret) return ret; - } - if (data->csi_) { - ret = data->csi_->setFormat(0, &subdevFormat); + ret = data->csi2()->setFormat(0, &subdevFormat); if (ret) return ret; - ret = data->csi_->getFormat(1, &subdevFormat); - if (ret) - return ret; + ret = data->csi2()->getFormat(1, &subdevFormat); } + if (ret) + return ret; V4L2DeviceFormat statsFormat; ret = stats_->getFormat(&statsFormat); @@ -973,8 +1029,6 @@ int PipelineHandlerMaliC55::configure(Camera *camera, /* * Propagate the format to the ISP sink pad and configure the input * crop rectangle (no crop at the moment). - * - * \todo Configure the CSI-2 receiver. */ ret = isp_->setFormat(0, &subdevFormat); if (ret) @@ -1058,18 +1112,18 @@ int PipelineHandlerMaliC55::configure(Camera *camera, /* We need to inform the IPA of the sensor configuration */ ipa::mali_c55::IPAConfigInfo ipaConfig{}; - ret = data->sensor_->sensorInfo(&ipaConfig.sensorInfo); + ret = data->sensor()->sensorInfo(&ipaConfig.sensorInfo); if (ret) return ret; - ipaConfig.sensorControls = data->sensor_->controls(); + ipaConfig.sensorControls = data->sensor()->controls(); /* * And we also need to tell the IPA the bayerOrder of the data (as * affected by any flips that we've configured) */ const Transform &combinedTransform = maliConfig->combinedTransform(); - BayerFormat::Order bayerOrder = data->sensor_->bayerOrder(combinedTransform); + BayerFormat::Order bayerOrder = data->sensor()->bayerOrder(combinedTransform); ControlInfoMap ipaControls; ret = data->ipa_->configure(ipaConfig, utils::to_underlying(bayerOrder), @@ -1283,7 +1337,7 @@ void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, if (!scalerCrop) return; - if (!data->sensor_) { + if (std::holds_alternative<MaliC55CameraData::Tpg>(data->input_)) { LOG(MaliC55, Error) << "ScalerCrop not supported for TPG"; return; } @@ -1291,7 +1345,7 @@ void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, Rectangle nativeCrop = *scalerCrop; IPACameraSensorInfo sensorInfo; - int ret = data->sensor_->sensorInfo(&sensorInfo); + int ret = data->sensor()->sensorInfo(&sensorInfo); if (ret) { LOG(MaliC55, Error) << "Failed to retrieve sensor info"; return; @@ -1573,10 +1627,11 @@ bool PipelineHandlerMaliC55::registerTPGCamera(MediaLink *link) } std::unique_ptr<MaliC55CameraData> data = - std::make_unique<MaliC55CameraData>(this, link->source()->entity()); + std::make_unique<MaliC55CameraData>(this); - if (data->init()) - return false; + int ret = data->initTpg(link->source()->entity()); + if (ret) + return ret; return registerMaliCamera(std::move(data), name); } @@ -1600,21 +1655,24 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) continue; std::unique_ptr<MaliC55CameraData> data = - std::make_unique<MaliC55CameraData>(this, sensor); - if (data->init()) - return false; + std::make_unique<MaliC55CameraData>(this); + + int ret = data->initInline(sensor); + if (ret) + return ret; - data->properties_ = data->sensor_->properties(); + data->properties_ = data->sensor()->properties(); - const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays(); + const CameraSensorProperties::SensorDelays &delays = + data->sensor()->sensorDelays(); std::unordered_map<uint32_t, DelayedControls::ControlParams> params = { { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } }, { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } }, }; - data->delayedCtrls_ = - std::make_unique<DelayedControls>(data->sensor_->device(), - params); + V4L2Subdevice *sensorSubdev = data->sensor()->device(); + data->delayedCtrls_ = std::make_unique<DelayedControls>(sensorSubdev, + params); isp_->frameStart.connect(data->delayedCtrls_.get(), &DelayedControls::applyControls);
In order to prepare to support memory input cameras, split the handling of the TPG and Inline camera cases. The Mali C55 pipeline handler uses the entity and subdevice stored in the CameraData to support both the handling of the TPG and of the Inline (CSI-2 + sensor) use cases. Adding support for memory cameras by using the CRU unit would add yet-another special case making the code harder to follow and more prone to errors. Split the handling of the TPG and Inline cameras by introducing an std::variant<> variable and to deflect the functions called on the camera data to the correct type by using overloaded std::visit<>(). Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/libcamera/pipeline/mali-c55/mali-c55.cpp | 276 ++++++++++++++++----------- 1 file changed, 167 insertions(+), 109 deletions(-)