Message-Id: <32234186-1802-4FDF-801A-B14E48FB86D8@goldelico.com>
Date:   Thu, 23 Sep 2021 07:52:08 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        linux-mips <linux-mips@...r.kernel.org>, list@...ndingux.net,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH v3 6/6] drm/ingenic: Attach bridge chain to encoders

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@...pouillou.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@...pouillou.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

Powered by blists - more mailing lists