drivers: video: controls: rename all CIDs to Linux counterpart #78802

josuah · 2024-09-21T20:21:27Z

This aims to improve compatibility between Linux and Zephyr sources in order to facilitate debugging and implementing solution that covers both system, such as protocols to control video systems.

The CIDs numbers are expected to be exaactly the same to further help debugging.

The existing CIDs are kept as compatibility defines to the new V4L2-style names.

josuah · 2024-09-21T23:24:58Z

I'm happy with applying clang-format too, but that means new controls submission will change many lines for alignment.

decsny · 2024-09-23T17:39:27Z

@ngphibang please help review

include/zephyr/drivers/video-controls.h

ngphibang

LGTM except a minor remark on the organization of the two last commits. Can we squash them ?

include/zephyr/drivers/video-controls.h

ngphibang · 2024-10-03T09:38:14Z

include/zephyr/drivers/video-controls.h

I think these should not be introduced in the previous commit and then removed here.
I think this commit can be squashed withthe previous one ?

I tried to decompose the steps as much as possible, and give a safe backup point to fall back to, but that's also very much possible to fallback to before the PR merge!

ngphibang · 2024-10-03T09:41:36Z

include/zephyr/drivers/video-controls.h

I understand that you add these macros in this commit and then remove them in the next commit to keep the bisectability (buildable). But it's simpler if we merge this commit with the next one, no ?

include/zephyr/drivers/video-controls.h

josuah · 2024-10-13T22:18:27Z

I picked the opportunity to change the documentation style, making enums in the middle of the CID list hopefully less intrusive, and make it easier to insert new CIDs (i.e. no alignment change needed).

Let me know if it was better before, easy for me to revert.

ngphibang · 2024-11-19T10:22:22Z

include/zephyr/drivers/video-controls.h

The video-controls.h only defines CID macros. It uses nothing from device.h, kernel.h, etc. Although it's not in this PR scope but I recommend to drop all included headers.

Good point! Instead of shuffling these headers, I removed them.
This might have been code added and removed later (refactoring).

ngphibang · 2024-11-25T08:43:32Z

@kartben Could you revisit this PR ? Thanks.

ngphibang · 2024-11-26T18:02:08Z

include/zephyr/drivers/video-controls.h

I just rechecked this VIDEO_CID_VENDOR_CLASS_BASE. In fact, this is Zephyr-specific and does not exist in Linux.
Its equivalent counterpart in Linux should be V4L2_CID_PRIVATE_BASE defined here. Could you change to this one (I also need this CID for the upcomming control framework PR) ? Thanks !

PS: Otherwise, I can change it in my PR just by a commit (it seems simpler this way)

Ah right, in include/uapi/linux/videodev2.h I did not check there!

/* User-class control IDs defined by V4L2 */ #define V4L2_CID_MAX_CTRLS 1024 /* IDs reserved for driver specific controls */ #define V4L2_CID_PRIVATE_BASE 0x08000000

I pushed the change.

ngphibang

VIDEO_CID_VENDOR_CLASS_BASE need to be changed to VIDEO_CID_PRIVATE_BASE to be aligned with Linux

The V4L2 and V4Z list of control IDs are slightly different. Use the same CIDs as Linux, with the exact same numbers to facilitate debug and co-development. This also rename the GENERIC class into BASE as in Linux. No change in the video CIDs names in this commit. Signed-off-by: Josuah Demangeon <[email protected]>

Refers to the CID numbers found in the Linux Kernel so that each CID of Zephyr matches the CIDs of Linux. The new CIDs introduced in Zephyr can then follow the same number and naming found on Linux. No renaming of the CID macro names is done yet. Signed-off-by: Josuah Demangeon <[email protected]>

Now that Control IDs have different classes, sort them to their apropriate classes sections. Signed-off-by: Josuah Demangeon <[email protected]>

This uses Linux V4L2 controls as a reference to give names to the CIDs. Apply the renaming down to the drivers that use them. Signed-off-by: Josuah Demangeon <[email protected]>

Make controls more readable and intuitive to extend, with definition of each control value to help disambiguate between similar controls, such as BRIGHTNESS vs GAIN. Move the class definition to the top of each relevant class, like it is on Linux for the V4L2_CID_..._CLASS_BASE variables. Signed-off-by: Josuah Demangeon <[email protected]>

The video CIDs got changed, and users will be impacted and need to rename some constants. Offer a strategy for finding the new names in the migration guide for the upcoming release. Signed-off-by: Josuah Demangeon <[email protected]>

josuah · 2024-11-26T22:27:49Z

force-push:

Update the VENDOR class to PRIVATE like in Linux.
Rebase on latest main

kartben · 2024-11-20T15:09:57Z

doc/releases/migration-guide-4.1.rst

Suggested change

In most cases, removing the category prefix is enough: ``VIDEO_CID_CAMERA_GAIN`` becomes

In most cases, removing the category prefix is enough, e.g. ``VIDEO_CID_CAMERA_GAIN`` becomes

kartben · 2024-11-27T09:03:57Z

include/zephyr/drivers/video-controls.h

+/** Amount of perceived light of the image, the luma (Y') value. */
+#define VIDEO_CID_BRIGHTNESS (VIDEO_CID_BASE + 0)
+
+/** Amount of difference between the bright colors and dark colors. */
+#define VIDEO_CID_CONTRAST (VIDEO_CID_BASE + 1)
+
+/** Colorfulness of the image while preserving its brightness */
+#define VIDEO_CID_SATURATION (VIDEO_CID_BASE + 2)
+
+/** Shift in the tint of every colors, clockwise in a RGB color wheel */
+#define VIDEO_CID_HUE (VIDEO_CID_BASE + 3)
+
+/** Amount of time an image sensor is exposed to light, affecting the brightness */
+#define VIDEO_CID_EXPOSURE (VIDEO_CID_BASE + 17)


I appreciate the effort to make sure all public API is documented but FWIW I find these oddly worded for some reason. I guess that's what happens when the names are already mostly self-explanatory and one doesn't want to literally paraphrase them in the comment. Not blocking though :)

Yes, I have same thoughts too and sometimes, the explanation is not totally correct as it is difficult to explain in short words, so it is better to not explain, developers who actually use these CID need to understand them, I think :-). But it's not a blocking point.

Reading them again, these do not help understanding what the control do.

Better not risk defining them slightly differently than Linux ones.
If there need to be any doc effort, this can go in Linux.

Frequent issue of newbies: they reinvent the wheel.

New PR to address these small changes. Thank you for the feedback . :)

How about a link to this page for reference?
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html

Yes, I think it's a good idea but I am not sure if it's OK in Zephyr to refer to documentation of a different project.

josuah added area: Drivers area: Video Video subsystem labels Sep 21, 2024

zephyrbot requested a review from loicpoulain September 21, 2024 20:21

josuah mentioned this pull request Sep 21, 2024

Add controls ov5640 #76144

Merged

josuah requested a review from decsny September 21, 2024 20:29

josuah force-pushed the pr-video-controls-cid-update branch from ef5cb5c to ad81bdc Compare September 21, 2024 20:54

loicpoulain previously approved these changes Sep 30, 2024

View reviewed changes

ngphibang reviewed Oct 1, 2024

View reviewed changes

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

josuah force-pushed the pr-video-controls-cid-update branch from ad81bdc to b8c33b8 Compare October 1, 2024 15:34

josuah dismissed loicpoulain’s stale review via 0d70c17 October 1, 2024 15:49

josuah force-pushed the pr-video-controls-cid-update branch from b8c33b8 to 0d70c17 Compare October 1, 2024 15:49

josuah requested a review from ngphibang October 1, 2024 15:59

ngphibang reviewed Oct 3, 2024

View reviewed changes

ngphibang previously approved these changes Oct 3, 2024

View reviewed changes

josuah added the DNM This PR should not be merged (Do Not Merge) label Oct 3, 2024

josuah force-pushed the pr-video-controls-cid-update branch from 0d70c17 to b5f04d0 Compare October 3, 2024 12:46

josuah removed the DNM This PR should not be merged (Do Not Merge) label Oct 3, 2024

