| Message ID | 20221030230500.74842-6-nicholas@rothemail.net |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicholas, Quoting Nicholas Roth via libcamera-devel (2022-10-30 23:05:00) > Currently, libcamera does not have information for the ov8858 sensor > used in the PinePhone Pro, a phone designed to run Linux. > > This commit adds metadata, especially that sensor gain is reported and > set in 1/16 discrete increments. > > For more information, see "5.8 manual exposure compensation/ manual > gain compensation" in [0] and the driver in [1]. > > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf > [1] https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++ > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 35056bec..f2040cbd 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -476,6 +476,17 @@ public: > }; > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > +class CameraSensorHelperOv8858 : public CameraSensorHelper > +{ > +public: > + CameraSensorHelperOv8858() > + { > + gainType_ = AnalogueGainLinear; > + gainConstants_.linear = { 1, 0, 0, 16 }; > + } > +}; > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) "My OV8858 is the second on the rear." Suddenly this doesn't work. - So we can only use "ov8858" here. We 'could' add a dupliate entry as +/* + * \todo: Deprecated: The PinePhonePro has devices listed as m00_f_ov8858. + * This entry should be removed as soon as the ov8858 driver is + * accepted into a mainline kernel. + */ +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858) But the problem there, is that suddenly users will find it 'works' until the driver gets into mainline, when it breaks. Given that the correct approach at that point will be to update to use the mainline driver, perhaps that's ok ... but I fear potentially unhappy users. And perhaps also worryingly, it removes the impetus to get the driver mainlined! So accepting 'm00_f_ov8858' at all ... seems to be really tricky. With just ov8858, and the 1.12 fix below this patch could probably go in quite rapidly though. You could easily have the +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) line locally while developing, but I'd expect that your device should be quickly moving along with the mainline integration, so you probably wouldn't need it. > + > class CameraSensorHelperOv8865 : public CameraSensorHelper > { > public: > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp > index e5f27f06..d0757c15 100644 > --- a/src/libcamera/camera_sensor_properties.cpp > +++ b/src/libcamera/camera_sensor_properties.cpp > @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen > */ > }, > } }, > + { "m00_f_ov8858", { And it won't be as short to support here here, I guess the whole table could be duplicated, but actually - missing camera-sensor properties don't cause the camera to fail to load - so I think this should simply be kept as ov8858 only. > + .unitCellSize = { 1200, 1200 }, http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf states this should be 1120, 1120. >> 1.12 μm x 1.12 μm pixel with OmniBSI-3™ technology > + .testPatternModes = { > + { controls::draft::TestPatternModeOff, 0 }, > + { controls::draft::TestPatternModeColorBars, 1 }, > + /* > + * No best corresponding test pattern for: > + * 1: "Vertical Color Bar Type 1", > + * 2: "Vertical Color Bar Type 2", > + * 3: "Vertical Color Bar Type 3", > + * 4: "Vertical Color Bar Type 4" > + */ > + }, > + } }, > { "ov8865", { > .unitCellSize = { 1400, 1400 }, > .testPatternModes = { > -- > 2.34.1 >
> So accepting 'm00_f_ov8858' at all ... seems to be really tricky. What I was trying to say in my earlier email is that the only requirement for the v4l2 "name" field be unique, although there are conventions. See comments above the regex logic in libcamera/src/libcamera/v4l2_subdevice.cpp. The current driver isn't doing anything that breaks the rules here as far as I can tell. A quick fix might be to modify the regex. A better fix might be to duplicate the logic in the Android HAL, which correctly reports the sensor as an "ov8858". Modifying the driver is an option as well, but for reasons pointed out above that will be problematic due to the downstream convention. -Nicholas On Mon, Oct 31, 2022 at 8:58 AM Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Nicholas, > > Quoting Nicholas Roth via libcamera-devel (2022-10-30 23:05:00) > > Currently, libcamera does not have information for the ov8858 sensor > > used in the PinePhone Pro, a phone designed to run Linux. > > > > This commit adds metadata, especially that sensor gain is reported and > > set in 1/16 discrete increments. > > > > For more information, see "5.8 manual exposure compensation/ manual > > gain compensation" in [0] and the driver in [1]. > > > > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf > > [1] > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++ > > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp > b/src/ipa/libipa/camera_sensor_helper.cpp > > index 35056bec..f2040cbd 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -476,6 +476,17 @@ public: > > }; > > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > > > +class CameraSensorHelperOv8858 : public CameraSensorHelper > > +{ > > +public: > > + CameraSensorHelperOv8858() > > + { > > + gainType_ = AnalogueGainLinear; > > + gainConstants_.linear = { 1, 0, 0, 16 }; > > + } > > +}; > > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) > > "My OV8858 is the second on the rear." Suddenly this doesn't work. > - So we can only use "ov8858" here. > > We 'could' add a dupliate entry as > > +/* > + * \todo: Deprecated: The PinePhonePro has devices listed as m00_f_ov8858. > + * This entry should be removed as soon as the ov8858 driver is > + * accepted into a mainline kernel. > + */ > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) > +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858) > > But the problem there, is that suddenly users will find it 'works' until > the driver gets into mainline, when it breaks. Given that the correct > approach at that point will be to update to use the mainline driver, > perhaps that's ok ... but I fear potentially unhappy users. > > And perhaps also worryingly, it removes the impetus to get the driver > mainlined! > > So accepting 'm00_f_ov8858' at all ... seems to be really tricky. > > With just ov8858, and the 1.12 fix below this patch could probably go in > quite rapidly though. You could easily have the > > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) > > line locally while developing, but I'd expect that your device should be > quickly moving along with the mainline integration, so you probably > wouldn't need it. > > > > > + > > class CameraSensorHelperOv8865 : public CameraSensorHelper > > { > > public: > > diff --git a/src/libcamera/camera_sensor_properties.cpp > b/src/libcamera/camera_sensor_properties.cpp > > index e5f27f06..d0757c15 100644 > > --- a/src/libcamera/camera_sensor_properties.cpp > > +++ b/src/libcamera/camera_sensor_properties.cpp > > @@ -146,6 +146,20 @@ const CameraSensorProperties > *CameraSensorProperties::get(const std::string &sen > > */ > > }, > > } }, > > + { "m00_f_ov8858", { > > And it won't be as short to support here here, I guess the whole table > could be duplicated, but actually - missing camera-sensor properties > don't cause the camera to fail to load - so I think this should simply > be kept as ov8858 only. > > > > + .unitCellSize = { 1200, 1200 }, > > http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf states > this should be 1120, 1120. > > >> 1.12 μm x 1.12 μm pixel with OmniBSI-3™ technology > > > + .testPatternModes = { > > + { controls::draft::TestPatternModeOff, 0 > }, > > + { > controls::draft::TestPatternModeColorBars, 1 }, > > + /* > > + * No best corresponding test pattern > for: > > + * 1: "Vertical Color Bar Type 1", > > + * 2: "Vertical Color Bar Type 2", > > + * 3: "Vertical Color Bar Type 3", > > + * 4: "Vertical Color Bar Type 4" > > + */ > > + }, > > + } }, > > { "ov8865", { > > .unitCellSize = { 1400, 1400 }, > > .testPatternModes = { > > -- > > 2.34.1 > > >
Hi Nicholas, Cc Sakari (Sensor drivers maintainer for the Kernel). Quoting Nicholas Roth (2022-10-31 16:22:40) > > So accepting 'm00_f_ov8858' at all ... seems to be really tricky. > > What I was trying to say in my earlier email is that the only requirement > for the v4l2 "name" field be unique, although there are conventions. See > comments above the regex logic > in libcamera/src/libcamera/v4l2_subdevice.cpp. * - The most common rule, used by I2C sensors, associates the model * name with the I2C bus number and address (e.g. 'imx219 0-0010'). The OV8858 is an I2C controlled sensor though right ? So we should follow the same convention. > The current driver isn't doing anything that breaks the rules here as far > as I can tell. > > A quick fix might be to modify the regex. A better fix might be to > duplicate the logic in the Android HAL, which correctly reports the sensor > as an "ov8858". Modifying the driver is an option as well, but for reasons > pointed out above that will be problematic due to the downstream convention. I understand, however there may be many downstream conventions, and only one upstream convention (for I2C based sensors). Your development will help downstream. When your driver is accepted upstream, it will end up in their kernel. Either by them chosing to move to it immediately, or when they rebase to a kernel where yours is merged. If they have pain from things changing, that is only due to the fact that they did not post it themselves at the earliest opportunity. Making libcamera compatible with lots of defined and undefined downstream conventions, vs - the conventions used in mainline, provides clear differences in maintenance and support. For instance, How should we generically handle another platform that uses a convention of "ov5640-front-0", and another one that has "FL IMX602"? You are (rightly, it's the device you have) looking at this from the point of view of getting libcamera to work on a single device, while we look at it from a point of view of having this camera work on /any/ device. Imagine that once you have your driver merged in the kernel, you will connect an OV8858 to a Raspberry Pi, and then use libcamera with it. If you are able to convince Sakari and Hans (who will maintain and handle the OV8858 driver in the kernel), that the name should be kept as it is, then we would have to consider how we'll track the naming in libcamera. But ... I will be surprised. -- Kieran > -Nicholas > > > On Mon, Oct 31, 2022 at 8:58 AM Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Hi Nicholas, > > > > Quoting Nicholas Roth via libcamera-devel (2022-10-30 23:05:00) > > > Currently, libcamera does not have information for the ov8858 sensor > > > used in the PinePhone Pro, a phone designed to run Linux. > > > > > > This commit adds metadata, especially that sensor gain is reported and > > > set in 1/16 discrete increments. > > > > > > For more information, see "5.8 manual exposure compensation/ manual > > > gain compensation" in [0] and the driver in [1]. > > > > > > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf > > > [1] > > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c > > > > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++ > > > src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp > > b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 35056bec..f2040cbd 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -476,6 +476,17 @@ public: > > > }; > > > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > > > > > +class CameraSensorHelperOv8858 : public CameraSensorHelper > > > +{ > > > +public: > > > + CameraSensorHelperOv8858() > > > + { > > > + gainType_ = AnalogueGainLinear; > > > + gainConstants_.linear = { 1, 0, 0, 16 }; > > > + } > > > +}; > > > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) > > > > "My OV8858 is the second on the rear." Suddenly this doesn't work. > > - So we can only use "ov8858" here. > > > > We 'could' add a dupliate entry as > > > > +/* > > + * \todo: Deprecated: The PinePhonePro has devices listed as m00_f_ov8858. > > + * This entry should be removed as soon as the ov8858 driver is > > + * accepted into a mainline kernel. > > + */ > > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) > > +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858) > > > > But the problem there, is that suddenly users will find it 'works' until > > the driver gets into mainline, when it breaks. Given that the correct > > approach at that point will be to update to use the mainline driver, > > perhaps that's ok ... but I fear potentially unhappy users. > > > > And perhaps also worryingly, it removes the impetus to get the driver > > mainlined! > > > > So accepting 'm00_f_ov8858' at all ... seems to be really tricky. > > > > With just ov8858, and the 1.12 fix below this patch could probably go in > > quite rapidly though. You could easily have the > > > > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) > > > > line locally while developing, but I'd expect that your device should be > > quickly moving along with the mainline integration, so you probably > > wouldn't need it. > > > > > > > > > + > > > class CameraSensorHelperOv8865 : public CameraSensorHelper > > > { > > > public: > > > diff --git a/src/libcamera/camera_sensor_properties.cpp > > b/src/libcamera/camera_sensor_properties.cpp > > > index e5f27f06..d0757c15 100644 > > > --- a/src/libcamera/camera_sensor_properties.cpp > > > +++ b/src/libcamera/camera_sensor_properties.cpp > > > @@ -146,6 +146,20 @@ const CameraSensorProperties > > *CameraSensorProperties::get(const std::string &sen > > > */ > > > }, > > > } }, > > > + { "m00_f_ov8858", { > > > > And it won't be as short to support here here, I guess the whole table > > could be duplicated, but actually - missing camera-sensor properties > > don't cause the camera to fail to load - so I think this should simply > > be kept as ov8858 only. > > > > > > > + .unitCellSize = { 1200, 1200 }, > > > > http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf states > > this should be 1120, 1120. > > > > >> 1.12 μm x 1.12 μm pixel with OmniBSI-3™ technology > > > > > + .testPatternModes = { > > > + { controls::draft::TestPatternModeOff, 0 > > }, > > > + { > > controls::draft::TestPatternModeColorBars, 1 }, > > > + /* > > > + * No best corresponding test pattern > > for: > > > + * 1: "Vertical Color Bar Type 1", > > > + * 2: "Vertical Color Bar Type 2", > > > + * 3: "Vertical Color Bar Type 3", > > > + * 4: "Vertical Color Bar Type 4" > > > + */ > > > + }, > > > + } }, > > > { "ov8865", { > > > .unitCellSize = { 1400, 1400 }, > > > .testPatternModes = { > > > -- > > > 2.34.1 > > > > >
Hi Nicholas, Kieran, On Mon, Oct 31, 2022 at 05:07:56PM +0000, Kieran Bingham wrote: > If you are able to convince Sakari and Hans (who will maintain and > handle the OV8858 driver in the kernel), that the name should be kept as > it is, then we would have to consider how we'll track the naming in > libcamera. But ... I will be surprised. For now you should just let v4l2_i2c_subdev_set_name() set the sub-device name field unless you really need something special (see e.g. the CCS driver). We should also pass information on the module to user space but how to do that has not been defined yet.
The BSP maintainer agreed to use the standard i2c naming convention
("ov8858 foo:bar" instead of "m00_f_ov8858 foo") in the upcoming 6.1
Manjaro-ARM PinePhone Pro kernel release. That might take some time,
but it's encouraging to see.
I'll go ahead and change the device name here to "ov8858" for my next
round of patches.
On Tue, Nov 1, 2022 at 7:08 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Nicholas, Kieran,
>
> On Mon, Oct 31, 2022 at 05:07:56PM +0000, Kieran Bingham wrote:
> > If you are able to convince Sakari and Hans (who will maintain and
> > handle the OV8858 driver in the kernel), that the name should be kept as
> > it is, then we would have to consider how we'll track the naming in
> > libcamera. But ... I will be surprised.
>
> For now you should just let v4l2_i2c_subdev_set_name() set the sub-device
> name field unless you really need something special (see e.g. the CCS
> driver). We should also pass information on the module to user space but
> how to do that has not been defined yet.
>
> --
> Kind regards,
>
> Sakari Ailus
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index 35056bec..f2040cbd 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -476,6 +476,17 @@ public: }; REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) +class CameraSensorHelperOv8858 : public CameraSensorHelper +{ +public: + CameraSensorHelperOv8858() + { + gainType_ = AnalogueGainLinear; + gainConstants_.linear = { 1, 0, 0, 16 }; + } +}; +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858) + class CameraSensorHelperOv8865 : public CameraSensorHelper { public: diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp index e5f27f06..d0757c15 100644 --- a/src/libcamera/camera_sensor_properties.cpp +++ b/src/libcamera/camera_sensor_properties.cpp @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen */ }, } }, + { "m00_f_ov8858", { + .unitCellSize = { 1200, 1200 }, + .testPatternModes = { + { controls::draft::TestPatternModeOff, 0 }, + { controls::draft::TestPatternModeColorBars, 1 }, + /* + * No best corresponding test pattern for: + * 1: "Vertical Color Bar Type 1", + * 2: "Vertical Color Bar Type 2", + * 3: "Vertical Color Bar Type 3", + * 4: "Vertical Color Bar Type 4" + */ + }, + } }, { "ov8865", { .unitCellSize = { 1400, 1400 }, .testPatternModes = {
Currently, libcamera does not have information for the ov8858 sensor used in the PinePhone Pro, a phone designed to run Linux. This commit adds metadata, especially that sensor gain is reported and set in 1/16 discrete increments. For more information, see "5.8 manual exposure compensation/ manual gain compensation" in [0] and the driver in [1]. [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf [1] https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c Signed-off-by: Nicholas Roth <nicholas@rothemail.net> --- src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++ src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++ 2 files changed, 25 insertions(+)