<div dir="ltr"><div dir="ltr"><div>> Agreed. The driver should be upstreamed</div><div><br></div><div>I'm going to send an initial version to linux-media today for comment. I'll include a link to their mailing list with the next set of changes.</div><div><br></div><div>> Give that the entity name will need to change when upstreaming, one way<br>> forward would be to patch the pinephone kernel driver with the correct<br>> entity name already, and use it in libcamera right away.<span class="gmail-im" style="color:rgb(80,0,80)"><br></span></div><div><br></div><div>TL;DR: Media node names do not seem to be standardized and various formats exist. I suggest adding an `else` case to [6] with an additional regex for the ov8858 driver for maximum compatibility with non-mainline ov8858 implementations while we sort out getting this upstreamed.</div><div>--</div><div><br></div>Upon reading the driver [0] more closely, it looks like "m00_f_ov8858" is just the <i>subdevice name </i>(set to "m{module_index:02d}_{facing}_{OV8858_NAME}" on :2936), not the driver name (set to OV8858_NAME == "ov8858" on :3004) or sensor name (set to OV8858_NAME == "ov8858" on :1778).<div><br></div><div>Reading the kernel docs [3], it looks like this is required to be unique:</div><div>"Afterwards you need to initialize sd->name with a unique name and set the module owner. This is done for you if you use the i2c helper functions."</div><div><br></div><div>The subdevice name <i>defaults</i> to the driver name in [4] when v4l2_i2c_subdev_init() invokes v4l2_i2c_subdev_set_name(sd, client, NULL, NULL), but this is not guaranteed to be the driver name if I understand correctly. In fact, this appears to solve for the case where there may be multiple ov8858s installed, which the default implementation would not. Though there is some ambiguity in the phrase "unique name," v4l2-subdev.h [5] seems to suggest that these must be unique among device instances, not drivers, since `struct v4l2_subdev` corresponds to a devnode and also specifies "@name: Name of the sub-device. Please notice that the name must be unique."</div><div><br></div><div>Meanwhile, both Megi's imx258 fork [1] and the mainline imx258 driver [2] use the default implementation, so we do not have this issue using libcamera with an imx258.</div><div><br></div><div><b>**libcamera subdevice name handling (regex guess)**</b></div><div>Currently, libcamera uses a regex to guess the model name from the subdevice name in v4l2_subdevice.cpp V4L2Subdevice::model() [6]. Instead, </div><div><br></div><div><b>**Proposed Solution**</b>:</div><div>I suggest adding an `else` case to [6] with an additional regex for the ov8858 driver for maximum compatibility with non-mainline ov8858 implementations while we sort out getting this upstreamed.</div><div><div><br></div><div>0: <a href="https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/ov8858.c">https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/ov8858.c</a></div><div>1: <a href="https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/imx258.c">https://github.com/megous/linux/blob/orange-pi-6.0/drivers/media/i2c/imx258.c</a></div><div>2: <a href="https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c">https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c</a></div></div><div>3: <a href="https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html">https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html</a></div><div>4: <a href="https://github.com/torvalds/linux/blob/master/drivers/media/v4l2-core/v4l2-i2c.c">https://github.com/torvalds/linux/blob/master/drivers/media/v4l2-core/v4l2-i2c.c</a></div><div>5: <a href="https://github.com/torvalds/linux/blob/master/include/media/v4l2-subdev.h">https://github.com/torvalds/linux/blob/master/include/media/v4l2-subdev.h</a></div><div>6: <a href="https://github.com/kbingham/libcamera/blob/master/src/libcamera/v4l2_subdevice.cpp">https://github.com/kbingham/libcamera/blob/master/src/libcamera/v4l2_subdevice.cpp</a></div></div><br><img width="0" height="0" class="mailtrack-img" alt="" style="display:flex" src="https://mailtrack.io/trace/mail/2d029f838aa30966f76ce4a5a322b36a167aa013.png?u=4192442"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Oct 30, 2022 at 11:21 AM 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">Hello,<br>
<br>
On Fri, Oct 28, 2022 at 11:25:38AM +0100, Kieran Bingham wrote:<br>
> Quoting Jacopo Mondi via libcamera-devel (2022-10-28 09:14:27)<br>
> > Hi Nicholas,<br>
> > + Laurent question below<br>
> > <br>
> > On Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:<br>
> > > > Nice this matches the CCS defined linear gain model.<br>
> > > > However the sensor allows to select two formats for the analogue gain<br>
> > > > the "real gain" format and "sensor gain" format. The selection is made by<br>
> > > > register 0x3503[2] and it would be nice to validate that the driver<br>
> > > > uses the format that you expect.<br>
> > ><br>
> > > I think I understand this better after reading the driver. In the register<br>
> > > value settings, I see:<br>
> > > {0x3502, 0x40}, // exposure L<br>
> > > {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain<br>
> > > {0x3505, 0x80}, // gain option<br>
> > ><br>
> > > The 0 low-nibbl indicates we're using "real gain."<br>
> > ><br>
> > > According to the datasheet:<br>
> > > 0x3503[2]=0, gain[7:0] is real gain format,<br>
> > > where low 4 bits are fraction bits, for<br>
> > > example, 0x10 is 1x gain, 0x28 is 2.5x gain<br>
> > ><br>
> > > This tells me that we need to divide reported gain from the driver by 16 to<br>
> > > get a properly-scaled double-value gain, which these gain constants do.<br>
> > <br>
> > Here you go!<br>
> > <br>
> > Looking at the driver you're using I indeed see 3503[2]=0 in all modes<br>
> > <br>
> > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera<br>
> > > > requires it to be expressed as a number of image lines. Camera sensor drivers<br>
> > > > that do not comply with this requirement will need to be adapted or will produce<br>
> > > > incorrect results.<br>
> > ><br>
> > > Looks like the driver multiples inputs x16 to convert to 1/16 line length<br>
> > > before writing to the register...<br>
> > ><br>
> > > case V4L2_CID_EXPOSURE:<br>
> > > /* 4 least significant bits of expsoure are fractional part */<br>
> > > ret = ov8858_write_reg(ov8858->client,<br>
> > > OV8858_REG_EXPOSURE,<br>
> > > OV8858_REG_VALUE_24BIT,<br>
> > > ctrl->val << 4);<br>
> > <br>
> > Great, it means that towards userspace the exposure is indeed in units<br>
> > of 1 line.<br>
> > <br>
> > > Let me know if this looks good, and what needs to happen to merge this!<br>
> > <br>
> > I think I had a comment on the pixel cell size.<br>
> > <br>
> > Apart from that, let me check with Laurent about the policy when it<br>
> > comes to supporting sensor without an upstream driver.<br>
> > <br>
> > I would not be super happy of adding support to "m00_f_ov8858" as if the<br>
> > driver will be upstreamed the entity name should at least change.<br>
> <br>
> Yes, this is problematic.<br>
> <br>
> Looking at the driver, it comes from: <br>
> <a href="https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c#L2936" rel="noreferrer" target="_blank">https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c#L2936</a><br>
> <br>
> This will likely have to be changed to get this merged upstream in the<br>
> kernel.<br>
> <br>
> > As a middle term solution we can add support for "m00_f_ov8858" with a<br>
> > todo note to move to a more canonical "ov8858" name once the driver is<br>
> > upstreamed, or at least submitted upstream.<br>
> <br>
> This is problematic too - there would be overlap - would we have to<br>
> duplicate the camera sensor properties? Or perhaps support registering a<br>
> camera sensor helper as mutliple names?<br>
> <br>
> <br>
> This is the 'why' upstreaming the driver is 'almost' a requirement.<br>
> Parts like that should be solved before integrating in libcamera<br>
> ideally. Perhaps we can workaround it this time as it's hopefully<br>
> manageable - but it means that we would then have libcamera 'supporting'<br>
> the pinephone in one version, but then losing it's support when we<br>
> suddenly change the names to match the upstream kernel until the<br>
> pinephone developers update their kernel.<br>
<br>
Agreed. The driver should be upstreamed, and while we don't require<br>
drivers to be merged upstream before support for them can be added to<br>
libcamera, we do require the drivers to be on their way to upstream.<br>
This means showing a reasonable effort to get it there, most likely with<br>
a patch posted to the linux-media mailing list.<br>
<br>
Give that the entity name will need to change when upstreaming, one way<br>
forward would be to patch the pinephone kernel driver with the correct<br>
entity name already, and use it in libcamera right away.<br>
<br>
> All a bit messy ... :S<br>
><br>
> > > On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth wrote:<br>
> > ><br>
> > > > > please pardon me for being pedantic, but I would:<br>
> > > > > libcamera: Add support for ov8858 sensor<br>
> > > > ><br>
> > > > > instead of:<br>
> > > > > ipa: libcamera: add metadata for the ov8858 sensor<br>
> > > ><br>
> > > > Happy to make the change.<br>
> > > ><br>
> > > > > The sensor doesn't seem to be supported mainline :(...<br>
> > > > > Let's start from the first point: where does this driver lives ? What<br>
> > > > > kernel are you using ?<br>
> > > > I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).<br>
> > > ><br>
> > > > It looks like my package manager is picking up the Manjaro kernel from<br>
> > > > <a href="https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config" rel="noreferrer" target="_blank">https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config</a>,<br>
> > > > which sets "CONFIG_VIDEO_OV8858=m" in /config otherwise has no<br>
> > > > 8858-related changes. This ultimately pulls the driver from<br>
> > > > <a href="https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c" rel="noreferrer" target="_blank">https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c</a><br>
> > > > .<br>
> > > ><br>
> > > > Looks like this has been in and out of Torvalds' peripheral view, e.g.:<br>
> > > > <a href="https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html" rel="noreferrer" target="_blank">https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html</a><br>
> > > ><br>
> > > > If you think that it would be valuable to try and mainline this, I'd be<br>
> > > > interested to do so once I can get libcamera working on my hardware, though<br>
> > > > I'll likely need some guidance in the process, since I'll be learning a lot.<br>
> > > ><br>
> > > > > Nice this matches the CCS defined linear gain model.<br>
> > > > > However the sensor allows to select two formats for the analogue gain<br>
> > > > > the "real gain" format and "sensor gain" format. The selection is made by<br>
> > > > > register 0x3503[2] and it would be nice to validate that the driver<br>
> > > > > uses the format that you expect.<br>
> > > > <br>
> > > > Frankly, I'm not sure what you mean by this, but I'll try to find out from<br>
> > > > the driver above and verify.<br>
> > > ><br>
> > > > > Knowing what driver you're using is relevant, in example, as the<br>
> > > > > OV5688 sensor computes exposure in 1/16 of line length. This is not<br>
> > > > > what libcamera expects, and sensor drivers are expected to express the<br>
> > > > > V4L2_CID_EXPOSURE control in line units.<br>
> > > ><br>
> > > > > From Documentation/sensor_driver_requirements.rst<br>
> > > ><br>
> > > > > While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera<br>
> > > > > requires it to be expressed as a number of image lines. Camera sensor drivers<br>
> > > > > that do not comply with this requirement will need to be adapted or will produce<br>
> > > > > incorrect results.<br>
> > > ><br>
> > > > Let me read the driver and get back to you.<br>
> > > ><br>
> > > ><br>
> > > > On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
> > > ><br>
> > > >> Hi Nicholas,<br>
> > > >><br>
> > > >> please pardon me for being pedantic, but I would:<br>
> > > >> libcamera: Add support for ov8858 sensor<br>
> > > >><br>
> > > >> instead of:<br>
> > > >> ipa: libcamera: add metadata for the ov8858 sensor<br>
> > > >><br>
> > > >> "metadata" is an overloaded already term and usually refers to the<br>
> > > >> properties associated to a captured frame (the term comes from the<br>
> > > >> Android lingo, but we use it too).<br>
> > > >><br>
> > > >> On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via<br>
> > > >> libcamera-devel wrote:<br>
> > > >> > From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
> > > >> ><br>
> > > >> > Currently, libcamera does not have information for the ov8858 sensor<br>
> > > >> > used in the PinePhone Pro, a phone designed to run Linux.<br>
> > > >> ><br>
> > > >> > This commit adds metadata, especially that sensor gain is reported and<br>
> > > >> > set in 1/16 discrete increments.<br>
> > > >> ><br>
> > > >> > For more information, see "5.8 manual exposure compensation/ manual<br>
> > > >> > gain compensation" in [0].<br>
> > > >> ><br>
> > > >> > [0] <a href="http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf" rel="noreferrer" target="_blank">http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf</a><br>
> > > >><br>
> > > >> The sensor doesn't seem to be supported mainline :(<br>
> > > >> libcamera has a policy that requires supported components to be<br>
> > > >> upstream or at least on their way to upstream (ie posted to the<br>
> > > >> linux-media mailing list). The policy is there because different<br>
> > > >> downstream driver implementations might behave differently one from<br>
> > > >> the other, making it impossible for libcamera to support the part<br>
> > > >> fully. The policy is a strict requirement for ISPs, I guess we're a<br>
> > > >> bit more elastic when it comes to sensor. Howver knowing what driver<br>
> > > >> you are using, where it lives, and if there's any plan to upstream it<br>
> > > >> would be great.<br>
> > > >><br>
> > > >> Let's start from the first point: where does this driver lives ? What<br>
> > > >> kernel are you using ?<br>
> > > >><br>
> > > >> Knowing what driver you're using is relevant, in example, as the<br>
> > > >> OV5688 sensor computes exposure in 1/16 of line length. This is not<br>
> > > >> what libcamera expects, and sensor drivers are expected to express the<br>
> > > >> V4L2_CID_EXPOSURE control in line units.<br>
> > > >><br>
> > > >> From Documentation/sensor_driver_requirements.rst<br>
> > > >><br>
> > > >> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera<br>
> > > >> requires it to be expressed as a number of image lines. Camera sensor drivers<br>
> > > >> that do not comply with this requirement will need to be adapted or will produce<br>
> > > >> incorrect results.<br>
> > > >><br>
> > > >> > Signed-off-by: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
> > > >> > ---<br>
> > > >> > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++<br>
> > > >> > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++<br>
> > > >> > 2 files changed, 25 insertions(+)<br>
> > > >> ><br>
> > > >> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp<br>
> > > >> > index 35056bec..f2040cbd 100644<br>
> > > >> > --- a/src/ipa/libipa/camera_sensor_helper.cpp<br>
> > > >> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp<br>
> > > >> > @@ -476,6 +476,17 @@ public:<br>
> > > >> > };<br>
> > > >> > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)<br>
> > > >> ><br>
> > > >> > +class CameraSensorHelperOv8858 : public CameraSensorHelper<br>
> > > >> > +{<br>
> > > >> > +public:<br>
> > > >> > + CameraSensorHelperOv8858()<br>
> > > >> > + {<br>
> > > >> > + gainType_ = AnalogueGainLinear;<br>
> > > >> > + gainConstants_.linear = { 1, 0, 0, 16 };<br>
> > > >> > + }<br>
> > > >> > +};<br>
> > > >><br>
> > > >> Nice this matches the CCS defined linear gain model.<br>
> > > >> However the sensor allows to select two formats for the analogue gain<br>
> > > >> the "real gain" format and "sensor gain" format. The selection is made by<br>
> > > >> register 0x3503[2] and it would be nice to validate that the driver<br>
> > > >> uses the format that you expect.<br>
> > > >><br>
> > > >> > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)<br>
> > > >><br>
> > > >> Ah! that's a weird name for the sensor entity :)<br>
> > > >><br>
> > > >> > +<br>
> > > >> > class CameraSensorHelperOv8865 : public CameraSensorHelper<br>
> > > >> > {<br>
> > > >> > public:<br>
> > > >> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp<br>
> > > >> > index e5f27f06..d0757c15 100644<br>
> > > >> > --- a/src/libcamera/camera_sensor_properties.cpp<br>
> > > >> > +++ b/src/libcamera/camera_sensor_properties.cpp<br>
> > > >> > @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen<br>
> > > >> > */<br>
> > > >> > },<br>
> > > >> > } },<br>
> > > >> > + { "m00_f_ov8858", {<br>
> > > >> > + .unitCellSize = { 1200, 1200 },<br>
> > > >><br>
> > > >> I read 1.12 um x 1.12 um<br>
> > > >><br>
> > > >> > + .testPatternModes = {<br>
> > > >> > + { controls::draft::TestPatternModeOff, 0 },<br>
> > > >> > + { controls::draft::TestPatternModeColorBars, 1 },<br>
> > > >> > + /*<br>
> > > >> > + * No best corresponding test pattern for:<br>
> > > >> > + * 1: "Vertical Color Bar Type 1",<br>
> > > >> > + * 2: "Vertical Color Bar Type 2",<br>
> > > >> > + * 3: "Vertical Color Bar Type 3",<br>
> > > >> > + * 4: "Vertical Color Bar Type 4"<br>
> > > >> > + */<br>
> > > >> > + },<br>
> > > >> > + } },<br>
> > > >> > { "ov8865", {<br>
> > > >> > .unitCellSize = { 1400, 1400 },<br>
> > > >> > .testPatternModes = {<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>