drivers: video: ov2640 driver: Enhancing ov2640 driver

[cornway]

Enabling proper zoom/scaling for smaller resolutions (e.g. QQVGA)
This is basically a copy paste from linux driver with some tweaks

This patch may need some refactoring/cleaning up, but generally I have verified it function properly
I found that without this patch, if i enable low resolution format (e.g. QVGA/QQVGA) I basically receive cropped image (top left corner) not scaled, so it makes it difficult to receive meaningfull image from camera for further processing

[josuah]

Thanks for the contribution!
However, Linux license is GPLv2 so it's not really possible to copy-paste from it.
Also, some possibly better way to handle it would be to allow the user to pass exactly any size they want.
Since it's computed from a macro, that might allow some runtime customization?

I think it supports arbitrary scaling, so this could be a good fit for the video_set_selection() API to allow the user to crop/scale at any supported size.

[dsseng]

This is basically a copy paste from linux driver with some tweaks

Please notice the ov2640 driver in Linux has no dual-licensing, and GPLv2 must not be copied into Zephyr. There seems to not be a significant code added, so let's wait for another review

P.S. Also please consider the SonarQube warnings

[dsseng]

Not sure if breaking these out to constants is wanted

[josuah]

It can be hard to remember what size is UXGA or QCIF, while 1600x1200 or 176x144 is obvious for the reader.
Maybe the goal was avoiding accidental typos?

[dsseng]

Then it would make sense to add numeric resolutions to the comments here, so they do not duplicate what's in the code

[cornway]

That was done to match window setting, so if format is meant to be supported - it is known that there is a scaling/windowing setting for it
Thank you

[cornway]

video_set_selection

Thank you for your comment
Seems video_set_selection() is not relevant here as it uses "st_stm32_dcmipp" compatible driver, which is not present in the device I'm working with
In my opinion, If I tell camera to output WxY resolution, I expect whole frame fit into that resolution, not the cropped part, especially if it is not obvious where that cropped region came from
OV2640 registers description is very vague, and this approach seems only working at the moment
I can refactor this code to avoid copy-pasting
Thank you, waiting for your reply

[cornway]

Honestly, I just wanted to ease the pain for those still working with that type of sensors, it took me some time to figure this out

Thanks for the contribution! However, Linux license is GPLv2 so it's not really possible to copy-paste from it. Also, some possibly better way to handle it would be to allow the user to pass exactly any size they want. Since it's computed from a macro, that might allow some runtime customization?

I think it supports arbitrary scaling, so this could be a good fit for the video_set_selection() API to allow the user to crop/scale at any supported size.

One more note: there is no possibility (seems to me) of having "any size" user wants, as there are a bunch of prescalers and dividers to be set to downsize properly, not every size fits into that combination
Thank you for your patience and time

[josuah]

Is this different from FIELD_PREP()? This would also break the chain from GPL code to Zephyr to modify this.

[cornway]

Looks similar, but in FIELD_PREP() mask must be pre-computed properly, there is no *shift arguments
Thank you for pointing that out, I think in that case I can just plug in FIELD_PREP

[josuah]

There is no shift argument, but the macros can understand where the mask starts and ends, and shifts the value accordingly.

representing its field position and width [1]

To pre-compute the mask, you can use GENMASK(lsb, msb), these two are typically used together and allow to use the value from the datasheet without risk of typo by manual tweaking the masks:

#define THIS_FIELD_SET(x) FIELD_PREP(GENMASK(7, 3), x)

Or even better IMHO, just a plain transcription of the datasheet data without any smart:

#define THIS_FIELD_MASK GENMASK(7, 3)

Then in the code:

    {THIS_ADDR, FIELD_PREP(THIS_FIELD_MASK, 323)},

[josuah]

For new code, would it be possible to use this even if it breaks the local style? Only if interested in helping with it, and no need to change the other locations.

	ret = ov2640_write_all(dev, uxga_regs, ARRAY_SIZE(uxga_regs));
	if (ret < 0) {
		return ret;
	}

[cornway]

Why not, let me plug this in

