Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

josuah
Copy link
Collaborator

@josuah josuah commented Sep 18, 2024

An issue faced while working on UVC: how to know the min/max value of a video control ID (CID)?
The UVC protocol supports GET_CUR, GET_DEF, GET_MIN, GET_MAX, GET_RES operation to requst a value whose storage length is given by GET_LEN.

The video_get_ctrl() API permits to retrieve a value from a device using standard CIDs from <zephyr/drivers/video-controls.h>. The CIDs do not define standard min/max values. This means knowing what value to apply needs knowing the particular sensor before-hand.

This commit introduces extra flags added to the CIDs asking for the current, minimum, maximum, or default value, with the same type.

The GET_CUR operation having a flag of 0x00, this makes all drivers implicitly support this new API, with an opt-in migration to also support the extra controls, correctly rejecting the unsupported extra operations by default.

Is this a reasonable way to solve this?

@josuah josuah added area: Drivers RFC Request For Comments: want input from the community area: Video Video subsystem labels Sep 18, 2024
@josuah josuah marked this pull request as draft September 18, 2024 00:52
@josuah
Copy link
Collaborator Author

josuah commented Sep 18, 2024

There are various ways that the controls can be implemented in the drivers, such as the image sensors:

Split functions for every controls.

static int sensor_get_ctrl_exposure(const struct device *dev, uint32_t cid, uint32_t *value)
{
        const struct sensor_config *cfg = dev->config;
        uint16_t u16;
        int ret;

        switch (cid & VIDEO_CTRL_GET_MASK) {
        case VIDEO_CTRL_GET_CUR:
                ret = sensor_read_reg(&cfg->i2c, SENSOR_EXPOSURE_REG, &u16, 2);
                *value = u16;
                return ret;
        case VIDEO_CTRL_GET_MIN:
                *(uint32_t *)value = 0;
                return 0;
        case VIDEO_CTRL_GET_MAX:
                *(uint32_t *)value = 320;
                return 0;
        case VIDEO_CTRL_GET_DEF:
                *(uint32_t *)value = 9;
                return 0;
        default:
                return -EINVAL;
        }
}

static int sensor_get_ctrl(const struct device *dev, unsigned int cid, void *value)
{
        switch (cid & ~VIDEO_GET_MASK) {
        case VIDEO_CID_CAMERA_EXPOSURE:
                return sensor_get_ctrl_exposure(dev, cid, value);
        default:
                return -ENOTSUP;
        }
}

Everything inline.

static int sensor_get_ctrl(const struct device *dev, unsigned int cid, void *value)
{
        const struct sensor_config *cfg = dev->config;
        uint16_t u16;
        int ret;

        switch (cid) {
        case VIDEO_CTRL_GET_CUR | VIDEO_CID_CAMERA_EXPOSURE:
                ret = sensor_read_reg(&cfg->i2c, SENSOR_EXPOSURE_REG, &u16, 2);
                *(uint32_t)value = u16;
                return ret;
        case VIDEO_CTRL_GET_MIN | VIDEO_CID_CAMERA_EXPOSURE:
                *(uint32_t *)value = 0;
                return 0;
        case VIDEO_CTRL_GET_MAX | VIDEO_CID_CAMERA_EXPOSURE:
                *(uint32_t *)value = 320;
                return 0;
        case VIDEO_CTRL_GET_DEF | VIDEO_CID_CAMERA_EXPOSURE:
                *(uint32_t *)value = 9;
                return 0;
        default:
                return -ENOTSUP;
        }
}

Helper function specifying the min/max/def.

static int video_ctrl_range_u32(unsigned int cid, void *value, uint32_t min, uint32_t max, uint32_t def)
{
        switch (cid & VIDEO_CTRL_GET_MASK) {
        case VIDEO_CTRL_GET_MIN:
                *(uint32_t *)value = min;
                return 0;
        case VIDEO_CTRL_GET_MAX:
                *(uint32_t *)value = max;
                return 0;
        case VIDEO_CTRL_GET_DEF:
                *(uint32_t *)value = def;
                return 0;
        case VIDEO_CTRL_GET_CUR:
                return 1;
        default:
                return -ENOTSUP;
        }
}

static int sensor_get_ctrl(const struct device *dev, unsigned int cid, void *value)
{
        uint16_t u16;
        int ret;

        switch (cid & ~VIDEO_CTRL_GET_MASK) {
        case VIDEO_CID_CAMERA_EXPOSURE:
                ret = video_get_ctrl_range(cid, value, 0, 320, 9);
                if (ret == 1) {
                        ret = sensor_read_reg(&cfg->i2c, SENSOR_EXPOSURE_REG, &u16, 2);
                        *(uint32_t)value = u16;
                }
                return ret;
        default:
                return -ENOTSUP;
        }
}

