Hello, On Thu, Sep 23, 2021 at 09:49:03AM +0100, Paul Cercueil wrote: > Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller a écrit : > > Hi Paul, > > thanks for another update. > > > > We have been delayed to rework the CI20 HDMI code on top of your series > > but it basically works in some situations. There is for example a problem > > if the EDID reports DRM_COLOR_FORMAT_YCRCB422 but it appears to be outside > > of your series. > > I think the SoC can output YCbCr as well, but I never tried to use it. > > > The only issue we have is described below. > > > >> Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>: > >> > >> Attach a top-level bridge to each encoder, which will be used for > >> negociating the bus format and flags. > >> > >> All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR. > >> > >> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------ > >> 1 file changed, 70 insertions(+), 22 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> index a5e2880e07a1..a05a9fa6e115 100644 > >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > >> @@ -21,6 +21,7 @@ > >> #include <drm/drm_atomic.h> > >> #include <drm/drm_atomic_helper.h> > >> #include <drm/drm_bridge.h> > >> +#include <drm/drm_bridge_connector.h> > >> #include <drm/drm_color_mgmt.h> > >> #include <drm/drm_crtc.h> > >> #include <drm/drm_crtc_helper.h> > >> @@ -108,6 +109,19 @@ struct ingenic_drm { > >> struct drm_private_obj private_obj; > >> }; > >> > >> +struct ingenic_drm_bridge { > >> + struct drm_encoder encoder; > >> + struct drm_bridge bridge, *next_bridge; > >> + > >> + struct drm_bus_cfg bus_cfg; > >> +}; > >> + > >> +static inline struct ingenic_drm_bridge * > >> +to_ingenic_drm_bridge(struct drm_encoder *encoder) > >> +{ > >> + return container_of(encoder, struct ingenic_drm_bridge, encoder); > >> +} > >> + > >> static inline struct ingenic_drm_private_state * > >> to_ingenic_drm_priv_state(struct drm_private_state *state) > >> { > >> @@ -668,11 +682,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, > >> { > >> struct ingenic_drm *priv = drm_device_get_priv(encoder->dev); > >> struct drm_display_mode *mode = &crtc_state->adjusted_mode; > >> - struct drm_connector *conn = conn_state->connector; > >> - struct drm_display_info *info = &conn->display_info; > >> + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder); > >> unsigned int cfg, rgbcfg = 0; > >> > >> - priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS; > >> + priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS; > >> > >> if (priv->panel_is_sharp) { > >> cfg = JZ_LCD_CFG_MODE_SPECIAL_TFT_1 | JZ_LCD_CFG_REV_POLARITY; > >> @@ -685,19 +698,19 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, > >> cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; > >> if (mode->flags & DRM_MODE_FLAG_NVSYNC) > >> cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW; > >> - if (info->bus_flags & DRM_BUS_FLAG_DE_LOW) > >> + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_DE_LOW) > >> cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW; > >> - if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) > >> + if (bridge->bus_cfg.flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) > >> cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE; > >> > >> if (!priv->panel_is_sharp) { > >> - if (conn->connector_type == DRM_MODE_CONNECTOR_TV) { > >> + if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) { > >> if (mode->flags & DRM_MODE_FLAG_INTERLACE) > >> cfg |= JZ_LCD_CFG_MODE_TV_OUT_I; > >> else > >> cfg |= JZ_LCD_CFG_MODE_TV_OUT_P; > >> } else { > >> - switch (*info->bus_formats) { > >> + switch (bridge->bus_cfg.format) { > >> case MEDIA_BUS_FMT_RGB565_1X16: > >> cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT; > >> break; > >> @@ -723,20 +736,29 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, > >> regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg); > >> } > >> > >> -static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder, > >> - struct drm_crtc_state *crtc_state, > >> - struct drm_connector_state *conn_state) > >> +static int ingenic_drm_bridge_attach(struct drm_bridge *bridge, > >> + enum drm_bridge_attach_flags flags) > >> +{ > >> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder); > >> + > >> + return drm_bridge_attach(bridge->encoder, ib->next_bridge, > >> + &ib->bridge, flags); > >> +} > >> + > >> +static int ingenic_drm_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_display_info *info = &conn_state->connector->display_info; > >> struct drm_display_mode *mode = &crtc_state->adjusted_mode; > >> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder); > >> > >> - if (info->num_bus_formats != 1) > >> - return -EINVAL; > >> + ib->bus_cfg = bridge_state->output_bus_cfg; > >> > >> if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV) > >> return 0; > >> > >> - switch (*info->bus_formats) { > >> + switch (bridge_state->output_bus_cfg.format) { > >> case MEDIA_BUS_FMT_RGB888_3X8: > >> case MEDIA_BUS_FMT_RGB888_3X8_DELTA: > >> /* > >> @@ -900,8 +922,16 @@ static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = { > >> }; > >> > >> static const struct drm_encoder_helper_funcs ingenic_drm_encoder_helper_funcs = { > >> - .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set, > >> - .atomic_check = ingenic_drm_encoder_atomic_check, > >> + .atomic_mode_set = ingenic_drm_encoder_atomic_mode_set, > >> +}; > >> + > >> +static const struct drm_bridge_funcs ingenic_drm_bridge_funcs = { > >> + .attach = ingenic_drm_bridge_attach, > >> + .atomic_check = ingenic_drm_bridge_atomic_check, > >> + .atomic_reset = drm_atomic_helper_bridge_reset, > >> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > >> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > >> + .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_propagate_bus_fmt, > >> }; > >> > >> static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = { > >> @@ -976,7 +1006,9 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > >> struct drm_plane *primary; > >> struct drm_bridge *bridge; > >> struct drm_panel *panel; > >> + struct drm_connector *connector; > >> struct drm_encoder *encoder; > >> + struct ingenic_drm_bridge *ib; > >> struct drm_device *drm; > >> void __iomem *base; > >> long parent_rate; > >> @@ -1154,20 +1186,36 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > >> bridge = devm_drm_panel_bridge_add_typed(dev, panel, > >> DRM_MODE_CONNECTOR_DPI); > >> > >> - encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL); > >> - if (IS_ERR(encoder)) { > >> - ret = PTR_ERR(encoder); > >> + ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder, > >> + NULL, DRM_MODE_ENCODER_DPI, NULL); > >> + if (IS_ERR(ib)) { > >> + ret = PTR_ERR(ib); > >> dev_err(dev, "Failed to init encoder: %d\n", ret); > >> return ret; > >> } > >> > >> - encoder->possible_crtcs = 1; > >> + encoder = &ib->encoder; > >> + encoder->possible_crtcs = drm_crtc_mask(&priv->crtc); > >> > >> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs); > >> > >> - ret = drm_bridge_attach(encoder, bridge, NULL, 0); > >> - if (ret) > >> + ib->bridge.funcs = &ingenic_drm_bridge_funcs; > >> + ib->next_bridge = bridge; > >> + > >> + ret = drm_bridge_attach(encoder, &ib->bridge, NULL, > >> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR makes it fundamentally incompatible > > with synopsys/dw_hdmi.c > > > > That driver checks for DRM_BRIDGE_ATTACH_NO_CONNECTOR being NOT present, > > since it wants to register its own connector through > > dw_hdmi_connector_create(). Does it ? The driver has static int dw_hdmi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct dw_hdmi *hdmi = bridge->driver_private; if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) return drm_bridge_attach(bridge->encoder, hdmi->next_bridge, bridge, flags); return dw_hdmi_connector_create(hdmi); } If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, it will create a connector, otherwise it will just attach to the next bridge. I'm using it on a Renesas platform with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set, without any issue as far as I can tell. > > It does it for a reason: the dw-hdmi is a multi-function driver which does > > HDMI and DDC/EDID stuff in a single driver (because I/O registers and power > > management seem to be shared). > > The IT66121 driver does all of that too, and does not need > DRM_BRIDGE_ATTACH_NO_CONNECTOR. The drm_bridge_funcs struct has > callbacks to handle cable detection and DDC stuff. > > > Since I do not see who could split this into a separate bridge and a connector driver > > and test it on multiple SoC platforms (there are at least 3 or 4), I think modifying > > the fundamentals of the dw-hdmi architecture just to get CI20 HDMI working is not > > our turf. > > You could have a field in the dw-hdmi pdata structure, that would > instruct the driver whether or not it should use the new API. Ugly, I > know, and would probably duplicate a lot of code, but that would allow > other drivers to be updated at a later date. > > > Therefore the code here should be able to detect if drm_bridge_attach() already > > creates and attaches a connector and then skip the code below. > > Not that easy, unfortunately. On one side we have dw-hdmi which checks > that DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set, and on the other side > we have other drivers like the IT66121 which will fail if this flag is > not set. > >> + if (ret) { > >> + dev_err(dev, "Unable to attach bridge\n"); > >> return ret; > >> + } > >> + > >> + connector = drm_bridge_connector_init(drm, encoder); > >> + if (IS_ERR(connector)) { > >> + dev_err(dev, "Unable to init connector\n"); > >> + return PTR_ERR(connector); > >> + } > >> + > >> + drm_connector_attach_encoder(connector, encoder); > >> } > >> > >> drm_for_each_encoder(encoder, drm) { > >> -- > >> 2.33.0 > > > > I haven't replaced v2 with v3 in our test tree yet, but will do asap. -- Regards, Laurent Pinchart