josuah requested a review from loicpoulain October 3, 2024 15:21

ngphibang reviewed Oct 10, 2024

View reviewed changes

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

josuah marked this pull request as draft October 11, 2024 07:38

josuah force-pushed the pr-video-controls-cid-update branch from b5f04d0 to 3a583c3 Compare October 13, 2024 16:39

josuah dismissed ngphibang’s stale review via 8d1464a October 13, 2024 17:32

josuah force-pushed the pr-video-controls-cid-update branch 2 times, most recently from 8d1464a to 63e2a69 Compare October 13, 2024 22:13

josuah marked this pull request as ready for review October 13, 2024 22:20

josuah force-pushed the pr-video-controls-cid-update branch from 63e2a69 to 02e23f2 Compare October 13, 2024 22:21

josuah force-pushed the pr-video-controls-cid-update branch from ab47c24 to 86049bf Compare November 19, 2024 00:50

zephyrbot added the Release Notes To be mentioned in the release notes label Nov 19, 2024

zephyrbot requested review from dkalowsk and mmahadevan108 November 19, 2024 00:51

josuah force-pushed the pr-video-controls-cid-update branch 2 times, most recently from 37086a1 to a5c88d0 Compare November 19, 2024 01:14

ngphibang reviewed Nov 19, 2024

View reviewed changes

josuah force-pushed the pr-video-controls-cid-update branch from a5c88d0 to aad1e7c Compare November 19, 2024 13:38

josuah removed the Release Notes Required Release notes required for this change label Nov 19, 2024

ngphibang approved these changes Nov 20, 2024

View reviewed changes

josuah requested review from kartben and loicpoulain November 20, 2024 10:05

kartben mentioned this pull request Nov 25, 2024

drivers: video: mt9m114: Add vertical and horizontal flip control #81830

Merged

ngphibang reviewed Nov 26, 2024

View reviewed changes

ngphibang requested changes Nov 26, 2024

View reviewed changes

josuah force-pushed the pr-video-controls-cid-update branch from aad1e7c to 1bafaf8 Compare November 26, 2024 22:25

josuah added 6 commits November 26, 2024 23:25

drivers: video: controls: Sort controls according to their class

b34d97a

Now that Control IDs have different classes, sort them to their apropriate classes sections. Signed-off-by: Josuah Demangeon <[email protected]>

drivers: video: controls: rename controls to Linux-like names

b41d529

This uses Linux V4L2 controls as a reference to give names to the CIDs. Apply the renaming down to the drivers that use them. Signed-off-by: Josuah Demangeon <[email protected]>

zephyrbot requested a review from fabiobaltieri November 26, 2024 22:25

josuah force-pushed the pr-video-controls-cid-update branch from 1bafaf8 to 845d408 Compare November 26, 2024 22:27

ngphibang approved these changes Nov 27, 2024

View reviewed changes

kartben approved these changes Nov 27, 2024

View reviewed changes

kartben merged commit 5be4aed into zephyrproject-rtos:main Nov 27, 2024
24 checks passed

josuah deleted the pr-video-controls-cid-update branch November 27, 2024 17:44

	In most cases, removing the category prefix is enough: ``VIDEO_CID_CAMERA_GAIN`` becomes
	In most cases, removing the category prefix is enough, e.g. ``VIDEO_CID_CAMERA_GAIN`` becomes

drivers: video: controls: rename all CIDs to Linux counterpart #78802

drivers: video: controls: rename all CIDs to Linux counterpart #78802

Uh oh!

Conversation

josuah commented Sep 21, 2024

Uh oh!

josuah commented Sep 21, 2024

Uh oh!

decsny commented Sep 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngphibang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngphibang Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josuah commented Oct 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang commented Nov 25, 2024

Uh oh!

ngphibang Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah commented Nov 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngphibang Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josuah Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ngphibang Oct 3, 2024 •

edited

Loading

ngphibang Nov 26, 2024 •

edited

Loading

ngphibang left a comment •

edited

Loading

ngphibang Nov 27, 2024 •

edited

Loading

josuah Nov 27, 2024 •

edited

Loading