@josuah
Copy link
Collaborator Author

josuah commented Sep 21, 2024

It is possible to know the default value by probing the systems at startup and storing the default value.
Then it is not required for every sensor to support a GET_DEF operation: only a min and max.
A compromise to make I suppose...

@josuah
Copy link
Collaborator Author

josuah commented Sep 21, 2024

The way Linux does this is to initialize new controls, which likely means memory allocation:
https://git.kernel.org//pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/imx415.c#n819

The last code block above with sensor_get_ctrl(const struct device *dev, unsigned int cid, void *value, uint32_t min, uint32_t max, uint32_t def) seems relatively close to Linux's v4l2_ctrl_new().

Maybe adding a set of "CID wrappers" like this could help simplifying the min/max handling while not introducing any memory allocation or involve a complete subsystem?

/* Standard helpers for simple situations fitting most controls */
int video_ctrl_range_u32(const struct device *dev, unsigned int cid, void *value,
                         uint32_t min, uint32_t max, uint32_t def);
int video_ctrl_range_i32(const struct device *dev, unsigned int cid, void *value,
                         int32_t min, int32_t max, int32_t def);
int video_ctrl_range_enum(const struct device *dev, unsigned int cid, void *value,
                          int min, int max, int def);

/* Customized helpers for customized structures */
int video_ctrl_range_roi(const struct device *dev, unsigned int cid, void *value,
                         struct video_ctrl_roi *min, video_ctrl_roi *max, video_ctrl_roi *def);

@loicpoulain
Copy link
Collaborator

Last block seems fair.

XenuIsWatching
XenuIsWatching previously approved these changes Sep 30, 2024
Copy link
Member

@XenuIsWatching XenuIsWatching left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is reasonable

@josuah
Copy link
Collaborator Author

josuah commented Oct 6, 2024

Force-push:

  • Implement the helpers discussed
  • Rebasing on more recent main

@josuah josuah added the DNM This PR should not be merged (Do Not Merge) label Oct 6, 2024
@josuah
Copy link
Collaborator Author

josuah commented Oct 6, 2024

I think @ngphibang might have been planning something around the video API CID types, which might obsolete this PR.
So I added a DNM flag to make sure it does not get in the way.

loicpoulain
loicpoulain previously approved these changes Oct 7, 2024
Copy link
Contributor

@ngphibang ngphibang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josuah Thanks for mentioning this ! Yes, I am working on improving the control aspect as well. This PR seems targeting part of this scope. But just go ahead merging this if we see that it's ok.

return ret;
}

if (value < min || value > max) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also check min > max

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first added it as an assert(), but the MAX is not always fixed at build-time, i.e. could come from I2C read.
This can happen even when the software is correct => cannot be assert()!

@@ -83,3 +84,45 @@ void video_buffer_release(struct video_buffer *vbuf)
VIDEO_COMMON_FREE(block->data);
}
}

int video_get_range_u32(unsigned int cid, uint32_t *value, uint32_t min, uint32_t max, uint32_t def)
Copy link
Contributor

@ngphibang ngphibang Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only check uint32_t ? What if the control values are of other types, e,g, int (control values can be negative), uint64_t (pixel_rate, link frequencies), menu (test pattern), etc. ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, this should be void * like video_get_ctrl().
If some concept of type is to be introduced, this should be a separate PR.
Thank you for the help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it cannot be uint64_t (as it does not fit in a pointer in some architectures supported by Zephyr) but it can be uint64_t *.

Maybe a first compromise is to support int32_t and uint32_t (or to be more correct, uintptr_t and intptr_t) is good enough for a first cycle? I am updating this branch to illustrate this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am waiting before introducing bool, menu, bitmap and extra types as I tried to focus on the mechanism that enable fetching a range that would fit the current API.

Let me know if I should include them on this PR, I'd be glad to give it a try!

@ngphibang
Copy link
Contributor

What if user specifies : VIDEO_CTRL_GET_CUR | VIDEO_CTRL_GET_MIN | VIDEO_CTRL_GET_MAX | VIDEO_CTRL_GET_DEF at the same time ?

@josuah
Copy link
Collaborator Author

josuah commented Oct 11, 2024

What if user specifies : VIDEO_CTRL_GET_CUR | VIDEO_CTRL_GET_MIN | VIDEO_CTRL_GET_MAX | VIDEO_CTRL_GET_DEF at the same time ?

This becomes GET_DEF:

#define VIDEO_CTRL_GET_CUR		0x00000000	/**< Get the current value */
#define VIDEO_CTRL_GET_MIN		0x00001000	/**< Get the minimum value */
#define VIDEO_CTRL_GET_MAX		0x00002000	/**< Get the maximum value */
#define VIDEO_CTRL_GET_DEF		0x00003000	/**< Get the default value */
#define VIDEO_CTRL_GET_MASK		0x0000f000	/**< Mask for get operations */

In the current implementation, this is not a bitmask of flags, but a field added inside of the CID that contains an integer value from 0 to 3.

Is this too confusing?
I am trying to find the best compromise.
Maybe some kind of more Linux-like API can be interesting too.

@josuah
Copy link
Collaborator Author

josuah commented Oct 11, 2024

Maybe a better solution can be visible after starting to use it a bit.
It is still possible to provide a Linux-like API as a helper utility on top.

@josuah josuah removed the DNM This PR should not be merged (Do Not Merge) label Oct 11, 2024
@josuah josuah requested a review from ngphibang October 11, 2024 20:51
@josuah josuah force-pushed the pr-video-get-ops branch 4 times, most recently from e40296b to 2665a9a Compare October 14, 2024 20:54
@josuah
Copy link
Collaborator Author

josuah commented Oct 14, 2024

Force-push: CI fixes.
I am learning to use twister more efficiently to avoid this.

Developing:

west twister --inline-logs --platform native_sim --aggressive-no-clean \
    --scenario tests/drivers/build_all/video/drivers.video.build

Before submitting:

west twister --inline-logs --platform native_sim \
    --scenario tests/drivers/build_all/video/drivers.video.build

@josuah josuah marked this pull request as ready for review October 14, 2024 22:50
@josuah
Copy link
Collaborator Author

josuah commented Oct 15, 2024

force-push: switch from *_u32 and *_i32 to _int and _int64 as this follows the Linux model, which seems to be adopted for all the CID data.

The video_get_ctrl() API permits to retrieve a value from a device using
standard CIDs from <zephyr/drivers/video-controls.h>. The CIDs do not come
with a range information, and to know which value to apply to a video
driver, knowing the minimum and maximum value before-hand is required.
This prevents building generic layers that handle any video devices, such
as protocols such as USB UVC, GigE Vision, or anything making use of
"zephyr,camera" chosen node.

This commit introduces extra flags added to the CIDs that indicates to the
target device that instead of returning the current value, they should
return the minimum, maximum, or default value instead, with the same type
as the current value.

The GET_CUR operation having a flag of 0x00, this makes all drivers
implicitly support this new API, with an opt-in migration to also support
the extra controls, correctly rejecting the unsupported extra operations by
default.

Signed-off-by: Josuah Demangeon <[email protected]>
This allows implementing the VIDEO_CTRL_GET_MIN/MAX/DEF controls on
drivers with minimum modification of the drivers.

A typical usage in a video_get_ctrl() implementation would be:

	ret = video_get_ctrl_range(cid, value, 0, 320, 9);
	if (ret <= 0) {
		return ret;
	}
	return sensor_read_u32(&cfg->i2c, EXPOSURE_REG, value, 2);

A typical usage in a video_set_ctrl() implementation would be:

	ret = video_check_range_u32(dev, cid, (uint32_t)value);
	if (ret < 0) {
		return ret;
	}
	return sensor_read_reg(&cfg->i2c, EXPOSURE_REG, value, 2);

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah
Copy link
Collaborator Author

josuah commented Nov 3, 2024

force-push:

@josuah josuah added the Release Notes Required Release notes required for this change label Nov 7, 2024
Copy link
Contributor

@ngphibang ngphibang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to come back to this a bit late, but I think there is a couple of issues within this PR.

  • The 1st commit is to add some "helper" functions to do sanity check (range check) on the control values and these are intended to be used by drivers. But would it be really helpful to simplify the code ? Looking at the ov5640 driver, for example, instead of calling one line of code for IN_RANGE macro to check the range in every set_ctrl(), if I use these "helper", it's still the same amount of code :-). And every drivers still have to repeatedly do the same sanity check. For me, it's only helpful if we can delegate all the sanity check to the video framework, drivers have to do nothing.

  • The 2nd commit is to add 4 macros which are intended to be combined with the CID parameters in get_ctrl() to query additional information for the min/max/default values beside the current control value. I think these information should be queried through another API, the query_ctrl() API rather than included in the get_ctrl(). The query_ctrl() API could query all things at once (min/max/default). This is not only for efficiency but also to keep things cleaner. I know that the UVC driver does it separately with GET_CUR, GET_MIN, GET_MAX, etc. but it does not mean we have to impose this constraint to everyone. You can always query all at the same time at the application level and call UVC separately for each value. And for a larger scope, the get_ctrl() and query_ctrl() are something that can be also handled by the video framework, drivers don't need to implement these API.

