[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 10:43 +0200, Boris Brezillon wrote:
[...]
> > > +static void
> > > +imx_pd_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
> > > +					 struct drm_bridge_state *bridge_state,
> > > +					 struct drm_crtc_state *crtc_state,
> > > +					 struct drm_connector_state *conn_state,
> > > +					 unsigned int *num_output_fmts,
> > > +					 u32 *output_fmts)
> > >  {
> > > +	struct drm_display_info *di = &conn_state->connector->display_info;
> > > +	struct drm_encoder *encoder = bridge_to_encoder(bridge);
> > > +	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> > > +
> > > +	*num_output_fmts = 1;
> > > +	if (imxpd->bus_format) {
> > > +		if (output_fmts)
> > > +			output_fmts[0] = imxpd->bus_format;  
> > 
> > This is the legacy DT configured interface-pix-fmt. Maybe this should be
> > moved to the last place.
> 
> Hm, shouldn't we restrict things to a single format when we have the
> DT prop defined?

Absolutely. This was just a cosmetic remark. I'm suggesting to put this
branch below the other two, to indicate its relative importance.

[...]
> > > +static void
> > > +imx_pd_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> > > +					struct drm_bridge_state *bridge_state,
> > > +					struct drm_crtc_state *crtc_state,
> > > +					struct drm_connector_state *conn_state,
> > > +					u32 output_fmt,
> > > +					unsigned int *num_input_fmts,
> > > +					u32 *input_fmts)
> > > +{
> > > +	struct drm_encoder *encoder = bridge_to_encoder(bridge);
> > > +	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> > > +
> > > +	*num_input_fmts = 0;
> > > +	if (output_fmt == MEDIA_BUS_FMT_FIXED) {
> > > +		*num_input_fmts = 1;  
> > 
> > I don't understand this. The bus format stored in imx_crtc_state by
> > atomic_check is used to configure the DC later. This should never be
> > MEDIA_BUS_FMT_FIXED, that would trigger a WARN_ON in
> > ipu_bus_format_to_map().
> 
> MEDIA_BUS_FMT_FIXED is the default value used when the next bridge in
> the chain does not support bus format negotiation. When the driver
> receives this value, it should pick a default value (which I might have
> gotten wrong here) so that it keeps working even if some elements of
> the bridge chain don't support negotiating the bus format yet.

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.

Is this something that is planned to be removed in the future, once
negotiation support is commonplace?

> > 
> > > +	} else if (!imxpd->bus_format) {
> > > +		unsigned int i;
> > > +
> > > +		for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) {
> > > +			if (imx_pd_bus_fmts[i] == output_fmt) {
> > > +				*num_input_fmts = 1;
> > > +				break;
> > > +			}
> > > +		}
> > > +	} else if (imxpd->bus_format == output_fmt) {
> > > +		*num_input_fmts = 1;
> > > +	}
> > > +
> > > +	if (*num_input_fmts && input_fmts)
> > > +		input_fmts[0] = MEDIA_BUS_FMT_FIXED;  
> > 
> > The parallel-display driver basically represents the wiring, pinmux, and
> > drivers between the IPU DI and the DISP pads. The input format should
> > always be identical to the output 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?

I'd like this to be consistent. If encoder-bridges don't use the input
bus format in general, I'm fine with this.

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.

> > > +static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
> > > +				      struct drm_bridge_state *bridge_state,
> > > +				      struct drm_crtc_state *crtc_state,
> > > +				      struct drm_connector_state *conn_state)
> > > +{
> > > +	struct drm_encoder *encoder = bridge_to_encoder(bridge);
> > >  	struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state);
> > >  	struct drm_display_info *di = &conn_state->connector->display_info;
> > >  	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> > > +	struct drm_bridge_state *next_bridge_state = NULL;
> > > +	struct drm_bridge *next_bridge;
> > > +	u32 bus_flags, bus_fmt;
> > > +	unsigned int i;
> > >  
> > > -	if (!imxpd->bus_format && di->num_bus_formats) {
> > > -		imx_crtc_state->bus_flags = di->bus_flags;
> > > -		imx_crtc_state->bus_format = di->bus_formats[0];
> > > -	} else {
> > > -		imx_crtc_state->bus_flags = imxpd->bus_flags;
> > > -		imx_crtc_state->bus_format = imxpd->bus_format;
> > > +	next_bridge = drm_bridge_chain_get_next_bridge(bridge);
> > > +	if (next_bridge)
> > > +		next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
> > > +								    next_bridge);
> > > +
> > > +	bus_fmt = bridge_state->output_bus_cfg.fmt;
> > > +	if (bus_fmt == MEDIA_BUS_FMT_FIXED)
> > > +		bus_fmt = 0;  
> > 
> > I would expect this to return with -EINVAL if bridge_state-
> > > output_bus_cfg.fmt is not in the list of supported formats.
> 
> 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.

regards
Philipp