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

From: Philipp Zabel <p.zabel@pengutronix.de>
To: Boris Brezillon <boris.brezillon@collabora.com>,
	dri-devel@lists.freedesktop.org
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Andrey Smirnov <andrew.smirnov@gmail.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Chris Healy <Chris.Healy@zii.aero>,
	kernel@collabora.com, Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH v2 17/21] drm/imx: pd: Use bus format/flags provided by the bridge when available
Date: Tue, 27 Aug 2019 10:15:19 +0200	[thread overview]
Message-ID: <1566893719.4102.1.camel@pengutronix.de> (raw)
In-Reply-To: <20190826152649.13820-18-boris.brezillon@collabora.com>

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.

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

> +	} else if (di->num_bus_formats) {
> +		*num_output_fmts = 1;

num_output_fmts is already set to one above.

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

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

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

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

> +	bridge_state->output_bus_cfg.flags = bus_flags;
> +	bridge_state->input_bus_cfg.flags = bus_flags;
> +	imx_crtc_state->bus_flags = bus_flags;
> +	imx_crtc_state->bus_format = bridge_state->output_bus_cfg.fmt;

This should be the input bus format.

>  	imx_crtc_state->di_hsync_pin = 2;
>  	imx_crtc_state->di_vsync_pin = 3;
>  
> @@ -143,10 +245,12 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
>  	.destroy = imx_drm_encoder_destroy,
>  };
>  
> -static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
> -	.enable = imx_pd_encoder_enable,
> -	.disable = imx_pd_encoder_disable,
> -	.atomic_check = imx_pd_encoder_atomic_check,
> +static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
> +	.enable = imx_pd_bridge_enable,
> +	.disable = imx_pd_bridge_disable,
> +	.atomic_check = imx_pd_bridge_atomic_check,
> +	.atomic_get_input_bus_fmts = imx_pd_bridge_atomic_get_input_bus_fmts,
> +	.atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
>  };
>  
>  static int imx_pd_register(struct drm_device *drm,
> @@ -166,7 +270,7 @@ static int imx_pd_register(struct drm_device *drm,
>  	 */
>  	imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
>  
> -	drm_encoder_helper_add(encoder, &imx_pd_encoder_helper_funcs);
> +	encoder->bridge.funcs = &imx_pd_bridge_funcs;
>  	drm_encoder_init(drm, encoder, &imx_pd_encoder_funcs,
>  			 DRM_MODE_ENCODER_NONE, NULL);

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel