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

josuah · 2024-09-18T00:51:57Z

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 · 2024-09-18T01:15:39Z

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 · 2024-09-21T17:10:15Z

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 · 2024-09-21T17:26:53Z

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 · 2024-09-30T16:34:38Z

Last block seems fair.

XenuIsWatching

I think this approach is reasonable

josuah · 2024-10-06T18:55:22Z

Force-push:

Implement the helpers discussed
Rebasing on more recent main

josuah · 2024-10-06T18:56:46Z

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.

ngphibang

@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.

ngphibang · 2024-10-10T11:40:38Z

drivers/video/video_common.c

+		return ret;
+	}
+
+	if (value < min || value > max) {


you should also check min > max

ngphibang · 2024-10-10T11:43:41Z

drivers/video/video_common.c

@@ -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)


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. ?

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.

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.

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 · 2024-10-10T12:02:30Z

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 · 2024-10-11T07:29:04Z

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.

loicpoulain · 2024-10-11T20:01:18Z

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.

I think it's ok. At least this is a solution, and we can adjust it later if necessary.

josuah · 2024-10-11T20:29:37Z

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.

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 <me@josuah.net>

josuah · 2024-10-14T20:57:25Z

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

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 <me@josuah.net>

josuah · 2024-10-15T19:00:13Z

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.

josuah added area: Drivers RFC Request For Comments: want input from the community area: Video Video subsystem labels Sep 18, 2024

josuah marked this pull request as draft September 18, 2024 00:52

zephyrbot requested a review from loicpoulain September 18, 2024 00:52

josuah force-pushed the pr-video-get-ops branch from 26f0023 to fd5e76b Compare September 18, 2024 01:47

This was referenced Sep 18, 2024

drivers: video: API enhancement #72959

Open

usb: device_next: new USB Video Class (UVC) implementation #76798

Open

josuah force-pushed the pr-video-get-ops branch from fd5e76b to 84b2763 Compare September 21, 2024 17:29

josuah marked this pull request as ready for review September 21, 2024 17:31

josuah force-pushed the pr-video-get-ops branch from 84b2763 to 96a0c3e Compare September 22, 2024 10:12

XenuIsWatching previously approved these changes Sep 30, 2024

View reviewed changes

josuah mentioned this pull request Oct 5, 2024

drivers: video: sw_generator: Hue Saturation Brightness controls #79458

Closed

josuah dismissed XenuIsWatching’s stale review via b6a2b3c October 6, 2024 17:26

josuah force-pushed the pr-video-get-ops branch from 96a0c3e to b6a2b3c Compare October 6, 2024 17:26

zephyrbot requested a review from ngphibang October 6, 2024 17:26

josuah force-pushed the pr-video-get-ops branch from b6a2b3c to 71de5b5 Compare October 6, 2024 17:27

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

loicpoulain previously approved these changes Oct 7, 2024

View reviewed changes

josuah dismissed loicpoulain’s stale review via ec5aeb9 October 8, 2024 04:17

josuah force-pushed the pr-video-get-ops branch from 71de5b5 to ec5aeb9 Compare October 8, 2024 04:17

ngphibang reviewed Oct 10, 2024

View reviewed changes

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

loicpoulain self-requested a review October 11, 2024 20:01

loicpoulain previously approved these changes Oct 11, 2024

View reviewed changes

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

josuah dismissed loicpoulain’s stale review via af11160 October 11, 2024 20:50

josuah force-pushed the pr-video-get-ops branch from ec5aeb9 to af11160 Compare October 11, 2024 20:50

josuah requested a review from ngphibang October 11, 2024 20:51

josuah force-pushed the pr-video-get-ops branch 2 times, most recently from 5b701f2 to c7b61f5 Compare October 14, 2024 12:12

josuah force-pushed the pr-video-get-ops branch 2 times, most recently from e40296b to 2665a9a Compare October 14, 2024 20:54

josuah marked this pull request as ready for review October 14, 2024 22:50

josuah requested review from loicpoulain and XenuIsWatching October 14, 2024 22:50

josuah force-pushed the pr-video-get-ops branch from 2665a9a to 1a4dcb1 Compare October 15, 2024 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

josuah commented Sep 18, 2024 •

edited

Loading

josuah commented Sep 18, 2024 •

edited

Loading

josuah commented Sep 21, 2024

josuah commented Sep 21, 2024 •

edited

Loading

loicpoulain commented Sep 30, 2024

XenuIsWatching left a comment

josuah commented Oct 6, 2024

josuah commented Oct 6, 2024

ngphibang left a comment •

edited

Loading

ngphibang Oct 10, 2024

ngphibang Oct 10, 2024 •

edited

Loading

josuah Oct 11, 2024

josuah Oct 11, 2024

josuah Oct 15, 2024

ngphibang commented Oct 10, 2024

josuah commented Oct 11, 2024

loicpoulain commented Oct 11, 2024

josuah commented Oct 11, 2024 •

edited

Loading

josuah commented Oct 14, 2024

josuah commented Oct 15, 2024

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

Are you sure you want to change the base?

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

Conversation

josuah commented Sep 18, 2024 • edited Loading

josuah commented Sep 18, 2024 • edited Loading

josuah commented Sep 21, 2024

josuah commented Sep 21, 2024 • edited Loading

loicpoulain commented Sep 30, 2024

XenuIsWatching left a comment

Choose a reason for hiding this comment

josuah commented Oct 6, 2024

josuah commented Oct 6, 2024

ngphibang left a comment • edited Loading

Choose a reason for hiding this comment

ngphibang Oct 10, 2024

Choose a reason for hiding this comment

ngphibang Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

josuah Oct 11, 2024

Choose a reason for hiding this comment

josuah Oct 11, 2024

Choose a reason for hiding this comment

josuah Oct 15, 2024

Choose a reason for hiding this comment

ngphibang commented Oct 10, 2024

josuah commented Oct 11, 2024

loicpoulain commented Oct 11, 2024

josuah commented Oct 11, 2024 • edited Loading

josuah commented Oct 14, 2024

josuah commented Oct 15, 2024

josuah commented Sep 18, 2024 •

edited

Loading

josuah commented Sep 18, 2024 •

edited

Loading

josuah commented Sep 21, 2024 •

edited

Loading

ngphibang left a comment •

edited

Loading

ngphibang Oct 10, 2024 •

edited

Loading

josuah commented Oct 11, 2024 •

edited

Loading