[josuah]

It might work to turn |= into a plain =, now that you added the if (ret < 0) check.

[josuah]

Sorry slightly long post to offer more context and offer choices to you as contributor.

Honestly, I just wanted to ease the pain for those still working with that type of sensors, it took me some time to figure this out

I agree the current sensors did not evolve as fast as the API did. They all need to get some spring clean-up to facilitate collaboration.

there is no possibility (seems to me) of having "any size" user wants

The datasheet possibly promised it but #define PER_SIZE_REG_SEQ(x, y, v_div, h_div, pclk_div) gets the dividers and clock hand crafted, for a preset of sizes. That's going to be a lot better than what is currently in the source, and does not burden

Currently Zephyr Video APIs are still somewhat documented in Linux documentation for the semantics, there's only basic function API usage in Zephyr. This is expected to change soon.

AFAIU, this macro does multiple things, which would ideally be dis-aggregated:

{CTRLI, CTRLI_LP_DP | CTRLI_V_DIV_SET(v_div) | CTRLI_H_DIV_SET(h_div)}, // [binning]
{ZMOW, ZMOW_OUTW_SET(x)}, // Vertical [cropping]
{ZMOH, ZMOH_OUTH_SET(y)}, // Horizontal [cropping]
{ZMHH, ZMHH_OUTW_SET(x) | ZMHH_OUTH_SET(y)}, // V/H [scaling]
{R_DVP_SP, pclk_div}, // [link-frequency]

That gives us 4 different APIs:

• video_set_format() for configuring [binning]
• video_set_selection() with VIDEO_SEL_TGT_CROP for [cropping]
• video_set_selection() with VIDEO_SEL_TGT_COMPOSE for [scaling]
• video_set_ctrl() with VIDEO_CID_LINK_FREQ for the link frequency.

(+) the tiny VPU/ISP inside the image sensor becomes a tool the MCU can make use of
(+) simpler driver, pushing all the work to the video subsystem
(-) more work on this PR to get there (maintainers can give a hand)
(-) APIs are still not completely documented (maintainers can give a hand)

For instance transfer a low-res image (strong binning, good for low-noise), run some image detection on it, and transfer a JPEG image cropped to the detection result at max resolution.

If interested in trying, feel welcome, glad to help with that.
Otherwise, I think just a bit of tuning and removing the GPL'ed parts is going to be enough.

Either way, thank you for your efforts on improving OV2640! Small low-power low-cost image sensors can do a lot with good drivers. :) Any bit help, and your current proposal does not increase maintenance burden.

[josuah]

Ah, there we go, a WIP pull request showing some way to integrate cropping into an image sensor driver:

• VIDEO: GC2145/stm32_dcmi support for CROP/Snapshot #93797

[cornway]

Sorry slightly long post to offer more context and offer choices to you as contributor.

Honestly, I just wanted to ease the pain for those still working with that type of sensors, it took me some time to figure this out

I agree the current sensors did not evolve as fast as the API did. They all need to get some spring clean-up to facilitate collaboration.

there is no possibility (seems to me) of having "any size" user wants

The datasheet possibly promised it but #define PER_SIZE_REG_SEQ(x, y, v_div, h_div, pclk_div) gets the dividers and clock hand crafted, for a preset of sizes. That's going to be a lot better than what is currently in the source, and does not burden

Currently Zephyr Video APIs are still somewhat documented in Linux documentation for the semantics, there's only basic function API usage in Zephyr. This is expected to change soon.

AFAIU, this macro does multiple things, which would ideally be dis-aggregated:

{CTRLI, CTRLI_LP_DP | CTRLI_V_DIV_SET(v_div) | CTRLI_H_DIV_SET(h_div)}, // [binning]
{ZMOW, ZMOW_OUTW_SET(x)}, // Vertical [cropping]
{ZMOH, ZMOH_OUTH_SET(y)}, // Horizontal [cropping]
{ZMHH, ZMHH_OUTW_SET(x) | ZMHH_OUTH_SET(y)}, // V/H [scaling]
{R_DVP_SP, pclk_div}, // [link-frequency]

