On Tue, 27 Aug 2019 14:51:20 +0200 Philipp Zabel p.zabel@pengutronix.de wrote:
[...]
I can do that if you like. Note that we are forwarding the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. I just assumed that input format wouldn't be used in the dummy bridge element (the one embedded in the encoder) since encoders only have an output end (input port is likely to be a SoC specific link between the CRTC and the encoder which we probably don't need/want to expose).
Then why (would this driver have to) implement get_input_fmts at all?
That's the only way we can check if an output format is supported: bus format negotiation is based on a trial and error logic that starts from the end of the pipeline (last bridge element) and goes backward until it reaches the first bridge (the dummy encoder bridge). A bus format setting is validated when all ->get_input_bus_fmts() hooks returned at least one possible format (*num_input_formats > 0) for the output format being tested by the next element in the chain. And that's why even the dummy encoder bridge has to implement this hook unless it only supports one output format (the core returns MEDIA_BUS_FMT_FIXED when ->get_input_bus_fmts is NULL).
This function (currently) also only returns MEDIA_BUS_FMT_FIXED, so there is no difference in return value (if queried with a supported output_fmt).
There's a small difference actually: when output_fmt != MEDIA_BUS_FMT_FIXED, we make sure output_fmt is something we support. If you don't implement ->get_input_bus_fmts() you'll just accept any format requested by the next element in the pipeline.
select_bus_fmt_recursive could just check atomic_get_output_bus_fmts if that is implemented, but atomic_get_input_bus_fmts isn't.
I'd like to use ->get_output_bus_fmts() only to retrieve supported output formats on the last bridge element, otherwise it makes the whole thing even more complex.
That point is moot though, if we propagate the output format to the input format.
I'll do that then (and mandate that all encoder drivers do the same).
[...]
As said above, we need a way to support bridge chains where not all elements support negotiating the bus format, that's what this fallback is for, but maybe 0 is not an appropriate value to mean 'pick the default format'.
We'd actually have to pick one here. If set, that should be imxpd->bus_format.
What if imxpd->bus_format is 0? Should I return -EINVAL?
I think so. That would certainly be easier to debug than "strange colours" when hooking up a bridge that is incompatible with whatever we'd choose.
Okay.