These issues do not only come from the PR but also from the current video framework limitation which is not implemented to support this.

@josuah
Copy link
Collaborator Author

josuah commented Nov 7, 2024

Thank you for taking the time to review this again.

instead of calling one line of code for IN_RANGE macro to check

Yes, no real advantage.

only helpful if we can delegate all the sanity check to the video framework, drivers have to do nothing.

This would be nice.

these information should be queried through another API, the query_ctrl() API

Like this?

struct video_ctrl_range {
        void *min;
        void *max;
        void *cur; /* Did you mean to include this field in `query_ctrl()` or leave it to `get_ctrl()`? */
        void *def; /* This one too? */
        int type; /* Other fields for allowing batch handling? */
};

struct video_ctrl_range result;

video_ctrl_query(dev, VIDEO_CID_..., &result);

I know that the UVC driver does it separately with GET_CUR, GET_MIN, GET_MAX, etc.

The UVC driver will be refactored as many times as needed. :)

And for a larger scope, the get_ctrl() and query_ctrl() are something that can be also handled by the video framework

I think on Linux, the controls are registered at init, and then this cached information is used at runtime. On Zephyr this could need some CONFIG_VIDEO_CONTROL_NUMBER for allocating the storage of the min/max/default at runtime.

An alternative is to ask the information to the drivers every time query_ctrl() is done like it is done for video_enum_frmival() for instance.

An alternative is to pass the whole table, like it is done for video_get_caps(). Very compact... but what about sensors that have MIN/MAX defined at runtime (i.e. Arducam Mega, HDMI source...)

struct video_ctrl_range {
        unsigned int cid;
        void *min, *max, *def;
};

const struct video_ctrl_range ctrls[] = {
        { .cid = VIDEO_CID_EXPOSURE, .min = (void *)0, .max = (void *)100, .def = (void *)50 },
        { .cid = VIDEO_CID_GAIN, .min = (void *)0, .max = (void *)100, .def = (void *)0 },
        {0},
};

@josuah
Copy link
Collaborator Author

josuah commented Nov 7, 2024

What is the way forward to propose an alternative? Did you plan to make a new PR? I'm glad to edit this one too.

@ngphibang
Copy link
Contributor

ngphibang commented Nov 7, 2024

In general, controls need to be cached in memory because reading control value directly from registers each time get_ctrl() get called is not only inefficient but also, sometimes impossible whenever the control is "scattered" to multiple registers and needs a complex computation (see the hue control in ov5640 driver).

what about sensors that have MIN/MAX defined at runtime (i.e. Arducam Mega, HDMI source...)

There are a few control types, in some contexts, need to be read at runtime directly from registers, such as gain, exposure, which we call volatile controls. They will need a dedicated API like get_volatile_ctrl(). Do the "Arducam Mega, HDMI source" that you mentioned fall into these types ? If not, could you give a concrete example ?

What is the way forward to propose an alternative? Did you plan to make a new PR? I'm glad to edit this one too.

Yes, as being said, I am working on this. The main part is not about the API or structure defintions, etc but something like how to make the controls visible from a device to the framework and vice versa, how to merge controls from the each device throughout the pipeline, etc. But it's good because through this, we can make the framework do "something" on behalf of the drivers for the 1st time. There are some technical challenges and time constraint but I am finalizing the last parts, hope to push the PR soon.
Ofcourse, feel free to discuss on the PR.

@josuah
Copy link
Collaborator Author

josuah commented Nov 7, 2024

Do the "Arducam Mega, HDMI source" that you mentioned fall into these types ?

The Arducam Mega allows a selection of image sensors to be connected, so in this kind of scenario, which controls are supported could depend on which sensor is connected.

NXP's tda1997x HDMI <-> MIPI receiver does have a tda1997x_g_volatile_ctrl as you predicted.

Yes, as being said, I am working on this.

My bad, yes you did. I will wait the new PR and will close this one.

@ngphibang
Copy link
Contributor

The Arducam Mega allows a selection of image sensors to be connected, so in this kind of scenario, which controls are supported could depend on which sensor is connected.

I am not sure to understand this. Does it mean which sensor is selected is done at compile time ? or runtime ? and can be changed at any moment ? If it's done at compile time, it is as other camera sensors.

@josuah
Copy link
Collaborator Author

josuah commented Nov 8, 2024

Here is the pull request for it:

https://github.com/zephyrproject-rtos/zephyr/pull/66994/files#diff-a6394ee84ba37d741b1aecea29085811d0f447fe128a655c3840eb59c3e86604

The same single driver is expected to be used for all cameras, and the variability is at construction time: there are cameras variants:

scrot_20241108_124349_825x701

Arducam Mega SPI protocol has a "Camera model" field:

scrot_20241108_124239_645x88

In their PR, Ardcuam reads this "SENSOR_ID" to tell which controls and resolutions are available:

drivers/video/arducam_mega.c

	switch (cam_id) {
	case ARDUCAM_SENSOR_5MP_1: /* 5MP-1 */
		fmts[8] = (struct video_format_cap)ARDUCAM_MEGA_VIDEO_FORMAT_CAP(
			2592, 1944, VIDEO_PIX_FMT_RGB565);
		fmts[17] = (struct video_format_cap)ARDUCAM_MEGA_VIDEO_FORMAT_CAP(
			2592, 1944, VIDEO_PIX_FMT_JPEG);
		fmts[26] = (struct video_format_cap)ARDUCAM_MEGA_VIDEO_FORMAT_CAP(
			2592, 1944, VIDEO_PIX_FMT_YUYV);
		support_resolution[8] = MEGA_RESOLUTION_WQXGA2;
		drv_data->info = &mega_infos[0];
		break;
	case ARDUCAM_SENSOR_3MP_1: /* 3MP-1 */
		...
	}

Then have defined for themselves several "camera profiles":

static struct arducam_mega_info mega_infos[] = {{
							.support_resolution = 7894,
							.support_special_effects = 63,
							.exposure_value_max = 30000,
							.exposure_value_min = 1,
							.gain_value_max = 1023,
							.gain_value_min = 1,
							.enable_focus = 1,
							.enable_sharpness = 0,
							.device_address = 0x78,
						}, ...};

So it is all done at startup time and relies of a database like storage of sensor capabilities. They would update the driver when more sensors are supported.

For now, enable_sharpness is not used anywhere (just logged) but I suppose this is meant to tell what controls to turn on/off.

I do not expect a camera to be plugged at runtime after init completes though.

[EDIT: my bad, it looks like they wish it to be possible upon request of the application]

https://github.com/zephyrproject-rtos/zephyr/pull/66994/files#diff-a6394ee84ba37d741b1aecea29085811d0f447fe128a655c3840eb59c3e86604R926-R929

	case VIDEO_CID_ARDUCAM_RESET:
		ret |= arducam_mega_soft_reset(dev);
		ret |= arducam_mega_check_connection(dev);
		break;

Where arducam_mega_check_connection reloads the table of formats.

@ngphibang
Copy link
Contributor

ngphibang commented Nov 12, 2024

Thank you for these infos ! I was not involved in the reviewing of the Arducam Mega driver but I think there are several aspects need to be reconsidered. Just for the control part, we see:

static int arducam_mega_get_ctrl(const struct device *dev, unsigned int cid, void *value)
{
	int ret = 0;

	switch (cid) {
	case VIDEO_CID_ARDUCAM_INFO:
		ret |= arducam_mega_get_info(dev, (struct arducam_mega_info *)value);
		break;
	default:
		return -ENOTSUP;
	}

	return ret;
}

The arducam mega info table:


> 						{
> 							.support_resolution = 7638,
> 							.support_special_effects = 319,
> 							.exposure_value_max = 30000,
> 							.exposure_value_min = 1,
> 							.gain_value_max = 1023,
> 							.gain_value_min = 1,
> 							.enable_focus = 0,
> 							.enable_sharpness = 1,
> 							.device_address = 0x78,
> 						}};

They declared a new vendor CID (VIDEO_CID_ARDUCAM_INFO) just for showing a group of control default values and resolutions.
These are not new control types. IMO, we should not declare a new CID just for this. These are still classic control types where the default values could be different for each sensor, so just use the classic CIDs with different default values.

Even the Arducam driver is common for a set of similar sensors, the code can be refactored to a common and particular parts. At hardware abstraction level, they can be considered as separate sensors. When sensor is changed (not sure at compile time or runtime), it is always possible to follow the normal process : stop stream / restart stream, control is re-initialized for the new sensor, etc.

@josuah
Copy link
Collaborator Author

josuah commented Nov 12, 2024

When sensor is changed (not sure at compile time or runtime), it is always possible to follow the normal process : stop stream / restart stream
At hardware abstraction level, they can be considered as separate sensors.

Yes good point, a very similar API but still different devices.
It looks like there are multiple ways to solve it without requiring changing the APIs or abusing CIDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Drivers area: Video Video subsystem Release Notes Required Release notes required for this change RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants