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

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Boris,

On Wed, 2019-10-23 at 17:45 +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>
> ---
> Hi Philipp,
> 
> I think I addressed all your comments except the addition of
> SYNC_DRIVE_POSEDGE/NEGEDGE flags, which would require changing
> ipu_crtc_mode_set_nofb() to take those flags into account or
> turning those flags into their PIXDATA counterpart. If you don't mind,
> I'd like to leave that for later.

I'm fine with this. I think it was phrased as a suggestion, too.

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

GBR888 likely isn't useful (it's only used for the internal TV Encoder
on i.MX5 so far), but it doesn't really hurt to include it either.

> +	MEDIA_BUS_FMT_RGB666_1X18,
> +	MEDIA_BUS_FMT_RGB666_1X24_CPADHI,
> +	MEDIA_BUS_FMT_RGB565_1X16,
> +};
> +
> +static u32 *
> +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)
> +{
> +	struct drm_display_info *di = &conn_state->connector->display_info;
> +	struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge);
> +	u32 *output_fmts;
> +
> +	if (!imxpd->bus_format && !di->num_bus_formats)
> +		*num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts);

In this case we could just:

		return kmemdup(imx_pd_bus_fmts, sizeof(imx_pd_bus_fmts));

and simplify the remaining function a tiny bit.

> +	else
> +		*num_output_fmts = 1;
> +
> +	output_fmts = kcalloc(*num_output_fmts, sizeof(*output_fmts),
> +			      GFP_KERNEL);
> +	if (!output_fmts)
> +		return NULL;
> +
> +	if (!imxpd->bus_format && di->num_bus_formats)
> +		output_fmts[0] = di->bus_formats[0];
> +	else if (!imxpd->bus_format)
> +		memcpy(output_fmts, imx_pd_bus_fmts,
> +		       ARRAY_SIZE(imx_pd_bus_fmts));
> +	else
> +		output_fmts[0] = imxpd->bus_format;
> +
> +	return output_fmts;
> +}
> +
> +static u32 *
> +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)
> +{
> +	struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge);
> +	u32 *input_fmts;
> +
> +	*num_input_fmts = 0;
> +	if (output_fmt == MEDIA_BUS_FMT_FIXED) {
> +		/*
> +		 * The next bridge does not support bus format negotiation,
> +		 * let's use the default RGB888 value.
> +		 */
> +		*num_input_fmts = 1;
> +		output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
> +	} 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;
> +			}
> +		}

If this loop was split out into a helper function, it could be reused in
.atomic_check below, for example:

		if (imx_pd_format_supported(output_fmt))
			*num_input_fmts = 1;

> +	} else if (imxpd->bus_format == output_fmt) {
> +		*num_input_fmts = 1;
> +	}
> +
> +	if (!*num_input_fmts)
> +		return NULL;
> +
> +	input_fmts = kcalloc(*num_input_fmts, sizeof(*input_fmts),
> +			     GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	input_fmts[0] = output_fmt;
> +	return input_fmts;
> +}
> +
> +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 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 imx_parallel_display *imxpd = bridge_to_imxpd(bridge);
> +	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);
> +
> +	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;
> +
> +	bus_fmt = bridge_state->input_bus_cfg.fmt;
> +	for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) {
> +		if (imx_pd_bus_fmts[i] == bus_fmt)
> +			break;
>  	}
> +
> +	if (i == ARRAY_SIZE(imx_pd_bus_fmts))
> +		return -EINVAL;

See above, this could just reuse:

	if (!imx_pd_format_supported(bus_fmt))
		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;
> +
> +	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->input_bus_cfg.fmt;
>  	imx_crtc_state->di_hsync_pin = 2;
>  	imx_crtc_state->di_vsync_pin = 3;
[...] 

Otherwise this looks good to me.

regards
Philipp