[PATCH v2 17/21] drm/imx: pd: Use bus format/flags provided by the bridge when available

[Philipp Zabel]

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 at 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