That gives us 4 different APIs:

* [`video_set_format()`](https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#gab93c2cb09bf5b0629b665cc4a079e3cd) for configuring `[binning]`

* [`video_set_selection()`](https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#ga21f2e7d6b5ec0c50ceeee580c6272613) with [VIDEO_SEL_TGT_CROP](https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#ggae375c0586e3505632cc69348935c9b54aa42c3de3eeefb5340a2a1877ec8c4b17) for `[cropping]`

* [`video_set_selection()`](https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#ga21f2e7d6b5ec0c50ceeee580c6272613) with [VIDEO_SEL_TGT_COMPOSE](https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#ggae375c0586e3505632cc69348935c9b54a0558ad68bff086cd3ff3f82b53946f49) for `[scaling]`

* [`video_set_ctrl()`](https://docs.zephyrproject.org/latest/doxygen/html/group__video__interface.html#ga1cce17a3dfc881a1080708c7bc417aac) with [VIDEO_CID_LINK_FREQ](https://docs.zephyrproject.org/latest/doxygen/html/group__video__controls.html#ga2142e2819c445b70d82067a3cfb193c8) for the link frequency.

(+) the tiny VPU/ISP inside the image sensor becomes a tool the MCU can make use of (+) simpler driver, pushing all the work to the video subsystem (-) more work on this PR to get there (maintainers can give a hand) (-) APIs are still not completely documented (maintainers can give a hand)

For instance transfer a low-res image (strong binning, good for low-noise), run some image detection on it, and transfer a JPEG image cropped to the detection result at max resolution.

If interested in trying, feel welcome, glad to help with that. Otherwise, I think just a bit of tuning and removing the GPL'ed parts is going to be enough.

Either way, thank you for your efforts on improving OV2640! Small low-power low-cost image sensors can do a lot with good drivers. :) Any bit help, and your current proposal does not increase maintenance burden.

Thank you for your contribution in this PR
Yes, what you are saying makes sense, and I'm definitely interested
So to sum up :

• I address comments regarding resolution constants
• Remove unnecessary changes (like code formatting), or make separate commit out of it
• Add this

ret = ov2640_write_all(dev, uxga_regs, ARRAY_SIZE(uxga_regs));
	if (ret < 0) {
		return ret;
	}

• And most important - remove GPL'ed parts (e.g. reuse FIELD_PREP() macro)

I'll follow up on adding proper API later on
Please correct me if I missed anything
Btw, I have another pr (linked one) hanging around, please take a look
#94536

I have a lot more stuff to commit later on, but just what to handle them one by one
Thank you

[josuah]

• I address comments regarding resolution constants
• Remove unnecessary changes (like code formatting), or make separate commit out of it
• Add this if (ret < 0) { return ret; }
• And most important - remove GPL'ed parts (e.g. reuse FIELD_PREP() macro)

I confirm that's all.
The improvements mentioned are long-term ideals.
I will propose some examples below.

Thank you again!

[josuah]

The XXXX(x) macros are only used once, maybe this can be all put inline here:

#define OV2640_SIZE_REGS(x, y, v_div, h_div, pclk_div)                                         \
	{CTRLI,                                                                                    \
	 CTRLI_LP_DP | FIELD_PREP(CTRLI_V_DIV_MASK, v_div) | FIELD_PREP(CTRLI_H_DIV_MASK, h_div)}, \
	{ZMOW, FIELD_PREP(ZMOW_OUTW_MASK, x)},                                                     \
	{ZMOH, FIELD_PREP(ZMOH_OUTH_MASK, y)},                                                     \
	{ZMHH, FIELD_PREP(ZMHH_OUTW_MASK, x) | FIELD_PREP(ZMHH_OUTH_MASK, y)},                     \
	{R_DVP_SP, pclk_div},                                                                      \
	{RESET, 0x00}

But that's just some minor thing to try to reduce driver-specific macros, nothing blocking.

[cornway]

@josuah

Kindly ask you to review
Thank you

[josuah]

Would it be possible to do these two changes? This might be an occasion to get familiar with the Zephyr doc on contributing PRs to be all set for subsequent PRs:

• In a PR, introducing something is expected to not be fixed by the next commits, so while it's appreciated that you kept the formatting changes separate, it will need to have the right formatting directly for newly contributed code.
• You might need to put different 1st line for the commits, to avoid confusion of which commit does what. Something very short is ok.
• We might need to keep 240x240 for now, or update the various samples using ESP32, as it is the default

Sorry for the late request for change, this PR did stay quite some time without review.

Thank you for your patience and for helping with image sensor driver maintenance!

[josuah]

By using %ux%u here instead of textual name, it becomes possible to save ~60 bytes. Worth considering?

[josuah]

I think this removes the 240x240 resolution used by Espressif to showcase their Zephyr camera support via the ESP32-S3 Eye: https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/drivers/video/capture/boards/esp32s3_eye_procpu.conf#L9-L10

zephyr/samples/drivers/video/capture/boards/esp32s3_eye_procpu.conf

Lines 9 to 10 in 67f2464

	CONFIG_VIDEO_FRAME_HEIGHT=240
	CONFIG_VIDEO_FRAME_WIDTH=240

But maybe that's not a problem if it's for something better.
Is the plan to move all the cropping/windowing support the video_set_selection() API in a latter PR?

If so, it is tempting to only remove the formats once they can be supported again through a different API.

[josuah]

It might work to turn |= into a plain =, now that you added the if (ret < 0) check.

[cornway]

I did my best, and hopefully I satisfied all the requested changes this time 😄
Kindly ask to review once again

[josuah]

This seems to still use the QVGA registers even though the resolution is announced as 240x240.

As a result, the capture sample works with the ESP32 S3 Eye, but instead of looking like this (before):

It looks like this (after):

I first rebased your PR on top of #95958 then ran it with this:

west build -b esp32s3_eye/esp32s3/procpu samples/drivers/video/capture

[cornway]

I see what the problem is
Let me fix it first and I let you know once this is done
Thank you for going through verification

[josuah]

Would it be possible to add a ov2640_240x240_regs that crops the width in the meantime that video_set_selection() is introduced? It helps with getting a working demo of the ESP32 driver.

[EDIT: thanks! I will test again as soon as I get a chance]

[cornway]

Pushed, please take a look
You may need to adjust these prescalers

static const struct ov2640_reg ov2640_240x240_regs[] = {
	OV2640_ZOOM_CONFIG(240, 240, 2, 2, 4),
};

2, 2, 4 if that doesn't work, unfortunately I don't have that board so can't check

Thank you!

[josuah]

About the commit message, here is what I understand:

• The current driver only supports UXGA as base resolution, and was making a combination of cropping and scaling as a way to support an arbitrary list of smaller resolutions (convenient to adapt the image output to the size of a display).
• The new driver does the same thing, but modifies more parameters (such as PCLK divider), which and only uses scaling and not cropping.

Enabling proper zoom/scaling for windowed resolutions; e.g. smaller than uxga.

Here is how I understand that zooming, scaling, and windowing (cropping?) relate (where FoV is Field of View):

Did I understand correctly? Your answer might as well be a good commit message btw.

Thank you!

[cornway]

#> About the commit message, here is what I understand:

* The current driver only supports UXGA as base resolution, and was making a combination of cropping and scaling as a way to support an arbitrary list of smaller resolutions (convenient to adapt the image output to the size of a display).

* The new driver does the same thing, but modifies more parameters (such as `PCLK` divider), which and only uses scaling and not cropping.

Enabling proper zoom/scaling for windowed resolutions; e.g. smaller than uxga.

Here is how I understand that zooming, scaling, and windowing (cropping?) relate (where FoV is Field of View):

Did I understand correctly? Your answer might as well be a good commit message btw.

Thank you!

To the best of my knowledge here - cropping is not implemented properly in that driver, I mean that sensor doesn't have clear description on cropped region registers and I couldn't find anything that shows how to utilize it; That leads to - without proper prescaling/dividing it just crops top left corner not the center as in your drawings
So in this implementation (this PR) - it just does scaling (down scaling exactly), not cropping
Without this PR - it doesn't do any scaling , but just cropping of top left corner of the frame

[cornway]

@josuah
Kindly reminder to review this patch and verify
Thank you 😃

[cornway]

Good day, can anyone take a look at this PR once again please ?
@josuah @dsseng
thank you so much

[dsseng]

I do not have any comments, I'm not on the video maintainer team and just dropped some general C programming tips. If no reviews follow in a week, feel free to use #pr-help on Discord to grab someone's attention

[cornway]

I do not have any comments, I'm not on the video maintainer team and just dropped some general C programming tips. If no reviews follow in a week, feel free to use #pr-help on Discord to grab someone's attention

Thank you
Will do !
Have a nice day

[josuah]

I rebased this on top of main and tested again:

Before: the test circle is not centered even though picture taken in front of the sensor (as you described)

After: the test circle is now an ovale, wrong proportions (limitation where it's not possible to set the offset of the readout insofar

This is a driver/doc/hardware limitation for another.
Though, this is IMHO a big improvement over cropping, in particular for every other format.

More background in a follow-up discussion.

Thanks for the improvements (and for waiting), LGTM!

[josuah]

You will need to rebase on latest main to fix the new jsonschema dependency. This happened on a few PRs.

[cornway]

I rebased this on top of main and tested again:

Before: the test circle is not centered even though picture taken in front of the sensor (as you described)

After: the test circle is now an ovale, wrong proportions (limitation where it's not possible to set the offset of the readout insofar

This is a driver/doc/hardware limitation for another. Though, this is IMHO a big improvement over cropping, in particular for every other format.

More background in a follow-up discussion.

Thanks for the improvements (and for waiting), LGTM!

Thanks, will do rebase
Thank you for verifying that
I have a pile of another improvements, just need this to go first 😄

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[avolmat-st]

Thanks for the update. My understanding is that this will pick a resolution in the list in order to apply it to the sensor and that resolution might not be the exact resolution asked by the application

(checking for width >= and height >= instead of width == / height ==).
In such case, upon a set_fmt, the resolution actually applied might be different from the one requested.
This is ok, however the information should be given back to the application in such case by setting back the new resolution into the set_fmt fmt pointer.

[avolmat-st]

Nit: indentation to fix for this line and below, since currently it makes it hard to easily understand that there is actually 5 registers being set

[cornway]

That was mostly the clang format caused this, I'll try to change it
Thank you

[avolmat-st]

Nit: maybe keep only 1 register setting per line to make it easier to read

[cornway]

That also came from clang-format

[avolmat-st]

(not modified in this patch but since I noticed it I mention it here)

In several places within the driver, the width / height are uint16_t while in video API (and in some function within this driver), this is uint32_t, so I think this should be fixed.
The first issue is within the set_fmt into which a uint32_t is put into a uint16_t.

[cornway]

Thanks for the update. My understanding is that this will pick a resolution in the list in order to apply it to the sensor and that resolution might not be the exact resolution asked by the application

(checking for width >= and height >= instead of width == / height ==). In such case, upon a set_fmt, the resolution actually applied might be different from the one requested. This is ok, however the information should be given back to the application in such case by setting back the new resolution into the set_fmt fmt pointer.

Thank you for pointing that out
Is it ok to push another commit that fixes that, or better to squash it in ?

[avolmat-st]

Thanks for the update. My understanding is that this will pick a resolution in the list in order to apply it to the sensor and that resolution might not be the exact resolution asked by the application
(checking for width >= and height >= instead of width == / height ==). In such case, upon a set_fmt, the resolution actually applied might be different from the one requested. This is ok, however the information should be given back to the application in such case by setting back the new resolution into the set_fmt fmt pointer.

Thank you for pointing that out Is it ok to push another commit that fixes that, or better to squash it in ?

I think it should be together with the modification which lead to having the set format not same as the one requested. This lead to having the behavior modified. So I'd go with doing it in the existing commit.