| Message ID | 20230531143946.23571-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch. On Wed, 31 May 2023 at 15:39, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Add a flag to the ipa->init() interface to indicate a mono sensor > variant. This flag will be used in a future commit to handle controls > that are invalid for mono sensors. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> All looks good to me. Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > --- > include/libcamera/ipa/raspberrypi.mojom | 1 + > src/ipa/rpi/common/ipa_base.cpp | 1 + > src/ipa/rpi/common/ipa_base.h | 1 + > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++- > 4 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index ba786e647ca1..d35289ee6229 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -21,6 +21,7 @@ struct SensorConfig { > > struct InitParams { > bool lensPresent; > + bool monoSensor; > }; > > struct InitResult { > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index db7a0eb3a1ca..150fe433d0df 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams ¶ms, Ini > } > > lensPresent_ = params.lensPresent; > + monoSensor_ = params.monoSensor; > > controller_.initialise(); > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > index 6f9c46bb16b1..39d00760d012 100644 > --- a/src/ipa/rpi/common/ipa_base.h > +++ b/src/ipa/rpi/common/ipa_base.h > @@ -87,6 +87,7 @@ private: > std::map<unsigned int, MappedFrameBuffer> buffers_; > > bool lensPresent_; > + bool monoSensor_; > ControlList libcameraMetadata_; > > std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 3bb5ec531e4f..12698056cda1 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration() > int CameraData::loadIPA(ipa::RPi::InitResult *result) > { > ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1); > + bool monoSensor = isMonoSensor(sensor_); > > if (!ipa_) > return -ENOENT; > @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > if (!configFromEnv || *configFromEnv == '\0') { > std::string model = sensor_->model(); > - if (isMonoSensor(sensor_)) > + if (monoSensor) > model += "_mono"; > configurationFile = ipa_->configurationFile(model + ".json"); > } else { > @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > ipa::RPi::InitParams params; > > params.lensPresent = !!sensor_->focusLens(); > + params.monoSensor = monoSensor; > int ret = platformInitIpa(params); > if (ret) > return ret; > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Wed, May 31, 2023 at 03:39:45PM +0100, Naushir Patuck via libcamera-devel wrote: > Add a flag to the ipa->init() interface to indicate a mono sensor > variant. This flag will be used in a future commit to handle controls > that are invalid for mono sensors. The patch itself seems to do the job, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'm however wondering if we could do a bit better. The need to identify the sensor type isn't specific to Raspberry Pi, so a solution that covers the other IPA modules would be nice. The IPU3 and RkISP1 IPA interfaces both pass an IPACameraSensorInfo structure to the init() function. Would it make sense to extend that with a mono flag, and use it for the Raspberry Pi IPA module too ? Another improvement would be to pass a CFA type instead of a mono flag, that would pave the way to support other CFA patterns (I'm thinking abour RGB+IR for instance). It may be too early to do so though. Finally, we already pass the sensor model in the IPASettings structure, which could be used on the IPA side to check what CFA pattern the sensor uses by adding that information to the camera sensor helpers. Any opinion about this ? > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/ipa/raspberrypi.mojom | 1 + > src/ipa/rpi/common/ipa_base.cpp | 1 + > src/ipa/rpi/common/ipa_base.h | 1 + > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++- > 4 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index ba786e647ca1..d35289ee6229 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -21,6 +21,7 @@ struct SensorConfig { > > struct InitParams { > bool lensPresent; > + bool monoSensor; > }; > > struct InitResult { > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index db7a0eb3a1ca..150fe433d0df 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams ¶ms, Ini > } > > lensPresent_ = params.lensPresent; > + monoSensor_ = params.monoSensor; > > controller_.initialise(); > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > index 6f9c46bb16b1..39d00760d012 100644 > --- a/src/ipa/rpi/common/ipa_base.h > +++ b/src/ipa/rpi/common/ipa_base.h > @@ -87,6 +87,7 @@ private: > std::map<unsigned int, MappedFrameBuffer> buffers_; > > bool lensPresent_; > + bool monoSensor_; > ControlList libcameraMetadata_; > > std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 3bb5ec531e4f..12698056cda1 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration() > int CameraData::loadIPA(ipa::RPi::InitResult *result) > { > ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1); > + bool monoSensor = isMonoSensor(sensor_); > > if (!ipa_) > return -ENOENT; > @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > if (!configFromEnv || *configFromEnv == '\0') { > std::string model = sensor_->model(); > - if (isMonoSensor(sensor_)) > + if (monoSensor) > model += "_mono"; > configurationFile = ipa_->configurationFile(model + ".json"); > } else { > @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > ipa::RPi::InitParams params; > > params.lensPresent = !!sensor_->focusLens(); > + params.monoSensor = monoSensor; > int ret = platformInitIpa(params); > if (ret) > return ret;
Hi Laurent, On Fri, 2 Jun 2023 at 08:46, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Wed, May 31, 2023 at 03:39:45PM +0100, Naushir Patuck via libcamera-devel wrote: > > Add a flag to the ipa->init() interface to indicate a mono sensor > > variant. This flag will be used in a future commit to handle controls > > that are invalid for mono sensors. > > The patch itself seems to do the job, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'm however wondering if we could do a bit better. The need to identify > the sensor type isn't specific to Raspberry Pi, so a solution that > covers the other IPA modules would be nice. The IPU3 and RkISP1 IPA > interfaces both pass an IPACameraSensorInfo structure to the init() > function. Would it make sense to extend that with a mono flag, and use > it for the Raspberry Pi IPA module too ? Sounds good, this would allow any IPA to get this flag. IPACameraSensorInfo is a good choice, rpi/rkisp/ipu3 all take this in during the configure() call. Rpi does not have IPACameraSensorInfo passed in on init, and the monoSensor_ flag is passed in init(). But really we don't need to know the state of the flag until configure() so it's not a problem. > > Another improvement would be to pass a CFA type instead of a mono flag, > that would pave the way to support other CFA patterns (I'm thinking > abour RGB+IR for instance). It may be too early to do so though. > I think I actually prefer this approach over a flag! We have the CFA in CameraSensor::properties (properties::draft::ColorFilterArrangement) which we can put in IPACameraSensorInfo. The CFA pattern might change when configuring (e.g. if a flip/mirror occurs) so this feels quite neat. What do you think? Let me know and I can rework this series. > Finally, we already pass the sensor model in the IPASettings structure, > which could be used on the IPA side to check what CFA pattern the sensor > uses by adding that information to the camera sensor helpers. Any > opinion about this ? This might not work for the IMX296 as we have both mono and colour variants. This is presuming that we only stick with one camera helper for IMX296 and the model string does not distinguish between the two? Regards, Naush > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > src/ipa/rpi/common/ipa_base.cpp | 1 + > > src/ipa/rpi/common/ipa_base.h | 1 + > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++- > > 4 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > index ba786e647ca1..d35289ee6229 100644 > > --- a/include/libcamera/ipa/raspberrypi.mojom > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > @@ -21,6 +21,7 @@ struct SensorConfig { > > > > struct InitParams { > > bool lensPresent; > > + bool monoSensor; > > }; > > > > struct InitResult { > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index db7a0eb3a1ca..150fe433d0df 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams ¶ms, Ini > > } > > > > lensPresent_ = params.lensPresent; > > + monoSensor_ = params.monoSensor; > > > > controller_.initialise(); > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > index 6f9c46bb16b1..39d00760d012 100644 > > --- a/src/ipa/rpi/common/ipa_base.h > > +++ b/src/ipa/rpi/common/ipa_base.h > > @@ -87,6 +87,7 @@ private: > > std::map<unsigned int, MappedFrameBuffer> buffers_; > > > > bool lensPresent_; > > + bool monoSensor_; > > ControlList libcameraMetadata_; > > > > std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 3bb5ec531e4f..12698056cda1 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration() > > int CameraData::loadIPA(ipa::RPi::InitResult *result) > > { > > ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1); > > + bool monoSensor = isMonoSensor(sensor_); > > > > if (!ipa_) > > return -ENOENT; > > @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > > if (!configFromEnv || *configFromEnv == '\0') { > > std::string model = sensor_->model(); > > - if (isMonoSensor(sensor_)) > > + if (monoSensor) > > model += "_mono"; > > configurationFile = ipa_->configurationFile(model + ".json"); > > } else { > > @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > > ipa::RPi::InitParams params; > > > > params.lensPresent = !!sensor_->focusLens(); > > + params.monoSensor = monoSensor; > > int ret = platformInitIpa(params); > > if (ret) > > return ret; > > -- > Regards, > > Laurent Pinchart
Hi Naush, On Fri, Jun 02, 2023 at 09:55:03AM +0100, Naushir Patuck wrote: > On Fri, 2 Jun 2023 at 08:46, Laurent Pinchart wrote: > > On Wed, May 31, 2023 at 03:39:45PM +0100, Naushir Patuck via libcamera-devel wrote: > > > Add a flag to the ipa->init() interface to indicate a mono sensor > > > variant. This flag will be used in a future commit to handle controls > > > that are invalid for mono sensors. > > > > The patch itself seems to do the job, so > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I'm however wondering if we could do a bit better. The need to identify > > the sensor type isn't specific to Raspberry Pi, so a solution that > > covers the other IPA modules would be nice. The IPU3 and RkISP1 IPA > > interfaces both pass an IPACameraSensorInfo structure to the init() > > function. Would it make sense to extend that with a mono flag, and use > > it for the Raspberry Pi IPA module too ? > > Sounds good, this would allow any IPA to get this flag. > > IPACameraSensorInfo is a good choice, rpi/rkisp/ipu3 all take this in during the > configure() call. Rpi does not have IPACameraSensorInfo passed in on init, and > the monoSensor_ flag is passed in init(). But really we don't need to know the > state of the flag until configure() so it's not a problem. > > > Another improvement would be to pass a CFA type instead of a mono flag, > > that would pave the way to support other CFA patterns (I'm thinking > > abour RGB+IR for instance). It may be too early to do so though. > > I think I actually prefer this approach over a flag! We have the CFA in > CameraSensor::properties (properties::draft::ColorFilterArrangement) which we > can put in IPACameraSensorInfo. The CFA pattern might change when configuring > (e.g. if a flip/mirror occurs) so this feels quite neat. > > What do you think? Let me know and I can rework this series. I see you've sent a v2 already, let's continue the discussion there. > > Finally, we already pass the sensor model in the IPASettings structure, > > which could be used on the IPA side to check what CFA pattern the sensor > > uses by adding that information to the camera sensor helpers. Any > > opinion about this ? > > This might not work for the IMX296 as we have both mono and colour variants. > This is presuming that we only stick with one camera helper for IMX296 and the > model string does not distinguish between the two? Good point. I think we can ignore this question for now, as adding the CFA pattern in the IPACameraSensorInfo works nicely. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > src/ipa/rpi/common/ipa_base.cpp | 1 + > > > src/ipa/rpi/common/ipa_base.h | 1 + > > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++- > > > 4 files changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > > index ba786e647ca1..d35289ee6229 100644 > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > @@ -21,6 +21,7 @@ struct SensorConfig { > > > > > > struct InitParams { > > > bool lensPresent; > > > + bool monoSensor; > > > }; > > > > > > struct InitResult { > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > index db7a0eb3a1ca..150fe433d0df 100644 > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams ¶ms, Ini > > > } > > > > > > lensPresent_ = params.lensPresent; > > > + monoSensor_ = params.monoSensor; > > > > > > controller_.initialise(); > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h > > > index 6f9c46bb16b1..39d00760d012 100644 > > > --- a/src/ipa/rpi/common/ipa_base.h > > > +++ b/src/ipa/rpi/common/ipa_base.h > > > @@ -87,6 +87,7 @@ private: > > > std::map<unsigned int, MappedFrameBuffer> buffers_; > > > > > > bool lensPresent_; > > > + bool monoSensor_; > > > ControlList libcameraMetadata_; > > > > > > std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > index 3bb5ec531e4f..12698056cda1 100644 > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration() > > > int CameraData::loadIPA(ipa::RPi::InitResult *result) > > > { > > > ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1); > > > + bool monoSensor = isMonoSensor(sensor_); > > > > > > if (!ipa_) > > > return -ENOENT; > > > @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > > > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); > > > if (!configFromEnv || *configFromEnv == '\0') { > > > std::string model = sensor_->model(); > > > - if (isMonoSensor(sensor_)) > > > + if (monoSensor) > > > model += "_mono"; > > > configurationFile = ipa_->configurationFile(model + ".json"); > > > } else { > > > @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) > > > ipa::RPi::InitParams params; > > > > > > params.lensPresent = !!sensor_->focusLens(); > > > + params.monoSensor = monoSensor; > > > int ret = platformInitIpa(params); > > > if (ret) > > > return ret;
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index ba786e647ca1..d35289ee6229 100644 --- a/include/libcamera/ipa/raspberrypi.mojom +++ b/include/libcamera/ipa/raspberrypi.mojom @@ -21,6 +21,7 @@ struct SensorConfig { struct InitParams { bool lensPresent; + bool monoSensor; }; struct InitResult { diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index db7a0eb3a1ca..150fe433d0df 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -139,6 +139,7 @@ int32_t IpaBase::init(const IPASettings &settings, const InitParams ¶ms, Ini } lensPresent_ = params.lensPresent; + monoSensor_ = params.monoSensor; controller_.initialise(); diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h index 6f9c46bb16b1..39d00760d012 100644 --- a/src/ipa/rpi/common/ipa_base.h +++ b/src/ipa/rpi/common/ipa_base.h @@ -87,6 +87,7 @@ private: std::map<unsigned int, MappedFrameBuffer> buffers_; bool lensPresent_; + bool monoSensor_; ControlList libcameraMetadata_; std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_; diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 3bb5ec531e4f..12698056cda1 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -1131,6 +1131,7 @@ int CameraData::loadPipelineConfiguration() int CameraData::loadIPA(ipa::RPi::InitResult *result) { ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1); + bool monoSensor = isMonoSensor(sensor_); if (!ipa_) return -ENOENT; @@ -1143,7 +1144,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE"); if (!configFromEnv || *configFromEnv == '\0') { std::string model = sensor_->model(); - if (isMonoSensor(sensor_)) + if (monoSensor) model += "_mono"; configurationFile = ipa_->configurationFile(model + ".json"); } else { @@ -1154,6 +1155,7 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result) ipa::RPi::InitParams params; params.lensPresent = !!sensor_->focusLens(); + params.monoSensor = monoSensor; int ret = platformInitIpa(params); if (ret) return ret;
Add a flag to the ipa->init() interface to indicate a mono sensor variant. This flag will be used in a future commit to handle controls that are invalid for mono sensors. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/ipa/raspberrypi.mojom | 1 + src/ipa/rpi/common/ipa_base.cpp | 1 + src/ipa/rpi/common/ipa_base.h | 1 + src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 +++- 4 files changed, 6 insertions(+), 1 deletion(-)