On Tue, 6 Oct 2020 10:07:39 +0300 Tomi Valkeinen tomi.valkeinen@ti.com wrote:

Adding Boris who added bus format negotiation.

On 06/10/2020 00:31, Nikhil Devshatwar wrote:

...

Hi all,

I am trying to convert the upstream tidss drm driver to new connector model. The connector is getting created by the tidss driver and bridges are attached with flag DRM_BRIDGE_ATTACH_NO_CONNECTOR Here are some questions, regarding this:

I was looking at this a bit, and below is my understanding. And I'm mostly talking about how things should be with new code, not legacy code. Things are probably a bit more complex if you mix bridges which implement different styles on how to deal with bus formats.

...

1. Most of the info regarding bus_format and bus flags is coming from

the bridges. Is it okay to not populate connector->display_info with bus_format and flags?

drm_display_info describes the connected display and what goes on the wire to the display.

For monitors that's quite clear, and the data in display_info would reflect what the last bridge needs to output. Most of the data comes from EDID, but I think bus format and flags do not. So a bridge would need to fill them in, which doesn't make sense when we have a chain of bridges (which would be the bridge to fill the data?). So for monitors, I think bus flags and formats in display_info are unused.

For panels, I'm not sure. We have the bridge/panel.c which wraps the actual panel driver, so afaics the panel is essentially the last bridge in the chain, and the connector is kind of a dummy connector. But the panel driver fills in the display_info, and that's where the bridge/panel.c gets the bus formats & flags for the negotiation.

Probably the above could be changed so that the panels take part of the negotiation process, and then the bus formats and flags fields in the display_info could be removed.

Yep, that'd be better to have the bus format/flags info provided by the panel itself rather than passed through display info.

...

2. The "drm_atomic_bridge_chain_select_bus_fmts" does the format

negotiation. So is it okay for the encoder to simply pick the bus_format from the first bridge's state?

Yes, I think that is the idea. The first bridge's input is what the display controller's encoder should output, and the negotiation should take care to provide something in the first bridge's state for the input.

Exactly.

...

3. What is the meaning of MEDIA_BUS_FMT_FIXED? Does it mean that the

bridge does not change the format from input to output?

I think it just means "undefined" here, and it's up to the drivers to decide what to do. I presume this is mostly for drivers that don't support the new stuff, as each bridge should be able to tell what formats & flags it supports.

Correct.

...

4. The bus_flags are available in bridge->timings->input_bus_flags and

also in bridge_state->input_bus_cfg.flags. Which one should be used?

I think bridge_state->input_bus_cfg. Although bridge->timings->input_bus_flags has some data that's not in input_bus_cfg. If the drivers support the negotiation, I don't think bridge->timings->input_bus_flags has any use.

Oh, I didn't realize there was an input_bus_flags in the timings struct. We should probably propagate those in drm_atomic_bridge_propagate_bus_flags().

Probably bridge->timings->input_bus_flags should be used as a fallback. So if a bridge is asked to use MEDIA_BUS_FMT_FIXED as output (i.e. the next bridge doesn't support negotiation), then the bridge might use a default format and also see if the next bridge has bridge->timings->input_bus_flags.

I think this could be automated in drm_atomic_bridge_propagate_bus_flags(). Right now we simply propagate the output bus flags to the input end [1], but it probably makes more sense to use the value in bridge->timings->input_bus_flags if present.

[1]https://elixir.bootlin.com/linux/v5.9-rc8/source/drivers/gpu/drm/drm_bridge....