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

[Boris Brezillon <boris.brezillon@collabora.com>]

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:
> [...]
> > > > +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.

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;
	}


> 
> [...]
> > > > +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.

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.

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

Ideally yes.

> 
> > >   
> > > > +	} 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?

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).

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

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

> 
> > > > +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.

What if imxpd->bus_format is 0? Should I return -EINVAL?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel