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 macro to deal with endpoints #73023

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

josuah
Copy link
Collaborator

@josuah josuah commented May 20, 2024

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

This depends on:

It introduces some way to point at devicetree objects and use them in the API.
It assumes that endpoint numbers can be passed where enum video_endpoint_id ep is present through the API.

Example:

vid0: videodev@10380000 {
	ports {
		#address-cells = <2>;
		#size-cells = <0>;

		vid0port0in: endpoint@0000 {
			reg = <0 0>;
			data-lanes = <0 1 2 3>;
			remote-endpoint = <&other>;
		};

		vid0port0out: endpoint@0001 {
			reg = <0 1>;
			data-lanes = <4 5 6 7>;
			remote-endpoint = <&other>;
		};

		vid0port1in: endpoint@0100 {
			reg = <1 0>;
			data-lanes = <0 1 2 3>;
			remote-endpoint = <&other>;
		};
	};
};

It is then possible to refer vid0port0out like this:

video_set_signal(DEVICE_DT_GET(DT_NODELABEL(vid0)), VIDEO_DT_ENDPOINT_ID(vid0port0out));

@josuah josuah marked this pull request as draft May 20, 2024 12:10
@zephyrbot zephyrbot added the area: Video Video subsystem label May 20, 2024
@josuah
Copy link
Collaborator Author

josuah commented May 20, 2024

This needs testing after #72950 is merged.

@loicpoulain
Copy link
Collaborator

loicpoulain commented May 20, 2024

Looks promising, would it make sense to have in/out type encoded/described?

@josuah
Copy link
Collaborator Author

josuah commented May 20, 2024

I picked the first nibble to encode a custom field that will say either "IN" or "OUT" or "EITHER" or "NONE", which is showcased here:

https://github.com/zephyrproject-rtos/zephyr/pull/73009/files#diff-70c4de6949b97c13ac893b4bfb16fbcc433593fbfa96ec8947bb5cb83c4d04bcR129-R136

This allows to reference individual endpoint through the video
interface, while also allowing the "catch-all" generic enums.

This defines semantics for each fields, helping driver developers know
precisely how to implement every filter.

Signed-off-by: Josuah Demangeon <me@josuah.net>
Apply the semantics defined in <zephyr/drivers/video.h> for specifying
the endpoint so that the user can pass either the endpoint number either
a generic placeholder (_ANY or _OUT).

Signed-off-by: Josuah Demangeon <me@josuah.net>
This allows referring to one particular interface more easily from
the application or drivers definitions.

The generated endpoint ID is extracted from the address: the reg
property from the devicetree, and encodes information about the port
direction.

Signed-off-by: Josuah Demangeon <me@josuah.net>
@josuah
Copy link
Collaborator Author

josuah commented May 21, 2024

This is now rebased on top of #73009.

@ngphibang
Copy link
Contributor

ngphibang commented May 22, 2024

@josuah : I will need sometimes to look at this closer. But does this resolve the "circular dependency" and the "chicken-egg" init order issues or this is just APIs to deal with endpoints ? So, my questions

  • With these, can we obtain a reference to the video object pointed by the remote-endpoint so that we don't need to use direct phandle references in DT (currently this method is used extensively in Zephyr) ?
  • If so, how do you handle the case that the "remote" video object is not available yet (due to the init order priority) ? As an example, the CSI driver needs to initialize before the mt9m114 camera sensor driver to provide the master clock for the sensor. But in the CSI driver, we need to obtain a reference to the mt9m114 sensor (to propagate formats, etc.)
  • ports / endpoints / remote-endpoints are not only limit to video, they are used for display and other things too. Why don't we put these things in the zephyr core instead of the video subsystem ?

@josuah
Copy link
Collaborator Author

josuah commented May 22, 2024

With these, can we obtain a reference to the video object pointed by the remote-endpoint so that we don't need to use direct phandle references in DT (currently this method is used extensively in Zephyr) ?

Yes, the various struct ..._dt_spec were the source if inspiration. Now this video_dt_spec is built to refer the local device: useful by the application only that would refer a VIDEO_DT_SPEC_GET(DT_NODELABEL(csi_0_out))...

The driver would still have to do something like this to follow the remote endpoint, and then call video_dt_spec on that:

spec = VIDEO_DT_SPEC_GET(DT_INST_PROP(DT_CHILD(DT_CHILD(n, ports), endpoint_0), remote_endpoint))

Maybe a VIDEO_DT_REMOTE_ENDPOINT_GET() could wrap that to help using it in drivers...


If so, how do you handle the case that the "remote" video object is not available yet (due to the init order priority) ? As an example, the CSI driver needs to initialize before the mt9m114 camera sensor driver to provide the master clock for the sensor. But in the CSI driver, we need to obtain a reference to the mt9m114 sensor (to propagate formats, etc.)

An application connecting two video ports might also face this problem, so maybe the solution is in the samples:
https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/subsys/video/capture/src/main.c#L50
https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/subsys/video/capture/src/main.c#L70

The calls that could be recursive (get/set feed parameters, and data movement) are set from application, after everything is initialized.

Is there a chance that recursive calls (get data/controls from this device that gets data/controls from this device [...]) happens during initialization?

One alternative would be to have an extra video_enable() API that allows all the initial recursive calls to be performed after initialization is done, like in USB:
https://docs.zephyrproject.org/latest/connectivity/usb/device_next/api/usbd.html#c.usbd_enable


ports / endpoints / remote-endpoints are not only limit to video, they are used for display and other things too. Why don't we put these things in the zephyr core instead of the video subsystem ?

This makes more sense as you said. And it looks like the "ports / port / endpoint / remote-endpoint" logic is even encoded into the devicetree tool itself:
https://github.com/torvalds/linux/blob/dccb07f2914cdab2ac3a5b6c98406f765acab803/scripts/dtc/checks.c#L1770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Video Video subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants