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

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