[libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor: Add Sony IMX708 sensor properties
Naushir Patuck
naush at raspberrypi.com
Mon Jan 23 13:36:13 CET 2023
Hi Laurent,
On Mon, 23 Jan 2023 at 12:12, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> On Mon, Jan 23, 2023 at 10:33:35AM +0000, Naushir Patuck wrote:
> > On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:
> > > On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:
> > > > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:
> > > > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via
> libcamera-devel wrote:
> > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)
> > > > > > > From: Nick Hollinghurst <nick.hollinghurst at raspberrypi.com>
> > > > > > >
> > > > > > > The IMX708 sensor driver advertises its module variants
> (narrow/wide angle lens,
> > > > > > > IR block/pass) by modifying the media entity name string. So
> add duplicate
> > > > > > > entries for each variant.
> > > > > >
> > > > > > The duplication here is a bit of a pain. Are there any sensor
> properties
> > > > > > we might register that would change in each of these variants?
> > > > > >
> > > > > > Or should we make the CameraSensorProperties class more
> intelligent so
> > > > > > it just does the lookup on the sensor only. Of course ideally we
> should
> > > > > > be able to identify the sensor and the variant separately from
> the
> > > > > > kernel - which might not be possible right now - in which case,
> given
> > > > > > how little information is actually duplicated here I'd be fine
> with this
> > > > > > for now.
> > > > >
> > > > > I'm concerned by this too. The kernel shouldn't report different
> entity
> > > > > names for different lenses. There should thus be a single entry in
> the
> > > > > camera sensors properties table, and lens information should be
> reported
> > > > > separately.
> > > >
> > > > I agree this is not the tidiest thing to do. Right now, we have no
> other way of
> > > > getting the variant details from the kernel driver into userland. I
> believe
> > > > @Dave Stevenson raised this topic at the last Embedded Linux
> Conference, but no
> > > > consensus was reached. Once we have a mechanism in the kernel to
> report lens
> > > > properties, we will remove this workaround. Would a \todo suffice
> for now?
> > >
> > > If adding a \todo was enough to make a mechanism for this appear
> > > upstream by magic, I'd say it's fine. Unfortunately, that doesn't match
> > > my experience :-)
> >
> > If only! :)
> >
> > > One middleground option would be to merge the first "variant" (the
> > > "imx708") when the driver gets posted to the linux-media mailing list,
> > > while the rest gets developed, but in any case it will require someone
> > > making a proposal upstream.
> >
> > We will post a variant-less driver to the mailing list and chat with
> Dave about
> > a proposal for module variant reporting.
>
> You could also include variant support in the driver posted to
> linux-media, so the proposal could be discussed during reviews. Up to
> you.
>
I think we will remove this to start with. I fear it will only muddy the
waters
for the bigger discussion. We can always refer back to the downstream
driver
to show examples of why it is required.
>
> > The reason we added the other variants in the CameraSensorProperties was
> to
> > quiet the warning log message when a device is not listed. Would it be
> ok to
> > keep all variants listed like this with a \todo to clean-up? If not, we
> will have to
> > make this change in our downstream tree which we are trying hard to
> deprecate :(
>
> What concerns me is when that would be cleaned up. Unless there's a real
> effort to design and upstream a mechanism for this, it won't happen, so
> I want to see someone taking the lead and bringing it to completion.
> Merging support for a device in libcamera before the kernel driver is
> ready upstream is already a middleground.
>
Got it, I'll remove the duplicate entries from the patch.
Regards,
Naush
>
> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > > >
> > > > > > > Signed-off-by: Nick Hollinghurst <
> nick.hollinghurst at raspberrypi.com>
> > > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > > ---
> > > > > > > src/libcamera/camera_sensor_properties.cpp | 16
> ++++++++++++++++
> > > > > > > 1 file changed, 16 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp
> b/src/libcamera/camera_sensor_properties.cpp
> > > > > > > index c3c2caced906..3afd500ea3be 100644
> > > > > > > --- a/src/libcamera/camera_sensor_properties.cpp
> > > > > > > +++ b/src/libcamera/camera_sensor_properties.cpp
> > > > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties
> *CameraSensorProperties::get(const std::string &sen
> > > > > > > */
> > > > > > > },
> > > > > > > } },
> > > > > > > + { "imx708", {
> > > > > > > + .unitCellSize = { 1400, 1400 },
> > > > > > > + .testPatternModes = {},
> > > > > > > + } },
> > > > > > > + { "imx708_noir", {
> > > > > > > + .unitCellSize = { 1400, 1400 },
> > > > > > > + .testPatternModes = {},
> > > > > > > + } },
> > > > > > > + { "imx708_wide", {
> > > > > > > + .unitCellSize = { 1400, 1400 },
> > > > > > > + .testPatternModes = {},
> > > > > > > + } },
> > > > > > > + { "imx708_wide_noir", {
> > > > > > > + .unitCellSize = { 1400, 1400 },
> > > > > > > + .testPatternModes = {},
> > > > > > > + } },
> > > > > > > { "ov2740", {
> > > > > > > .unitCellSize = { 1400, 1400 },
> > > > > > > .testPatternModes = {
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230123/3fe698a3/attachment.htm>
More information about the libcamera-devel
mailing list