| Message ID | 20210422094102.371772-10-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, On Thu, Apr 22, 2021 at 06:40:59PM +0900, Paul Elder wrote: > Add controls to IPU3Controls that are necessary for FULL compliance. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 28e849a4..70a5e9ce 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -48,8 +48,27 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > static constexpr Size IPU3ViewfinderSize(1280, 720); > > +const std::array<const ControlValue, 3> IPU3NoiseReductionModeValues = { > + static_cast<int32_t>(controls::draft::NoiseReductionModeOff), > + static_cast<int32_t>(controls::draft::NoiseReductionModeFast), > + static_cast<int32_t>(controls::draft::NoiseReductionModeHighQuality), > +}; > + > static const ControlInfoMap::Map IPU3Controls = { > { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, > + { &controls::draft::BlackLevelLocked, ControlInfo(false, true) }, > + { &controls::AeLocked, ControlInfo(false, true) }, > + { &controls::draft::AePrecaptureTrigger, > + ControlInfo(controls::draft::AePrecaptureTriggerValues) }, > + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > + { &controls::AwbLocked, ControlInfo(false, true) }, > + { &controls::draft::EdgeMode, > + ControlInfo(controls::draft::EdgeModeValues) }, > + { &controls::draft::NoiseReductionMode, > + ControlInfo(IPU3NoiseReductionModeValues) }, Interesting you've ruled out IPU3NoiseReductionModeZSL. What if we plumb an IPA that supports that ? We have two IPAs for IPU3, depending on which one is in use we might have different features supported. Going forward, do we need the IPA capabilities to be discoverable ? > + { &controls::draft::SensorSensitivity, ControlInfo(32, 2400) }, This should probably come from the sensor. The following V4L2 control is described as: V4L2_CID_ISO_SENSITIVITY (integer menu) Determines ISO equivalent of an image sensor indicating the sensor’s sensitivity to light. The numbers are expressed in arithmetic scale, as per ISO 12232:2006 standard, where doubling the sensor sensitivity is represented by doubling the numerical ISO value. Applications should interpret the values as standard ISO values multiplied by 1000, e.g. control value 800 stands for ISO 0.8. Drivers will usually support only a subset of standard ISO values. The effect of setting this control while the V4L2_CID_ISO_SENSITIVITY_AUTO control is set to a value other than V4L2_CID_ISO_SENSITIVITY_MANUAL is undefined, drivers should ignore such requests. It's good the standard the control refers to is the same as android.sensor.sensitivity control: The sensitivity is the standard ISO sensitivity value,as defined in ISO 12232:2006. Anyway, that's more a policy decision. Do we require/mandate that control in sensor drivers (keeping in mind we keep rising the bar for drivers to qualify libcamera-compatible) or do we go through the sensor database ? Thanks j > + { &controls::draft::TonemapMode, > + ControlInfo(controls::draft::TonemapModeValues) }, > }; > > class IPU3CameraData : public CameraData > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Tue, Apr 27, 2021 at 12:31:46PM +0200, Jacopo Mondi wrote: > Hi Paul, > > On Thu, Apr 22, 2021 at 06:40:59PM +0900, Paul Elder wrote: > > Add controls to IPU3Controls that are necessary for FULL compliance. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 28e849a4..70a5e9ce 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -48,8 +48,27 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > static constexpr Size IPU3ViewfinderSize(1280, 720); > > > > +const std::array<const ControlValue, 3> IPU3NoiseReductionModeValues = { > > + static_cast<int32_t>(controls::draft::NoiseReductionModeOff), > > + static_cast<int32_t>(controls::draft::NoiseReductionModeFast), > > + static_cast<int32_t>(controls::draft::NoiseReductionModeHighQuality), > > +}; > > + > > static const ControlInfoMap::Map IPU3Controls = { > > { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, > > + { &controls::draft::BlackLevelLocked, ControlInfo(false, true) }, > > + { &controls::AeLocked, ControlInfo(false, true) }, > > + { &controls::draft::AePrecaptureTrigger, > > + ControlInfo(controls::draft::AePrecaptureTriggerValues) }, > > + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, > > + { &controls::AwbLocked, ControlInfo(false, true) }, > > + { &controls::draft::EdgeMode, > > + ControlInfo(controls::draft::EdgeModeValues) }, > > + { &controls::draft::NoiseReductionMode, > > + ControlInfo(IPU3NoiseReductionModeValues) }, > > Interesting you've ruled out IPU3NoiseReductionModeZSL. > What if we plumb an IPA that supports that ? We have two IPAs for CTS complains if you don't support reprocessing but do support noise reduction ZSL. > IPU3, depending on which one is in use we might have different > features supported. Going forward, do we need the IPA capabilities to > be discoverable ? I think that would be better, yeah. > > > + { &controls::draft::SensorSensitivity, ControlInfo(32, 2400) }, > > This should probably come from the sensor. > > The following V4L2 control is described as: > > V4L2_CID_ISO_SENSITIVITY (integer menu) > Determines ISO equivalent of an image sensor indicating the sensor’s > sensitivity to light. The numbers are expressed in arithmetic scale, > as per ISO 12232:2006 standard, where doubling the sensor sensitivity > is represented by doubling the numerical ISO value. Applications > should interpret the values as standard ISO values multiplied by 1000, > e.g. control value 800 stands for ISO 0.8. Drivers will usually > support only a subset of standard ISO values. The effect of setting > this control while the V4L2_CID_ISO_SENSITIVITY_AUTO control is set to > a value other than V4L2_CID_ISO_SENSITIVITY_MANUAL is undefined, > drivers should ignore such requests. > > It's good the standard the control refers to is the same as > android.sensor.sensitivity control: > > The sensitivity is the standard ISO sensitivity value,as defined in ISO 12232:2006. > > Anyway, that's more a policy decision. Do we require/mandate that > control in sensor drivers (keeping in mind we keep rising the bar for > drivers to qualify libcamera-compatible) or do we go through the > sensor database ? If the sensor driver doesn't support it, then we could just drop to LIMITED hardware level. This is only needed for FULL. So for libcamera compatibility I think it's not an issue. Paul > > > + { &controls::draft::TonemapMode, > > + ControlInfo(controls::draft::TonemapModeValues) }, > > }; > > > > class IPU3CameraData : public CameraData > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 28e849a4..70a5e9ce 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -48,8 +48,27 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; static constexpr Size IPU3ViewfinderSize(1280, 720); +const std::array<const ControlValue, 3> IPU3NoiseReductionModeValues = { + static_cast<int32_t>(controls::draft::NoiseReductionModeOff), + static_cast<int32_t>(controls::draft::NoiseReductionModeFast), + static_cast<int32_t>(controls::draft::NoiseReductionModeHighQuality), +}; + static const ControlInfoMap::Map IPU3Controls = { { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, + { &controls::draft::BlackLevelLocked, ControlInfo(false, true) }, + { &controls::AeLocked, ControlInfo(false, true) }, + { &controls::draft::AePrecaptureTrigger, + ControlInfo(controls::draft::AePrecaptureTriggerValues) }, + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) }, + { &controls::AwbLocked, ControlInfo(false, true) }, + { &controls::draft::EdgeMode, + ControlInfo(controls::draft::EdgeModeValues) }, + { &controls::draft::NoiseReductionMode, + ControlInfo(IPU3NoiseReductionModeValues) }, + { &controls::draft::SensorSensitivity, ControlInfo(32, 2400) }, + { &controls::draft::TonemapMode, + ControlInfo(controls::draft::TonemapModeValues) }, }; class IPU3CameraData : public CameraData
Add controls to IPU3Controls that are necessary for FULL compliance. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)