Implement video control framework #82158

ngphibang · 2024-11-27T18:37:36Z

This PR implements / reworks the video control framework. Moreover, the infrastructure built around it can also be served for futher improvements of the video framework such as pipeline management and things like having the framework do most of the common works on behalf of the drivers.

All the implementation happens inside the framework and for drivers-only usage, no public header added. There is also no API breaking except grouping of control cid and value parameters under a video_control struct. But this is minor and just for a small optimization, not because of the new implementation requirement.

Currently, the set/get_ctrl() APIs in the video framework are defined as below:

static inline int video_get/set_ctrl(const struct device *dev, unsigned int cid, void *value)
{
	const struct video_driver_api *api = (const struct video_driver_api *)dev->api;

	if (api->get/set_ctrl == NULL) {
		return -ENOSYS;
	}

	return api->get/set_ctrl(dev, cid, value);
}

It is the common pattern of other video APIs (e.g. get/set_format) and also other subsystems (e.g. display). This means all the work will be done by drivers, the framework does nothing.

Apart from this, a problem with this API is that, it makes all drivers in a pipeline propagate the get_ctrl() from root to sensor. At the sensor, the control value is obtained by reading directly the registers. While this works for some "volatile" controls like gain, exposure, in most of the cases, reading control value directly from registers are not only inefficient but also sometimes impossible, e.g. controls that need "complex" computation and "scatter" through several registers, e.g. hue control in ov5640 driver.

The purpose of this framework improvement is to have the video framework do as much as possible common works on behalf of drivers as it is the purpose / benefit of having a framework.

The core idea is controls from different devices in a video pipeline need to be cached in memory. Once a device completed its initialization, its controls list is added to the root device of the pipeline (i.e. the device that represents the pipeline to communicate with applications). The framework then uses these information to do get_ctrl(), sanity check, type check, etc.

Tested on ov5640 -> mipicsi2rx -> csi camera pipeline (i.MX RT1170)

This PR depends on #80649 which I cherry-picked here (the first 3 commits).

TODO list

Add get_volatile_ctrl() API for controls that needs to be read freshly from hardware e.g. gain, exposure (in some contexts).
Add control cluster
Add query_ctrl() API to query min, max, default value of a control
Add (more) supports for controls of menu type
Add correct support for controls of int64 type (pixel rate)
Automatically apply set_ctrl() to default values at boot
Fix capture / capture to lvgl samples
Migrate others drivers / boards that have controls to the new video control framework: ov2640 and dcmi on stm32 boards, lcd-cam on ESP32S3 boards, (future) mt9m114 on RT10xx boards.
Move the video framework implementation to drivers/video/core
Add new files to maintenance list
Test on mt9m114 -> csi camera pipeline on NXP RT10xx board.

Add port/endpoint DT helpers so that drivers can retrieve the node id of the interested port/endpoint just by its id. Also, add helpers to retrieve the peer remote device object from the local endpoint. Signed-off-by: Phi Bang Nguyen <[email protected]> Co-developed-by: Josuah Demangeon <[email protected]>

Drop the driver-defined macros. Use the endpoint DT helpers of the video framework instead. Signed-off-by: Phi Bang Nguyen <[email protected]>

Introduce a new video device structure which represents the root device in a video pipeline. A root device is the one that represents the whole pipeline to communicate with the application. It is usually the device assigned to the "zephyr,camera" chosen node. Root device then needs to hold a video device structure and register it with the video framework. Signed-off-by: Phi Bang Nguyen <[email protected]>

Video devices are initialized at different points in the boot process. Each device will hold a video_notifier struct and register it with the framework at its initialization step. This struct contains a list of "children" devices, i.e. source devices that it waits for and a parent device, i.e. "sink" device who waits for it. This way, the framework will have a complete / hierachical view of each video pipeline. Signed-off-by: Phi Bang Nguyen <[email protected]>

Group control's id and value under a video control struct Signed-off-by: Phi Bang Nguyen <[email protected]>

The core idea is controls from different devices in a video pipeline need to be cached in memory. Once a device finished its initialization, its controls list is added to the root device's controls list. The framework then uses these information to do all common works on behalf of drivers including get_ctrl, sanity check, type check, etc. Signed-off-by: Phi Bang Nguyen <[email protected]>

The brightness control value mask is wrong. Fix it. Signed-off-by: Phi Bang Nguyen <[email protected]>

In video_mcux_csi_init_0() function, there are things independant to driver instance. Move them to video_mcux_csi_init(). Signed-off-by: Phi Bang Nguyen <[email protected]>

Migrate drivers that have controls to the new video control framework. - Remove set_ctrl() from video driver APIs. Device drivers don't need and should not implement set_ctrl() anymore. - Device drivers don't need to do sanity checks on control values anymore. - Root device of the pipeline needs to hold a video_device struct and register it with the framework. - Each device will hold a list of its children devices, i.e. the source device it waits for. - Device driver that have controls need to initialize its controls list (handler) at initialization step. - Each device driver in the pipeline needs to hold a video_notifier struct and register it with the framework. Signed-off-by: Phi Bang Nguyen <[email protected]>

decsny · 2024-11-27T18:46:25Z

Is this going to be tested?

jilaypandya · 2024-11-27T19:01:43Z

drivers/video/video_ctrls.h

+
+/*  Control types */
+enum video_ctrl_type {
+	VIDEO_CTRL_TYPE_INTEGER = 1,


This enums have to be documented

I think the enums are already self-explanatory here, no ? The type of the control either an integer or an int64, etc.
Could you give some insights on what we should put here for documentation ?

Have a look over here

zephyr/include/zephyr/drivers/sensor.h

Line 61 in baa49f6

enum sensor_channel {

#82377

All the enums need to be documented even though it might be self-explanatory. Also the enum declaration needs to have doxygen@brief.

jilaypandya · 2024-11-27T19:03:57Z

drivers/video/video_ctrls.h

+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+#ifndef ZEPHYR_INCLUDE_VIDEO_CTRLS_H_


ZEPHYR_INCLUDE_DRIVERS_VIDEO_VIDEO_CTRLS_H , i believe is the correct namespace

yes, will do.

jilaypandya · 2024-11-27T19:12:59Z

drivers/video/video_ctrls.h

+int video_ctrl_init(struct video_ctrl *ctrl, struct video_ctrl_handler *hdl, uint32_t id,
+		    int32_t min, int32_t max, uint32_t step, int32_t def);
+
+struct video_ctrl *video_ctrl_find(struct video_ctrl_handler *hdl, uint32_t id);


shall we return the struct by pointer instead?

jilaypandya · 2024-11-27T19:16:48Z

drivers/video/ov5640.c

 	struct ov5640_reg brightness_params[] = {{SDE_CTRL8_REG, value >= 0 ? 0x01 : 0x09},
-						 {SDE_CTRL7_REG, abs(value) & 0xff}};
+						 {SDE_CTRL7_REG, (abs(value) << 4) & 0xf0}};


Suggested change

{SDE_CTRL7_REG, (abs(value) << 4) & 0xf0}};

{SDE_CTRL7_REG, (abs(value) << BIT(2)) & GENMASK(15,8)}};

I believe you mean (abs(value) << BIT(4)) & GENMASK(7,4).

Could you explain the benefit of using these macros here ?

IMO, GENMASK() will help if we need to generate something dynamically that is only known at runtime like GENMASK(x, y) or in case it is difficult to do the calculation, e.g. GENMASK(63, 51). Here, the mask is very clear and simple, we just put 0xf0 why have to pass by this macro ?

I believe you mean (abs(value) << BIT(4)) & GENMASK(7,4).

yep i meant GENMAST(7,4). Would it not be more readable, to replace (abs(value) << BIT(4)) & GENMASK(7,4) with some #define? Yes you are right 0xf0 is pretty straightforward,

jilaypandya · 2024-11-27T19:24:17Z

drivers/video/video_async.c

