| Message ID | 20200326145927.324919-7-jacopo@jmondi.org |
|---|---|
| State | RFC |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your discussion points ;-) On 2020-03-26 15:59:27 +0100, Jacopo Mondi wrote: > Register lens properties in the ov5670 sensor handler. > > This patch is not intended for merge as we know lens properties do no > belong to the sensor handler, but I am including it anyhow to trigger > discussions on where they would be more appropriately defined. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/sensor/ov5670.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/libcamera/sensor/ov5670.cpp b/src/libcamera/sensor/ov5670.cpp > index d7339b9792e1..8c6a6114c9bf 100644 > --- a/src/libcamera/sensor/ov5670.cpp > +++ b/src/libcamera/sensor/ov5670.cpp > @@ -46,6 +46,12 @@ int OV5670::initProperties() > properties::BayerFilterGRBG); > properties_.set(properties::ISOSensitivityRange, { 50, 800 }); > > + /* Lens Properties. */ > + properties_.set(properties::LensApertures, { 0.0f }); > + properties_.set(properties::LensFocalLengths, { 3.69f }); Would it make sens to try and record aperture information in device tree? It is one form of hardware description right? We still will need a way to express them some other way, maybe in a platform configuration file? > + properties_.set(properties::LensHyperfocalDistances, { 0.0f }); > + properties_.set(properties::LensMinimumFocusDistance, 3.69f); For these I'm not sure as i recall my optics they some what depends to some degree on the properties above? > + > return CameraSensor::initProperties(); > } > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Apr 08, 2020 at 01:09:21AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your discussion points ;-) > > On 2020-03-26 15:59:27 +0100, Jacopo Mondi wrote: > > Register lens properties in the ov5670 sensor handler. > > > > This patch is not intended for merge as we know lens properties do no > > belong to the sensor handler, but I am including it anyhow to trigger > > discussions on where they would be more appropriately defined. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/sensor/ov5670.cpp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/libcamera/sensor/ov5670.cpp b/src/libcamera/sensor/ov5670.cpp > > index d7339b9792e1..8c6a6114c9bf 100644 > > --- a/src/libcamera/sensor/ov5670.cpp > > +++ b/src/libcamera/sensor/ov5670.cpp > > @@ -46,6 +46,12 @@ int OV5670::initProperties() > > properties::BayerFilterGRBG); > > properties_.set(properties::ISOSensitivityRange, { 50, 800 }); > > > > + /* Lens Properties. */ > > + properties_.set(properties::LensApertures, { 0.0f }); > > + properties_.set(properties::LensFocalLengths, { 3.69f }); > > Would it make sens to try and record aperture information in device > tree? It is one form of hardware description right? We still will need a > way to express them some other way, maybe in a platform configuration > file? > That's an open discussion, the patch is DNI for this reason :) I see two possible issues in going for a DT based solution. 1) We don't only have DT based devices But actually for ACPI based platforms there seems to be already support for parsing lens related properties https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-fwnode.c#L1093 They're device properties https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/media/video-interfaces.txt#L80 As well as the camera module related properties we have (almost) upstreamed https://patchwork.kernel.org/project/linux-media/list/?series=272849 So they might be a good fit, but 2) Lens properties depends on the camera module module, not on the sensor The DT/ACPI describes the image sensor as an i2c device. The board/platform .dts file usually adds an i2c device child node to describe the image sensor on the i2c bus, but the same sensor could actually be coupled with different lenses in different casing depending on the camera module manufacturer. Putting lens properties there would make the i2c child node describe the camera module, not the sensor only. I can't tell how a big deal that would be, to me it's more than acceptable if not desirable, but I might be missing some issue. Another option would be to provide a phandle in the i2c child node that points to a 'lens' device node which would only contain read-only properties but I'm not sure how well that would play with the existing 'lens-focus' property that points to the lens VCM controller... 3) Stand-alone camera modules report lens/casing information, it's harder to get the same info for image sensor integrated in a device like a laptop/tablet/phone. A configuration file opens up a lot of other issues, mostly regarding distribution of device specific information that libcamera would require to function properyl, and we have refreined from going that way for this reason. > > + properties_.set(properties::LensHyperfocalDistances, { 0.0f }); > > + properties_.set(properties::LensMinimumFocusDistance, 3.69f); > > For these I'm not sure as i recall my optics they some what depends to > some degree on the properties above? > Do you mean they could be calculated ? I honestly can't tell without refreshing my already poor knowledge of optics... > > + > > return CameraSensor::initProperties(); > > } > > > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
diff --git a/src/libcamera/sensor/ov5670.cpp b/src/libcamera/sensor/ov5670.cpp index d7339b9792e1..8c6a6114c9bf 100644 --- a/src/libcamera/sensor/ov5670.cpp +++ b/src/libcamera/sensor/ov5670.cpp @@ -46,6 +46,12 @@ int OV5670::initProperties() properties::BayerFilterGRBG); properties_.set(properties::ISOSensitivityRange, { 50, 800 }); + /* Lens Properties. */ + properties_.set(properties::LensApertures, { 0.0f }); + properties_.set(properties::LensFocalLengths, { 3.69f }); + properties_.set(properties::LensHyperfocalDistances, { 0.0f }); + properties_.set(properties::LensMinimumFocusDistance, 3.69f); + return CameraSensor::initProperties(); }
Register lens properties in the ov5670 sensor handler. This patch is not intended for merge as we know lens properties do no belong to the sensor handler, but I am including it anyhow to trigger discussions on where they would be more appropriately defined. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/sensor/ov5670.cpp | 6 ++++++ 1 file changed, 6 insertions(+)