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 10:15:19 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Hi Boris,
> 
> On Mon, 2019-08-26 at 17:26 +0200, Boris Brezillon wrote:
> > Now that bridges can expose the bus format/flags they expect, we can
> > use those instead of the relying on the display_info provided by the
> > connector (which is only valid if the encoder is directly connected
> > to bridge element driving the panel/display).
> > 
> > We also explicitly expose the bus formats supported by our encoder by
> > filling encoder->output_bus_caps with proper info.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Changes in v2:
> > * Adjust things to match the new bus-format negotiation infra
> > ---
> >  drivers/gpu/drm/imx/parallel-display.c | 136 ++++++++++++++++++++++---
> >  1 file changed, 120 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > index 35518e5de356..df47cb74759f 100644
> > --- a/drivers/gpu/drm/imx/parallel-display.c
> > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > @@ -89,37 +89,139 @@ static struct drm_encoder *imx_pd_connector_best_encoder(
> >  	return &imxpd->encoder;
> >  }
> >  
> > -static void imx_pd_encoder_enable(struct drm_encoder *encoder)
> > +static void imx_pd_bridge_enable(struct drm_bridge *bridge)
> >  {
> > +	struct drm_encoder *encoder = bridge_to_encoder(bridge);
> >  	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> >  
> >  	drm_panel_prepare(imxpd->panel);
> >  	drm_panel_enable(imxpd->panel);
> >  }
> >  
> > -static void imx_pd_encoder_disable(struct drm_encoder *encoder)
> > +static void imx_pd_bridge_disable(struct drm_bridge *bridge)
> >  {
> > +	struct drm_encoder *encoder = bridge_to_encoder(bridge);
> >  	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
> >  
> >  	drm_panel_disable(imxpd->panel);
> >  	drm_panel_unprepare(imxpd->panel);
> >  }
> >  
> > -static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder,
> > -				       struct drm_crtc_state *crtc_state,
> > -				       struct drm_connector_state *conn_state)
> > +static const u32 imx_pd_bus_fmts[] = {
> > +	MEDIA_BUS_FMT_RGB888_1X24,
> > +	MEDIA_BUS_FMT_RGB565_1X16,
> > +	MEDIA_BUS_FMT_RGB666_1X18,
> > +	MEDIA_BUS_FMT_RGB666_1X24_CPADHI,  
> 
> ipu-dc.c also supports MEDIA_BUS_FMT_BGR888_1X24.

Will add that one to the list.

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

> 
> > +	} else if (di->num_bus_formats) {
> > +		*num_output_fmts = 1;  
> 
> num_output_fmts is already set to one above.

Correct, I'll get rid of this assignment.

> 
> > +		if (output_fmts)
> > +			output_fmts[0] = di->bus_formats[0];
> > +	} else {
> > +		*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));
> > +	}
> > +}
> > +
> > +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.

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

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

> 
> > +
> > +	if (next_bridge_state)
> > +		bus_flags = next_bridge_state->input_bus_cfg.flags;
> > +	else if (!imxpd->bus_format && di->num_bus_formats)
> > +		bus_flags = di->bus_flags;
> > +	else
> > +		bus_flags = imxpd->bus_flags;
> > +
> > +	for (i = 0; bus_fmt && i < ARRAY_SIZE(imx_pd_bus_fmts); i++) {
> > +		if (imx_pd_bus_fmts[i] == bridge_state->output_bus_cfg.fmt)
> > +			break;
> >  	}
> > +
> > +	if (i == ARRAY_SIZE(imx_pd_bus_fmts))
> > +		return -EINVAL;
> > +
> > +	if (bus_flags &
> > +	    ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
> > +	      DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
> > +	      DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE))
> > +		return -EINVAL;  
> 
> This could allow DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE/NEGEDGE as well, if
> they are not opposite to PIXDATA_DRIVE.

Okay, will add those flags and check that PIXDATA and SYNC are
consistent.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel