drivers: video: introduce semantics for `enum endpoint_id` #73009

josuah · 2024-05-20T08:55:57Z

This is part of a series of proposal for incremental changes for the Video API:

drivers: video: API enhancement #72959

This adds slightly new semantics for endpoint_id:

IN is not "the main input endpoint" anymore but "each and every input endpoint"
OUT is not "the main output endpoint" anymore but "each and every output endpoint"
~~ANY~~ ALL is not "one endpoint of choice" anymore but "every single endpoint"
NONE is not "does not affect any interface" anymore but "affects something not bound to any interface"
Other values >= 0 are allowed to specify a particular interface.

erwango · 2024-05-20T08:59:44Z

loicpoulain · 2024-05-20T16:42:06Z

would it make sense to have a bit assigned to OUT/IN in the ID and corresponding macro, so that we can have some type enforcement, like VIDEO_EP_IN(index) VIDEO_EP_OUT(index), e.g:
VIDEO_EP_IN(0)
VIDEO_EP_OUT(0)
VIDEO_EP_OUT(ANY)
...
VIDEO_EP_IS_OUT(ep_id)

josuah · 2024-05-20T18:47:34Z

It could be an extra reg index, making the depth field mandatory, with 0 = Out and 1 = In.

(A) This can be a nibble:
i.e. endpoint 3 out -> endpoint@30 { reg = <0x3 0>; };
i.e. endpoint 1 in -> endpoint@11 { reg = <0x1 1>; };
i.e. port 7 endpoint 2 out -> endpoint@720 { reg = <0x7 0x2 0>; };

(B) This can be a byte:
i.e. endpoint 3 out -> endpoint@0300 { reg = <0x03 0>; };
i.e. endpoint 1 in -> endpoint@0101 { reg = <0x01 0>; };
i.e. port 7 endpoint 2 out -> endpoint@070200 { reg = <0x07 0x02 0>; };

(C) This can be a nibble after bytes:
i.e. endpoint 3 out -> endpoint@030 { reg = <0x03 0>; };
i.e. endpoint 1 in -> endpoint@011 { reg = <0x01 0>; };
i.e. port 7 endpoint 2 out -> endpoint@07020 { reg = <0x07 0x02 0>; };

This can also be hidden behind a macro on the reg:
i.e. endpoint 3 out -> endpoint@030 { reg = <0x03 VIDEO_ID_OUT>; };
i.e. endpoint 1 in -> endpoint@011 { reg = <0x01 VIDEO_ID_IN>; };
i.e. port 7 endpoint 2 out -> endpoint@07020 { reg = <0x07 0x02 VIDEO_ID_OUT>; };

With the matching macro definitions in C, i.e. for a nibble:
i.e. endpoint 3 out -> VIDEO_EP_OUT(3)
i.e. endpoint 1 in -> VIDEO_EP_IN(1)
i.e. port 7 endpoint 2 out -> VIDEO_EP_OUT(0x72) or VIDEO_EP_OUT(0x0702)

[EDIT: selected option (A)]

josuah · 2024-05-21T03:46:53Z

I changed VIDEO_EP_IN/OUT(ANY) to VIDEO_EP_ANY_IN/OUT as was easier to implement and still works with VIDEO_EP_IS_IN/OUT().

VIDEO_EP_IS_IN/OUT(VIDEO_EP_ANY) is returning true for both IN/OUT.

Extra VIDEO_DIR_IN/OUT/ANY/NONE were also introduced to avoid magic numbers through the source.

dleach02 · 2024-05-23T14:56:42Z

@josuah please address the CI issues

josuah · 2024-05-23T15:00:15Z

@josuah please address the CI issues

Yes, some more API changes are needed. converting back to a draft, sorry for the burden involved.

loicpoulain · 2024-05-31T19:27:40Z

drivers/video/video_mcux_csi.c

@@ -121,7 +121,16 @@ static int video_mcux_csi_set_fmt(const struct device *dev, enum video_endpoint_
 	unsigned int bpp = video_pix_fmt_bpp(fmt->pixelformat);
 	status_t ret;

-	if (!bpp || ep != VIDEO_EP_OUT) {
+	switch (ep) {


That repeated pattern would probably deserve a macro or function.

I will propose something in another PR when there will be a pretext to touch to the video drivers again (to limit review burden), but it seems possible to use macros with the new scheme:

#define VIDEO_EP_OUT_SELECTED(ep, match) \ ((ep) == VIDEO_EP_OUT || (ep) == VIDEO_EP_ALL || (ep) == (match)) #define VIDEO_EP_IN_SELECTED(ep, match) \ ((ep) == VIDEO_EP_IN || (ep) == VIDEO_EP_ALL || (ep) == (match)) /* Filter away, 0x00 used when only 1 endpoint supported */ if (bpp == 0 || !VIDEO_EP_OUT_SELECTED(ep, 0x00)) { return -EINVAL; } /* Matches endpoint 0x00 or 0x01 or 0x02 */ if (VIDEO_EP_OUT_SELECTED(ep, 0x00)) { work_on_ep0(); } if (VIDEO_EP_OUT_SELECTED(ep, 0x01)) { work_on_ep1(); } if (VIDEO_EP_IN_SELECTED(ep, 0x02)) { work_on_ep2(); } /* If ep == VIDEO_EP_ALL, then all endpoint are enacted... this works */

josuah · 2024-07-28T00:26:08Z

Here is a simpler, less intrusive attempt: just give negative values to VIDEO_EP_IN VIDEO_EP_OUT etc.
Individual interface numbers can still be given on enum video_endpoint_id ep arguments.

josuah · 2024-07-28T00:29:10Z

include/zephyr/drivers/video.h

@@ -449,7 +453,7 @@ static inline int video_stream_stop(const struct device *dev)
 	}

 	ret = api->stream_stop(dev);
-	video_flush(dev, VIDEO_EP_ANY, true);


This is the only place I have found VIDEO_EP_ANY used.
If video_endpoint_id is a direction, "ANY" makes a lot of sense, but if allowing it to be an endpoint number, "ALL" seems less ambiguous (i.e. here flushing all endpoints rather than flushing endpoints for any direction).

josuah · 2024-07-28T00:33:12Z

include/zephyr/drivers/video.h

+	/** Targets some part of the video device not bound to an endpoint */
+	VIDEO_EP_NONE = -1,
+	/** Targets all input or output endpoints of the device */
+	VIDEO_EP_ALL = -2,
+	/** Targets all input endpoints of the device */
+	VIDEO_EP_IN = -3,
+	/** Targets all output endpoints of the device */
+	VIDEO_EP_OUT = -4,


This does not encode any direction information into the video_endpoint_id, which allow using endpoint IDs exactly as in the manufacturer datasheet. Does it make sense?

josuah · 2024-07-28T00:34:09Z

drivers/video/video_mcux_csi.c

@@ -155,7 +155,7 @@ static int video_mcux_csi_set_fmt(const struct device *dev, enum video_endpoint_
 	status_t ret;
 	struct video_format format = *fmt;

-	if (!bpp || ep != VIDEO_EP_OUT) {
+	if (bpp != 0 || (ep != VIDEO_EP_OUT && ep != VIDEO_EP_ALL)) {


Changing !bpp to bpp != 0 in the process, let me know if I should avoid it.
[EDIT: sorry I should pay more attention! !bpp is bpp == 0!]

This define the enum values so that they never clash with custom endpoint IDs passed numerically, for devices that supports more than one IN or OUT enpoint. This also documents semantics for VIDEO_EP_IN/OUT/ALL/NONE, while renaming VIDEO_EP_ANY to VIDEO_EP_ALL to remove the ambiguity when there are several IN and/or OUT endpoints. Signed-off-by: Josuah Demangeon <[email protected]>

josuah · 2024-08-16T11:48:10Z

As pointed out by @XenuIsWatching, one use-case would be:

Some Cameras may be a bit 'advanced', and be able to stream out multiple frame types out at once such as a IR image along with a RGB image, I'm not sure how that would be currently implemented to select which frame type to enable or set a ctrl for. Maybe enum video_endpoint_id ep could be used for it? -- #72959 (comment)

Something like this could work after this gets merged:

/* Configure format RGB on 0 and InfraRed on 1 */
video_set_format(video_dev, 0, &fmt_rgb);
video_set_format(video_dev, 1, &fmt_ir);

/* Enqueue transfers separately to endpoints 0 and 1 */
video_enqueue(video_dev, 0, &vbuf_rgb);
video_enqueue(video_dev, 1, &vbuf_ir);

/* Enqueue transfers on both endpoints at the same time in the same feed */
video_enqueue(video_dev, VIDEO_EP_OUT, &vbuf_either);

Maybe a flag would need to be added to struct video_buffer to remind which endpoint this was coming from, but this would be for another PR.

dleach02 · 2024-08-20T03:01:56Z

Folks, I'm the assignee because I guess I won the files touched lottery. I need some of the relevant subject matter experts to weigh in and give their +1/-1 so we can move this PR along.

loicpoulain

LGTM

The current video drivers did not allow VIDEO_EP_ALL to be selected, as used by video_stream_stop(). This adds the VIDEO_EP_ALL to each video driver that filtered endpoints. Signed-off-by: Josuah Demangeon <[email protected]>

josuah · 2024-08-20T11:01:17Z

Sorry, I did a beginner mistake by converting !bpp to bpp != 0 instead of bpp == 0.
I will pay more attention in the future.
@dleach02 @loicpoulain could you review once again?

dleach02 · 2024-08-23T17:31:36Z

LGTM

@loicpoulain can you give this a +1 then?

@erwango and ST driver was also touched so +1?

josuah requested review from erwango, ABOSTM, FRASTM, mmahadevan108 and dleach02 as code owners May 20, 2024 08:55

zephyrbot added area: Video Video subsystem platform: NXP Drivers NXP Semiconductors, drivers platform: STM32 ST Micro STM32 labels May 20, 2024

zephyrbot requested review from ajarmouni-st, danieldegrasse, dbaluta, decsny, gautierg-st, GeorgeCGV, loicpoulain and manuargue May 20, 2024 08:56

zephyrbot assigned dleach02 May 20, 2024

josuah mentioned this pull request May 20, 2024

drivers: video: API enhancement #72959

Open

josuah mentioned this pull request May 20, 2024

dts: flatten video's device tree endpoints definition #72950

Closed

josuah force-pushed the pr-video-endpoint-id-semantics branch 3 times, most recently from 8229acb to 2bbfd19 Compare May 20, 2024 23:57

josuah mentioned this pull request May 21, 2024

drivers: video: Introduce macro to deal with endpoints #73023

Closed

josuah marked this pull request as draft May 23, 2024 15:00

loicpoulain reviewed May 31, 2024

View reviewed changes

josuah force-pushed the pr-video-endpoint-id-semantics branch from 2bbfd19 to 86a99a2 Compare July 28, 2024 00:17

josuah commented Jul 28, 2024

View reviewed changes

josuah marked this pull request as ready for review July 28, 2024 00:34

josuah requested a review from loicpoulain July 28, 2024 00:34

zephyrbot requested a review from marwaiehm-st July 28, 2024 00:35

josuah force-pushed the pr-video-endpoint-id-semantics branch from 86a99a2 to bea302c Compare July 28, 2024 17:52

loicpoulain previously approved these changes Aug 20, 2024

View reviewed changes

josuah force-pushed the pr-video-endpoint-id-semantics branch from bea302c to 7745d17 Compare August 20, 2024 10:58

josuah dismissed loicpoulain’s stale review via 4efb6b8 August 20, 2024 11:00

josuah force-pushed the pr-video-endpoint-id-semantics branch from 7745d17 to 4efb6b8 Compare August 20, 2024 11:00

josuah requested a review from loicpoulain August 20, 2024 11:01

dleach02 approved these changes Aug 23, 2024

View reviewed changes

loicpoulain approved these changes Aug 23, 2024

View reviewed changes

nashif merged commit c0f8658 into zephyrproject-rtos:main Aug 24, 2024
23 checks passed

josuah deleted the pr-video-endpoint-id-semantics branch August 24, 2024 13:08

trunghieulenxp mentioned this pull request Aug 28, 2024

Improve NXP CSI and MIPI_CSI2Rx drivers #76475

Merged

This was referenced Sep 18, 2024

drivers: video: esp32s3: Add set_signal video api function #78566

Merged

drivers: video: video_mcux_smartdma: add SMARTDMA video driver #72827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: video: introduce semantics for `enum endpoint_id` #73009

drivers: video: introduce semantics for `enum endpoint_id` #73009

josuah commented May 20, 2024 •

edited

Loading

erwango commented May 20, 2024

loicpoulain commented May 20, 2024 •

edited

Loading

josuah commented May 20, 2024 •

edited

Loading

josuah commented May 21, 2024

dleach02 commented May 23, 2024

josuah commented May 23, 2024

loicpoulain May 31, 2024

josuah Aug 23, 2024

josuah commented Jul 28, 2024

josuah Jul 28, 2024

josuah Jul 28, 2024

josuah Jul 28, 2024 •

edited

Loading

josuah commented Aug 16, 2024

dleach02 commented Aug 20, 2024

loicpoulain left a comment

josuah commented Aug 20, 2024

dleach02 commented Aug 23, 2024 •

edited

Loading

drivers: video: introduce semantics for enum endpoint_id #73009

drivers: video: introduce semantics for enum endpoint_id #73009

Conversation

josuah commented May 20, 2024 • edited Loading

erwango commented May 20, 2024

loicpoulain commented May 20, 2024 • edited Loading

josuah commented May 20, 2024 • edited Loading

josuah commented May 21, 2024

dleach02 commented May 23, 2024

josuah commented May 23, 2024

loicpoulain May 31, 2024

Choose a reason for hiding this comment

josuah Aug 23, 2024

Choose a reason for hiding this comment

josuah commented Jul 28, 2024

josuah Jul 28, 2024

Choose a reason for hiding this comment

josuah Jul 28, 2024

Choose a reason for hiding this comment

josuah Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

josuah commented Aug 16, 2024

dleach02 commented Aug 20, 2024

loicpoulain left a comment

Choose a reason for hiding this comment

josuah commented Aug 20, 2024

dleach02 commented Aug 23, 2024 • edited Loading

drivers: video: introduce semantics for `enum endpoint_id` #73009

drivers: video: introduce semantics for `enum endpoint_id` #73009

josuah commented May 20, 2024 •

edited

Loading

loicpoulain commented May 20, 2024 •

edited

Loading

josuah commented May 20, 2024 •

edited

Loading

josuah Jul 28, 2024 •

edited

Loading

dleach02 commented Aug 23, 2024 •

edited

Loading