+		/* Find the parent notifier in the global notifiers registry */
+		if (!found_parent) {
+			for (i = 0; i < nf->children_num; ++i) {
+				if (nf->children_devs[i] == notifier->dev) {


Trying to reduce nested ifs :)

Suggested change

if (nf->children_devs[i] == notifier->dev) {

if (nf->children_devs[i] != notifier->dev) {

continue;

}

decsny · 2024-11-27T19:52:17Z

include/zephyr/drivers/video.h

+#define DT_REMOTE_DEVICE_NO_PORTS(ep)                                                              \
+	DT_GPARENT(DT_NODELABEL(DT_STRING_TOKEN(ep, remote_endpoint_label)))
+
+#define DEVICE_DT_GET_REMOTE_DEVICE(ep)                                                            \


shouldn't these be in include/zephyr/devicetree and cases added to devicetree tests?

This is cherry-picked from #80649. Could we discuss there, maybe you can give us some insights ?

In fact, we 'd like to do so but the problem is these macros use remote_endpoint_label property which existes only in video-interfaces binding. If we move them into Zephyr devicetree.h, we need to define remote_endpoint_label system-wide but we don't know how to do that.

decsny · 2024-11-27T19:53:31Z

drivers/video/video_mcux_csi.c

@@ -502,7 +492,7 @@ PINCTRL_DT_INST_DEFINE(0);

 static const struct video_mcux_csi_config video_mcux_csi_config_0 = {
 	.base = (CSI_Type *)DT_INST_REG_ADDR(0),
-	.source_dev = DEVICE_DT_INST_GET_SOURCE_DEV(0),
+	.source_dev = DEVICE_DT_GET_REMOTE_DEVICE(DT_INST_ENDPOINT_BY_ID(0, 0, 0)),


just make these changes to use the helpers in one commit , you dont need one commit per file when they are all doing the same thing , its a mass change

decsny · 2024-11-27T19:55:43Z

include/zephyr/drivers/video-controls.h

@@ -161,6 +161,11 @@ enum video_power_line_frequency {
 * @}
 */

+struct video_control {


these four lines also don't seem to need their own commit, just add it in the commit adding the rest of the video controls code

decsny · 2024-11-27T19:57:04Z

drivers/video/ov5640.c

@@ -743,12 +743,12 @@ static int ov5640_set_ctrl_brightness(const struct device *dev, int value)
 {
 	const struct ov5640_config *cfg = dev->config;

-	if (!IN_RANGE(value, -UINT8_MAX, UINT8_MAX)) {
+	if (!IN_RANGE(value, -15, 15)) {


these brightness values fix seem unrelated to rest of the PR

decsny · 2024-11-27T19:57:54Z

drivers/video/video_mcux_csi.c

@@ -396,6 +396,8 @@ static int video_mcux_csi_init(const struct device *dev)
 	struct video_mcux_csi_data *data = dev->data;
 	int err;

+	data->dev = dev;


this change also seems unrelated to the rest of the PR also, typo in commit message (independant -> independent)

decsny · 2024-11-27T19:59:22Z

drivers/video/video_async.h

is this supposed to be a public header ?

No, there are no public header in this PR. They are all internal, for driver usage only.

josuah · 2024-11-27T23:15:36Z

I like the direction it is going! Extending the video APIs is mandatory to support some hardware. It also could benefit from reusing mechanisms already present in Zephyr to make it fit more naturally (presentation below). Unless I missed some requirements or goals.

Here is how I understand it:

Currently every driver stores their const struct device *source_dev somewhere locally in their struct xxx_config without a standard way to access or declare these pointers.

AFAIU, your proposal is to register source_dev along with other related structs into iterable sections that can be looped through to navigate this graph of video devices at runtime:

video_async_init(&data->notifier, dev, &data->vdev, NULL, config->source_devs, 1);

Going from a struct device * to a struct video_notifier * requires looping through all the struct video_notifiers * registered as there is no standard offset for the struct video_notifier *.

It seems like grouping all of these related structures into just one struct video_data accessible like this would remove the need to have all these accessor, register, ... functions like what USB UDC drivers do:

struct video_data *data = dev->data;
struct xxx_data = data->priv;

For the Network Ethernet drivers, it seems like being solved with helper functions:
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/net/ethernet.h#L583-L584

struct video_data *data = get_video_data(dev);

It looked like this could avoid any iterable sections this way.

Was it something you considered and discarded the idea? If so, I'd be curious to learn why as maybe I missed something important in this.

I will continue the review when I get a chance. Thank you!

josuah · 2024-11-27T23:32:28Z

Is this going to be tested?

There is a fake MIPI RX driver and fake imager made for this user-case of unit-testing API changes:

video: emul: virtual driver for an imager and RX engine #79482

These are "fake" but actually produce frames, so can be tested in hardware too.

drivers/video/ov5640.c

josuah · 2024-11-28T19:56:56Z

This PR could also provide some way to list all existing video devices via DEVICE_API_IS(video, dev):

ngphibang · 2024-11-28T20:22:29Z

This PR could also provide some way to list all existing video devices via DEVICE_API_IS(video, dev):

#82102
#71773

I will rebase this PR once the video API is moved to use DEVICE_API but I don't think this PR is related to that one.

josuah · 2024-11-29T02:38:28Z

drivers/video/video_async.c

+/* Find a registered notifier associated with this device */
+struct video_notifier *video_async_find_notifier(const struct device *dev)
+{
+	struct video_notifier *nf;
+
+	SYS_SLIST_FOR_EACH_CONTAINER(&notifiers_list, nf, node) {
+		if (nf->dev == dev) {
+			return nf;
+		}
+	}
+
+	return NULL;
+}


I would be in favor of a struct video_device_data to avoid any notifiers_list:

struct video_device_data *vdata = dev->data; struct video_notifier *nf = &vdata->notifier; struct thisdriver_data *data = vdata->data;

Is there anywhere where this would not work?

josuah · 2024-11-29T03:18:42Z

drivers/video/video_device.c

+struct video_device *video_find_vdev(const struct device *dev)
+{
+	for (uint8_t i = 0; i < VIDEO_NUM_DEVICES; ++i) {
+		if (video_devices[i] != NULL && video_devices[i]->dev == dev) {
+			return video_devices[i];
+		}
+	}
+
+	return NULL;
+}


Same as here, I think this can be removed altogether if it is possible to have a vdata = dev->data pointer.

josuah · 2024-11-29T03:24:37Z

drivers/video/video_async.c

+
+LOG_MODULE_REGISTER(video_async, CONFIG_VIDEO_LOG_LEVEL);
+
+static sys_slist_t notifiers_list;


I think some of this can be replaced by this new device API.

Each of Zephyr struct device *dev is on a static array, and it is possible to get that array at runtime with z_device_get_all_static().

Combined with this new API that allows to check which API a device belongs to, it is already possible to loop through all video devices.

Would that allow to remove some/all looping mechanisms and list to maintain?

drivers/video/video_ctrls.c

josuah · 2024-11-29T03:37:12Z

drivers/video/video_ctrls.c

+	if (control->val < ctrl->min || control->val > ctrl->max) {
+		LOG_ERR("Control value is invalid\n");
+		return -EINVAL;
+	}


Is that not checked during registration? Can this change at runtime? If so, that can become a TOCTOU issue if a context switch happens right after this line?

josuah · 2024-11-29T03:47:19Z

drivers/video/video_ctrls.c

+	default:
+		*type = VIDEO_CTRL_TYPE_INTEGER;
+		break;


I like the fact that controls are integer by default. This means only exceptions have to be maintained, and VIDEO_CTRL_TYPE_INTEGER fits most things.

loicpoulain · 2024-12-05T20:16:34Z

drivers/video/video_device.h

+	const struct device *dev;
+};
+
+int video_register_vdev(struct video_device *vdev, const struct device *dev);


could it simply be video_device_register/unregister()?

I followed the naming pattern with the prefix video_dosomething() but yes we can simplify it as your suggestion.

ngphibang added 10 commits November 27, 2024 18:41

drivers: video: mipi_csi2rx: Use endpoint DT helpers

8b3c3c5

Drop the driver-defined macros. Use the endpoint DT helpers of the video framework instead. Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: csi: Use endpoint DT helpers

b98753e

Drop the driver-defined macros. Use the endpoint DT helpers of the video framework instead. Signed-off-by: Phi Bang Nguyen <[email protected]>

include: video-controls: Add video_control struct

a7ea463

Group control's id and value under a video control struct Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: ov5640: Fix brightness control

356e98b

The brightness control value mask is wrong. Fix it. Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: csi: Move instance-independant stuffs to common init

1583d64

In video_mcux_csi_init_0() function, there are things independant to driver instance. Move them to video_mcux_csi_init(). Signed-off-by: Phi Bang Nguyen <[email protected]>

ngphibang requested review from mmahadevan108 and dleach02 as code owners November 27, 2024 18:37

zephyrbot added platform: NXP Drivers NXP Semiconductors, drivers area: Video Video subsystem labels Nov 27, 2024

zephyrbot requested review from danieldegrasse, dbaluta, decsny, josuah, loicpoulain, manuargue and MarkWangChinese November 27, 2024 18:38

zephyrbot assigned dleach02 and mmahadevan108 Nov 27, 2024

jilaypandya reviewed Nov 27, 2024

View reviewed changes

decsny reviewed Nov 27, 2024

View reviewed changes

josuah added Enhancement Changes/Updates/Additions to existing features RFC Request For Comments: want input from the community Release Notes Required Release notes required for this change labels Nov 27, 2024

josuah reviewed Nov 28, 2024

View reviewed changes

drivers/video/ov5640.c Show resolved Hide resolved

ngphibang changed the title ~~Implement video control framework and more~~ Implement video control framework Nov 28, 2024

ngphibang mentioned this pull request Nov 29, 2024

include: devicetree: Add port and endpoint macros #80649

Merged

josuah reviewed Nov 29, 2024

View reviewed changes

drivers/video/video_ctrls.c Show resolved Hide resolved

josuah reviewed Nov 29, 2024

View reviewed changes

josuah mentioned this pull request Nov 29, 2024

drivers: video: introduce "GET" sub-operations #78603

Closed

loicpoulain reviewed Dec 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement video control framework #82158

Implement video control framework #82158

ngphibang commented Nov 27, 2024 •

edited

Loading

decsny commented Nov 27, 2024

jilaypandya Nov 27, 2024

ngphibang Nov 29, 2024

jilaypandya Dec 1, 2024

jilaypandya Nov 27, 2024

ngphibang Nov 29, 2024

jilaypandya Nov 27, 2024

jilaypandya Nov 27, 2024

ngphibang Nov 29, 2024 •

edited

Loading

jilaypandya Dec 1, 2024

jilaypandya Nov 27, 2024

decsny Nov 27, 2024

ngphibang Nov 29, 2024

decsny Nov 27, 2024

decsny Nov 27, 2024

decsny Nov 27, 2024

decsny Nov 27, 2024

decsny Nov 27, 2024

ngphibang Nov 29, 2024

josuah commented Nov 27, 2024 •

edited

Loading

josuah commented Nov 27, 2024 •

edited

Loading

josuah commented Nov 28, 2024

ngphibang commented Nov 28, 2024

josuah Nov 29, 2024

josuah Nov 29, 2024 •

edited

Loading

josuah Nov 29, 2024 •

edited

Loading

josuah Nov 29, 2024 •

edited

Loading

josuah Nov 29, 2024

loicpoulain Dec 5, 2024

ngphibang Dec 5, 2024

	{SDE_CTRL7_REG, (abs(value) << 4) & 0xf0}};
	{SDE_CTRL7_REG, (abs(value) << BIT(2)) & GENMASK(15,8)}};

-				if (nf->children_devs[i] == notifier->dev) {
+				if (nf->children_devs[i] != notifier->dev) {
+				    continue;
+				}


		LOG_MODULE_REGISTER(video_async, CONFIG_VIDEO_LOG_LEVEL);

		static sys_slist_t notifiers_list;

Implement video control framework #82158

Are you sure you want to change the base?

Implement video control framework #82158

Conversation

ngphibang commented Nov 27, 2024 • edited Loading

decsny commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Nov 27, 2024 • edited Loading

josuah commented Nov 27, 2024 • edited Loading

josuah commented Nov 28, 2024

ngphibang commented Nov 28, 2024

Choose a reason for hiding this comment

josuah Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

josuah Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

josuah Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngphibang commented Nov 27, 2024 •

edited

Loading

ngphibang Nov 29, 2024 •

edited

Loading

josuah commented Nov 27, 2024 •

edited

Loading

josuah commented Nov 27, 2024 •

edited

Loading

josuah Nov 29, 2024 •

edited

Loading

josuah Nov 29, 2024 •

edited

Loading

josuah Nov 29, 2024 •

edited

Loading