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