drivers: video: ov7670: Set default format to RGB565 QVGA

[ngphibang]

The default format for ov7670 is currently VGA YUYV and it counts on the smartdma to reset the format to RGB565 QVGA when get_format() is called.

Recently, set_format() is decoupled from get_format() so this assumption is nolonger correct.

Set the default format to RGB565 QVGA instead.

[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

[josuah]

SmartDMA seems like the main user of this sensor so far.

Maybe other users will want a different default format at some point but RGB565 makes it work with most displays out of the box, and a small resolution is a good starting point too try to get it to work in most case (low memory requirement).

[avolmat-st]

Thanks for the patch. I understand that this seems to be done so that the smartdma interface get_format will not return -ENOTSUP when being requested by the application. Is that right ?
While this work in this case, especially since this sensor is apparently only used with this interface, it seems to me that normally such change shouldn't be done and normally the application (or video api user) should query the formats and pick one if this is not the want it wants to use.

Maybe it would be better to open an issue to clean this get_fmt, get_caps mechanism and have sample apps be doing state of the art configuration so that other applications can use them as reference.

Back to this get_fmt, my understanding is that this should be reflecting the current format, which should be a valid one for that device. So, considering from the interface point of view, it means that whenever the interface is being requested for a format, it should return something actually feasible. Normally a format setting is always validated, hence upon a set_format the format returned by get_fmt is always going to be valid.
However on the first call (prior any set_fmt), the get_fmt should probably gather information from the device before (here the sensor) and figure out, probably based on its caps, the format to pick which is acceptable by both the sensor and himself. That format would be set to the sensor AND applied as well to the interface. Then it would be returned to the get_fmt api caller as the current format. (this is only done on the first get_fmt actually).
We could think about doing this at init time but then we will probably hit initialization order issue, with the sensor device not yet being ready when the interface init (and I am not sure there is a xlate mechanism in Zephyr ... might not be necessary anyway).

Regarding the caps, currently it is done by sharing const table of formats, which is ok for interface which does not impact the format, but don't really work for device able to transform data (ex: dcmipp). Maybe another approach, similar to the frmival, index based retrieval of a caps would be more appropriate since it would allow to modify or even skip the entry given by the sensor if it is not applicable on that interface.

Probably I could propose some patches in those area.

[ngphibang]

I understand that this seems to be done so that the smartdma interface get_format will not return -ENOTSUP when being requested by the application. Is that right ?

Right

While this work in this case, especially since this sensor is apparently only used with this interface, it seems to me that normally such change shouldn't be done and normally the application (or video api user) should query the formats and pick one if this is not the want it wants to use.

Well, normally, user should query the caps of the camera pipeline, choose a format supported in the caps and set the format they want between those.

In Zephyr samples (in general) and the video sample in particular, we want something work "out of the box" that's why we set a default format that is supported for a "chosen" board (chosen "by default" in the video sample, e.g. ov5640 for rt1170). Then in the sample, we get this default format and set it again to the pipeline. This is just for convenience.

We only have one default format and cannot satisfy everyone, so if there are other boards that want other formats from the same sensor, we can set the format (and resolution) in the board config file in the sample with

• CONFIG_VIDEO_PIXEL_FORMAT, CONFIG_VIDEO_FRAME_WIDTH, CONFIG_VIDEO_FRAME_HEIGHT

https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/drivers/video/capture/src/main.c#L154-#L164

This is simple and working currently.

In Linux, there are try_format() (and try_frmival, etc.) mechanism looks like what you said. It's fine to me too (but more complicated)

[ngphibang]

For ov7670, since there is no other boards (in the sample) that wants to set the default format other than RGB565 QVGA, I took this. If not, I can set it in the board config file.

Anyways, the frdm mcxn947 needs this format otherwise, the camera feature is nolonger working on this board with the video sample.

[avolmat-st]

I understand and this is fine to do it that way (changing the default) now.

For me, the CONFIG_VIDEO_PIXEL_FORMAT is related to what the application is trying to capture. Here the main issue is related to what is returned (or not in the current case) when we request a format to the interface (smartdma in this example).
Because the default format of the sensor is not something supported by the interface (smartdma), the smartdma is returning NOTSUP to the application.

In such situation, either we say that:

• NOTSUP is actually not really an error, it simply means that we cannot return a valid format and thus we can't return the value, but still, the application MUST in this case perform a set_format afterward
  this is not the case in our sample app, since currently upon receiving an error on get_format the sample app will stop

or

• at the interface (smartdma) level, the driver (not the application) should figure out a common format itself and set both sensor and himself so that it is capable to return a valid format

Yet another approach would be to say in the doc that depending on the configuration / capabilities of the sensor and interfaces, the get_format, at startup might return an error but even in such situation the application should not give up and try to set a format via set_fmt (well, in fact this is kind of the 1 case)

[ngphibang]

Yes, this is the weak point of the current get/set_fotmat(). It is weird that get_fmt() return an error. That's why I think the 2nd approach should be implemented, as in Linux, format should be "tried" and negotiated along the pipeline to return a valid format for the whole pipeline.

Probably I could propose some patches in those area.

Nice to see this :-)

[avolmat-st]

Good, thanks, I also think the same.

[josuah]

@iabdalkader I do not mean to spam, but I noticed you committed to that sensor.
Just to let you know of the change of default value.

[iabdalkader]

@iabdalkader I do not mean to spam, but I noticed you committed to that sensor.
Just to let you know of the change of default value.

Thanks! I only used it for testing, it was never enabled by default for the Giga as far as I know. Even if it is used in the future, I think the user should set the format explicitly.