| Message ID | 20210922205555.496871-7-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ingenic: Various improvements v3 | expand |
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. The only issue we have is described below. > Am 22.09.2021 um 22:55 schrieb Paul Cercueil <paul@crapouillou.net>: > > 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@crapouillou.net> > --- > 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(). 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). 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. 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. > + 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. BR and thanks, Nikolaus
Hi Nikolaus, Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller <hns@goldelico.com> 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@crapouillou.net>: >> >> 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@crapouillou.net> >> --- >> 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(). > > 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. Cheers, -Paul >> + 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. > > BR and thanks, > Nikolaus > >
Hi Paul, > Am 23.09.2021 um 10:49 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le jeu., sept. 23 2021 at 07:52:08 +0200, H. Nikolaus Schaller <hns@goldelico.com> 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. Maybe there is code missing or something else. We have not yet deeply researched. Except that when ignoring DRM_COLOR_FORMAT_YCRCB422 capability it uses RGB and works. > >>> + 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(). >> 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. Yes, would be very ugly. But generally who has the knowledge (and time) to do this work? And has a working platform to test (jz4780 isn't a good development environment)? The driver seems to have a turbulent history starting 2013 in staging/imx and apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer? > >> 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. Ok, I see. You have to handle contradicting cases here. Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first and retry if it fails without? But IMHO the return value (in error case) is not well defined. So there must be a test if a connector has been created (I do not know how this would work). Another suggestion: can you check if there is a downstream connector defined in device tree (dw-hdmi does not need such a definition)? If not we call it with 0 and if there is one we call it with DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one? Just some ideas how to solve without touching hdmi drivers. BR and thanks, Nikolaus
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@crapouillou.net>: > >> > >> 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@crapouillou.net> > >> --- > >> 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.
Hi Nikolaus, On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote: > > Am 23.09.2021 um 10:49 schrieb Paul Cercueil: > > 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. > > Maybe there is code missing or something else. We have not yet deeply researched. > Except that when ignoring DRM_COLOR_FORMAT_YCRCB422 capability it uses RGB > and works. > > >>> + 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(). > >> 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. > > Yes, would be very ugly. > > But generally who has the knowledge (and time) to do this work? > And has a working platform to test (jz4780 isn't a good development environment)? > > The driver seems to have a turbulent history starting 2013 in staging/imx and > apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer? "Maintainer" would be an overstatement. I've worked on that driver in the past, and I still use it, but don't have time to really maintain it. I've also been told that Synopsys required all patches for that driver developed using documentation under NDA to be submitted internally to them first before being published, so I decided to stop contributing instead of agreeing with this insane process. There's public documentation about the IP in some NXP reference manuals though, so it should be possible to still move forward without abiding by this rule. > >> 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. > > Ok, I see. You have to handle contradicting cases here. > > Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first > and retry if it fails without? > > But IMHO the return value (in error case) is not well defined. So there > must be a test if a connector has been created (I do not know how this > would work). > > Another suggestion: can you check if there is a downstream connector defined in > device tree (dw-hdmi does not need such a definition)? > If not we call it with 0 and if there is one we call it with > DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one? I haven't followed the ful conversation, what the reason why DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? We're moving towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it will have to be done eventually. > Just some ideas how to solve without touching hdmi drivers. > > BR and thanks, > Nikolaus
Hi Laurent, > Am 23.09.2021 um 11:27 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > Hi Nikolaus, > > On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote: >> >>>>> + 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(). >>>> 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. >> >> Yes, would be very ugly. >> >> But generally who has the knowledge (and time) to do this work? >> And has a working platform to test (jz4780 isn't a good development environment)? >> >> The driver seems to have a turbulent history starting 2013 in staging/imx and >> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer? > > "Maintainer" would be an overstatement. I've worked on that driver in > the past, and I still use it, but don't have time to really maintain it. > I've also been told that Synopsys required all patches for that driver > developed using documentation under NDA to be submitted internally to > them first before being published, so I decided to stop contributing > instead of agreeing with this insane process. There's public > documentation about the IP in some NXP reference manuals though, so it > should be possible to still move forward without abiding by this rule. > >>>> 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. >> >> Ok, I see. You have to handle contradicting cases here. >> >> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first >> and retry if it fails without? >> >> But IMHO the return value (in error case) is not well defined. So there >> must be a test if a connector has been created (I do not know how this >> would work). >> >> Another suggestion: can you check if there is a downstream connector defined in >> device tree (dw-hdmi does not need such a definition)? >> If not we call it with 0 and if there is one we call it with >> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one? > > I haven't followed the ful conversation, what the reason why > DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? The synopsys driver creates its own connector through dw_hdmi_connector_create() because the IP handles DDC/EDID directly. Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the right thing to do on current platforms that use it. For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to make HDMI work. Now this patch for drm/ingenic wants the opposite definition and create its own connector. This fails even if we remove the check (then we have two interfering connectors). > We're moving > towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it > will have to be done eventually. So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi. IMHO it should either handle this situation gracefully or include a fix for dw-hdmi.c to keep it compatible. BR and thanks, Nikolaus
Hi Nikolaus, On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller wrote: > > Am 23.09.2021 um 11:27 schrieb Laurent Pinchart: > > On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote: > >> > >>>>> + 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(). > >>>> 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. > >> > >> Yes, would be very ugly. > >> > >> But generally who has the knowledge (and time) to do this work? > >> And has a working platform to test (jz4780 isn't a good development environment)? > >> > >> The driver seems to have a turbulent history starting 2013 in staging/imx and > >> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer? > > > > "Maintainer" would be an overstatement. I've worked on that driver in > > the past, and I still use it, but don't have time to really maintain it. > > I've also been told that Synopsys required all patches for that driver > > developed using documentation under NDA to be submitted internally to > > them first before being published, so I decided to stop contributing > > instead of agreeing with this insane process. There's public > > documentation about the IP in some NXP reference manuals though, so it > > should be possible to still move forward without abiding by this rule. > > > >>>> 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. > >> > >> Ok, I see. You have to handle contradicting cases here. > >> > >> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first > >> and retry if it fails without? > >> > >> But IMHO the return value (in error case) is not well defined. So there > >> must be a test if a connector has been created (I do not know how this > >> would work). > >> > >> Another suggestion: can you check if there is a downstream connector defined in > >> device tree (dw-hdmi does not need such a definition)? > >> If not we call it with 0 and if there is one we call it with > >> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one? > > > > I haven't followed the ful conversation, what the reason why > > DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? > > The synopsys driver creates its own connector through dw_hdmi_connector_create() > because the IP handles DDC/EDID directly. That doesn't require creating a connector though. The driver implements drm_bridge_funcs.get_edid(), which is used to get the EDID without the need to create a connector in the dw-hdmi driver. > Hence it checks for ! DRM_BRIDGE_ATTACH_NO_CONNECTOR which seems to be the > right thing to do on current platforms that use it. > > For CI20/jz4780 we just add a specialisation of the generic dw-hdmi to > make HDMI work. > > Now this patch for drm/ingenic wants the opposite definition and create its own > connector. This fails even if we remove the check (then we have two interfering > connectors). > > > We're moving > > towards requiring DRM_BRIDGE_ATTACH_NO_CONNECTOR for all new code, so it > > will have to be done eventually. > > So from my view drm/ingenic wants to already enforce this rule and breaks dw-hdmi. > > IMHO it should either handle this situation gracefully or include a fix for > dw-hdmi.c to keep it compatible.
Hi Laurent, > Am 23.09.2021 um 12:03 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > Hi Nikolaus, > > On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller wrote: >>> Am 23.09.2021 um 11:27 schrieb Laurent Pinchart: >>> On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller wrote: >>>> >>>>>>> + 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(). >>>>>> 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. >>>> >>>> Yes, would be very ugly. >>>> >>>> But generally who has the knowledge (and time) to do this work? >>>> And has a working platform to test (jz4780 isn't a good development environment)? >>>> >>>> The driver seems to have a turbulent history starting 2013 in staging/imx and >>>> apparently it was generalized since then... Is Laurent currently dw-hdmi maintainer? >>> >>> "Maintainer" would be an overstatement. I've worked on that driver in >>> the past, and I still use it, but don't have time to really maintain it. >>> I've also been told that Synopsys required all patches for that driver >>> developed using documentation under NDA to be submitted internally to >>> them first before being published, so I decided to stop contributing >>> instead of agreeing with this insane process. There's public >>> documentation about the IP in some NXP reference manuals though, so it >>> should be possible to still move forward without abiding by this rule. >>> >>>>>> 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. >>>> >>>> Ok, I see. You have to handle contradicting cases here. >>>> >>>> Would it be possible to run it with DRM_BRIDGE_ATTACH_NO_CONNECTOR first >>>> and retry if it fails without? >>>> >>>> But IMHO the return value (in error case) is not well defined. So there >>>> must be a test if a connector has been created (I do not know how this >>>> would work). >>>> >>>> Another suggestion: can you check if there is a downstream connector defined in >>>> device tree (dw-hdmi does not need such a definition)? >>>> If not we call it with 0 and if there is one we call it with >>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one? >>> >>> I haven't followed the ful conversation, what the reason why >>> DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? >> >> The synopsys driver creates its own connector through dw_hdmi_connector_create() >> because the IP handles DDC/EDID directly. > > That doesn't require creating a connector though. The driver implements > drm_bridge_funcs.get_edid(), which is used to get the EDID without the > need to create a connector in the dw-hdmi driver. Ah, ok. But then we still have issues. Firstly I would assume that get_edid only works properly if it is initialized through dw_hdmi_connector_create(). Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to dw_hdmi_bridge_attach() indeed does not call dw_hdmi_connector_create() but returns 0. This patch 6/6 makes drm/ingenic unconditionally require a connector to be attached which is defined somewhere else (device tree e.g. "connector-hdmi") unrelated to dw-hdmi. Current upstream code for drm/ingenic does not init/attach such a connector on its own so it did work before. I.e. I think we can't just use parts of dw-hdmi. If drm_bridge_attach() would return some errno if DRM_BRIDGE_ATTACH_NO_CONNECTOR is set, initialization in ingenic_drm_bind() would fail likewise with "Unable to attach bridge". So in any case dw-hdmi is broken by this drm/ingenic patch unless someone reworks it to make it compatible. Another issue is that dw_hdmi_connector_create() does not only do dcd/edid but appears to detects hot plug and does some special initialization. So we probably loose hotplug detect if we just use drm_bridge_funcs.get_edid(). I come to the conclusion that not creating a specific connector in dw-hdmi and relying on a generic connector does not seem to be an option with current code proposals. In such a situation the question is what the least invasive surgery is to avoid complications and lenghty regression tests on unknown platforms. IMHO it is leaving (mature) dw-hdmi untouched and make attachment of a connector in ingenic_drm_bind() depend on some condition. BR and thanks, Nikolaus
Hi Laurent, > IMHO it is leaving (mature) dw-hdmi untouched and make attachment of a connector > in ingenic_drm_bind() depend on some condition. Since I don't know details of the DRM bridge/encoder/connector APIs), let me reformulate the quersion for a condition specifically. How can one find out in this code fragment from Paul's patch if drm_brige_attach() did create a connector or not? I.e. did call drm_connector_attach_encoder(connector, hdmi->bridge.encoder); on its own? @@ -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); 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); + 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); } A problem may be that "connector" is unknown before drm_bridge_connector_init() is called. Then I think I can propose a fallback solution to drm_bridge_attach(, 0) if drm_bridge_attach(, DRM_BRIDGE_ATTACH_NO_CONNECTOR) fails. BR and thanks, Nikolaus
Hi Nikolaus, Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Laurent, > >> Am 23.09.2021 um 12:03 schrieb Laurent Pinchart >> <laurent.pinchart@ideasonboard.com>: >> >> Hi Nikolaus, >> >> On Thu, Sep 23, 2021 at 11:55:56AM +0200, H. Nikolaus Schaller >> wrote: >>>> Am 23.09.2021 um 11:27 schrieb Laurent Pinchart: >>>> On Thu, Sep 23, 2021 at 11:19:23AM +0200, H. Nikolaus Schaller >>>> wrote: >>>>> >>>>>>>> + 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(). >>>>>>> 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. >>>>> >>>>> Yes, would be very ugly. >>>>> >>>>> But generally who has the knowledge (and time) to do this work? >>>>> And has a working platform to test (jz4780 isn't a good >>>>> development environment)? >>>>> >>>>> The driver seems to have a turbulent history starting 2013 in >>>>> staging/imx and >>>>> apparently it was generalized since then... Is Laurent currently >>>>> dw-hdmi maintainer? >>>> >>>> "Maintainer" would be an overstatement. I've worked on that >>>> driver in >>>> the past, and I still use it, but don't have time to really >>>> maintain it. >>>> I've also been told that Synopsys required all patches for that >>>> driver >>>> developed using documentation under NDA to be submitted >>>> internally to >>>> them first before being published, so I decided to stop >>>> contributing >>>> instead of agreeing with this insane process. There's public >>>> documentation about the IP in some NXP reference manuals though, >>>> so it >>>> should be possible to still move forward without abiding by this >>>> rule. >>>> >>>>>>> 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. >>>>> >>>>> Ok, I see. You have to handle contradicting cases here. >>>>> >>>>> Would it be possible to run it with >>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR first >>>>> and retry if it fails without? >>>>> >>>>> But IMHO the return value (in error case) is not well defined. >>>>> So there >>>>> must be a test if a connector has been created (I do not know >>>>> how this >>>>> would work). >>>>> >>>>> Another suggestion: can you check if there is a downstream >>>>> connector defined in >>>>> device tree (dw-hdmi does not need such a definition)? >>>>> If not we call it with 0 and if there is one we call it with >>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR and create one? >>>> >>>> I haven't followed the ful conversation, what the reason why >>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR can't always be use here ? >>> >>> The synopsys driver creates its own connector through >>> dw_hdmi_connector_create() >>> because the IP handles DDC/EDID directly. >> >> That doesn't require creating a connector though. The driver >> implements >> drm_bridge_funcs.get_edid(), which is used to get the EDID without >> the >> need to create a connector in the dw-hdmi driver. > > Ah, ok. > > But then we still have issues. > > Firstly I would assume that get_edid only works properly if it is > initialized > through dw_hdmi_connector_create(). > > Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to > dw_hdmi_bridge_attach() indeed does not call > dw_hdmi_connector_create() > but returns 0. > > This patch 6/6 makes drm/ingenic unconditionally require a connector > to be attached which is defined somewhere else (device tree e.g. > "connector-hdmi") > unrelated to dw-hdmi. Current upstream code for drm/ingenic does not > init/attach > such a connector on its own so it did work before. > > I.e. I think we can't just use parts of dw-hdmi. The fact that Laurent is using dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's possible here as well. There's no reason why it shouldn't work with ingenic-drm. The ingenic-drm driver does not need to create any connector. The "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the list. > If drm_bridge_attach() would return some errno if > DRM_BRIDGE_ATTACH_NO_CONNECTOR > is set, initialization in ingenic_drm_bind() would fail likewise with > "Unable to attach bridge". > > So in any case dw-hdmi is broken by this drm/ingenic patch unless > someone > reworks it to make it compatible. Where would the errno be returned? Why would drm_bridge_attach() return an error code? > Another issue is that dw_hdmi_connector_create() does not only do > dcd/edid > but appears to detects hot plug and does some special initialization. > So we probably loose hotplug detect if we just use > drm_bridge_funcs.get_edid(). There's drm_bridge_funcs.detect(). Cheers, -Paul > I come to the conclusion that not creating a specific connector in > dw-hdmi > and relying on a generic connector does not seem to be an option with > current > code proposals. > > In such a situation the question is what the least invasive surgery > is to > avoid complications and lenghty regression tests on unknown platforms. > IMHO it is leaving (mature) dw-hdmi untouched and make attachment of > a connector > in ingenic_drm_bind() depend on some condition. > > BR and thanks, > Nikolaus > >
Hi Paul, > Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Laurent, >> Ah, ok. >> But then we still have issues. >> Firstly I would assume that get_edid only works properly if it is initialized >> through dw_hdmi_connector_create(). >> Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to >> dw_hdmi_bridge_attach() indeed does not call dw_hdmi_connector_create() >> but returns 0. >> This patch 6/6 makes drm/ingenic unconditionally require a connector >> to be attached which is defined somewhere else (device tree e.g. "connector-hdmi") >> unrelated to dw-hdmi. Current upstream code for drm/ingenic does not init/attach >> such a connector on its own so it did work before. >> I.e. I think we can't just use parts of dw-hdmi. > > The fact that Laurent is using dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's possible here as well. There's no reason why it shouldn't work with ingenic-drm. That is interesting and Laurent can probably comment on differences between his setup (I wasn't able to deduce what device you are referring to) and dw-hdmi. For jz4780 we tried that first. I do not remember the exact reasons but we wasted weeks trying to but failed to get it working. While the dw-hdmi connector simply works on top of upstream and fails only if we apply your patch. Another issue is how you want to tell connector-hdmi to use the extra i2c bus driver for ddc which is not available directly as a standard i2c controller of the jz4780. hdmi-connector.yaml defines: ddc-i2c-bus: description: phandle link to the I2C controller used for DDC EDID probing $ref: /schemas/types.yaml#/definitions/phandle So we would need some ddc-i2c-bus = <&i2c-controller-inside-the dw-hdmi>. But that i2c-controller-inside-the dw-hdmi does not exist in device tree and can not be added unless someone significantly rewrites dw-hdmi to register and expose it as i2c controller. > > The ingenic-drm driver does not need to create any connector. The "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the list. Sure. It does not *create* a connector. It expects that it can safely call drm_bridge_connector_init() to get a pointer to a newly created connector. But if we use the dw-hdmi connector, there is no such connector and "next bridge". Or can you tell me how to make the dw-hdmi connector created by dw_hdmi_connector_create() become the "next bridge" in the list for your driver? But without significantly rewriting dw-hdmi.c (and testing). > >> If drm_bridge_attach() would return some errno if DRM_BRIDGE_ATTACH_NO_CONNECTOR >> is set, initialization in ingenic_drm_bind() would fail likewise with "Unable to attach bridge". >> So in any case dw-hdmi is broken by this drm/ingenic patch unless someone >> reworks it to make it compatible. > > Where would the errno be returned? Why would drm_bridge_attach() return an error code? Currently dw_hdmi_bridge_attach() returns 0 if it is asked to support DRM_BRIDGE_ATTACH_NO_CONNECTOR. This is not treated as an error by drm_bridge_attach(). Here it could return an error (-ENOTSUPP?) instead, to allow for error handling by its caller. But that raises an error message like "failed to attach bridge to encoder" and the bridge is reset and detached. > >> Another issue is that dw_hdmi_connector_create() does not only do dcd/edid >> but appears to detects hot plug and does some special initialization. >> So we probably loose hotplug detect if we just use drm_bridge_funcs.get_edid(). > > There's drm_bridge_funcs.detect(). You mean in dw-hdmi? Yes, it calls dw_hdmi_bridge_detect() which calls dw_hdmi_detect(). This does some read_hpd. Anyways, this does not solve the problem that with your drm/ingenic proposal the dw-hdmi subsystem (hdmi + ddc) can no longer be initialized properly unless someone fixes either. So IMHO this should be treated as a significant blocking point for your patch because it breaks something that is working upstream and there seems to be no rationale to change it. Your commit message just says: "All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR." but gives no reason why. I fully understand that you want to change it and Laurent said that it will become standard in the far future. Therefore I suggest to find a way that you can find out if a connector has already been created by drm_bridge_attach() to stay compatible with current upstream code. I even want to help here but I don't know how to detect the inverse of drm_connector_attach_encoder(), i.e. is_drm_encoder_attached_to_any_connector(). BR and thanks, Nikolaus
Le jeu., sept. 23 2021 at 20:52:23 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >> Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Nikolaus, >> >> Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller >> <hns@goldelico.com> a écrit : >>> Hi Laurent, >>> Ah, ok. >>> But then we still have issues. >>> Firstly I would assume that get_edid only works properly if it is >>> initialized >>> through dw_hdmi_connector_create(). >>> Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR >>> to >>> dw_hdmi_bridge_attach() indeed does not call >>> dw_hdmi_connector_create() >>> but returns 0. >>> This patch 6/6 makes drm/ingenic unconditionally require a >>> connector >>> to be attached which is defined somewhere else (device tree e.g. >>> "connector-hdmi") >>> unrelated to dw-hdmi. Current upstream code for drm/ingenic does >>> not init/attach >>> such a connector on its own so it did work before. >>> I.e. I think we can't just use parts of dw-hdmi. >> >> The fact that Laurent is using dw-hdmi with >> DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's >> possible here as well. There's no reason why it shouldn't work with >> ingenic-drm. > > That is interesting and Laurent can probably comment on differences > between > his setup (I wasn't able to deduce what device you are referring to) > and dw-hdmi. > > For jz4780 we tried that first. I do not remember the exact reasons > but we wasted > weeks trying to but failed to get it working. While the dw-hdmi > connector simply works > on top of upstream and fails only if we apply your patch. > > Another issue is how you want to tell connector-hdmi to use the extra > i2c bus driver > for ddc which is not available directly as a standard i2c controller > of the jz4780. > > hdmi-connector.yaml defines: > > ddc-i2c-bus: > description: phandle link to the I2C controller used for DDC EDID > probing > $ref: /schemas/types.yaml#/definitions/phandle > > So we would need some ddc-i2c-bus = <&i2c-controller-inside-the > dw-hdmi>. > > But that i2c-controller-inside-the dw-hdmi does not exist in device > tree > and can not be added unless someone significantly rewrites dw-hdmi to > register and expose it as i2c controller. No, you don't need to do that at all. Just don't set the "ddc-i2c-bus" property. >> >> The ingenic-drm driver does not need to create any connector. The >> "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the >> list. > > Sure. It does not *create* a connector. It expects that it can safely > call > drm_bridge_connector_init() to get a pointer to a newly created > connector. > > But if we use the dw-hdmi connector, there is no such connector and > "next bridge". We don't want to use the dw-hdmi connector. Your "next bridge" is the "hdmi-connector" that should be wired in DTS. > Or can you tell me how to make the dw-hdmi connector created by > dw_hdmi_connector_create() become the "next bridge" in the list for > your driver? > But without significantly rewriting dw-hdmi.c (and testing). Wire it to the LCD node in DTS... See how we do it for the IT66121 driver: https://github.com/OpenDingux/linux/blob/jz-5.15/arch/mips/boot/dts/ingenic/rg350m.dts#L114-L134 >> >>> If drm_bridge_attach() would return some errno if >>> DRM_BRIDGE_ATTACH_NO_CONNECTOR >>> is set, initialization in ingenic_drm_bind() would fail likewise >>> with "Unable to attach bridge". >>> So in any case dw-hdmi is broken by this drm/ingenic patch unless >>> someone >>> reworks it to make it compatible. >> >> Where would the errno be returned? Why would drm_bridge_attach() >> return an error code? > > Currently dw_hdmi_bridge_attach() returns 0 if it is asked to support > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > This is not treated as an error by drm_bridge_attach(). > > Here it could return an error (-ENOTSUPP?) instead, to allow for > error handling > by its caller. And why would you do that? If you don't want to attach a connector, then drm_bridge_attach() doesn't need to do much. So it's normal that it returns zero. > But that raises an error message like "failed to attach bridge to > encoder" and > the bridge is reset and detached. > >> >>> Another issue is that dw_hdmi_connector_create() does not only do >>> dcd/edid >>> but appears to detects hot plug and does some special >>> initialization. >>> So we probably loose hotplug detect if we just use >>> drm_bridge_funcs.get_edid(). >> >> There's drm_bridge_funcs.detect(). > > You mean in dw-hdmi? Yes, it calls dw_hdmi_bridge_detect() which > calls dw_hdmi_detect(). > This does some read_hpd. > > Anyways, this does not solve the problem that with your drm/ingenic > proposal the > dw-hdmi subsystem (hdmi + ddc) can no longer be initialized properly > unless someone > fixes either. > > So IMHO this should be treated as a significant blocking point for > your patch > because it breaks something that is working upstream and there seems > to be no > rationale to change it. > > Your commit message just says: > "All the bridges are now attached with > DRM_BRIDGE_ATTACH_NO_CONNECTOR." > but gives no reason why. > > I fully understand that you want to change it and Laurent said that > it will become > standard in the far future. Therefore I suggest to find a way that > you can find out > if a connector has already been created by drm_bridge_attach() to > stay compatible > with current upstream code. No, absolutely not. There is nothing upstream yet that can bind the ingenic-drm driver with the dw-hdmi driver. This is your downstream patch. I'm not breaking anything that's upstream, so there is no blocking point. Besides, even with your downstream patch I don't see any reason why the dw-hdmi driver wouldn't work with this patch, provided it's wired properly, and you never did show a proof of failure either. You come up with "possible points where it will fail" but these are based on your assumptions on how the drivers should be working together, and I think you somehow miss the whole picture. Start by wiring things properly, like in my previously linked DTS, and *test*. If it fails, tell us where it fails. Because your "it doesn't work" arguments have zero weight otherwise. If I can find some time this weekend I will test it myself. Cheers, -Paul > I even want to help here but I don't know how to detect the inverse of > drm_connector_attach_encoder(), i.e. > is_drm_encoder_attached_to_any_connector(). > > BR and thanks, > Nikolaus > > >
Hi Paul, > Am 23.09.2021 um 21:39 schrieb Paul Cercueil <paul@crapouillou.net>: > > > > Le jeu., sept. 23 2021 at 20:52:23 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >>> Am 23.09.2021 um 15:30 schrieb Paul Cercueil <paul@crapouillou.net>: >>> Hi Nikolaus, >>> Le jeu., sept. 23 2021 at 13:41:28 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>> Hi Laurent, >>>> Ah, ok. >>>> But then we still have issues. >>>> Firstly I would assume that get_edid only works properly if it is initialized >>>> through dw_hdmi_connector_create(). >>>> Next, in the current code, passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to >>>> dw_hdmi_bridge_attach() indeed does not call dw_hdmi_connector_create() >>>> but returns 0. >>>> This patch 6/6 makes drm/ingenic unconditionally require a connector >>>> to be attached which is defined somewhere else (device tree e.g. "connector-hdmi") >>>> unrelated to dw-hdmi. Current upstream code for drm/ingenic does not init/attach >>>> such a connector on its own so it did work before. >>>> I.e. I think we can't just use parts of dw-hdmi. >>> The fact that Laurent is using dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR on Renesas makes me think that it's possible here as well. There's no reason why it shouldn't work with ingenic-drm. >> That is interesting and Laurent can probably comment on differences between >> his setup (I wasn't able to deduce what device you are referring to) and dw-hdmi. >> For jz4780 we tried that first. I do not remember the exact reasons but we wasted >> weeks trying to but failed to get it working. While the dw-hdmi connector simply works >> on top of upstream and fails only if we apply your patch. >> Another issue is how you want to tell connector-hdmi to use the extra i2c bus driver >> for ddc which is not available directly as a standard i2c controller of the jz4780. >> hdmi-connector.yaml defines: >> ddc-i2c-bus: >> description: phandle link to the I2C controller used for DDC EDID probing >> $ref: /schemas/types.yaml#/definitions/phandle >> So we would need some ddc-i2c-bus = <&i2c-controller-inside-the dw-hdmi>. >> But that i2c-controller-inside-the dw-hdmi does not exist in device tree >> and can not be added unless someone significantly rewrites dw-hdmi to >> register and expose it as i2c controller. > > No, you don't need to do that at all. Just don't set the "ddc-i2c-bus" property. And then ddc from dw-hdmi works? > >>> The ingenic-drm driver does not need to create any connector. The "connector-hdmi" is connected to dw-hdmi as the "next bridge" in the list. >> Sure. It does not *create* a connector. It expects that it can safely call >> drm_bridge_connector_init() to get a pointer to a newly created connector. >> But if we use the dw-hdmi connector, there is no such connector and "next bridge". > > We don't want to use the dw-hdmi connector. Your "next bridge" is the "hdmi-connector" that should be wired in DTS. > >> Or can you tell me how to make the dw-hdmi connector created by >> dw_hdmi_connector_create() become the "next bridge" in the list for your driver? >> But without significantly rewriting dw-hdmi.c (and testing). > > Wire it to the LCD node in DTS... > > See how we do it for the IT66121 driver: > https://github.com/OpenDingux/linux/blob/jz-5.15/arch/mips/boot/dts/ingenic/rg350m.dts#L114-L134 That is how we already tried. > >>>> If drm_bridge_attach() would return some errno if DRM_BRIDGE_ATTACH_NO_CONNECTOR >>>> is set, initialization in ingenic_drm_bind() would fail likewise with "Unable to attach bridge". >>>> So in any case dw-hdmi is broken by this drm/ingenic patch unless someone >>>> reworks it to make it compatible. >>> Where would the errno be returned? Why would drm_bridge_attach() return an error code? >> Currently dw_hdmi_bridge_attach() returns 0 if it is asked to support >> DRM_BRIDGE_ATTACH_NO_CONNECTOR. >> This is not treated as an error by drm_bridge_attach(). >> Here it could return an error (-ENOTSUPP?) instead, to allow for error handling >> by its caller. > > And why would you do that? If you don't want to attach a connector, then drm_bridge_attach() doesn't need to do much. So it's normal that it returns zero. > >> But that raises an error message like "failed to attach bridge to encoder" and >> the bridge is reset and detached. >>>> Another issue is that dw_hdmi_connector_create() does not only do dcd/edid >>>> but appears to detects hot plug and does some special initialization. >>>> So we probably loose hotplug detect if we just use drm_bridge_funcs.get_edid(). >>> There's drm_bridge_funcs.detect(). >> You mean in dw-hdmi? Yes, it calls dw_hdmi_bridge_detect() which calls dw_hdmi_detect(). >> This does some read_hpd. >> Anyways, this does not solve the problem that with your drm/ingenic proposal the >> dw-hdmi subsystem (hdmi + ddc) can no longer be initialized properly unless someone >> fixes either. >> So IMHO this should be treated as a significant blocking point for your patch >> because it breaks something that is working upstream and there seems to be no >> rationale to change it. >> Your commit message just says: >> "All the bridges are now attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR." >> but gives no reason why. >> I fully understand that you want to change it and Laurent said that it will become >> standard in the far future. Therefore I suggest to find a way that you can find out >> if a connector has already been created by drm_bridge_attach() to stay compatible >> with current upstream code. > > No, absolutely not. There is nothing upstream yet that can bind the ingenic-drm driver with the dw-hdmi driver. We have proposed it a while ago using nothing special. > This is your downstream patch. I'm not breaking anything that's upstream, so there is no blocking point. > > Besides, even with your downstream patch I don't see any reason why the dw-hdmi driver wouldn't work with this patch, provided it's wired properly, and you never did show a proof of failure either. You come up with "possible points where it will fail" but these are based on your assumptions on how the drivers should be working together, and I think you somehow miss the whole picture. Yes, maybe. > > Start by wiring things properly, like in my previously linked DTS, and *test*. If it fails, tell us where it fails. Well, I tell where drm_bridge_attach fails with DRM_BRIDGE_ATTACH_NO_CONNECTOR... > Because your "it doesn't work" arguments have zero weight otherwise. I hope I still can find it. So I can't promise anything. We have had it complete in DTS and added code to parse it. It may have been wiped out by cleaning up patch series during rebase. > > If I can find some time this weekend I will test it myself. You may be faster than me. BR and thanks, Nikolaus
On Thursday, 23 September 2021 22:23:28 CEST H. Nikolaus Schaller wrote: > > > Am 23.09.2021 um 21:39 schrieb Paul Cercueil <paul@crapouillou.net>: > > > > Start by wiring things properly, like in my previously linked DTS, and > > *test*. If it fails, tell us where it fails. > > Well, I tell where drm_bridge_attach fails with > DRM_BRIDGE_ATTACH_NO_CONNECTOR... I tried to piece together this entire discussion from the mailing list archives, but there appear to be two approaches that "work", in that they activate the LCD controller with the HDMI peripheral: 1. Nikolaus's approach, which involves getting the Synopsys driver to create a connector and then avoiding the call to drm_bridge_connector_init in the Ingenic DRM driver. 2. My approach, which just involves changing the Synopsys driver to set the bridge type in dw_hdmi_probe like this: hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA; Otherwise, I don't see how the bridge's (struct drm_bridge) type will be set. And this causes drm_bridge_connector_init to fail because it tests the bridge type. Now, I just reintroduced the HDMI connector to the device tree as follows: hdmi_connector { compatible = "hdmi-connector"; label = "hdmi"; type = "a"; port { hdmi_connector_in: endpoint { remote-endpoint = <&dw_hdmi_out>; }; }; }; And then I added a second port to the HDMI peripheral node as follows: port@1 { reg = <1>; dw_hdmi_out: endpoint { remote-endpoint = <&hdmi_connector_in>; }; }; And I removed any of the above hacks. What I observe, apart from an inactive LCD controller (and ingenic-drm driver), is the following in /sys/devices/ platform/10180000.hdmi/: consumer:platform:13050000.lcdc0 consumer:platform:hdmi_connector Maybe I don't understand the significance of "consumer" here, but the LCD controller and the HDMI connector obviously have rather different roles. Then again, the device tree is defining bidirectional relationships, so maybe this is how they manifest themselves. > > Because your "it doesn't work" arguments have zero weight otherwise. > > I hope I still can find it. So I can't promise anything. > We have had it complete in DTS and added code to parse it. > It may have been wiped out by cleaning up patch series during rebase. I suppose the question is whether this is actually handled already. I would have thought that either the DRM framework would be able to identify such relationships in a generic way or that the Synopsys driver would need to do so. This might actually be happening, given that the sysfs entries are there, but I might also imagine that something extra would be required to set the bridge type. I did start writing some code to look up a remote endpoint for the second port, find the connector type, and then set it, but it was probably after midnight on that occasion as well. Short-circuiting this little dance and setting the bridge type indicated that this might ultimately be the right approach, but it would probably also mean introducing a point of specialisation to the Synopsys driver so that device-specific drivers can define a function to set the connector type. Otherwise, I can't see the Synopsys driver working for devices like the JZ4780, but then again, it seems that all the other devices seem to incorporate the Synopsys functionality in a different way and do not need to deal with connectors at all. > > If I can find some time this weekend I will test it myself. > > You may be faster than me. So, when I wrote about approaches that "work", I can seemingly get the LCD controller and HDMI peripheral registers set up to match a non-Linux environment where I can demonstrate a functioning display, and yet I don't get a valid signal in the Linux environment. Nikolaus can actually get HDMI output, but there may be other factors introduced by the Linux environment that frustrate success for me, whereas my non-Linux environment is much simpler and can reliably reproduce a successful result. For me, running modetest yields plenty of information about encoders, connectors (and the supported modes via the EDID, thanks to my HDMI-A hack), CRTCs, and planes. But no framebuffers are reported. Paul
Hi Paul, Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie <paul@boddie.org.uk> a écrit : > On Thursday, 23 September 2021 22:23:28 CEST H. Nikolaus Schaller > wrote: >> >> > Am 23.09.2021 um 21:39 schrieb Paul Cercueil >> <paul@crapouillou.net>: >> > >> > Start by wiring things properly, like in my previously linked >> DTS, and >> > *test*. If it fails, tell us where it fails. >> >> Well, I tell where drm_bridge_attach fails with >> DRM_BRIDGE_ATTACH_NO_CONNECTOR... > > I tried to piece together this entire discussion from the mailing list > archives, but there appear to be two approaches that "work", in that > they > activate the LCD controller with the HDMI peripheral: > > 1. Nikolaus's approach, which involves getting the Synopsys driver to > create a > connector and then avoiding the call to drm_bridge_connector_init in > the > Ingenic DRM driver. > > 2. My approach, which just involves changing the Synopsys driver to > set the > bridge type in dw_hdmi_probe like this: > > hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA; > > Otherwise, I don't see how the bridge's (struct drm_bridge) type will > be set. The bridge's type is set in hdmi-connector, from DTS. The 'type = "a"' will result in the bridge's .type to be set to DRM_MODE_CONNECTOR_HDMIA. > And this causes drm_bridge_connector_init to fail because it tests > the bridge > type. > > Now, I just reintroduced the HDMI connector to the device tree as > follows: > > hdmi_connector { > compatible = "hdmi-connector"; > label = "hdmi"; > > type = "a"; > > port { > hdmi_connector_in: endpoint { > remote-endpoint = <&dw_hdmi_out>; > }; > }; > }; > > And then I added a second port to the HDMI peripheral node as follows: > > port@1 { > reg = <1>; > dw_hdmi_out: endpoint { > remote-endpoint = > <&hdmi_connector_in>; > }; > }; > > And I removed any of the above hacks. What I observe, apart from an > inactive > LCD controller (and ingenic-drm driver), is the following in > /sys/devices/ > platform/10180000.hdmi/: > > consumer:platform:13050000.lcdc0 > consumer:platform:hdmi_connector > > Maybe I don't understand the significance of "consumer" here, but the > LCD > controller and the HDMI connector obviously have rather different > roles. Then > again, the device tree is defining bidirectional relationships, so > maybe this > is how they manifest themselves. > >> > Because your "it doesn't work" arguments have zero weight >> otherwise. >> >> I hope I still can find it. So I can't promise anything. >> We have had it complete in DTS and added code to parse it. >> It may have been wiped out by cleaning up patch series during >> rebase. > > I suppose the question is whether this is actually handled already. I > would > have thought that either the DRM framework would be able to identify > such > relationships in a generic way or that the Synopsys driver would need > to do > so. This might actually be happening, given that the sysfs entries > are there, > but I might also imagine that something extra would be required to > set the > bridge type. > > I did start writing some code to look up a remote endpoint for the > second > port, find the connector type, and then set it, but it was probably > after > midnight on that occasion as well. Short-circuiting this little dance > and > setting the bridge type indicated that this might ultimately be the > right > approach, but it would probably also mean introducing a point of > specialisation to the Synopsys driver so that device-specific drivers > can > define a function to set the connector type. > > Otherwise, I can't see the Synopsys driver working for devices like > the > JZ4780, but then again, it seems that all the other devices seem to > incorporate the Synopsys functionality in a different way and do not > need to > deal with connectors at all. > >> > If I can find some time this weekend I will test it myself. >> >> You may be faster than me. > > So, when I wrote about approaches that "work", I can seemingly get > the LCD > controller and HDMI peripheral registers set up to match a non-Linux > environment where I can demonstrate a functioning display, and yet I > don't get > a valid signal in the Linux environment. > > Nikolaus can actually get HDMI output, but there may be other factors > introduced by the Linux environment that frustrate success for me, > whereas my > non-Linux environment is much simpler and can reliably reproduce a > successful > result. > > For me, running modetest yields plenty of information about encoders, > connectors (and the supported modes via the EDID, thanks to my HDMI-A > hack), > CRTCs, and planes. But no framebuffers are reported. Could you paste the result of "modetest -a -c -p" somewhere maybe? If you have info about the CRTCs, encoders, connectors and EDID info, then I would assume it is very close to working fine. For your "no framebuffer" issue, keep in mind that CONFIG_FB and CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default. If that doesn't fix anything, that probably means that one .atomic_check() fails, so it would be a good place to start debugging. Cheers, -Paul
Hi Paul, > Am 23.09.2021 um 22:23 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > >> Because your "it doesn't work" arguments have zero weight otherwise. > > I hope I still can find it. So I can't promise anything. > We have had it complete in DTS and added code to parse it. > It may have been wiped out by cleaning up patch series during rebase. I was able to locate it and place it on top of your ingenic-drm-drv v3 and our synopsys hdmi v3 [1] (+ unpublished work). This [2] should save you a lot of time making dw-hdmi work on jz4780 at all, so you can focus on our mistakes instead of starting from scratch. Features: - based on v5.15-rc2 - (the first two patches are LetuxOS and build system related and can be ignored for this discussion) - contains some significant patch from drm-next not yet upstream - contains your v3 series as is - (initially) disables your DRM_BRIDGE_ATTACH_NO_CONNECTOR (is reverted in the last patch) - adds synopsys stuff and DT schema - adds jz4780.dtsi and ci20.dts - adds ci20_defconfig - (adds some (optional) jz4780 specific features we likely do not need now) - adds something to dw-hdmi to properly notify HPD - adds a hdmi-regulator so that HPD power can be turned on/off - (attempt to configure the dw-hdmi unwedge feature) - then we add the hdmi-connector to replace the dw-hdmi connector to device tree - and finally re-enable DRM_BRIDGE_ATTACH_NO_CONNECTOR The result is a) without the last patch I get a proper setup with framebuffer and edid. Unfortunateley without any image on HDMI. b) if last patch is included (so that DRM_BRIDGE_ATTACH_NO_CONNECTOR is required as by your [patch v3 6/6] again) I get: [ 4.351200] [drm:drm_bridge_attach [drm]] *ERROR* failed to attach bridge /hdmi@10180000 to encoder DPI-34: -22 [ 4.474346] [drm:drm_bridge_attach [drm]] *ERROR* failed to attach bridge (null) to encoder DPI-34: -22 [ 4.562125] ingenic-drm 13050000.lcdc0: Unable to attach bridge [ 4.568103] ingenic-drm: probe of 13050000.lcdc0 failed with error -22 Maybe you can spot the bug in the code much quicker than we can. I do not know what Paul Boddie did differently if this initialization with connector-hdmi works for him and does not fail likewise. BR and thanks, Nikolaus [1]: https://lore.kernel.org/linux-mips/8e873f17fcc9aeb326d99b7c2c8cd25b61dca6f5.1628399442.git.hns@goldelico.com/T/ [2]: https://git.goldelico.com/?p=letux-kernel.git;a=shortlog;h=refs/heads/upstream%2Bjz4780%2Bhdmi-connector
On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote: > > Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie > > > > 2. My approach, which just involves changing the Synopsys driver to > > set the bridge type in dw_hdmi_probe like this: > > > > hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA; > > > > Otherwise, I don't see how the bridge's (struct drm_bridge) type will > > be set. > > The bridge's type is set in hdmi-connector, from DTS. The 'type = "a"' > will result in the bridge's .type to be set to DRM_MODE_CONNECTOR_HDMIA. Actually, I found that hdmi-connector might not have been available because CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the connector does get detected and enabled. However, the Synopsys driver remains unaware of it, and so the bridge type in the Synopsys driver remains unset. I do see that the connector sets the type on a bridge in its own private structure, so there would be a need to propagate this type to the actual bridge. In other words, what the connector does is distinct from the Synopsys driver which acts as the bridge with regard to the Ingenic driver. Perhaps the Synopsys driver should set the connector's bridge as the next bridge, or maybe something is supposed to discover that the connector may act as (or provide) a bridge after the Synopsys driver in the chain and then back- propagate the bridge type along the chain. [...] > > And I removed any of the above hacks. What I observe, apart from an > > inactive LCD controller (and ingenic-drm driver), is the following in > > /sys/devices/platform/10180000.hdmi/: > > > > consumer:platform:13050000.lcdc0 > > consumer:platform:hdmi_connector Interestingly, with the connector driver present, these sysfs entries no longer appear. [...] > > For me, running modetest yields plenty of information about encoders, > > connectors (and the supported modes via the EDID, thanks to my HDMI-A > > hack), CRTCs, and planes. But no framebuffers are reported. > > Could you paste the result of "modetest -a -c -p" somewhere maybe? I had to specify -M ingenic-drm as well, but here you go... ---- Connectors: id encoder status name size (mm) modes encoders 35 34 connected HDMI-A-1 340x270 17 34 modes: index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot) #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type: preferred, driver #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags: phsync, pvsync; type: driver #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync, pvsync; type: driver #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync, pvsync; type: driver #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync, pvsync; type: driver #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync, nvsync; type: driver #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync, nvsync; type: driver #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync, nvsync; type: driver #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync, pvsync; type: driver #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync, pvsync; type: driver #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync, pvsync; type: driver #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync, pvsync; type: driver #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync, nvsync; type: driver #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync, nvsync; type: driver #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync, nvsync; type: driver #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync, nvsync; type: driver #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync, pvsync; type: driver props: 1 EDID: flags: immutable blob blobs: value: 00ffffffffffff00047232ad01010101 2d0e010380221b782aaea5a6544c9926 145054bfef0081808140714f01010101 010101010101302a009851002a403070 1300520e1100001e000000ff00343435 3030353444454330300a000000fc0041 4c313731350a202020202020000000fd 00384c1e520e000a2020202020200051 2 DPMS: flags: enum enums: On=0 Standby=1 Suspend=2 Off=3 value: 0 5 link-status: flags: enum enums: Good=0 Bad=1 value: 0 6 non-desktop: flags: immutable range values: 0 1 value: 0 4 TILE: flags: immutable blob blobs: value: 20 CRTC_ID: flags: object value: 32 CRTCs: id fb pos size 32 39 (0,0) (1280x1024) #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, pvsync; type: props: 22 ACTIVE: flags: range values: 0 1 value: 1 23 MODE_ID: flags: blob blobs: value: e0a5010000053005a005980600000004 010404042a0400003c00000005000000 00000000000000000000000000000000 00000000000000000000000000000000 00000000 19 OUT_FENCE_PTR: flags: range values: 0 18446744073709551615 value: 0 24 VRR_ENABLED: flags: range values: 0 1 value: 0 28 GAMMA_LUT: flags: blob blobs: value: 29 GAMMA_LUT_SIZE: flags: immutable range values: 0 4294967295 value: 256 Planes: id crtc fb CRTC x,y x,y gamma size possible crtcs 31 32 39 0,0 0,0 0 0x00000001 formats: XR15 RG16 RG24 XR24 XR30 props: 8 type: flags: immutable enum enums: Overlay=0 Primary=1 Cursor=2 value: 1 17 FB_ID: flags: object value: 39 18 IN_FENCE_FD: flags: signed range values: -1 2147483647 value: -1 20 CRTC_ID: flags: object value: 32 13 CRTC_X: flags: signed range values: -2147483648 2147483647 value: 0 14 CRTC_Y: flags: signed range values: -2147483648 2147483647 value: 0 15 CRTC_W: flags: range values: 0 2147483647 value: 1280 16 CRTC_H: flags: range values: 0 2147483647 value: 1024 9 SRC_X: flags: range values: 0 4294967295 value: 0 10 SRC_Y: flags: range values: 0 4294967295 value: 0 11 SRC_W: flags: range values: 0 4294967295 value: 83886080 12 SRC_H: flags: range values: 0 4294967295 value: 67108864 33 0 0 0,0 0,0 0 0x00000001 formats: C8 XR15 RG16 RG24 XR24 XR30 props: 8 type: flags: immutable enum enums: Overlay=0 Primary=1 Cursor=2 value: 0 17 FB_ID: flags: object value: 0 18 IN_FENCE_FD: flags: signed range values: -1 2147483647 value: -1 20 CRTC_ID: flags: object value: 0 13 CRTC_X: flags: signed range values: -2147483648 2147483647 value: 0 14 CRTC_Y: flags: signed range values: -2147483648 2147483647 value: 0 15 CRTC_W: flags: range values: 0 2147483647 value: 0 16 CRTC_H: flags: range values: 0 2147483647 value: 0 9 SRC_X: flags: range values: 0 4294967295 value: 0 10 SRC_Y: flags: range values: 0 4294967295 value: 0 11 SRC_W: flags: range values: 0 4294967295 value: 0 12 SRC_H: flags: range values: 0 4294967295 value: 0 ---- > If you have info about the CRTCs, encoders, connectors and EDID info, > then I would assume it is very close to working fine. > > For your "no framebuffer" issue, keep in mind that CONFIG_FB and > CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default. Yes, I discovered that CONFIG_FB was not enabled, so I did so. > If that doesn't fix anything, that probably means that one > .atomic_check() fails, so it would be a good place to start debugging. There will be other things to verify in the Ingenic driver. As noted many months ago, colour depth information has to be set in the DMA descriptors and not the control register, but we are managing to do this successfully, as far as I can tell, although there is always the potential for error. Paul
Hi Paul & Nikolaus, If you spent some time debugging the issue instead of complaining that my patchset breaks things... The fix is a one-liner in your downstream ingenic-dw-hdmi.c: .output_port = 1 in the ingenic_dw_hdmi_plat_data struct. Absolutely nothing else needs to be changed for HDMI to work here. Cheers, -Paul Le sam., sept. 25 2021 at 17:55:03 +0200, Paul Boddie <paul@boddie.org.uk> a écrit : > On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote: >> >> Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie >> > >> > 2. My approach, which just involves changing the Synopsys driver >> to >> > set the bridge type in dw_hdmi_probe like this: >> > >> > hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA; >> > >> > Otherwise, I don't see how the bridge's (struct drm_bridge) type >> will >> > be set. >> >> The bridge's type is set in hdmi-connector, from DTS. The 'type = >> "a"' >> will result in the bridge's .type to be set to >> DRM_MODE_CONNECTOR_HDMIA. > > Actually, I found that hdmi-connector might not have been available > because > CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the > connector > does get detected and enabled. However, the Synopsys driver remains > unaware of > it, and so the bridge type in the Synopsys driver remains unset. > > I do see that the connector sets the type on a bridge in its own > private > structure, so there would be a need to propagate this type to the > actual > bridge. In other words, what the connector does is distinct from the > Synopsys > driver which acts as the bridge with regard to the Ingenic driver. > > Perhaps the Synopsys driver should set the connector's bridge as the > next > bridge, or maybe something is supposed to discover that the connector > may act > as (or provide) a bridge after the Synopsys driver in the chain and > then back- > propagate the bridge type along the chain. > > [...] > >> > And I removed any of the above hacks. What I observe, apart from >> an >> > inactive LCD controller (and ingenic-drm driver), is the >> following in >> > /sys/devices/platform/10180000.hdmi/: >> > >> > consumer:platform:13050000.lcdc0 >> > consumer:platform:hdmi_connector > > Interestingly, with the connector driver present, these sysfs entries > no > longer appear. > > [...] > >> > For me, running modetest yields plenty of information about >> encoders, >> > connectors (and the supported modes via the EDID, thanks to my >> HDMI-A >> > hack), CRTCs, and planes. But no framebuffers are reported. >> >> Could you paste the result of "modetest -a -c -p" somewhere maybe? > > I had to specify -M ingenic-drm as well, but here you go... > > ---- > > Connectors: > id encoder status name size (mm) modes encoders > 35 34 connected HDMI-A-1 340x270 17 34 > modes: > index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot) > #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 > flags: > phsync, pvsync; type: preferred, driver > #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 > flags: > phsync, pvsync; type: driver > #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 > flags: phsync, > pvsync; type: driver > #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: > phsync, > pvsync; type: driver > #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: > phsync, > pvsync; type: driver > #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: > nhsync, > nvsync; type: driver > #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: > nhsync, > nvsync; type: driver > #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: > nhsync, > nvsync; type: driver > #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: > phsync, > pvsync; type: driver > #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: > phsync, > pvsync; type: driver > #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: > phsync, > pvsync; type: driver > #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: > phsync, > pvsync; type: driver > #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: > nhsync, > nvsync; type: driver > #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: > nhsync, > nvsync; type: driver > #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: > nhsync, > nvsync; type: driver > #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: > nhsync, > nvsync; type: driver > #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: > nhsync, > pvsync; type: driver > props: > 1 EDID: > flags: immutable blob > blobs: > > value: > 00ffffffffffff00047232ad01010101 > 2d0e010380221b782aaea5a6544c9926 > 145054bfef0081808140714f01010101 > 010101010101302a009851002a403070 > 1300520e1100001e000000ff00343435 > 3030353444454330300a000000fc0041 > 4c313731350a202020202020000000fd > 00384c1e520e000a2020202020200051 > 2 DPMS: > flags: enum > enums: On=0 Standby=1 Suspend=2 Off=3 > value: 0 > 5 link-status: > flags: enum > enums: Good=0 Bad=1 > value: 0 > 6 non-desktop: > flags: immutable range > values: 0 1 > value: 0 > 4 TILE: > flags: immutable blob > blobs: > > value: > 20 CRTC_ID: > flags: object > value: 32 > > CRTCs: > id fb pos size > 32 39 (0,0) (1280x1024) > #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: > phsync, > pvsync; type: > props: > 22 ACTIVE: > flags: range > values: 0 1 > value: 1 > 23 MODE_ID: > flags: blob > blobs: > > value: > e0a5010000053005a005980600000004 > 010404042a0400003c00000005000000 > 00000000000000000000000000000000 > 00000000000000000000000000000000 > 00000000 > 19 OUT_FENCE_PTR: > flags: range > values: 0 18446744073709551615 > value: 0 > 24 VRR_ENABLED: > flags: range > values: 0 1 > value: 0 > 28 GAMMA_LUT: > flags: blob > blobs: > > value: > 29 GAMMA_LUT_SIZE: > flags: immutable range > values: 0 4294967295 > value: 256 > > Planes: > id crtc fb CRTC x,y x,y gamma size possible crtcs > 31 32 39 0,0 0,0 0 0x00000001 > formats: XR15 RG16 RG24 XR24 XR30 > props: > 8 type: > flags: immutable enum > enums: Overlay=0 Primary=1 Cursor=2 > value: 1 > 17 FB_ID: > flags: object > value: 39 > 18 IN_FENCE_FD: > flags: signed range > values: -1 2147483647 > value: -1 > 20 CRTC_ID: > flags: object > value: 32 > 13 CRTC_X: > flags: signed range > values: -2147483648 2147483647 > value: 0 > 14 CRTC_Y: > flags: signed range > values: -2147483648 2147483647 > value: 0 > 15 CRTC_W: > flags: range > values: 0 2147483647 > value: 1280 > 16 CRTC_H: > flags: range > values: 0 2147483647 > value: 1024 > 9 SRC_X: > flags: range > values: 0 4294967295 > value: 0 > 10 SRC_Y: > flags: range > values: 0 4294967295 > value: 0 > 11 SRC_W: > flags: range > values: 0 4294967295 > value: 83886080 > 12 SRC_H: > flags: range > values: 0 4294967295 > value: 67108864 > 33 0 0 0,0 0,0 0 0x00000001 > formats: C8 XR15 RG16 RG24 XR24 XR30 > props: > 8 type: > flags: immutable enum > enums: Overlay=0 Primary=1 Cursor=2 > value: 0 > 17 FB_ID: > flags: object > value: 0 > 18 IN_FENCE_FD: > flags: signed range > values: -1 2147483647 > value: -1 > 20 CRTC_ID: > flags: object > value: 0 > 13 CRTC_X: > flags: signed range > values: -2147483648 2147483647 > value: 0 > 14 CRTC_Y: > flags: signed range > values: -2147483648 2147483647 > value: 0 > 15 CRTC_W: > flags: range > values: 0 2147483647 > value: 0 > 16 CRTC_H: > flags: range > values: 0 2147483647 > value: 0 > 9 SRC_X: > flags: range > values: 0 4294967295 > value: 0 > 10 SRC_Y: > flags: range > values: 0 4294967295 > value: 0 > 11 SRC_W: > flags: range > values: 0 4294967295 > value: 0 > 12 SRC_H: > flags: range > values: 0 4294967295 > value: 0 > > ---- > >> If you have info about the CRTCs, encoders, connectors and EDID >> info, >> then I would assume it is very close to working fine. >> >> For your "no framebuffer" issue, keep in mind that CONFIG_FB and >> CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default. > > Yes, I discovered that CONFIG_FB was not enabled, so I did so. > >> If that doesn't fix anything, that probably means that one >> .atomic_check() fails, so it would be a good place to start >> debugging. > > There will be other things to verify in the Ingenic driver. As noted > many > months ago, colour depth information has to be set in the DMA > descriptors and > not the control register, but we are managing to do this > successfully, as far > as I can tell, although there is always the potential for error. > > Paul > >
Hi Paul, > Am 25.09.2021 um 21:08 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Paul & Nikolaus, > > If you spent some time debugging the issue we did ... > instead of complaining that my patchset breaks things... ... we did have a working version (without hdmi-connector) and bisect pointed at your patch... So we debugged that. So the lesson is: don't trust bisect. And failed to make it work with hdmi-connector because the ingenic-drm-drv reported errors. > > The fix is a one-liner in your downstream ingenic-dw-hdmi.c: > .output_port = 1 > in the ingenic_dw_hdmi_plat_data struct. Cool. How did you find that? > > Absolutely nothing else needs to be changed for HDMI to work here. Great and thanks. Will test asap and if it works as well, we can clean up a v4 patch set for next week review. BR and thanks, Nikolaus > > Cheers, > -Paul > > > Le sam., sept. 25 2021 at 17:55:03 +0200, Paul Boddie <paul@boddie.org.uk> a écrit : >> On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote: >>> Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie >>> > >>> > 2. My approach, which just involves changing the Synopsys driver to >>> > set the bridge type in dw_hdmi_probe like this: >>> > >>> > hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA; >>> > >>> > Otherwise, I don't see how the bridge's (struct drm_bridge) type will >>> > be set. >>> The bridge's type is set in hdmi-connector, from DTS. The 'type = "a"' >>> will result in the bridge's .type to be set to DRM_MODE_CONNECTOR_HDMIA. >> Actually, I found that hdmi-connector might not have been available because >> CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the connector >> does get detected and enabled. However, the Synopsys driver remains unaware of >> it, and so the bridge type in the Synopsys driver remains unset. >> I do see that the connector sets the type on a bridge in its own private >> structure, so there would be a need to propagate this type to the actual >> bridge. In other words, what the connector does is distinct from the Synopsys >> driver which acts as the bridge with regard to the Ingenic driver. >> Perhaps the Synopsys driver should set the connector's bridge as the next >> bridge, or maybe something is supposed to discover that the connector may act >> as (or provide) a bridge after the Synopsys driver in the chain and then back- >> propagate the bridge type along the chain. >> [...] >>> > And I removed any of the above hacks. What I observe, apart from an >>> > inactive LCD controller (and ingenic-drm driver), is the following in >>> > /sys/devices/platform/10180000.hdmi/: >>> > >>> > consumer:platform:13050000.lcdc0 >>> > consumer:platform:hdmi_connector >> Interestingly, with the connector driver present, these sysfs entries no >> longer appear. >> [...] >>> > For me, running modetest yields plenty of information about encoders, >>> > connectors (and the supported modes via the EDID, thanks to my HDMI-A >>> > hack), CRTCs, and planes. But no framebuffers are reported. >>> Could you paste the result of "modetest -a -c -p" somewhere maybe? >> I had to specify -M ingenic-drm as well, but here you go... >> ---- >> Connectors: >> id encoder status name size (mm) modes encoders >> 35 34 connected HDMI-A-1 340x270 17 34 >> modes: >> index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot) >> #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: >> phsync, pvsync; type: preferred, driver >> #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 flags: >> phsync, pvsync; type: driver >> #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 flags: phsync, >> pvsync; type: driver >> #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 flags: phsync, >> pvsync; type: driver >> #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 flags: phsync, >> pvsync; type: driver >> #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 flags: nhsync, >> nvsync; type: driver >> #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 flags: nhsync, >> nvsync; type: driver >> #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: nhsync, >> nvsync; type: driver >> #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: phsync, >> pvsync; type: driver >> #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: phsync, >> pvsync; type: driver >> #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: phsync, >> pvsync; type: driver >> #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: phsync, >> pvsync; type: driver >> #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: nhsync, >> nvsync; type: driver >> #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: nhsync, >> nvsync; type: driver >> #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: nhsync, >> nvsync; type: driver >> #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: nhsync, >> nvsync; type: driver >> #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: nhsync, >> pvsync; type: driver >> props: >> 1 EDID: >> flags: immutable blob >> blobs: >> value: >> 00ffffffffffff00047232ad01010101 >> 2d0e010380221b782aaea5a6544c9926 >> 145054bfef0081808140714f01010101 >> 010101010101302a009851002a403070 >> 1300520e1100001e000000ff00343435 >> 3030353444454330300a000000fc0041 >> 4c313731350a202020202020000000fd >> 00384c1e520e000a2020202020200051 >> 2 DPMS: >> flags: enum >> enums: On=0 Standby=1 Suspend=2 Off=3 >> value: 0 >> 5 link-status: >> flags: enum >> enums: Good=0 Bad=1 >> value: 0 >> 6 non-desktop: >> flags: immutable range >> values: 0 1 >> value: 0 >> 4 TILE: >> flags: immutable blob >> blobs: >> value: >> 20 CRTC_ID: >> flags: object >> value: 32 >> CRTCs: >> id fb pos size >> 32 39 (0,0) (1280x1024) >> #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: phsync, >> pvsync; type: >> props: >> 22 ACTIVE: >> flags: range >> values: 0 1 >> value: 1 >> 23 MODE_ID: >> flags: blob >> blobs: >> value: >> e0a5010000053005a005980600000004 >> 010404042a0400003c00000005000000 >> 00000000000000000000000000000000 >> 00000000000000000000000000000000 >> 00000000 >> 19 OUT_FENCE_PTR: >> flags: range >> values: 0 18446744073709551615 >> value: 0 >> 24 VRR_ENABLED: >> flags: range >> values: 0 1 >> value: 0 >> 28 GAMMA_LUT: >> flags: blob >> blobs: >> value: >> 29 GAMMA_LUT_SIZE: >> flags: immutable range >> values: 0 4294967295 >> value: 256 >> Planes: >> id crtc fb CRTC x,y x,y gamma size possible crtcs >> 31 32 39 0,0 0,0 0 0x00000001 >> formats: XR15 RG16 RG24 XR24 XR30 >> props: >> 8 type: >> flags: immutable enum >> enums: Overlay=0 Primary=1 Cursor=2 >> value: 1 >> 17 FB_ID: >> flags: object >> value: 39 >> 18 IN_FENCE_FD: >> flags: signed range >> values: -1 2147483647 >> value: -1 >> 20 CRTC_ID: >> flags: object >> value: 32 >> 13 CRTC_X: >> flags: signed range >> values: -2147483648 2147483647 >> value: 0 >> 14 CRTC_Y: >> flags: signed range >> values: -2147483648 2147483647 >> value: 0 >> 15 CRTC_W: >> flags: range >> values: 0 2147483647 >> value: 1280 >> 16 CRTC_H: >> flags: range >> values: 0 2147483647 >> value: 1024 >> 9 SRC_X: >> flags: range >> values: 0 4294967295 >> value: 0 >> 10 SRC_Y: >> flags: range >> values: 0 4294967295 >> value: 0 >> 11 SRC_W: >> flags: range >> values: 0 4294967295 >> value: 83886080 >> 12 SRC_H: >> flags: range >> values: 0 4294967295 >> value: 67108864 >> 33 0 0 0,0 0,0 0 0x00000001 >> formats: C8 XR15 RG16 RG24 XR24 XR30 >> props: >> 8 type: >> flags: immutable enum >> enums: Overlay=0 Primary=1 Cursor=2 >> value: 0 >> 17 FB_ID: >> flags: object >> value: 0 >> 18 IN_FENCE_FD: >> flags: signed range >> values: -1 2147483647 >> value: -1 >> 20 CRTC_ID: >> flags: object >> value: 0 >> 13 CRTC_X: >> flags: signed range >> values: -2147483648 2147483647 >> value: 0 >> 14 CRTC_Y: >> flags: signed range >> values: -2147483648 2147483647 >> value: 0 >> 15 CRTC_W: >> flags: range >> values: 0 2147483647 >> value: 0 >> 16 CRTC_H: >> flags: range >> values: 0 2147483647 >> value: 0 >> 9 SRC_X: >> flags: range >> values: 0 4294967295 >> value: 0 >> 10 SRC_Y: >> flags: range >> values: 0 4294967295 >> value: 0 >> 11 SRC_W: >> flags: range >> values: 0 4294967295 >> value: 0 >> 12 SRC_H: >> flags: range >> values: 0 4294967295 >> value: 0 >> ---- >>> If you have info about the CRTCs, encoders, connectors and EDID info, >>> then I would assume it is very close to working fine. >>> For your "no framebuffer" issue, keep in mind that CONFIG_FB and >>> CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default. >> Yes, I discovered that CONFIG_FB was not enabled, so I did so. >>> If that doesn't fix anything, that probably means that one >>> .atomic_check() fails, so it would be a good place to start debugging. >> There will be other things to verify in the Ingenic driver. As noted many >> months ago, colour depth information has to be set in the DMA descriptors and >> not the control register, but we are managing to do this successfully, as far >> as I can tell, although there is always the potential for error. >> Paul > >
Le sam., sept. 25 2021 at 21:26:42 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >> Am 25.09.2021 um 21:08 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Paul & Nikolaus, >> >> If you spent some time debugging the issue > > we did ... By saying that you didn't debug, I mean that you did not try to see why you had these errors - where the error codes were coming from, etc., to have a clear understanding of why it fails. >> instead of complaining that my patchset breaks things... > > ... we did have a working version (without hdmi-connector) > and bisect pointed at your patch... So we debugged that. > > So the lesson is: don't trust bisect. > > And failed to make it work with hdmi-connector because the > ingenic-drm-drv reported errors. > >> >> The fix is a one-liner in your downstream ingenic-dw-hdmi.c: >> .output_port = 1 >> in the ingenic_dw_hdmi_plat_data struct. > > Cool. How did you find that? You had this: [ 4.474346] [drm:drm_bridge_attach [drm]] *ERROR* failed to attach bridge (null) to encoder DPI-34: -22 (null) means you're printing a NULL pointer. So I could see that hdmi->next_bridge was NULL. The place that sets it is dw_hdmi_parse_dt, which will return early with code 0, before next_bridge is set, if plat_data->output_port == 0, which was your case. Since your hdmi-connector is wired at port #1, then .output_port should be 1 as well. Cheers, -Paul >> >> Absolutely nothing else needs to be changed for HDMI to work here. > > Great and thanks. > > Will test asap and if it works as well, we can clean up a v4 patch set > for next week review. > > BR and thanks, > Nikolaus > >> >> Cheers, >> -Paul >> >> >> Le sam., sept. 25 2021 at 17:55:03 +0200, Paul Boddie >> <paul@boddie.org.uk> a écrit : >>> On Friday, 24 September 2021 10:29:02 CEST Paul Cercueil wrote: >>>> Le ven., sept. 24 2021 at 00:51:39 +0200, Paul Boddie >>>> > >>>> > 2. My approach, which just involves changing the Synopsys >>>> driver to >>>> > set the bridge type in dw_hdmi_probe like this: >>>> > >>>> > hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA; >>>> > >>>> > Otherwise, I don't see how the bridge's (struct drm_bridge) >>>> type will >>>> > be set. >>>> The bridge's type is set in hdmi-connector, from DTS. The 'type = >>>> "a"' >>>> will result in the bridge's .type to be set to >>>> DRM_MODE_CONNECTOR_HDMIA. >>> Actually, I found that hdmi-connector might not have been >>> available because >>> CONFIG_DRM_DISPLAY_CONNECTOR was not enabled. Rectifying this, the >>> connector >>> does get detected and enabled. However, the Synopsys driver >>> remains unaware of >>> it, and so the bridge type in the Synopsys driver remains unset. >>> I do see that the connector sets the type on a bridge in its own >>> private >>> structure, so there would be a need to propagate this type to the >>> actual >>> bridge. In other words, what the connector does is distinct from >>> the Synopsys >>> driver which acts as the bridge with regard to the Ingenic driver. >>> Perhaps the Synopsys driver should set the connector's bridge as >>> the next >>> bridge, or maybe something is supposed to discover that the >>> connector may act >>> as (or provide) a bridge after the Synopsys driver in the chain >>> and then back- >>> propagate the bridge type along the chain. >>> [...] >>>> > And I removed any of the above hacks. What I observe, apart >>>> from an >>>> > inactive LCD controller (and ingenic-drm driver), is the >>>> following in >>>> > /sys/devices/platform/10180000.hdmi/: >>>> > >>>> > consumer:platform:13050000.lcdc0 >>>> > consumer:platform:hdmi_connector >>> Interestingly, with the connector driver present, these sysfs >>> entries no >>> longer appear. >>> [...] >>>> > For me, running modetest yields plenty of information about >>>> encoders, >>>> > connectors (and the supported modes via the EDID, thanks to my >>>> HDMI-A >>>> > hack), CRTCs, and planes. But no framebuffers are reported. >>>> Could you paste the result of "modetest -a -c -p" somewhere maybe? >>> I had to specify -M ingenic-drm as well, but here you go... >>> ---- >>> Connectors: >>> id encoder status name size (mm) modes encoders >>> 35 34 connected HDMI-A-1 340x270 17 34 >>> modes: >>> index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot) >>> #0 1280x1024 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 >>> flags: >>> phsync, pvsync; type: preferred, driver >>> #1 1280x1024 75.02 1280 1296 1440 1688 1024 1025 1028 1066 135000 >>> flags: >>> phsync, pvsync; type: driver >>> #2 1280x960 60.00 1280 1376 1488 1800 960 961 964 1000 108000 >>> flags: phsync, >>> pvsync; type: driver >>> #3 1152x864 75.00 1152 1216 1344 1600 864 865 868 900 108000 >>> flags: phsync, >>> pvsync; type: driver >>> #4 1024x768 75.03 1024 1040 1136 1312 768 769 772 800 78750 >>> flags: phsync, >>> pvsync; type: driver >>> #5 1024x768 70.07 1024 1048 1184 1328 768 771 777 806 75000 >>> flags: nhsync, >>> nvsync; type: driver >>> #6 1024x768 60.00 1024 1048 1184 1344 768 771 777 806 65000 >>> flags: nhsync, >>> nvsync; type: driver >>> #7 832x624 74.55 832 864 928 1152 624 625 628 667 57284 flags: >>> nhsync, >>> nvsync; type: driver >>> #8 800x600 75.00 800 816 896 1056 600 601 604 625 49500 flags: >>> phsync, >>> pvsync; type: driver >>> #9 800x600 72.19 800 856 976 1040 600 637 643 666 50000 flags: >>> phsync, >>> pvsync; type: driver >>> #10 800x600 60.32 800 840 968 1056 600 601 605 628 40000 flags: >>> phsync, >>> pvsync; type: driver >>> #11 800x600 56.25 800 824 896 1024 600 601 603 625 36000 flags: >>> phsync, >>> pvsync; type: driver >>> #12 640x480 75.00 640 656 720 840 480 481 484 500 31500 flags: >>> nhsync, >>> nvsync; type: driver >>> #13 640x480 72.81 640 664 704 832 480 489 492 520 31500 flags: >>> nhsync, >>> nvsync; type: driver >>> #14 640x480 66.67 640 704 768 864 480 483 486 525 30240 flags: >>> nhsync, >>> nvsync; type: driver >>> #15 640x480 59.94 640 656 752 800 480 490 492 525 25175 flags: >>> nhsync, >>> nvsync; type: driver >>> #16 720x400 70.08 720 738 846 900 400 412 414 449 28320 flags: >>> nhsync, >>> pvsync; type: driver >>> props: >>> 1 EDID: >>> flags: immutable blob >>> blobs: >>> value: >>> 00ffffffffffff00047232ad01010101 >>> 2d0e010380221b782aaea5a6544c9926 >>> 145054bfef0081808140714f01010101 >>> 010101010101302a009851002a403070 >>> 1300520e1100001e000000ff00343435 >>> 3030353444454330300a000000fc0041 >>> 4c313731350a202020202020000000fd >>> 00384c1e520e000a2020202020200051 >>> 2 DPMS: >>> flags: enum >>> enums: On=0 Standby=1 Suspend=2 Off=3 >>> value: 0 >>> 5 link-status: >>> flags: enum >>> enums: Good=0 Bad=1 >>> value: 0 >>> 6 non-desktop: >>> flags: immutable range >>> values: 0 1 >>> value: 0 >>> 4 TILE: >>> flags: immutable blob >>> blobs: >>> value: >>> 20 CRTC_ID: >>> flags: object >>> value: 32 >>> CRTCs: >>> id fb pos size >>> 32 39 (0,0) (1280x1024) >>> #0 60.02 1280 1328 1440 1688 1024 1025 1028 1066 108000 flags: >>> phsync, >>> pvsync; type: >>> props: >>> 22 ACTIVE: >>> flags: range >>> values: 0 1 >>> value: 1 >>> 23 MODE_ID: >>> flags: blob >>> blobs: >>> value: >>> e0a5010000053005a005980600000004 >>> 010404042a0400003c00000005000000 >>> 00000000000000000000000000000000 >>> 00000000000000000000000000000000 >>> 00000000 >>> 19 OUT_FENCE_PTR: >>> flags: range >>> values: 0 18446744073709551615 >>> value: 0 >>> 24 VRR_ENABLED: >>> flags: range >>> values: 0 1 >>> value: 0 >>> 28 GAMMA_LUT: >>> flags: blob >>> blobs: >>> value: >>> 29 GAMMA_LUT_SIZE: >>> flags: immutable range >>> values: 0 4294967295 >>> value: 256 >>> Planes: >>> id crtc fb CRTC x,y x,y gamma size possible crtcs >>> 31 32 39 0,0 0,0 0 0x00000001 >>> formats: XR15 RG16 RG24 XR24 XR30 >>> props: >>> 8 type: >>> flags: immutable enum >>> enums: Overlay=0 Primary=1 Cursor=2 >>> value: 1 >>> 17 FB_ID: >>> flags: object >>> value: 39 >>> 18 IN_FENCE_FD: >>> flags: signed range >>> values: -1 2147483647 >>> value: -1 >>> 20 CRTC_ID: >>> flags: object >>> value: 32 >>> 13 CRTC_X: >>> flags: signed range >>> values: -2147483648 2147483647 >>> value: 0 >>> 14 CRTC_Y: >>> flags: signed range >>> values: -2147483648 2147483647 >>> value: 0 >>> 15 CRTC_W: >>> flags: range >>> values: 0 2147483647 >>> value: 1280 >>> 16 CRTC_H: >>> flags: range >>> values: 0 2147483647 >>> value: 1024 >>> 9 SRC_X: >>> flags: range >>> values: 0 4294967295 >>> value: 0 >>> 10 SRC_Y: >>> flags: range >>> values: 0 4294967295 >>> value: 0 >>> 11 SRC_W: >>> flags: range >>> values: 0 4294967295 >>> value: 83886080 >>> 12 SRC_H: >>> flags: range >>> values: 0 4294967295 >>> value: 67108864 >>> 33 0 0 0,0 0,0 0 0x00000001 >>> formats: C8 XR15 RG16 RG24 XR24 XR30 >>> props: >>> 8 type: >>> flags: immutable enum >>> enums: Overlay=0 Primary=1 Cursor=2 >>> value: 0 >>> 17 FB_ID: >>> flags: object >>> value: 0 >>> 18 IN_FENCE_FD: >>> flags: signed range >>> values: -1 2147483647 >>> value: -1 >>> 20 CRTC_ID: >>> flags: object >>> value: 0 >>> 13 CRTC_X: >>> flags: signed range >>> values: -2147483648 2147483647 >>> value: 0 >>> 14 CRTC_Y: >>> flags: signed range >>> values: -2147483648 2147483647 >>> value: 0 >>> 15 CRTC_W: >>> flags: range >>> values: 0 2147483647 >>> value: 0 >>> 16 CRTC_H: >>> flags: range >>> values: 0 2147483647 >>> value: 0 >>> 9 SRC_X: >>> flags: range >>> values: 0 4294967295 >>> value: 0 >>> 10 SRC_Y: >>> flags: range >>> values: 0 4294967295 >>> value: 0 >>> 11 SRC_W: >>> flags: range >>> values: 0 4294967295 >>> value: 0 >>> 12 SRC_H: >>> flags: range >>> values: 0 4294967295 >>> value: 0 >>> ---- >>>> If you have info about the CRTCs, encoders, connectors and EDID >>>> info, >>>> then I would assume it is very close to working fine. >>>> For your "no framebuffer" issue, keep in mind that CONFIG_FB and >>>> CONFIG_FRAMEBUFFER_CONSOLE are now disabled by default. >>> Yes, I discovered that CONFIG_FB was not enabled, so I did so. >>>> If that doesn't fix anything, that probably means that one >>>> .atomic_check() fails, so it would be a good place to start >>>> debugging. >>> There will be other things to verify in the Ingenic driver. As >>> noted many >>> months ago, colour depth information has to be set in the DMA >>> descriptors and >>> not the control register, but we are managing to do this >>> successfully, as far >>> as I can tell, although there is always the potential for error. >>> Paul >> >> >
Hi Paul, > Am 25.09.2021 um 21:39 schrieb Paul Cercueil <paul@crapouillou.net>: > > > > Le sam., sept. 25 2021 at 21:26:42 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >>> Am 25.09.2021 um 21:08 schrieb Paul Cercueil <paul@crapouillou.net>: >>> Hi Paul & Nikolaus, >>> If you spent some time debugging the issue >> we did ... > > By saying that you didn't debug, We did - but sometimes you don't see the wood for the trees. > (null) means you're printing a NULL pointer. So I could see that hdmi->next_bridge was NULL. I remember we did find this, but did not understand that it should be initialized by dw-hdmi. And because we though that dw-hdmi has it its own connector, it is ok that way. > The place that sets it is dw_hdmi_parse_dt, which will return early with code 0, before next_bridge is set, if plat_data->output_port == 0, which was your case. Well, we were still at 5.14 when we did these initial attempts to use hdmi-connector with synopsys. Back then, there was no dw_hdmi_parse_dt and no output_port. IAW: we did not even have a chance to make it work on top of 5.14 the hdmi-connector way. And were sucessful. I just noticed this when trying to backport the last puzzle piece... Well, it is always difficult to hit a moving target. > Since your hdmi-connector is wired at port #1, then .output_port should be 1 as well. Anyways it works now for 5.14.8 (our internal test) and 5.15-rc3. And v4 of the jz4780 hdmi stuff will follow. BR and thanks, Nikolaus
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); + 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) {
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@crapouillou.net> --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 92 +++++++++++++++++------ 1 file changed, 70 insertions(+), 22 deletions(-)