drivers: video: Introduce macro to deal with endpoints

[josuah]

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

• drivers: video: API enhancement #72959

This depends on:

• dts: flatten video's device tree endpoints definition #72950 (not anymore)
• dts: bindings: video: Add common video interface binding #74415

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]

This needs testing after #72950 is merged.

[loicpoulain]

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

[josuah]

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

[josuah]

This is now rebased on top of #73009.

[ngphibang]

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

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

[josuah]

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

If some hardware datasheet defines an endpoint address 0x8 IN, and 0x80 OUT, and we encode it as 0x81 and 0x800 this could be confusing.

[josuah]

force-push: A completely new approach, much more minimal, not specific to video, and leveraging the remote-endpoint-label introduced in #74415

In the context of video devices, this would allow replacing the source = <...>; and sensor = <...>; used through the devicetree by an unified interface.

I think I will need to add tests for it.

[ngphibang]

I didn't look into details yet but this is what I want to do and glad to see that you are faster :-). It should be a good work-around to retrieve the phandle of the peer device and would be very useful (as I am not sure with the current design of Zephyr DT whether it's possible to do circular dependency with remote-endpoint phandle or not).

Need to pay attention that we can have either "ports -> port -> endpoint" or "port->endpoint".

And of course, it should be tested working :-).

[ngphibang]

Ah, one more point, I think this should be at the 1st time, in the video section only as remote-endpoint-label is valid only in video. This way it will be accepted and merged faster.

[ngphibang]

Ah ... looking closer : DT_NODELABEL(DT_STRING_TOKEN(DT_CHILD(DT_CHILD(node, port), ep), remote_endpoint_label))

It seems the macro just allow to retrieve the string specified in remote_endpoint_label property ? I think the main part is "how to parse through all the devicetree to retrieve the phandle of the peer video device from the string" is still missing ?? This is the challenge part.

[josuah]

I happened to need this for UVC. :)

as I am not sure with the current design of Zephyr DT whether it's possible to do circular dependency with remote-endpoint phandle or not

There is #57708 getting in the way, after which the devicetree can be converted from remote-endpoint-label and the macros as well, and it should hopefully be a smooth transition.

Need to pay attention that we can have either "ports -> port -> endpoint" or "port->endpoint".

I thought we could start with the most basic port > endpoint first to un-block the video situation, and then introduce ports -> port -> endpoint later. For instance, a DT_ENDPOINT(node, port, endpoint) that works for both ports { port@0 ... }; and port ....

in the video section only as remote-endpoint-label is valid only in video

I can move the macros from devicetree.h, but then maybe this should be a VIDEO_DT_... prefix? Video devices would need to be converted from VIDEO_DT_... to DT_... once generalized.

how to parse through all the devicetree to retrieve the phandle of the peer video device from the string

remote-endpoint-label specifies a label:, and what is left is to leverage the existing DT_NODELABLE("label") infrastructure which gives an immediate access. Instead of a string ("label"), it is a token (label).
build/zephyr/include/generated/zephyr/devicetree_generated.h gives a %_STRING_TOKEN suffix that can be passed to the DT_NODELABEL() for every string. A happy accident!

Code at the origin of this PR:
https://github.com/tinyvision-ai-inc/zephyr/blob/pr-usb-uvc-auto/drivers/video/video_emul_mipi_rx.c#L318-L336

I might have time this week-end for this.
Thank you for the prompt feedback!

[ngphibang]

build/zephyr/include/generated/zephyr/devicetree_generated.h gives a %_STRING_TOKEN suffix that can be passed to the DT_NODELABEL() for every string. A happy accident!

Okay, will check this later. But just to emphasize that circular dependency is very important as we need "async init" mechanism as in Linux. Without this, we encounter some deadlock / chicken-egg issues (the one in mt9m114 / CSI on RT1064 and ov7770 / SmartDMA is an example which do not work on upstream up to now unless doing a work-around). I can't understand why Zephyr does not take into account all of this when designing device tree. Will create an issue (for mt9m114 / csi and ov7770 / SmartDMA) and emphasize the problem in the Circular dependency issue (I wanted to do this for a long time but always forgot).

[josuah]

circular dependency is very important

Ah right I forgot that phandles are also used to organize the init sequence!

At the devicetree level: (step 1) the macros are generated, (step 2) the driver macros access the devicetree macros.
This cuts the chicken-and-egg problem at macro-level, but not init-level.

[ngphibang]

This cuts the chicken-and-egg problem at macro-level, but not init-level.

I know, "async init" is more than that. But with the work-around it is sufficient to resolve the current chicken-egg issue in mt9m114 / csi and ov7770 / smartDMA as it happens at compile time due to init priority conflict.

[josuah]

force-push: simplify the macro, add tests

Here is a reduced version with only DT_REMOTE_ENDPOINT(local_endpoint_node) which returns the remote_endpoint_node:

&source {
        port {
                source_out: endpoint {
                        remote-endpoint-label = "sink_in";
                        source-property = <1>;
                };
        };
};

&sink {
        port {
                sink_in: endpoint {
                        remote-endpoint-label = "source_out";
                        sink-property = <2>;
                };
        };
};

DT_PROP(DT_REMOTE_ENDPOINT(DT_NODELABEL(sink_in)), source_property) -> 1
DT_PROP(DT_REMOTE_ENDPOINT(DT_NODELABEL(source_out)), sink_property) -> 2

To handle DT_REMOTE_ENDPOINT() from the parent node:

• "ports -> port -> endpoint": call it on DT_CHILD(DT_CHILD(DT_CHILD(node, ports), port), endpoint)
• "port -> endpoint": call it on DT_CHILD(DT_CHILD(node, port), endpoint)

[ngphibang]

Given real macro usages in drivers as below:

Case 1:

#define DEVICE_DT_INST_GET_SOURCE_DEV(n)                                                           \
	DEVICE_DT_GET(DT_PARENT(DT_GPARENT(DT_NODELABEL(DT_STRING_TOKEN(                           \
		DT_CHILD(DT_INST_CHILD(n, port), endpoint), remote_endpoint_label)))))

the node parameter in this case will be DT_CHILD(DT_INST_CHILD(n, port), endpoint)

Case 2:

#define DEVICE_DT_INST_GET_SENSOR_DEV(n)                                                           \
	DEVICE_DT_GET(DT_GPARENT(DT_NODELABEL(                                                     \
		DT_STRING_TOKEN(DT_CHILD(DT_CHILD(DT_INST_CHILD(n, ports), port_1), endpoint),     \
				remote_endpoint_label))))

the node parameter will be DT_CHILD(DT_CHILD(DT_INST_CHILD(n, ports), port_1), endpoint)

There's still endpointcommon for all cases. Is it possible to take endpoint in the DT_REMOTE_ENDPOINT api ?

Also, to retrieve the peer device object, we always has at least DT_GPARENT, is it possible to also take this in the macro API ?

[josuah]

It is possible to reduce the overhead by putting endpoint in common to each, but then comes looping over every endpoint:

#define ENDPOINT(n) DT_PROP(DT_REMOTE_ENDPOINT(n), link_parameter),

int link_parameters[] = {
        DT_FOREACH_CHILD(DT_INST_CHILD(DT_DRV_INST(0), port), ENDPOINT)
};

So this means that it is also useful to expose an intermediate macro that can be used in both case, only wrapping the remote-endpoint-label -> nodelabel -> node conversion.

It will also be possible to have some macro using COND_CODE_1() to make the "ports" vs "port" abstraction.

#define DT_PORTS(n) \
COND_CODE_1(DT_HAS_PROP(n, ports), (DT_CHILD(n, ports)), (n))

#define DT_INST_PORTS(n) \
DT_PORTS(DT_DRV_INST(n))

/* Example */
DEVICE_DT_GET(DT_GPARENT(DT_REMOTE_ENDPOINT(
        DT_CHILD(DT_CHILD(DT_INST_PORTS(0), port_3, endpoint_2)))))

This will return the node that contains all the ports, for instance:

#define DT_PORTS_REMOTE_ENDPOINT(n, port, endpoint) \
DT_REMOTE_ENDPOINT(DT_CHILD(DT_CHILD(DT_PORTS(DT_DRV_INST(0)), port), endpoint))

