video: Drop video endpoint id in video APIs #87070

ngphibang · 2025-03-13T18:06:19Z

In the video framework, the video_endpoint_id parameter put in each API function has nothing to do with its meaning and has no usage. They should be removed.

Currently, video drivers in Zephyr upstream are all "typical" video devices e.g., camera sensor, receiver, encoders, etc. which have only one buffer queue and does not need to distinguish which buffer queue it operates on. However, devices like ISP, PxP fall into another catergory which (Linux) calls m2m devices. For this kind of devices, the driver needs two separate buffer queues and hence needs to distinguish between input (which Linux calls "output") and output (which Linux calls "capture") side.

To support this kind of devices, such as this PR and the NXP PxP, add a video_buf_type to distinguish the buffer queue:

get_caps(), set/get_format(), enqueue()/dequeue() : the buffer type is embeded in the video_caps, video_format and video_buffer structs itself.
video_stream_start/stop() : buffer type is sent as a parameter
set/get_ctrl() : no need as control IDs are already different for input and output (and also, we rarely has to set controls on both input and output)
set/get/enum_frmival() : N/A.

josuah

It would simplify the application development. I wait CI and answers but am in favor of it!

drivers/video/video_esp32_dvp.c

include/zephyr/drivers/video.h

josuah · 2025-03-19T01:05:59Z

Something I notice now that I read #82158 again...

avolmat-st · 2025-04-15T07:33:45Z

Hi,

I am currently writing the driver for the DCMIPP camera block of the STM32N6 which has actually 3 outputs. In a sense there is a single device but can have up to 3 streams of data going out.
While most of driver have been using the endpoint with simply VIDEO_EP_OUT, it seemed possible to use this in order to distinguish which pipe to control using this endpoint mechanism. set_stream didn't mentioned any endpoint so this a missing point for a device such as this one since it would be necessary to mention which pipe to start / stop.

Proposed video_buf_type is great thing in order to distinguish between capture and output buffers in case of M2M devices, however this remove the way to select between multiple output (or inputs) of the same type.

In the Linux world, such device is exposed using a multiple video devices (as well as multiple subdevices as well), however in Zephyr I am not aware of a way to have multiple devices which would thus be adressed via the usual set of function (fmt/stream etc etc).

What would you propose to handle such case ?

avolmat-st

Within commit log of

drivers: video: Add video_buf_type

incomming -> incoming
outcomming -> outcoming

avolmat-st · 2025-05-12T10:56:18Z

tests/drivers/video/api/src/video_emul.c

 	zexpect_not_null(vbuf);
 	zexpect_equal(vbuf->bytesused, vbuf->size);

 	/* Nothing left in the queue, possible to stop */
-	zexpect_ok(video_stream_stop(rx_dev));
+	zexpect_ok(video_stream_stop(rx_dev, VIDEO_BUF_TYPE_OUTPUT));


VIDEO_BUF_TYPE_OUTPUT is introduced in the following commit hence this commit won't probably build (?). This line should probably be added within the next commit introducing the buf type enum.

Yes, I mistook the commit id when rebasing to fix CI.

avolmat-st · 2025-05-12T11:00:36Z

drivers/video/video_mcux_smartdma.c

@@ -162,7 +163,7 @@ static int nxp_video_sdma_enqueue(const struct device *dev, struct video_buffer
 	k_fifo_put(&data->fifo_in, vbuf);
 	if (data->stream_starved) {
 		/* Kick SmartDMA off */
-		nxp_video_sdma_set_stream(dev, true);
+		nxp_video_sdma_set_stream(dev, true, vbuf->type);


is vbuf->type correct here ? Considering that there is a type field (buf_type) within the video buffer structure this can work however I do not see any place where the video buffer type is being set.

Yes, it's pointed out in the commit message, for those APIs, the application needs to set the type field but I forgot to do it. Thanks !

dleach02 · 2025-05-12T21:57:39Z

Coding guidelines complains about the signal variable is a reserved indentifier in some video functions k_poll_signal *signal but it is NOT related to the changes:

Error: drivers/video/video_mcux_csi.c:374:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - signal
drivers/video/video_sw_generator.c:229:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - signal
include/zephyr/drivers/video.h:711:WARNING: Violation to rule 21.2 (Should not used a reserved identifier) - signal
Error: Process completed with exit code 1.

@ngphibang it is because you modified the line. The project may have turned on some checking here. I'll note that I see inconsistent naming where we sometimes use "sig" and other times use "signal". My recommendation is to change the "signal" to "sig" to be compliant.

include/zephyr/drivers/video.h

avolmat-st

very minor comment regarding function parameter description, apart from that LGTM.

ngphibang · 2025-05-13T07:12:37Z

@ngphibang it is because you modified the line. The project may have turned on some checking here. I'll note that I see inconsistent naming where we sometimes use "sig" and other times use "signal". My recommendation is to change the "signal" to "sig" to be compliant.

I added a commit to rename the signal variables to avoid code compliance violation for this PR (though this is not related to the PR changes).

avolmat-st

LGTM.

When stopping, the framework calls driver's stop callback then driver's flush callback. Hence, driver's flush callback does not need to call stop callback again. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

The video endpoints are already described in the devicetree. The video_endpoint_id parameter in each video API is not necessary and has no usage. Drop it. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

M2M devices like ISPs or PxP have two separate buffer queues, i.e. incoming and outcoming queues. For each API, the driver needs to distinguish on which queue it needs to take action. Add video buffer type to support this kind of devices. - get_caps(), set/get_format(), enqueue()/dequeue(): the buffer type is embeded in the video_caps, video_format and video_buffer structs - video_stream_start/stop() : buffer type needs is sent as a parameter Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

Rename signal variables to sig to be compliant with code rule 21.2 Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

ngphibang · 2025-05-14T08:30:53Z

Rebased as #89627 has been merged.

sonarqubecloud · 2025-05-14T09:17:21Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

josuah

I will happily rebase my PRs as soon as I am back. :]

sylvioalves · 2025-05-17T08:43:35Z

drivers/video/video_esp32_dvp.c

-	if (ep != VIDEO_EP_OUT) {
-		return -EINVAL;
-	}
-
 	/* ESP32 produces full frames */
 	caps->min_line_count = caps->max_line_count = LINE_COUNT_HEIGHT;

 	/* Forward the message to the source device */
 	return video_get_caps(config->source_dev, ep, caps);


Missed updating this line here. #90090

Yes, I missed it. Thank you ! Searching all "ep" pattern was difficult ... Having esp32 svp driver built-only test would help ...

I am checking PR Twister tests.. sounds there is no video test case at all. Is it? I mean, there is some sample code but none is being triggered in CI.

Yes, test case for esp32 dvp was missing in build_all/video tests. I can add it at the end of the day (unless you do it faster)

I updated the PR. Thanks!

zephyrbot added platform: ESP32 Espressif ESP32 area: Samples Samples platform: NXP Drivers NXP Semiconductors, drivers area: Video Video subsystem platform: STM32 ST Micro STM32 labels Mar 13, 2025

zephyrbot requested review from dbaluta, decsny, dleach02, josuah, kartben, loicpoulain, LucasTambor, manuargue, marekmatej, mmahadevan108, nashif, raffarost, Raymond0225, sylvioalves and uLipe March 13, 2025 18:07

zephyrbot assigned dleach02 and mmahadevan108 Mar 13, 2025

ngphibang changed the title ~~video : Remove video endpoint id in video APIs~~ video: Drop video endpoint id in video APIs Mar 13, 2025

josuah reviewed Mar 13, 2025

View reviewed changes

drivers/video/video_esp32_dvp.c Show resolved Hide resolved

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

include/zephyr/drivers/video.h Show resolved Hide resolved

josuah added area: API Changes to public APIs Release Notes Required Release notes required for this change labels Mar 13, 2025

This was referenced Mar 13, 2025

driver: video: esp32: add video_flush() callback #86915

Merged

Implement video control framework #82158

Merged

ngphibang dismissed uLipe’s stale review via b76d869 May 11, 2025 20:59

ngphibang force-pushed the remove_video_endpoint_id branch 2 times, most recently from b76d869 to f305207 Compare May 12, 2025 08:53

avolmat-st requested changes May 12, 2025

View reviewed changes

ngphibang force-pushed the remove_video_endpoint_id branch 2 times, most recently from f477409 to c722650 Compare May 12, 2025 20:34

josuah mentioned this pull request May 12, 2025

drivers: video: API enhancement #72959

Open

avolmat-st reviewed May 13, 2025

View reviewed changes

include/zephyr/drivers/video.h Show resolved Hide resolved

avolmat-st requested changes May 13, 2025

View reviewed changes

ngphibang force-pushed the remove_video_endpoint_id branch from c722650 to 9169626 Compare May 13, 2025 07:10

avolmat-st previously approved these changes May 13, 2025

View reviewed changes

uLipe previously approved these changes May 13, 2025

View reviewed changes

ngphibang added 4 commits May 14, 2025 09:56

drivers: video: esp32: Fix flush callback

3e952cc

When stopping, the framework calls driver's stop callback then driver's flush callback. Hence, driver's flush callback does not need to call stop callback again. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

drivers: video: Drop video_endpoint_id

cf2c07a

The video endpoints are already described in the devicetree. The video_endpoint_id parameter in each video API is not necessary and has no usage. Drop it. Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

drivers: video: Rename signal variables to avoid compliance violation

1cbc868

Rename signal variables to sig to be compliant with code rule 21.2 Signed-off-by: Phi Bang Nguyen <phibang.nguyen@nxp.com>

ngphibang dismissed stale reviews from uLipe and avolmat-st via 1cbc868 May 14, 2025 08:22

ngphibang force-pushed the remove_video_endpoint_id branch from 9169626 to 1cbc868 Compare May 14, 2025 08:22

avolmat-st approved these changes May 14, 2025

View reviewed changes

josuah approved these changes May 14, 2025

View reviewed changes

dleach02 approved these changes May 16, 2025

View reviewed changes

kartben merged commit 7b42139 into zephyrproject-rtos:main May 16, 2025
26 checks passed

sylvioalves reviewed May 17, 2025

View reviewed changes

ngphibang mentioned this pull request May 18, 2025

video: drivers misc fixes #87366

Merged

ngphibang deleted the remove_video_endpoint_id branch June 24, 2025 15:57

video: Drop video endpoint id in video APIs #87070

video: Drop video endpoint id in video APIs #87070

Uh oh!

Conversation

ngphibang commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josuah commented Mar 19, 2025

Uh oh!

avolmat-st commented Apr 15, 2025

Uh oh!

avolmat-st left a comment

Choose a reason for hiding this comment

Uh oh!

avolmat-st May 12, 2025

Choose a reason for hiding this comment

Uh oh!

ngphibang May 12, 2025

Choose a reason for hiding this comment

Uh oh!

avolmat-st May 12, 2025

Choose a reason for hiding this comment

Uh oh!

ngphibang May 12, 2025

Choose a reason for hiding this comment

Uh oh!

dleach02 commented May 12, 2025

Uh oh!

Uh oh!

avolmat-st left a comment

Choose a reason for hiding this comment

Uh oh!

ngphibang commented May 13, 2025

Uh oh!

avolmat-st left a comment

Choose a reason for hiding this comment

Uh oh!

ngphibang commented May 14, 2025

Uh oh!

sonarqubecloud bot commented May 14, 2025

Quality Gate passed

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sylvioalves May 17, 2025

Choose a reason for hiding this comment

Uh oh!

ngphibang May 17, 2025 • 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!

sylvioalves May 17, 2025 • 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!

ngphibang May 17, 2025 • 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!

sylvioalves May 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngphibang commented Mar 13, 2025 •

edited

Loading

ngphibang May 17, 2025 •

edited

Loading

sylvioalves May 17, 2025 •

edited

Loading

ngphibang May 17, 2025 •

edited

Loading