| Message ID | 20220911200147.375198-1-mike.rudenko@gmail.com |
|---|---|
| Headers | show |
| Series | Add Omnivision OV4689 image sensor driver | expand |
On 11/09/2022 22:01, Mikhail Rudenko wrote: > Add device-tree binding documentation for OV4689 image sensor driver, > and the relevant MAINTAINERS entries. > > Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> Too many "media" prefixes in the subject. Also you duplicated dt bindings as prefix and commit msg (skip the latter). > --- > .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ > MAINTAINERS | 7 + > 2 files changed, 148 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > new file mode 100644 > index 000000000000..376330b5572a > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml > @@ -0,0 +1,141 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV4689 CMOS > + > +maintainers: > + - Mikhail Rudenko <mike.rudenko@gmail.com> > + > +description: | > + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel > + image sensor. Ihis chip supports high frame rate speeds up to 90 fps > + at 2688x1520 resolution. It is programmable through an I2C > + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 > + connection. > + > +allOf: > + - $ref: /schemas/media/video-interface-devices.yaml# > + > +properties: > + compatible: > + const: ovti,ov4689 > + > + reg: > + maxItems: 1 > + > + clocks: > + description: > + External clock (XVCLK) for the sensor, 6-64 MHz > + maxItems: 1 > + > + clock-names: true This has to be strictly defined - which name you expect. > + > + dovdd-supply: > + description: > + Digital I/O voltage supply, 1.7-3.0 V > + > + avdd-supply: > + description: > + Analog voltage supply, 2.6-3.0 V > + > + dvdd-supply: > + description: > + Digital core voltage supply, 1.1-1.3 V > + > + powerdown-gpios: > + maxItems: 1 You can skip here maxItems - it is defined by gpio-consumer-common. > + description: > + GPIO connected to the powerdown pin (active low) > + > + reset-gpios: > + maxItems: 1 > + description: > + GPIO connected to the reset pin (active low) > + Best regards, Krzysztof
Hi Krzysztof, On 2022-09-12 at 12:55 +02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 11/09/2022 22:01, Mikhail Rudenko wrote: >> Add device-tree binding documentation for OV4689 image sensor driver, >> and the relevant MAINTAINERS entries. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> > > Too many "media" prefixes in the subject. I see, will drop the first "media:" in v3. > Also you duplicated dt > bindings as prefix and commit msg (skip the latter). Just to be clear, do you mean dropping "device-tree binding" phrase from the commit message? >> --- >> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ >> MAINTAINERS | 7 + >> 2 files changed, 148 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> new file mode 100644 >> index 000000000000..376330b5572a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >> @@ -0,0 +1,141 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Omnivision OV4689 CMOS >> + >> +maintainers: >> + - Mikhail Rudenko <mike.rudenko@gmail.com> >> + >> +description: | >> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel >> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps >> + at 2688x1520 resolution. It is programmable through an I2C >> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >> + connection. >> + >> +allOf: >> + - $ref: /schemas/media/video-interface-devices.yaml# >> + >> +properties: >> + compatible: >> + const: ovti,ov4689 >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + description: >> + External clock (XVCLK) for the sensor, 6-64 MHz >> + maxItems: 1 >> + >> + clock-names: true > > This has to be strictly defined - which name you expect. Will fix in v3. Or maybe we should drop clock-names altogether and use devm_clk_get(&client->dev, NULL) in the driver instead (I've seen this approach in some existing drivers)? >> + >> + dovdd-supply: >> + description: >> + Digital I/O voltage supply, 1.7-3.0 V >> + >> + avdd-supply: >> + description: >> + Analog voltage supply, 2.6-3.0 V >> + >> + dvdd-supply: >> + description: >> + Digital core voltage supply, 1.1-1.3 V >> + >> + powerdown-gpios: >> + maxItems: 1 > > You can skip here maxItems - it is defined by gpio-consumer-common. Ack, will fix in v3. Does this also apply to reset-gpios? >> + description: >> + GPIO connected to the powerdown pin (active low) >> + >> + reset-gpios: >> + maxItems: 1 >> + description: >> + GPIO connected to the reset pin (active low) >> + > > Best regards, > Krzysztof Thanks for review, -- Best regards, Mikhail Rudenko
Hi Dave, On 2022-09-14 at 10:58 +01, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > Hi Mikhail > > On Sun, 11 Sept 2022 at 21:02, Mikhail Rudenko <mike.rudenko@gmail.com> wrote: >> >> Hello, >> >> this series implements support for Omnivision OV4689 image >> sensor. The Omnivision OV4689 is a high performance, 1/3-inch, 4 >> megapixel image sensor. Ihis chip supports high frame rate speeds up >> to 90 fps at 2688x1520 resolution. It is programmable through an I2C >> interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >> connection. >> >> The driver is based on Rockchip BSP kernel [1]. It implements 4-lane CSI-2 >> and single 2688x1520 @ 30 fps mode. The driver was tested on Rockchip >> 3399-based FriendlyElec NanoPi M4 board with MCAM400 camera module. >> >> While porting the driver, I stumbled upon two issues: >> >> (1) In the original driver, horizontal total size (HTS) was set to a >> value (2584) lower then the frame width (2688), resulting in negative >> hblank. In this driver, I increased HTS to 2688, but fps dropped from >> 29.88 to 28.73. What is the preferred way to handle this? > > This is one of the joys of sensors - they don't all work in the same way. > > I don't have an official datasheet for OV4689 from Omnivision, but > found one on the internet [1]. That should allow you to reverse the > PLL configuration to confirm that the pixel rate is the value you've > computed based on link frequency (they aren't necessarily related). Do > the frame rate calculations work using width + HBLANK, height + > VBLANK, and pixel rate? > The datasheet claims the sensor supports 2688x1520 @ 90 fps, so > something doesn't hold true between 4 data lanes at 500MHz/1Gbit/s per > lane when your default hts/vts is 2688x1554 and it only gives > 28.73fps. Seems like those 90 fps is about CSI throughput, not actual sensor performance. I've checked the datasheet and the register values, and it seems like the pixel clock is 126 Mhz in this configuration (the maximum is 150 MHz according to the datasheet). This corresponds to a theoretical fps of 30.16 at hts=2688 and vts=1554. At the same time the observed fps is 28.73. I'm not sure where those 1.43 frames are lost, hope to do more experimentation with VTS and HTS over the weekend. > I have seen modes in sensors where the HTS register is in units of 2 > pixels, so what range of HTS (and VTS) values actually works on this > sensor? (I don't see it documented, but I'm not surprised). > > [1] https://cdn.hackaday.io/files/19354828041536/OV4689-OmniVision.pdf > >> (2) The original driver exposes analog gain range 0x0 - 0x7ff, but the >> gain is not linear across that range. Instead, it is piecewise linear >> (and discontinuous). 0x0-0xff register values result in 0x-2x gain, >> 0x100-0x1ff to 0x-4x, 0x300-0x3ff to 0x-8x, and 0x700-0x7ff to 0x-16x, >> with more linear segments in between. Rockchip's camera engine code >> chooses one of the above segments depenging on the desired gain >> value. The question is, how should we proceed keeping in mind >> libcamera use case? Should the whole 0x0-0x7ff be exposed as-is and >> libcamera will do the mapping, or the driver will do the mapping >> itself and expose some logical gain units not tied to the actual gain >> register value? Meanwhile, this driver conservatively exposes only >> 0x0-0xf8 gain register range. > > The datasheet linked above says "for the gain formula, please contact > your local OmniVision FAE" :-( > I would assume that the range is from 1x rather than 0x - people > rarely want a totally black image that 0x would give. Or is it ranges > of 1x - 2x, 2x - 4x, 4x - 8x, and 8x - 16x? A picture is worth a thousand words, so I've attached the results of my experimentation with the gain register. They were obtained with Rockchip 3399, with AEC, AGC and black level subtraction disabled. The image was converted from 10-bit RGGB to 8-bit YUV 4:2:0 by the Rockchip ISP. > Other sensors expose the full range of the register via > V4L2_CID_ANALOGUE_GAIN, and require userspace (mainly libcamera now) > to know how to convert a gain into the register value. If the gain > range goes up to x16, then exposing that would be useful. I'd advocate > just exposing the full range of 0x000 - 0x7ff, as then you can have > the accuracy of 256 values between x1 to x2, but also the full range. I also like this approach, although libcamera's CameraSensorHelper doesn't support piecewise-linear gain code mapping yet. Nevertheless, I believe exposing the full range is a good idea and will do so in v3. > I might see if I can pick up one of these sensors and see if I can get > it running on a Raspberry Pi. Thanks for trying to upstream this - > it's nice to have such a range of sensor drivers to choose from. > > Dave > Thanks for your elucidating tips! -- Best regards, Mikhail Rudenko
On 15/09/2022 13:16, Mikhail Rudenko wrote: > > I see, will drop the first "media:" in v3. > >> Also you duplicated dt >> bindings as prefix and commit msg (skip the latter). > > Just to be clear, do you mean dropping "device-tree binding" phrase from > the commit message? Ah, sorry, I meant in the subject. You already have dt-bindings as prefix, so no "DT bindings" at the end. Just: media: dt-bindings: i2c: document OV4689 > >>> --- >>> .../bindings/media/i2c/ovti,ov4689.yaml | 141 ++++++++++++++++++ >>> MAINTAINERS | 7 + >>> 2 files changed, 148 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >>> new file mode 100644 >>> index 000000000000..376330b5572a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml >>> @@ -0,0 +1,141 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov4689.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Omnivision OV4689 CMOS >>> + >>> +maintainers: >>> + - Mikhail Rudenko <mike.rudenko@gmail.com> >>> + >>> +description: | >>> + The Omnivision OV4689 is a high performance, 1/3-inch, 4 megapixel >>> + image sensor. Ihis chip supports high frame rate speeds up to 90 fps >>> + at 2688x1520 resolution. It is programmable through an I2C >>> + interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >>> + connection. >>> + >>> +allOf: >>> + - $ref: /schemas/media/video-interface-devices.yaml# >>> + >>> +properties: >>> + compatible: >>> + const: ovti,ov4689 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + description: >>> + External clock (XVCLK) for the sensor, 6-64 MHz >>> + maxItems: 1 >>> + >>> + clock-names: true >> >> This has to be strictly defined - which name you expect. > > Will fix in v3. Or maybe we should drop clock-names altogether and use > devm_clk_get(&client->dev, NULL) in the driver instead (I've seen this > approach in some existing drivers)? Yes, usually clock-names for one entry does not make sense. > >>> + >>> + dovdd-supply: >>> + description: >>> + Digital I/O voltage supply, 1.7-3.0 V >>> + >>> + avdd-supply: >>> + description: >>> + Analog voltage supply, 2.6-3.0 V >>> + >>> + dvdd-supply: >>> + description: >>> + Digital core voltage supply, 1.1-1.3 V >>> + >>> + powerdown-gpios: >>> + maxItems: 1 >> >> You can skip here maxItems - it is defined by gpio-consumer-common. > > Ack, will fix in v3. Does this also apply to reset-gpios? No. https://elixir.bootlin.com/linux/v6.0-rc1/source/Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml Best regards, Krzysztof
Hi Mikhail & Sakari On Thu, 22 Sept 2022 at 10:55, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Mikhail, > > On Sun, Sep 11, 2022 at 11:01:33PM +0300, Mikhail Rudenko wrote: > > Hello, > > > > this series implements support for Omnivision OV4689 image > > sensor. The Omnivision OV4689 is a high performance, 1/3-inch, 4 > > megapixel image sensor. Ihis chip supports high frame rate speeds up > > to 90 fps at 2688x1520 resolution. It is programmable through an I2C > > interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 > > connection. > > > > The driver is based on Rockchip BSP kernel [1]. It implements 4-lane CSI-2 > > and single 2688x1520 @ 30 fps mode. The driver was tested on Rockchip > > 3399-based FriendlyElec NanoPi M4 board with MCAM400 camera module. > > > > While porting the driver, I stumbled upon two issues: > > > > (1) In the original driver, horizontal total size (HTS) was set to a > > value (2584) lower then the frame width (2688), resulting in negative > > hblank. In this driver, I increased HTS to 2688, but fps dropped from > > 29.88 to 28.73. What is the preferred way to handle this? > > If horizontal total size is less than the frame width, something is > certainly wrong there. You can't have negative horizontal blanking. Neither > it can be zero. Something certainly seems odd. To continue my thoughts from earlier in this patch set, Omnivision's Product Brief [1] states: The 1/3-inch OV4689 can capture full-resolution 4-megapixel high definition (HD) video at 90 frames per second (fps), 1080p HD at 120 fps, and binned 720p HD at 180 fps The datasheet section 2.1 states: The OV4689 color image sensor is a high performance, 4 megapixel RAW image sensor that delivers 2688x1520 at 90 fps using OmniBSI-2™ pixel technology. So 4MP 90fps or 1080p120 should be achievable somehow. 2688x1520 @ 90fps is 367.7MPix/s, and that tallies quite nicely with table 2-9 listing the DAC PLL speed limitation of 360-378MHz. Exactly how that is then converted into PCLK or SCLK is unclear. Ideally you'd be able to contact an Omnivision FAE to confirm, but that means you need to be buying modules directly from them or otherwise have a business relationship. I do note that there is an NVidia Tegra driver for OV4689 at [2]. I wonder if analysis of that would reveal anything. I have just been looking at the ov9282 driver and the timings don't tally there either - configure it for 60fps and you get 30fps. The TIMING_HTS register appears to be in units of 2 pixels. The default is 0x2d8 (728 decimal) on a 1280x720 mode, but consider it as units of 2 pixels and HTS of 1456 (1280 active and hblank of 176) does match up. It works in the general case too. Looking at the OV4689 datasheet again, the default for TIMING_HTS (0x380c/d) is 0x5f8 (1528 decimal) when HOUTPUT_SIZE (0x3808/9) is 0x1200 (4608 decimal). Whilst there are no guarantees that default register settings will stream in any sensible form, it does imply TIMING_HTS is not in units of 1 pixel, and potentially 4 pixels.
Hi Dave, On 2022-09-22 at 11:43 +01, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > Hi Mikhail & Sakari > > On Thu, 22 Sept 2022 at 10:55, Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: >> >> Hi Mikhail, >> >> On Sun, Sep 11, 2022 at 11:01:33PM +0300, Mikhail Rudenko wrote: >> > Hello, >> > >> > this series implements support for Omnivision OV4689 image >> > sensor. The Omnivision OV4689 is a high performance, 1/3-inch, 4 >> > megapixel image sensor. Ihis chip supports high frame rate speeds up >> > to 90 fps at 2688x1520 resolution. It is programmable through an I2C >> > interface, and sensor output is sent via 1/2/4 lane MIPI CSI-2 >> > connection. >> > >> > The driver is based on Rockchip BSP kernel [1]. It implements 4-lane CSI-2 >> > and single 2688x1520 @ 30 fps mode. The driver was tested on Rockchip >> > 3399-based FriendlyElec NanoPi M4 board with MCAM400 camera module. >> > >> > While porting the driver, I stumbled upon two issues: >> > >> > (1) In the original driver, horizontal total size (HTS) was set to a >> > value (2584) lower then the frame width (2688), resulting in negative >> > hblank. In this driver, I increased HTS to 2688, but fps dropped from >> > 29.88 to 28.73. What is the preferred way to handle this? >> >> If horizontal total size is less than the frame width, something is >> certainly wrong there. You can't have negative horizontal blanking. Neither >> it can be zero. > > Something certainly seems odd. > > To continue my thoughts from earlier in this patch set, Omnivision's > Product Brief [1] states: > The 1/3-inch OV4689 can capture full-resolution 4-megapixel high > definition (HD) video at 90 frames per second (fps), 1080p HD at 120 > fps, and binned 720p HD at 180 fps > > The datasheet section 2.1 states: > The OV4689 color image sensor is a high performance, 4 megapixel RAW > image sensor that delivers 2688x1520 at 90 fps using OmniBSI-2™ pixel > technology. > > So 4MP 90fps or 1080p120 should be achievable somehow. > > 2688x1520 @ 90fps is 367.7MPix/s, and that tallies quite nicely with > table 2-9 listing the DAC PLL speed limitation of 360-378MHz. Exactly > how that is then converted into PCLK or SCLK is unclear. > Ideally you'd be able to contact an Omnivision FAE to confirm, but > that means you need to be buying modules directly from them or > otherwise have a business relationship. > I do note that there is an NVidia Tegra driver for OV4689 at [2]. I > wonder if analysis of that would reveal anything. > > I have just been looking at the ov9282 driver and the timings don't > tally there either - configure it for 60fps and you get 30fps. The > TIMING_HTS register appears to be in units of 2 pixels. The default is > 0x2d8 (728 decimal) on a 1280x720 mode, but consider it as units of 2 > pixels and HTS of 1456 (1280 active and hblank of 176) does match up. > It works in the general case too. > > Looking at the OV4689 datasheet again, the default for TIMING_HTS > (0x380c/d) is 0x5f8 (1528 decimal) when HOUTPUT_SIZE (0x3808/9) is > 0x1200 (4608 decimal). Whilst there are no guarantees that default > register settings will stream in any sensible form, it does imply > TIMING_HTS is not in units of 1 pixel, and potentially 4 pixels. > From the Rockchip BSP driver it plainly does stream at 30 (or 29.88) > fps with TIMING_HTS < HOUTPUT_SIZE, so a quick test of reducing it > further would be worthwhile. What does the default of 0x2d8 give you > as a frame rate, and are the images correct? Thanks for sharing this! Actually, I'm going to do some experimentation with these registers next weekend, and post the result here. > Just some thoughts. > Dave -- Best regards, Mikhail Rudenko