#define DT_INST_PORTS_REMOTE_ENDPOINT(n, port, endpoint) \
DT_PORTS(DT_DRV_INST(n))

/* Example */
DEVICE_DT_GET(DT_GPARENT(DT_INST_PORTS_REMOTE_ENDPOINT(0, port_3, endpoint_1)))

Then, there is going down from the other device, and we might need the same check for "ports" present or not.

#define DT_ENDPOINT_DEVICE(n) \
COND_CODE_1(DT_PROP_HAS(DT_GPARENT(n), port), (DT_GPARENT(n)), (DT_PARENT(DT_GPARENT(n))))

/* Example */
DEVICE_DT_GET(DT_ENDPOINT_DEVICE(DT_INST_PORTS_REMOTE_ENDPOINT(0, port_3, endpoint_1)))

And then why not combine them two, as the most frequent use-case for simple drivers would be to pick the device connected through a particular endpoint:

#define DT_REMOTE_DEVICE(n, port, endpoint) \
DT_ENDPOINT_DEVICE(DT_INST_PORTS_REMOTE_ENDPOINT(0, port, endpoint))

#define DT_INST_REMOTE_DEVICE(n, port, endpoint) \
DT_REMOTE_DEVICE(DT_DRV_INST(n), port, endpoint)

/* Example */
DEVICE_DT_GET(DT_INST_REMOTE_DEVICE(0, port, endpoint))

Would it make sense to decompose the implementation this much?

The intermediate macros would need to be implemented anyway to avoid making them too large, so we might as well document and test each intermediate step.

[ngphibang]

I think the current implementation (in the commit) just helps to make a bit shortcut but still let drivers deal with "how to get the endpoint node id", "how to deal with different cases like whether there is ports node, whether port / endpoint has an id (i.e. port@id, endpoint@id)", etc. Then, there are some ideas in your comments, yes. However, we need macros APIs that work for all cases, kinds of

• DT_INST_ENDPOINT_BY_ID(n, pid, id) : to get the (local) endpoint node id from the port id and endpoint id
• DEVICE_DT_GET_REMOTE_DEVICE(ep) : to get the peer remote device from the endpoint node id

Another problem is that although port / endpoint / remote-endpoint-label and these macros are not specific to the video subsystem, they shouldn't be applied for the whole Zephyr for the time-being (i.e. included in device.h / devicetree.h) because without a binding which comes together (e.g. the video-interfaces binding), it will not work (i.e. compile).

It is much easier to make it in video.h first, then generalize it later (which requires to introduce remote-endpoint-label in a generic Zephyr binding though, e.g. the base.yaml).

It's difficult to explain all by words than by code for all aspects so I made this PR, hope you don't mind, I added you as the co-author in the core commit though. Feel free to discuss.

[josuah]

• DT_INST_ENDPOINT_BY_ID(n, pid, id) : to get the (local) endpoint node id from the port id and endpoint id

Great to work with IDs! This is a better API than what I proposed I think...
Maybe there need to be a COND_CODE_1() to ignore port rather than port_##pid, but that is an implementation detail.

• DEVICE_DT_GET_REMOTE_DEVICE(ep) : to get the peer remote device from the endpoint node id

This is probably all what 90% of users need, and combines very well with the above:

DEVICE_DT_GET_REMOTE_DEVICE(DT_INST_ENDPOINT_BY_ID(n, 2, 3))

without a binding which comes together (e.g. the video-interfaces binding), it will not work (i.e. compile).

If a macro is not called, it would not cause any compilation issue. And users who want to use it can also define their remote-endpoint-label string binding without issue. Maybe I missed something... I do not want to break any build of course!

It is much easier to make it in video.h first, then generalize it later (which requires to introduce remote-endpoint-label in a generic Zephyr binding though, e.g. the base.yaml).

It probably cannot be put in base.yaml due to the port { ... }; vs ports { port { ... }; };, but I see your point.
No idea if DT_... macros can be accepted in video.h, but let's discuss this on #80649.

I made this PR

Good idea! Do feel free to cherry-pick the tests or snippets or skip them.

[josuah]

Closing this PR to focus efforts on #80649