On Tue, 2019-08-27 at 11:57 +0200, Boris Brezillon wrote:
On Tue, 27 Aug 2019 11:23:02 +0200 Philipp Zabel p.zabel@pengutronix.de wrote:
On Tue, 2019-08-27 at 10:43 +0200, Boris Brezillon wrote:
[...]
Absolutely. This was just a cosmetic remark. I'm suggesting to put this branch below the other two, to indicate its relative importance.
Okay, something like that?
*num_output_fmts = 1; if (!imxpd->bus_format && di->num_bus_formats) { if (output_fmts) output_fmts[0] = di->bus_formats[0]; } else if (!imxpd->bus_format) { *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); if (output_fmts) memcpy(output_fmts, imx_pd_bus_fmts, ARRAY_SIZE(imx_pd_bus_fmts)); } else if (output_fmts) { output_fmts[0] = imxpd->bus_format; }
Yes, thank you.
[...]
That was unexpected, as MEDIA_BUS_FMT_FIXED is documented as:
"MEDIA_BUS_FMT_FIXED shall be used by host-client pairs, where the data format is fixed."
in include/uapi/linux/media-bus-format.h. I read this as something for a fixed link between two known devices where there is only one possible format. Here we can't possibly know what the other side expects.
Well, it's more or less what happens right now without the bus format negotiation: bridge elements consider that the link uses a fixed format that's known by both ends in advance.
Ok. And with the legacy DT provided bus format we can even choose a sensible format.
[...]
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). select_bus_fmt_recursive could just check atomic_get_output_bus_fmts if that is implemented, but atomic_get_input_bus_fmts isn't.
That point is moot though, if we propagate the output format to the input format.
I'd like this to be consistent. If encoder-bridges don't use the input bus format in general, I'm fine with this.
Propagating output to input doesn't hurt, and if you think that's clearer this way I'm perfectly fine adjusting the driver accordingly.
Yes, I'd like that.
I was just confused that the bridge takes part in input format negotiation and then carries on using the output format to configure its input.
Maybe I'm wrong, but I thought it was the parallel (AKA DPI) encoder output we were configuring here. Anyway, I'll choose one semantic and document it.
Semantics :) You are not wrong, the IPU DIs technically output DPI compatible signals, internally. Those are fed through muxes either into the HDMI/LVDS/TV encoders, or directly into the IOMUX block, which drives them onto the DISP pads.
The IPU driver is from a time before bridges and of-graph bindings were established. We chose to consider the whole IDMAC/DP/DC/DI path as crtc, and the HDMI / LVDS / IOMUX/DISP blocks as encoder, because the crtc- encoder assignment mapped well to configuring the muxes between the components. Technically we could just as well consider IDMAC/DP/part-of-DC to be the crtc and part-of-DC/DI to be a DPI encoder to which the HDMI/LVDS/TV bridges are connected, but today we'd still miss the possibility to multiplex several encoders into the same bridge.
[...]
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.
regards Philipp