<div dir="ltr">Thanks, Dave.<br><div><br></div><div>I'll prepare another version that removes the offending "m"!</div><div><br></div><div>David</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 28 Jun 2021 at 10:16, Dave Stevenson <<a href="mailto:dave.stevenson@raspberrypi.com">dave.stevenson@raspberrypi.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, 28 Jun 2021 at 08:02, Laurent Pinchart<br>
<<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br>
><br>
> Hi David,<br>
><br>
> On Mon, Jun 28, 2021 at 07:47:39AM +0100, David Plowman wrote:<br>
> > On Mon, 28 Jun 2021 at 07:17, Laurent Pinchart wrote:<br>
> > > On Fri, Jun 18, 2021 at 11:04:30PM +0100, David Plowman wrote:<br>
> > > > The necessary tuning file and CamHelper is added for the ov9281 sensor<br>
> > > > (which the driver names as the "mov9281").<br>
> > ><br>
> > > This bothers me a little bit, as it will likely need to be renamed to<br>
> > > ov9281 when upstreaming. Why is there an "m" prefix, and could it be<br>
> > > dropped ?<br>
> ><br>
> > Yes, I'm suspicious that the "m" is actually a reference to "mono",<br>
> > thereby opening the way for colour versions of this sensor. In fact it<br>
> > would be helpful for the driver to advertise a different name in such<br>
> > cases so that a more appropriate tuning file can be identified (though<br>
> > as we know, that still isn't really good enough...!) But maybe<br>
> > something a little less cryptic than just "m" would help?<br>
><br>
> Is there a colour version of the OV9281 ? If not, I'd just drop the<br>
> prefix. If there is, the media entity should be named according to the<br>
> Omnivision naming scheme.<br>
<br>
No colour version that I'm aware of.<br>
<br>
The m was present in the Rockchip original driver that I based this on<br>
- <a href="https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c#L1103" rel="noreferrer" target="_blank">https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c#L1103</a><br>
No issues at all in removing it.<br>
<br>
Dave<br>
<br>
> > > > The ov9281 is a 1280x800 monochrome global shutter sensor. To enable<br>
> > > > it, please add<br>
> > > ><br>
> > > > dtoverlay=ov9281<br>
> > > ><br>
> > > > to the /boot/config.txt file and reboot the Pi.<br>
> > > ><br>
> > > > Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > > Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > > ---<br>
> > > > src/ipa/raspberrypi/cam_helper_mov9281.cpp | 65 +++++++++++++++<br>
> > > > src/ipa/raspberrypi/data/meson.build | 1 +<br>
> > > > src/ipa/raspberrypi/data/mov9281.json | 92 ++++++++++++++++++++++<br>
> > > > src/ipa/raspberrypi/meson.build | 1 +<br>
> > > > 4 files changed, 159 insertions(+)<br>
> > > > create mode 100644 src/ipa/raspberrypi/cam_helper_mov9281.cpp<br>
> > > > create mode 100644 src/ipa/raspberrypi/data/mov9281.json<br>
> > > ><br>
> > > > diff --git a/src/ipa/raspberrypi/cam_helper_mov9281.cpp b/src/ipa/raspberrypi/cam_helper_mov9281.cpp<br>
> > > > new file mode 100644<br>
> > > > index 00000000..29519cb0<br>
> > > > --- /dev/null<br>
> > > > +++ b/src/ipa/raspberrypi/cam_helper_mov9281.cpp<br>
> > > > @@ -0,0 +1,65 @@<br>
> > > > +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> > > > +/*<br>
> > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited<br>
> > > > + *<br>
> > > > + * cam_helper_mov9281.cpp - camera information for ov9281 sensor<br>
> > > > + */<br>
> > > > +<br>
> > > > +#include <assert.h><br>
> > > > +<br>
> > > > +#include "cam_helper.hpp"<br>
> > > > +<br>
> > > > +using namespace RPiController;<br>
> > > > +<br>
> > > > +class CamHelperOv9281 : public CamHelper<br>
> > > > +{<br>
> > > > +public:<br>
> > > > + CamHelperOv9281();<br>
> > > > + uint32_t GainCode(double gain) const override;<br>
> > > > + double Gain(uint32_t gain_code) const override;<br>
> > > > + void GetDelays(int &exposure_delay, int &gain_delay,<br>
> > > > + int &vblank_delay) const override;<br>
> > > > +<br>
> > > > +private:<br>
> > > > + /*<br>
> > > > + * Smallest difference between the frame length and integration time,<br>
> > > > + * in units of lines.<br>
> > > > + */<br>
> > > > + static constexpr int frameIntegrationDiff = 4;<br>
> > > > +};<br>
> > > > +<br>
> > > > +/*<br>
> > > > + * OV9281 doesn't output metadata, so we have to use the "unicam parser" which<br>
> > > > + * works by counting frames.<br>
> > > > + */<br>
> > > > +<br>
> > > > +CamHelperOv9281::CamHelperOv9281()<br>
> > > > + : CamHelper(nullptr, frameIntegrationDiff)<br>
> > > > +{<br>
> > > > +}<br>
> > > > +<br>
> > > > +uint32_t CamHelperOv9281::GainCode(double gain) const<br>
> > > > +{<br>
> > > > + return static_cast<uint32_t>(gain * 16.0);<br>
> > > > +}<br>
> > > > +<br>
> > > > +double CamHelperOv9281::Gain(uint32_t gain_code) const<br>
> > > > +{<br>
> > > > + return static_cast<double>(gain_code) / 16.0;<br>
> > > > +}<br>
> > > > +<br>
> > > > +void CamHelperOv9281::GetDelays(int &exposure_delay, int &gain_delay,<br>
> > > > + int &vblank_delay) const<br>
> > > > +{<br>
> > > > + /* The driver appears to behave as follows: */<br>
> > > > + exposure_delay = 2;<br>
> > > > + gain_delay = 2;<br>
> > > > + vblank_delay = 2;<br>
> > > > +}<br>
> > > > +<br>
> > > > +static CamHelper *Create()<br>
> > > > +{<br>
> > > > + return new CamHelperOv9281();<br>
> > > > +}<br>
> > > > +<br>
> > > > +static RegisterCamHelper reg("mov9281", &Create);<br>
> > > > diff --git a/src/ipa/raspberrypi/data/meson.build b/src/ipa/raspberrypi/data/meson.build<br>
> > > > index 92ad3272..f8baab6d 100644<br>
> > > > --- a/src/ipa/raspberrypi/data/meson.build<br>
> > > > +++ b/src/ipa/raspberrypi/data/meson.build<br>
> > > > @@ -4,6 +4,7 @@ conf_files = files([<br>
> > > > 'imx219.json',<br>
> > > > 'imx290.json',<br>
> > > > 'imx477.json',<br>
> > > > + 'mov9281.json',<br>
> > > > 'ov5647.json',<br>
> > > > 'se327m12.json',<br>
> > > > 'uncalibrated.json',<br>
> > > > diff --git a/src/ipa/raspberrypi/data/mov9281.json b/src/ipa/raspberrypi/data/mov9281.json<br>
> > > > new file mode 100644<br>
> > > > index 00000000..ecd262be<br>
> > > > --- /dev/null<br>
> > > > +++ b/src/ipa/raspberrypi/data/mov9281.json<br>
> > > > @@ -0,0 +1,92 @@<br>
> > > > +{<br>
> > > > + "rpi.black_level":<br>
> > > > + {<br>
> > > > + "black_level": 4096<br>
> > > > + },<br>
> > > > + "rpi.lux":<br>
> > > > + {<br>
> > > > + "reference_shutter_speed": 2000,<br>
> > > > + "reference_gain": 1.0,<br>
> > > > + "reference_aperture": 1.0,<br>
> > > > + "reference_lux": 800,<br>
> > > > + "reference_Y": 20000<br>
> > > > + },<br>
> > > > + "rpi.noise":<br>
> > > > + {<br>
> > > > + "reference_constant": 0,<br>
> > > > + "reference_slope": 2.5<br>
> > > > + },<br>
> > > > + "rpi.sdn":<br>
> > > > + {<br>
> > > > + },<br>
> > > > + "rpi.agc":<br>
> > > > + {<br>
> > > > + "metering_modes":<br>
> > > > + {<br>
> > > > + "centre-weighted": {<br>
> > > > + "weights": [4, 4, 4, 2, 2, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0]<br>
> > > > + }<br>
> > > > + },<br>
> > > > + "exposure_modes":<br>
> > > > + {<br>
> > > > + "normal":<br>
> > > > + {<br>
> > > > + "shutter": [ 100, 15000, 30000, 60000, 120000 ],<br>
> > > > + "gain": [ 1.0, 2.0, 3.0, 4.0, 6.0 ]<br>
> > > > + }<br>
> > > > + },<br>
> > > > + "constraint_modes":<br>
> > > > + {<br>
> > > > + "normal":<br>
> > > > + [<br>
> > > > + { "bound": "LOWER", "q_lo": 0.98, "q_hi": 1.0, "y_target": [ 0, 0.4, 1000, 0.4 ] }<br>
> > > > + ]<br>
> > > > + },<br>
> > > > + "y_target": [ 0, 0.16, 1000, 0.165, 10000, 0.17 ]<br>
> > > > + },<br>
> > > > + "rpi.alsc":<br>
> > > > + {<br>
> > > > + "n_iter": 0,<br>
> > > > + "luminance_strength": 1.0,<br>
> > > > + "corner_strength": 1.5<br>
> > > > + },<br>
> > > > + "rpi.contrast":<br>
> > > > + {<br>
> > > > + "ce_enable": 0,<br>
> > > > + "gamma_curve": [<br>
> > > > + 0, 0,<br>
> > > > + 1024, 5040,<br>
> > > > + 2048, 9338,<br>
> > > > + 3072, 12356,<br>
> > > > + 4096, 15312,<br>
> > > > + 5120, 18051,<br>
> > > > + 6144, 20790,<br>
> > > > + 7168, 23193,<br>
> > > > + 8192, 25744,<br>
> > > > + 9216, 27942,<br>
> > > > + 10240, 30035,<br>
> > > > + 11264, 32005,<br>
> > > > + 12288, 33975,<br>
> > > > + 13312, 35815,<br>
> > > > + 14336, 37600,<br>
> > > > + 15360, 39168,<br>
> > > > + 16384, 40642,<br>
> > > > + 18432, 43379,<br>
> > > > + 20480, 45749,<br>
> > > > + 22528, 47753,<br>
> > > > + 24576, 49621,<br>
> > > > + 26624, 51253,<br>
> > > > + 28672, 52698,<br>
> > > > + 30720, 53796,<br>
> > > > + 32768, 54876,<br>
> > > > + 36864, 57012,<br>
> > > > + 40960, 58656,<br>
> > > > + 45056, 59954,<br>
> > > > + 49152, 61183,<br>
> > > > + 53248, 62355,<br>
> > > > + 57344, 63419,<br>
> > > > + 61440, 64476,<br>
> > > > + 65535, 65535<br>
> > > > + ]<br>
> > > > + }<br>
> > > > +}<br>
> > > > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build<br>
> > > > index 230356d3..93f04e9e 100644<br>
> > > > --- a/src/ipa/raspberrypi/meson.build<br>
> > > > +++ b/src/ipa/raspberrypi/meson.build<br>
> > > > @@ -22,6 +22,7 @@ rpi_ipa_sources = files([<br>
> > > > 'cam_helper_imx219.cpp',<br>
> > > > 'cam_helper_imx290.cpp',<br>
> > > > 'cam_helper_imx477.cpp',<br>
> > > > + 'cam_helper_mov9281.cpp',<br>
> > > > 'controller/controller.cpp',<br>
> > > > 'controller/histogram.cpp',<br>
> > > > 'controller/algorithm.cpp',<br>
><br>
> --<br>
> Regards,<br>
><br>
> Laurent Pinchart<br>
</blockquote></div>