Add support for Arducam OV64A40 64Mpx camera module

[jmondi]

Hello,
this pull request adds support for the Arducam OV64A40 camera module by providing a driver for the sensor and the lens driver.

The driver uses the CCI helpers which are included in the pull request.

Support for the driver has been sent upstream but not yet included in linux-media.

[6by9]

From a very quick review:

• dtoverlay needs documenting in the README file.
• BU64754 driver had comments on linux-media against the use of regulator events. Is there an intention to address those? I know we have patches to existing VCM drivers doing the same thing, but it really wants to be a minimal change on top of what is going to be accepted in mainline.
• This version of the OV64A40 driver differs from mainline in at least supporting multiple link frequencies (and introduces a checkpatch warning in the process). What other changes were made? I was going to run through a review on mainline on the assumption that this was the same, but it's not.
• What is the use case for the user-configurable link frequency? Why would you choose not to run at the faster speed (which isn't the default)?
• There's a TODO of "Restrict supported link frequencies." listed in the OV64A40 driver. When is that intended to be fixed?
• Is building the modules only for 2711 and 2712 intentional due to the amount of memory (potentially) available? There are 1GB Pi4's around.
• Kieran is listed as maintainer for the VCM driver. There is no maintainer listed for the sensor driver. We'll accept out of tree drivers, but if there are significant fixups then we do defer back to the original committer, or the driver gets dropped.

[jmondi]

Hi Dave,
thanks for the quick feedback

From a very quick review:

• dtoverlay needs documenting in the README file.

Ah! didn't know! I'll add it

• BU64754 driver had comments on linux-media against the use of regulator events. Is there an intention to address those? I know we have patches to existing VCM drivers doing the same thing, but it really wants to be a minimal change on top of what is going to be accepted in mainline.

deferring to @kbingham

• This version of the OV64A40 driver differs from mainline in at least supporting multiple link frequencies (and introduces a checkpatch warning in the process). What other changes were made? I was going to run through a review on mainline on the assumption that this was the same, but it's not.

Several changes compared to v1 sent to the ML.

More or less from memory:

• link frequency support
• invert vflip bit setting
• remove 1280x720 mode as it was broken
• expanded VTS range to 24 bits
• Reduce max resolution output to 9152x6944. Using full output resolution causes green frames to be produced by the ISP (the raw frames are however correct)
• reduce max analogue gain to 16x

I plan to send a v2 of the driver to linux-media

• What is the use case for the user-configurable link frequency? Why would you choose not to run at the faster speed (which isn't the default)?

user configurable link frequencies (I presume you mean through the associated control) allows to increase the frame rate, but for compatibility reasons with arducam produced accessories (I presume the csi-2 serializers) the lower frequency is selected by default.

• There's a TODO of "Restrict supported link frequencies." listed in the OV64A40 driver. When is that intended to be fixed?

Yeah, I should take care of that, before sending it to the ml probably

• Is building the modules only for 2711 and 2712 intentional due to the amount of memory (potentially) available? There are 1GB Pi4's around.

no, not intentional. What other defconfigs should I take care of ?

• Kieran is listed as maintainer for the VCM driver. There is no maintainer listed for the sensor driver. We'll accept out of tree drivers, but if there are significant fixups then we do defer back to the original committer, or the driver gets dropped.

I should really add myself there

Thanks for the feedback!

[6by9]

• This version of the OV64A40 driver differs from mainline in at least supporting multiple link frequencies (and introduces a checkpatch warning in the process). What other changes were made? I was going to run through a review on mainline on the assumption that this was the same, but it's not.

Several changes compared to v1 sent to the ML.

More or less from memory:

* link frequency support

* invert vflip bit setting

* remove 1280x720 mode as it was broken

* expanded VTS range to 24 bits

* Reduce max resolution output to 9152x6944. Using full output resolution causes green frames to be produced by the ISP (the raw frames are however correct)

Naush mentioned that you'd previously discussed something funny giving green images. I guess we ought to investigate.
Could you send me a couple of raw files at full res?

* reduce max analogue gain to 16x

I plan to send a v2 of the driver to linux-media

If you send that, I'll do a review on it there. The ideal is for us to apply the accepted patch for this driver, but I appreciate that can take some time.

• What is the use case for the user-configurable link frequency? Why would you choose not to run at the faster speed (which isn't the default)?

user configurable link frequencies (I presume you mean through the associated control) allows to increase the frame rate, but for compatibility reasons with arducam produced accessories (I presume the csi-2 serializers) the lower frequency is selected by default.

You don't hotplug serialisers, so isn't this better done in DT? Overlays support overrides, so you drop the link frequency if other factors require it. We've done something similar on imx708 as it was too close to GPS frequencies. (I've never understood why the choice of link frequency should ever be left to userspace).

Normally I'd expect the serialisers and deserialisers to require DT entries of their own, but I can foresee how these have been implemented.

• Is building the modules only for 2711 and 2712 intentional due to the amount of memory (potentially) available? There are 1GB Pi4's around.

no, not intentional. What other defconfigs should I take care of ?

arm/configs/bcmrpi_defconfig is Pi 0&1 - probably skip that one as 512MB is too little RAM for these huge images.
arm/configs/bcm2709_defconfig is Pi 2 & 3
arm64/configs/bcmrpi3_defconfig is 64bit Pi 3 (generally superceded by bcm2711_defconfig).

arm/configs/bcm2835_defconfig is the upstream defconfig, so please do NOT change that one.

[jmondi]

• This version of the OV64A40 driver differs from mainline in at least supporting multiple link frequencies (and introduces a checkpatch warning in the process). What other changes were made? I was going to run through a review on mainline on the assumption that this was the same, but it's not.

Several changes compared to v1 sent to the ML.
More or less from memory:

* link frequency support

* invert vflip bit setting

* remove 1280x720 mode as it was broken

* expanded VTS range to 24 bits

* Reduce max resolution output to 9152x6944. Using full output resolution causes green frames to be produced by the ISP (the raw frames are however correct)

Naush mentioned that you'd previously discussed something funny giving green images. I guess we ought to investigate. Could you send me a couple of raw files at full res?

* reduce max analogue gain to 16x

I plan to send a v2 of the driver to linux-media

If you send that, I'll do a review on it there. The ideal is for us to apply the accepted patch for this driver, but I appreciate that can take some time.

• What is the use case for the user-configurable link frequency? Why would you choose not to run at the faster speed (which isn't the default)?

user configurable link frequencies (I presume you mean through the associated control) allows to increase the frame rate, but for compatibility reasons with arducam produced accessories (I presume the csi-2 serializers) the lower frequency is selected by default.

You don't hotplug serialisers, so isn't this better done in DT? Overlays support overrides, so you drop the link frequency if other factors require it. We've done something similar on imx708 as it was too close to GPS frequencies.

Are you suggesting then to use 456MHz as default and reduce the link frequency by using the link-frequencies DT properties, possibly in the serializer/deserializers overlays ?

[6by9]

Are you suggesting then to use 456MHz as default and reduce the link frequency by using the link-frequencies DT properties, possibly in the serializer/deserializers overlays ?

Yes.
Nothing in libcamera is going to change the link frequency, so reality would be it never gets changed. Why compromise the performance of the sensor for a serializer/deserializers that may or may not be present?

Now I can predict the reality of how Arducam's serializer/deserializer has been implemented with some MCU configuring it all rather then letting Linux do it, but at least loading via dtoverlay=ov64a40,link_freq=360000000 is obvious as to what is going on.

I understand the choice of link frequency to be a system level decision, (eg the imx708 tweak to avoid interference with GPS signals) so why involve userspace?

Unrelated, but just looking at the driver again, that ov64a40_init table is MASSIVE! You have 2189 lines in that table at 2 CCI_REG8's per line, so 4378 writes. 100kHz bus (by default) and 4 times 9 bits per write, I make 1.57seconds! And you're writing it on every call to ov64a40_start_streaming which is one of the more time sensitive paths. Seeing as the first write is (0x0103, 0x01) which is the soft reset, that really is a sledgehammer.
A large number of them are consecutive registers so could be defined as CCI_REG32 even if the docs don't cover them explicitly or state that they are 32 bit. A 4 CCI_REG8 writes takes 16 bytes. 1 CCI_REG32 takes 8 bytes. 50% saving.
A quick analysis says 3931 of the 4378 are consecutive, with many being far longer runs than just 4 byte values. Support the register address auto-increment more generically, and that table could be condensed down significantly.

I also noticed in passing that register 0x3f00 is set to the same value 3 times.

[jmondi]

Are you suggesting then to use 456MHz as default and reduce the link frequency by using the link-frequencies DT properties, possibly in the serializer/deserializers overlays ?

Yes. Nothing in libcamera is going to change the link frequency, so reality would be it never gets changed. Why compromise the performance of the sensor for a serializer/deserializers that may or may not be present?

Now I can predict the reality of how Arducam's serializer/deserializer has been implemented with some MCU configuring it all rather then letting Linux do it, but at least loading via dtoverlay=ov64a40,link_freq=360000000 is obvious as to what is going on.

That's true, I concur that selecting the link_frequency could easily be done with a dt param or in the overlay if needed. Let me check with arducam and try to use the highest link_freq by default

I understand the choice of link frequency to be a system level decision, (eg the imx708 tweak to avoid interference with GPS signals) so why involve userspace?

Unrelated, but just looking at the driver again, that ov64a40_init table is MASSIVE! You have 2189 lines in that table at 2 CCI_REG8's per line, so 4378 writes. 100kHz bus (by default) and 4 times 9 bits per write, I make 1.57seconds! And you're writing it on every call to ov64a40_start_streaming which is one of the more time sensitive paths. Seeing as the first write is (0x0103, 0x01) which is the soft reset, that really is a sledgehammer. A large number of them are consecutive registers so could be defined as CCI_REG32 even if the docs don't cover them explicitly or state that they are 32 bit. A 4 CCI_REG8 writes takes 16 bytes. 1 CCI_REG32 takes 8 bytes. 50% saving. A quick analysis says 3931 of the 4378 are consecutive, with many being far longer runs than just 4 byte values. Support the register address auto-increment more generically, and that table could be condensed down significantly.

The chip supports address auto-increment, so for consecutive addresses it could be possible indeed to use REG32 or even REG64 to avoid repeating the "slave_address[7:0]" + "register[15:8] register[7:0]" bytes at the beginning of each transaction.

Of course this will make it much harder to later untangle registers and move them away from register tables.

Also, I suspect most of those registers are set to the POR default, but of course comparing each of them with the datasheet default values is a quite tedious and error-prone job.

I see what I can do.

I also noticed in passing that register 0x3f00 is set to the same value 3 times.

and 0x350b as well, and there might be others indeed

[kbingham]

BU64754 driver had comments on linux-media against the use of regulator events. Is there an intention to address those? I know we have patches to existing VCM drivers doing the same thing, but it really wants to be a minimal change on top of what is going to be accepted in mainline.

Yes, absolutely - haven't got round to a next revision yet - but it's on my plate to look at this. Hopefully soon after I finish working on the IMX283.

And I still envisage a set of VCM helpers to abstract all the vcm drivers out from all the parts that are common. There's very little module specific handling in vcm drivers as far as I can see so far.

[jmondi]

Unrelated, but just looking at the driver again, that ov64a40_init table is MASSIVE! You have 2189 lines in that table at 2 CCI_REG8's per line, so 4378 writes. 100kHz bus (by default) and 4 times 9 bits per write, I make 1.57seconds! And you're writing it on every call to ov64a40_start_streaming which is one of the more time sensitive paths. Seeing as the first write is (0x0103, 0x01) which is the soft reset, that really is a sledgehammer. A large number of them are consecutive registers so could be defined as CCI_REG32 even if the docs don't cover them explicitly or state that they are 32 bit. A 4 CCI_REG8 writes takes 16 bytes. 1 CCI_REG32 takes 8 bytes. 50% saving. A quick analysis says 3931 of the 4378 are consecutive, with many being far longer runs than just 4 byte values. Support the register address auto-increment more generically, and that table could be condensed down significantly.

The chip supports address auto-increment, so for consecutive addresses it could be possible indeed to use REG32 or even REG64 to avoid repeating the "slave_address[7:0]" + "register[15:8] register[7:0]" bytes at the beginning of each transaction.

Of course this will make it much harder to later untangle registers and move them away from register tables.

Also, I suspect most of those registers are set to the POR default, but of course comparing each of them with the datasheet default values is a quite tedious and error-prone job.

I see what I can do.

I also noticed in passing that register 0x3f00 is set to the same value 3 times.

and 0x350b as well, and there might be others indeed

A quick update as I've had a more detailed look to that huge table..

Most registers there reported are undocumented in the datasheet version I have. There are very long sequences of undocumented registers initialized to the same identical values. I suspect those registers could probably be removed completely, but who knows...

Back to the topic of re-assembling RGB8 into REG32 writes, being most registers undocumented, I cannot tell if registers are 8 bits or longer, even if all the documented ones I have are 8 bits.

To be honest I'm not sure how to better move forward with that table, I understand that it's a very long startup delay, even more so because the suggested delay before streaming is started is a full frame exposure time, which at max resolution could go up to 500 milliseconds...

[6by9]

A quick update as I've had a more detailed look to that huge table..

Most registers there reported are undocumented in the datasheet version I have. There are very long sequences of undocumented registers initialized to the same identical values. I suspect those registers could probably be removed completely, but who knows...

Back to the topic of re-assembling RGB8 into REG32 writes, being most registers undocumented, I cannot tell if registers are 8 bits or longer, even if all the documented ones I have are 8 bits.

To be honest I'm not sure how to better move forward with that table, I understand that it's a very long startup delay, even more so because the suggested delay before streaming is started is a full frame exposure time, which at max resolution could go up to 500 milliseconds...

I'd missed the delay added after sending OV64A40_REG_SMIA = OV64A40_REG_SMIA_STREAMING - ouch!

You've had similar comments from Sakari on linux-media

You seem to be using autosuspend, but you still do not try to avoid writes of presumably the same register values if the sensor was powered on. The register writes usually take the most of the time there.

If each of the modes includes the same set of registers rather than relying on defaults or the common list, then you can send the common list once in ov64a40_power_on, and it doesn't need to be sent again unless the autosuspend delay expires and actually powers the sensor down. You get the pain once on the first start_streaming, but not when just changing modes (eg preview to still capture).

CCI is largely just specifying that the register address auto-increments on each byte written, therefore you can write a 32bit value via <i2c addr> <reg> <reg> <data 0> <data 1> <data 2> <data 3> (reverse if the opposite endian).
I don't have a datasheet for this sensor, but does it claim to be CCI compliant, or just that it auto-increments the address?

There's nothing stopping you extending that further to write as many consecutive bytes as happen to occur.
eg taking a snippet of the table:

				{CCI_REG8(0x3d00), 0x40},
	{CCI_REG8(0x3d01), 0x40}, {CCI_REG8(0x3d02), 0x40},
	{CCI_REG8(0x3d03), 0x40}, {CCI_REG8(0x3d04), 0x40},
	{CCI_REG8(0x3d05), 0x40}, {CCI_REG8(0x3d06), 0x40},
	{CCI_REG8(0x3d07), 0x40}, {CCI_REG8(0x3d08), 0x40},
	{CCI_REG8(0x3d09), 0x40}, {CCI_REG8(0x3d0a), 0x40},
	{CCI_REG8(0x3d0b), 0xcd}, {CCI_REG8(0x3d0c), 0xcd},
	{CCI_REG8(0x3d0d), 0xcd}, {CCI_REG8(0x3d0e), 0xcd},
	{CCI_REG8(0x3d0f), 0xcd}, 

could become

{ 0x3d00, { 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd } }

Write that on the bus and 16 * 4 = 48 bytes transferred becomes 19 bytes for a 60% reduction in time. 32 consecutive registers would give a 64% saving (35 bytes instead of 96).
It's not as readable, but seeing as all this is undocumented it could just as well be a binary firmware file rather than source code.
You get similar patterns in LCD configurations, eg https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/tiny/ili9341.c#L73-L110

CCI may be the current darling, but it doesn't solve all problems in the most efficient way.

[jmondi]

A quick update as I've had a more detailed look to that huge table..
Most registers there reported are undocumented in the datasheet version I have. There are very long sequences of undocumented registers initialized to the same identical values. I suspect those registers could probably be removed completely, but who knows...
Back to the topic of re-assembling RGB8 into REG32 writes, being most registers undocumented, I cannot tell if registers are 8 bits or longer, even if all the documented ones I have are 8 bits.
To be honest I'm not sure how to better move forward with that table, I understand that it's a very long startup delay, even more so because the suggested delay before streaming is started is a full frame exposure time, which at max resolution could go up to 500 milliseconds...

I'd missed the delay added after sending OV64A40_REG_SMIA = OV64A40_REG_SMIA_STREAMING - ouch!

You've had similar comments from Sakari on linux-media

You seem to be using autosuspend, but you still do not try to avoid writes of presumably the same register values if the sensor was powered on. The register writes usually take the most of the time there.

If each of the modes includes the same set of registers rather than relying on defaults or the common list, then you can send the common list once in ov64a40_power_on, and it doesn't need to be sent again unless the autosuspend delay expires and actually powers the sensor down. You get the pain once on the first start_streaming, but not when just changing modes (eg preview to still capture).

I can certainly do this to pay the long write price once only

CCI is largely just specifying that the register address auto-increments on each byte written, therefore you can write a 32bit value via <i2c addr> <reg> <reg> <data 0> <data 1> <data 2> <data 3> (reverse if the opposite endian). I don't have a datasheet for this sensor, but does it claim to be CCI compliant, or just that it auto-increments the address?

It doesn't mention CCI, no suprises, but it reports a 'sequential write from random location" mode which is compatible

There's nothing stopping you extending that further to write as many consecutive bytes as happen to occur. eg taking a snippet of the table:

				{CCI_REG8(0x3d00), 0x40},
	{CCI_REG8(0x3d01), 0x40}, {CCI_REG8(0x3d02), 0x40},
	{CCI_REG8(0x3d03), 0x40}, {CCI_REG8(0x3d04), 0x40},
	{CCI_REG8(0x3d05), 0x40}, {CCI_REG8(0x3d06), 0x40},
	{CCI_REG8(0x3d07), 0x40}, {CCI_REG8(0x3d08), 0x40},
	{CCI_REG8(0x3d09), 0x40}, {CCI_REG8(0x3d0a), 0x40},
	{CCI_REG8(0x3d0b), 0xcd}, {CCI_REG8(0x3d0c), 0xcd},
	{CCI_REG8(0x3d0d), 0xcd}, {CCI_REG8(0x3d0e), 0xcd},
	{CCI_REG8(0x3d0f), 0xcd}, 

could become

{ 0x3d00, { 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd } }

Write that on the bus and 16 * 4 = 48 bytes transferred becomes 19 bytes for a 60% reduction in time. 32 consecutive registers would give a 64% saving (35 bytes instead of 96). It's not as readable, but seeing as all this is undocumented it could just as well be a binary firmware file rather than source code. You get similar patterns in LCD configurations, eg https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/tiny/ili9341.c#L73-L110

I got that part

CCI may be the current darling, but it doesn't solve all problems in the most efficient way.

It's not about CCI being pretty or not, it's about reworking a table of 4k entries most of them undocumented. As I presume those settings have been received from the manufacturer, I would be tempted to say that if the manufacturer is not happy with the startup performances of their sensor, they're very welcome to contribute to the driver and make it more performant.

[6by9]

It's not about CCI being pretty or not, it's about reworking a table of 4k entries most of them undocumented. As I presume those settings have been received from the manufacturer, I would be tempted to say that if the manufacturer is not happy with the startup performances of their sensor, they're very welcome to contribute to the driver and make it more performant.

Knowing Omnivision and Sony, they'll have sent through a spreadsheet with settings.

I'll leave it to you as you're going to be the listed maintainer. At least doing it only once removes most of the pain.

See what further comments Sakari makes.

[jmondi]

• Reduce max resolution output to 9152x6944. Using full output resolution causes green frames to be produced by the ISP (the raw frames are however correct)

Naush mentioned that you'd previously discussed something funny giving green images. I guess we ought to investigate. Could you send me a couple of raw files at full res?

I can only capture RAW on the pi5 due to the size of the images. In which format would you prefer to receive the images ?

[6by9]

I can only capture RAW on the pi5 due to the size of the images. In which format would you prefer to receive the images ?

Ideally just the raw image data, but I can cope with a DNG or similar.

If necessary I could probably generate an image of the relevant size, but having a genuine image removes one variable.

[jmondi]

I can only capture RAW on the pi5 due to the size of the images. In which format would you prefer to receive the images ?

Ideally just the raw image data, but I can cope with a DNG or similar.

yeah but you know on the pi5 I can either get 9152x6944-RG16 or 9152x6944-PC1R. Any preference ?

If necessary I could probably generate an image of the relevant size, but having a genuine image removes one variable.

[6by9]

9152x6944-RG16

[jmondi]

9152x6944-RG16

and to make it actually useful I should probably send you the full res (9248x6944) one that generates green images in some output resolutions

And for your reference this a table of the results we had

Output size	AspectRatio	9152x6944	9248x6944
4624x3472	4:3	OK	OK
3840x2160	16:9	OK	KO
3840x2880	4:3	OK	KO
2312x1736	4:3	OK	OK
1920x1440	4:3	KO	KO
1920x1080	16:9	KO	KO
1280x960	4:3	OK	KO
1280x720	16:9	OK	KO

[jmondi]

Changelog for this new version

• the ov64a40 is now the one sent as v4 to the linux-media mailing list, backported to v6.1
  (https://patchwork.linuxtv.org/project/linux-media/list/?series=11670)
• break-out from the driver the commit that reduces sizes to 9152x6944 to ease rebasing on the driver once merged upstream
• Fix BU64754 issues in runtime_pm handling
• Adjust the RPi integration:
  • Add overlays to README
  • Add link-frequency dtparam properties to the overlay
  • Add options to all the defconfigs

[jmondi]

I copied the overlay integration from fa45fc7 but dtoverlaycheck still fails. What am I missing ?

[pelwell]

These added parameters are all misnamed, but the overlay looks good once they've been renamed.

[jmondi]

oh that was really stupid, sorry, I didn't notice. Fixed a re-pushed

[pelwell]

Are you happy with this, @6by9?

[6by9]

The media subsystem requires checkpatch.pl --strict (https://elixir.free-electrons.com/linux/latest/source/Documentation/driver-api/media/maintainer-entry-profile.rst#L153), which flags up a couple of extra CHECKs

CHECK: multiple assignments should be avoided
#3111: FILE: drivers/media/i2c/ov64a40.c:3040:
+	fse->min_width = fse->max_width = mode->width;

CHECK: multiple assignments should be avoided
#3112: FILE: drivers/media/i2c/ov64a40.c:3041:
+	fse->min_height = fse->max_height = mode->height;

CHECK: Alignment should match open parenthesis
#3402: FILE: drivers/media/i2c/ov64a40.c:3331:
+	ov64a40->link_freq = v4l2_ctrl_new_int_menu(hdlr, &ov64a40_ctrl_ops,
+					    V4L2_CID_LINK_FREQ,

CHECK: Alignment should match open parenthesis
#3414: FILE: drivers/media/i2c/ov64a40.c:3343:
+	ov64a40->hblank = v4l2_ctrl_new_std(hdlr, &ov64a40_ctrl_ops,
+				   V4L2_CID_HBLANK, hblank_val, hblank_val, 1,

CHECK: Blank lines aren't necessary before a close brace '}'
#3505: FILE: drivers/media/i2c/ov64a40.c:3434:
+
+	}

CHECK: Alignment should match open parenthesis
#3525: FILE: drivers/media/i2c/ov64a40.c:3454:
+	ov64a40->link_frequencies = devm_kcalloc(ov64a40->dev,
+					v4l2_fwnode.nr_of_link_frequencies,

Other comments inline.

[6by9]

I assume you mean ov64a40_init_controls, but I'm not sure what there is to fix here.

[jmondi]

maybe not a FIXME but rather a note to explain why there's a ppl * 4 below

[6by9]

Again, what is there to fix? This appears to be a statement of how the sensor works internally rather than something that needs fixing.

Potentially more obvious is to define OV64A40_PIXEL_RATE to be the genuine pixel rate, and compensate when computing you delay in start_streaming.

[jmondi]

ok, these might not be FIXME items.

It's not about the delay in start_streaming, it's about userspace compute a correct frame duration

[6by9]

The only other place OV64A40_PIXEL_RATE is used is in the delay at start streaming.

#define OV64A40_PIXEL_RATE 300000000 as that is the actual pixel rate, but your calc in start_streaming becomes

	delay += DIV_ROUND_UP(timings->ppl * 4 * ov64a40->exposure->cur.val,
			      OV64A40_PIXEL_RATE / 1000 / 1000);

[jmondi]

As I'm anyway registering PIXEL_RATE * 4 as the control value, I can indeed do that

[6by9]

When is that being validated and by whom?

[jmondi]

again this was a note to record the min vblank is not characterized in the datasheet

[6by9]

Isn't that just a CCI16 write of ctrl->val << 1, or are the two registers the wrong way round?

Nit pick: comment says xxx_GAIN, whilst the register is GAIN_xxx.

[jmondi]

The coarse gain is 7 bits long. We should be safe as we define a max value for the control and could probably write this in one go, but if you don't particularly mind, I would keep it this way to be explicit on how the registers are assembled

[6by9]

The ctrl_handler does guarantee that the value will remain in range.

Swapping to

#define OV64A40_REG_MEC_LONG_GAIN	CCI_REG16(0x3508)
...
		cci_write(ov64a40->cci, OV64A40_REG_MEC_LONG_GAIN, ctrl->val << 1, &ret);

gets rid of ov64a40_set_anagain totally.

It felt fairly arbitrary that V4L2_CID_VBLANK was written out in full in ov64a40_set_ctrl, but V4L2_CID_ANALOGUE_GAIN was split out as a separate function.

[6by9]

This table is far shorter than the other mode reg_lists. Now that you're not sending ov64a40_init with the CCI_REG8(0x0103), 0x01 reset every time you start_streaming, do none of the extra settings matter in this mode such that a mode switch from another mode to it still works correctly?

[jmondi]

I can only rely on the registers list received from the vendor so I trust they are correct but I can try to start two streaming sessions one after the other with different settings

[6by9]

Please do.

Currently if you set mode 4624x3472, start_streaming, stop_streaming, set mode 9248x6944, start_streaming, then the first mode set will have written registers 0x034b, 0x3504, 0x360d, 0x368a, etc to a value that may or may not be the default, but are then not reset to the default on starting the second mode.

With no datasheet and no sensor I can't say if they're the defaults or not.

[jmondi]

You're right and mixing modes doesn't work as expected.

I tried expanding the 9248x6944 by resetting registers to the values specified in the init table, but that's not enough as I get sub-optimal results (all white frames, or hangs) depending on how I alternate modes.

Again, most registers are not documented and I just a confirmation from Arducam that there are no plans to reduce the register table sizes compared to what OV provided and which gives the best results.

All in all, I'm going to re-write the full init table at start_stream time, as I cannot possibly test all the combinations of modes and check which undocumented register has been overwritten or not.

[6by9]

Up to you.

Personally I would have dumped the default values of all the registers written by any of these mode specific tables, and then added the default values to the tables wherever the register is missing.

[6by9]

Is this not a CCI_REG16 for PLL1_MULTIPLIER? Ditto PLL2_MULTIPLIER below.

[jmondi]

yes, but since these are written once only per streaming session I don't think it makes a difference ?

[6by9]

You could say that about any register though, so why bother with CCI?

[jmondi]

what does CCI have to do with this ? It's the second salty comment about CCI, what's wrong with it ? it saves every driver from re-implementing the same write/read routines, I guess that's a win. Anywa if these two registers bother you particularly, I can change them.

[kbingham]

combining split high/low registers to a single value makes things clearer and easier to comprehend IMO. That's what I particularly like about CCI_REG16() at least.

[6by9]

what does CCI have to do with this ? It's the second salty comment about CCI, what's wrong with it ? it saves every driver from re-implementing the same write/read routines, I guess that's a win. Anywa if these two registers bother you particularly, I can change them.

I'm making comments on the patch as I'm reading through. You're using the CCI framework, but there appears to be interpretations of the register map that have been missed.
I'd make exactly the same comment if I was reading and reviewing it on linux-media (I'm happy to review the patches sent there if you'd prefer).

The other issue over the length of the init table is that I'd like to see efficient system performance, and over sending I2C commands is quite a long way from that. It's nothing against CCI per se, just that it's not a great fit there.

[6by9]

Your cam_helper says the gain code is gain * 128, so do values below 128 (ie below unity gain) actually work?

[jmondi]

I might have missed why they shouldn't..

[6by9]

@davidplowman will correct me I'm sure, but aiui what would the point be of capturing the photons, potentially saturating the well and blowing out the image, and then reducing the amplitude of that image. If you really want that, apply a <x1.0 digital gain later.

A value of 0 for x0.0 would give a totally black image.

Very few sensors actually do sensible things with analogue gain <x1.0, hence my question of whether it works or not. Sony and OnSemi tend to use the weird gain formulae as then a register value of 0 falls out as x1.0.

[6by9]

Observation: Max here is 0x7ff, or x15.99. set_anagain says that COARSE_GAIN (ie the integer bit) is 7 bits wide, which would allow x127.99.

Is this just sloppiness in the datasheet, and the top 3 bits of COARSE_GAIN are required to be 0 then?

[jmondi]

Probably it's a datasheet issue. Arducam has tested all the supported gain values and found that values larger than 0x7ff corrupts the image and required to reduce max gain

[6by9]

x15.99 sounds a fairly typical max analogue gain.
Potentially a cleanup of the comment below over how many bits are available for COARSE_GAIN.

[6by9]

OV64A40_PLL_DEFAULT as 0x1480 is only valid on a 360MHz link frequency in full res mode.

Use ov64a40_get_timings to get the actual ppl of the mode, otherwise the initial mode is going to have the wrong HBLANK if on a 456MHz link freq. (It also means that OV64A40_PLL_DEFAULT is redundant).

[jmondi]

ah right, thanks

[jmondi]

The media subsystem requires checkpatch.pl --strict (https://elixir.free-electrons.com/linux/latest/source/Documentation/driver-api/media/maintainer-entry-profile.rst#L153), which flags up a couple of extra CHECKs

CHECK: multiple assignments should be avoided
#3111: FILE: drivers/media/i2c/ov64a40.c:3040:
+	fse->min_width = fse->max_width = mode->width;

CHECK: multiple assignments should be avoided
#3112: FILE: drivers/media/i2c/ov64a40.c:3041:
+	fse->min_height = fse->max_height = mode->height;

CHECK: Alignment should match open parenthesis
#3402: FILE: drivers/media/i2c/ov64a40.c:3331:
+	ov64a40->link_freq = v4l2_ctrl_new_int_menu(hdlr, &ov64a40_ctrl_ops,
+					    V4L2_CID_LINK_FREQ,

CHECK: Alignment should match open parenthesis
#3414: FILE: drivers/media/i2c/ov64a40.c:3343:
+	ov64a40->hblank = v4l2_ctrl_new_std(hdlr, &ov64a40_ctrl_ops,
+				   V4L2_CID_HBLANK, hblank_val, hblank_val, 1,

CHECK: Blank lines aren't necessary before a close brace '}'
#3505: FILE: drivers/media/i2c/ov64a40.c:3434:
+
+	}

CHECK: Alignment should match open parenthesis
#3525: FILE: drivers/media/i2c/ov64a40.c:3454:
+	ov64a40->link_frequencies = devm_kcalloc(ov64a40->dev,
+					v4l2_fwnode.nr_of_link_frequencies,

Other comments inline.

I had to chose between going > 80 cols or non aligning to the open parenthesis. Knowing Sakari cares a lot about 80 cols, I went for the second option

[6by9]

I had to chose between going > 80 cols or non aligning to the open parenthesis. Knowing Sakari cares a lot about 80 cols, I went for the second option

They are only CHECKs, but I expect you'd get the same query in a review on linux-media.

You're permitted to split the line after the =, so

	ov64a40->link_freq =
		v4l2_ctrl_new_int_menu(hdlr, &ov64a40_ctrl_ops,
				       V4L2_CID_LINK_FREQ,
				       ov64a40->num_link_frequencies - 1,
				       0, ov64a40->link_frequencies);

complies with both requirements.

hblank is just splitting the lists of parameters in different places

	ov64a40->hblank = v4l2_ctrl_new_std(hdlr, &ov64a40_ctrl_ops,
					    V4L2_CID_HBLANK, hblank_val,
					    hblank_val, 1, hblank_val);

	ov64a40->link_frequencies =
			devm_kcalloc(ov64a40->dev,
				     v4l2_fwnode.nr_of_link_frequencies,
				     sizeof(v4l2_fwnode.link_frequencies[0]),
				     GFP_KERNEL);

likewise is permitted.

As for multiple assignments, it's only 2 extra lines of code. I give up arguing at that point.
Most other drivers seem to do

	fse->min_width = WIDTH;
	fse->max_width = fse->min_width;
	fse->min_height = HEIGHT;
	fse->max_height = fse->min_height;

[kbingham]

I meant to ask this before. But why do you use hex values here and then a /* comment */ to state the value in decimal? Why not just use the decimal value? To me that's more readable - and doesn't need a comment to state the hex value - so the two wouldn't ever get out of sync.

The units on these vts/ppl/top/left entries make far more sense in decimal...

[kbingham]

Particularly here - I don't think we should mix and match hex and decimal ... decimal values make more sense all through.

[6by9]

I'd linked to test firmware back in #5708 (comment)

I've just kicked the firmware builds again, and it's been merged.

We release firmware and prebuilt kernels together through https://github.com/raspberrypi/rpi-firmware, so as long the firmware change is merged before this PR (which it has been), then you'll get both together. Up to you when you test the firmware change.

[ArduCAM]

I think the name is spelled wrong here.

[pelwell]

It does look like it should be avdd-supply.

[jmondi]

yes, thanks for spotting. Fixed in next version.

[ArduCAM]

I'd linked to test firmware back in #5708 (comment)

I've just kicked the firmware builds again, and it's been merged.

We release firmware and prebuilt kernels together through https://github.com/raspberrypi/rpi-firmware, so as long the firmware change is merged before this PR (which it has been), then you'll get both together. Up to you when you test the firmware change.

Executing sudo SKIP_KERNEL=1 rpi-update did not update to the expected firmware version,

pi@raspberrypi:~ $ vcgencmd version
Oct 17 2023 15:39:16 
Copyright (c) 2012 Broadcom
version 30f0c5e4d076da3ab4f341d88e7d505760b93ad7 (clean) (release) (start)

but downloaded and decompressed it and updated it to the expected version,

pi@raspberrypi:~ $ vcgencmd version
Nov 17 2023 17:25:49 
Copyright (c) 2012 Broadcom
version a7a69ddc8d93019ea3c75681cffcd6820f8bbb14 (tainted) (release) (start)

and the 64mp capture was normal, and the all-green picture no longer appeared:
64mp-3.zip

[jmondi]

• Remove 9152 size reduction after the firmware fix has been confirmed to work by @ArduCAM
• Add 48MP 8000x6000 mode
• Fix regulator name in dts overlay as reported by @ArduCAM and @pelwell

[pelwell]

OK to merge, @6by9?

[6by9]

Passing comment on the new mode looking odd. If you say that is the mode you want, then fine by me.

Everything else looks fine.

[6by9]

left + width = 624 + 8671 = 9295. Isn't the sensor only 9286 wide?
top + height = 472 + 6503 = 6975. The sensor is 6976 pixels high.

So this isn't a centre crop as would normally be expected.

[ArduCAM]

left + width = 624 + 8671 = 9295. Isn't the sensor only 9286 wide? top + height = 472 + 6503 = 6975. The sensor is 6976 pixels high.

So this isn't a centre crop as would normally be expected.

After reading the code, width is actually horizontal_end, and it does not add left.

[jmondi]

@ArduCAM who has provided me the mode info

[6by9]

OK, I'll leave it to the end of the day for a response, but otherwise I'm happy to merge.

[6by9]

If it's really horizontal_end as Arducam say, then the variables should be named appropriately. width and height have fairly obvious meanings which don't match the use here. Doubly confusing as width and height in digital_crop do appear to be the width and height of the region.

[jmondi]

ooook

I'll rename

[jmondi]

Ah wait, it's a struct v4l2_rect I can't pick arbitrary name and I don't think it's worth defining a dedicated type just for this (I'm sure someone would ask during review why a custom type is needed)

[6by9]

In which case make it actually represent width and height, and do the maths to compute OV64A40_REG_TIMING_CTRL4 and OV64A40_REG_TIMING_CTRL6 in ov64a40_program_geometry.

[jmondi]

ok

[ArduCAM]

left + width = 624 + 8671 = 9295. Isn't the sensor only 9286 wide?
top + height = 472 + 6503 = 6975. The sensor is 6976 pixels high.

So this isn't a centre crop as would normally be expected.

width is actually horizontal_end, and it does not add left.
(I left a comment yesterday, but it says pending and others can't seem to see it.)

[ArduCAM]

cam0 = <&i2c_frag>, "target:0=",<&i2c_vc>,

The i2c_vc here should be replaced by i2c_csi_dsi0.

[jmondi]

ups. I wonder where this comes from as no other overlay uses i2c_vc.

I'll fix, sorry for the hiccup

[6by9]

Updated for Pi5.
i2c_vc is GPIOs 0&1, and on Pi0-4 will also be aliased to i2c_csi_dsi0.
On Pi5 both i2c_csi_dsi and i2c_csi_dsi0 are dedicated I2C buses.

[jmondi]

does it mean it is correct to use i2c_csi_dsi0 ?

[jmondi]

does it mean it is correct to use i2c_csi_dsi0 ?

@6by9 can you confirm this so I can send a new version and possibly send the same driver version to mainline this morning ?

[ArduCAM]

left + width = 624 + 8671 = 9295. Isn't the sensor only 9286 wide? top + height = 472 + 6503 = 6975. The sensor is 6976 pixels high.

So this isn't a centre crop as would normally be expected.

After reading the code, width is actually horizontal_end, and it does not add left.

[ArduCAM]

Okay, it looks like I forgot to submit a review. . (not very familiar with GitHub’s review process...)

[ArduCAM]

In addition, can we enable pi zero compilation by default? Although large resolution cannot be used, small resolution can be used normally (to avoid receiving complaints from users that the camera cannot be detected)

[jmondi]

In addition, can we enable pi zero compilation by default? Although large resolution cannot be used, small resolution can be used normally (to avoid receiving complaints from users that the camera cannot be detected)

According to https://www.raspberrypi.com/documentation/computers/linux_kernel.html#default_configuration I should enable the sensor and the lens compilation for bcmrpi_defconfig. I'll do so unless anyone expresses concerns regarding the devices limited resources

[6by9]

In addition, can we enable pi zero compilation by default? Although large resolution cannot be used, small resolution can be used normally (to avoid receiving complaints from users that the camera cannot be detected)

Without suitable warnings I think you're going to get just as many complaints that the default behaviour of rpicam-still in selecting the highest resolution mode for the still capture fails as for not being detected.

According to https://www.raspberrypi.com/documentation/computers/linux_kernel.html#default_configuration I should enable the sensor and the lens compilation for bcmrpi_defconfig. I'll do so unless anyone expresses concerns regarding the devices limited resources

I haven't checked the module sizes, but it's not going to make a huge difference if enabled on Pi0&1.

Largely it comes down to Arducam documenting what can and can't work. As it's a third party sensor it won't be in our docs.

[jmondi]

• s/i2c_vc/i2c_csi_dsi0 in overlay
• Change analogue_crop handling to have width/height represent the window sizes and not the line/col end
• Add support for test patterns
• Enable symbols in bcmrpi_defconfig

Hopefully this should be the last version ? I'll send the same version of the sensor driver to mainline. Any requested change that doesn't dramatically impact the driver can be later backported on the version merged here ?

[6by9]

Looks good - thanks.

@pelwell anything else you want to raise? Otherwise hit the merge button.

[pelwell]

No - I think we've been round the loop often enough. Merging.

[6by9]

Just a thought over the analogue crops before you send your patch set to the mailing list. You've defined the widths and heights as odd numbers. That is almost certainly incorrect as it messes up Bayer orders with flips.

The conversion from left and width to end pixel is normally left + width - 1, as one count is inclusive and one is exclusive.

[jmondi]

Just a thought over the analogue crops before you send your patch set to the mailing list. You've defined the widths and

Too late :)

heights as odd numbers. That is almost certainly incorrect as it messes up Bayer orders with flips.

The conversion from left and width to end pixel is normally left + width - 1, as one count is inclusive and one is exclusive.

Correct. You can comment there and I can fix it, then send a patch on top for your bsp ? (I already have one smatch warning fixed in the mainline version and not here. Sorry, but keeping the two in sync is rather painful)

[6by9]

You hadn't sent the patch when I started the comment, so had nowhere else to post it.

Happy to shift to mailing list review comments now, and we can fix up this tree once it gets merged.