[PATCH v2 0/4] Converter R-Car DU to the DRM bridge connector helper

[PATCH v2 0/4] Converter R-Car DU to the DRM bridge connector helper

[Laurent Pinchart]

Hello,

This patch series converts the R-Car DU driver to use the DRM bridge
connector helper drm_bridge_connector_init().

The bulk of the v1 series was converting the adv7511, simple-bridge and
dw-hdmi drivers to make connector creation optional (through the
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag), and have already been merged. v2
includes the remaining patches.

Patch 1/4 addresses the rcar-lvds driver. Instead of implementing direct
support for DRM_BRIDGE_ATTACH_NO_CONNECTOR, it simply removes code that
shouldn't have been in the driver in the first place by switching to the
panel bridge helper.

Patches 2/4 then adds support to the dw-hdmi driver to attach to a
downstream bridge if one is specified in DT. As the DT port number
corresponding to the video output differs between platforms that
integrate the dw-hdmi (some of them even don't have a video output port,
which should probably be fixed, but that's out of scope for this
series), the port number has to be specified by the platform glue layer.
Patch 3/4 does so for the R-Car dw-hdmi driver.

Patch 4/4 finally makes use of the drm_bridge_connector_init() helper.

The series depends on "[PATCH 27/27] drm: Add default modes for
connectors in unknown state" ([1]) to avoid a breakage on the VGA output
on R-Car Gen3 platforms. That patch has already been approved, and I'll
get it merged as a prerequisite.

The series has been tested on the Renesas R-Car Salvator-XS and Draak
boards with the VGA, HDMI and LVDS outputs.

[1] https://lore.kernel.org/dri-devel/20200526011505.31884-28-laurent.pinchart+renesas@ideasonboard.com/

Laurent Pinchart (4):
  drm: rcar-du: lvds: Convert to DRM panel bridge helper
  drm: bridge: dw-hdmi: Attach to next bridge if available
  drm: rcar-du: dw-hdmi: Set output port number
  drm: rcar-du: Use drm_bridge_connector_init() helper

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  54 +++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  25 ++++-
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c    |   1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c       | 120 +++-------------------
 include/drm/bridge/dw_hdmi.h              |   2 +
 5 files changed, 88 insertions(+), 114 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 0/4] Converter R-Car DU to the DRM bridge connector helper

[Laurent Pinchart]

Hello,

This patch series converts the R-Car DU driver to use the DRM bridge
connector helper drm_bridge_connector_init().

The bulk of the v1 series was converting the adv7511, simple-bridge and
dw-hdmi drivers to make connector creation optional (through the
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag), and have already been merged. v2
includes the remaining patches.

Patch 1/4 addresses the rcar-lvds driver. Instead of implementing direct
support for DRM_BRIDGE_ATTACH_NO_CONNECTOR, it simply removes code that
shouldn't have been in the driver in the first place by switching to the
panel bridge helper.

Patches 2/4 then adds support to the dw-hdmi driver to attach to a
downstream bridge if one is specified in DT. As the DT port number
corresponding to the video output differs between platforms that
integrate the dw-hdmi (some of them even don't have a video output port,
which should probably be fixed, but that's out of scope for this
series), the port number has to be specified by the platform glue layer.
Patch 3/4 does so for the R-Car dw-hdmi driver.

Patch 4/4 finally makes use of the drm_bridge_connector_init() helper.

The series depends on "[PATCH 27/27] drm: Add default modes for
connectors in unknown state" ([1]) to avoid a breakage on the VGA output
on R-Car Gen3 platforms. That patch has already been approved, and I'll
get it merged as a prerequisite.

The series has been tested on the Renesas R-Car Salvator-XS and Draak
boards with the VGA, HDMI and LVDS outputs.

[1] https://lore.kernel.org/dri-devel/20200526011505.31884-28-laurent.pinchart+renesas@ideasonboard.com/

Laurent Pinchart (4):
  drm: rcar-du: lvds: Convert to DRM panel bridge helper
  drm: bridge: dw-hdmi: Attach to next bridge if available
  drm: rcar-du: dw-hdmi: Set output port number
  drm: rcar-du: Use drm_bridge_connector_init() helper

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  54 +++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  25 ++++-
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c    |   1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c       | 120 +++-------------------
 include/drm/bridge/dw_hdmi.h              |   2 +
 5 files changed, 88 insertions(+), 114 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Laurent Pinchart]

Replace the manual panel handling with usage of the DRM panel bridge
helper. This simplifies the driver, and brings support for
DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++-------------------------
 1 file changed, 12 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 70dbbe44bb23..1b360e06658c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -63,7 +63,6 @@ struct rcar_lvds {
 	struct drm_bridge bridge;
 
 	struct drm_bridge *next_bridge;
-	struct drm_connector connector;
 	struct drm_panel *panel;
 
 	void __iomem *mmio;
@@ -80,73 +79,11 @@ struct rcar_lvds {
 #define bridge_to_rcar_lvds(b) \
 	container_of(b, struct rcar_lvds, bridge)
 
-#define connector_to_rcar_lvds(c) \
-	container_of(c, struct rcar_lvds, connector)
-
 static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
 {
 	iowrite32(data, lvds->mmio + reg);
 }
 
-/* -----------------------------------------------------------------------------
- * Connector & Panel
- */
-
-static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
-{
-	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
-
-	return drm_panel_get_modes(lvds->panel, connector);
-}
-
-static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
-					    struct drm_atomic_state *state)
-{
-	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
-	const struct drm_display_mode *panel_mode;
-	struct drm_connector_state *conn_state;
-	struct drm_crtc_state *crtc_state;
-
-	conn_state = drm_atomic_get_new_connector_state(state, connector);
-	if (!conn_state->crtc)
-		return 0;
-
-	if (list_empty(&connector->modes)) {
-		dev_dbg(lvds->dev, "connector: empty modes list\n");
-		return -EINVAL;
-	}
-
-	panel_mode = list_first_entry(&connector->modes,
-				      struct drm_display_mode, head);
-
-	/* We're not allowed to modify the resolution. */
-	crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
-	    crtc_state->mode.vdisplay != panel_mode->vdisplay)
-		return -EINVAL;
-
-	/* The flat panel mode is fixed, just copy it to the adjusted mode. */
-	drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
-
-	return 0;
-}
-
-static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
-	.get_modes = rcar_lvds_connector_get_modes,
-	.atomic_check = rcar_lvds_connector_atomic_check,
-};
-
-static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
 /* -----------------------------------------------------------------------------
  * PLL Setup
  */
@@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
 	/* Turn the output on. */
 	lvdcr0 |= LVDCR0_LVRES;
 	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
-
-	if (lvds->panel) {
-		drm_panel_prepare(lvds->panel);
-		drm_panel_enable(lvds->panel);
-	}
 }
 
 static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
@@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
-	if (lvds->panel) {
-		drm_panel_disable(lvds->panel);
-		drm_panel_unprepare(lvds->panel);
-	}
-
 	rcar_lvds_write(lvds, LVDCR0, 0);
 	rcar_lvds_write(lvds, LVDCR1, 0);
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
@@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge,
 			    enum drm_bridge_attach_flags flags)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
-	struct drm_connector *connector = &lvds->connector;
-	struct drm_encoder *encoder = bridge->encoder;
-	int ret;
 
-	/* If we have a next bridge just attach it. */
-	if (lvds->next_bridge)
-		return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
-					 bridge, flags);
-
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
-	/* Otherwise if we have a panel, create a connector. */
-	if (!lvds->panel)
-		return 0;
-
-	ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
-				 DRM_MODE_CONNECTOR_LVDS);
-	if (ret < 0)
-		return ret;
-
-	drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
-
-	ret = drm_connector_attach_encoder(connector, encoder);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
-static void rcar_lvds_detach(struct drm_bridge *bridge)
-{
+	return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
+				 flags);
 }
 
 static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
 	.attach = rcar_lvds_attach,
-	.detach = rcar_lvds_detach,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
@@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 		 * that we are expected to generate even pixels from the primary
 		 * encoder, and odd pixels from the companion encoder.
 		 */
-		if (lvds->next_bridge && lvds->next_bridge->timings &&
+		if (lvds->next_bridge->timings &&
 		    lvds->next_bridge->timings->dual_link)
 			lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
 		else
@@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 	if (ret)
 		goto done;
 
+	if (lvds->panel) {
+		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
+							      lvds->panel);
+		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
+			ret = -EINVAL;
+			goto done;
+		}
+	}
+
 	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
 		ret = rcar_lvds_parse_dt_companion(lvds);
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Laurent Pinchart]

Replace the manual panel handling with usage of the DRM panel bridge
helper. This simplifies the driver, and brings support for
DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++-------------------------
 1 file changed, 12 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 70dbbe44bb23..1b360e06658c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -63,7 +63,6 @@ struct rcar_lvds {
 	struct drm_bridge bridge;
 
 	struct drm_bridge *next_bridge;
-	struct drm_connector connector;
 	struct drm_panel *panel;
 
 	void __iomem *mmio;
@@ -80,73 +79,11 @@ struct rcar_lvds {
 #define bridge_to_rcar_lvds(b) \
 	container_of(b, struct rcar_lvds, bridge)
 
-#define connector_to_rcar_lvds(c) \
-	container_of(c, struct rcar_lvds, connector)
-
 static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
 {
 	iowrite32(data, lvds->mmio + reg);
 }
 
-/* -----------------------------------------------------------------------------
- * Connector & Panel
- */
-
-static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
-{
-	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
-
-	return drm_panel_get_modes(lvds->panel, connector);
-}
-
-static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
-					    struct drm_atomic_state *state)
-{
-	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
-	const struct drm_display_mode *panel_mode;
-	struct drm_connector_state *conn_state;
-	struct drm_crtc_state *crtc_state;
-
-	conn_state = drm_atomic_get_new_connector_state(state, connector);
-	if (!conn_state->crtc)
-		return 0;
-
-	if (list_empty(&connector->modes)) {
-		dev_dbg(lvds->dev, "connector: empty modes list\n");
-		return -EINVAL;
-	}
-
-	panel_mode = list_first_entry(&connector->modes,
-				      struct drm_display_mode, head);
-
-	/* We're not allowed to modify the resolution. */
-	crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
-	    crtc_state->mode.vdisplay != panel_mode->vdisplay)
-		return -EINVAL;
-
-	/* The flat panel mode is fixed, just copy it to the adjusted mode. */
-	drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
-
-	return 0;
-}
-
-static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
-	.get_modes = rcar_lvds_connector_get_modes,
-	.atomic_check = rcar_lvds_connector_atomic_check,
-};
-
-static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
 /* -----------------------------------------------------------------------------
  * PLL Setup
  */
@@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
 	/* Turn the output on. */
 	lvdcr0 |= LVDCR0_LVRES;
 	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
-
-	if (lvds->panel) {
-		drm_panel_prepare(lvds->panel);
-		drm_panel_enable(lvds->panel);
-	}
 }
 
 static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
@@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
-	if (lvds->panel) {
-		drm_panel_disable(lvds->panel);
-		drm_panel_unprepare(lvds->panel);
-	}
-
 	rcar_lvds_write(lvds, LVDCR0, 0);
 	rcar_lvds_write(lvds, LVDCR1, 0);
 	rcar_lvds_write(lvds, LVDPLLCR, 0);
@@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge,
 			    enum drm_bridge_attach_flags flags)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
-	struct drm_connector *connector = &lvds->connector;
-	struct drm_encoder *encoder = bridge->encoder;
-	int ret;
 
-	/* If we have a next bridge just attach it. */
-	if (lvds->next_bridge)
-		return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
-					 bridge, flags);
-
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
-	/* Otherwise if we have a panel, create a connector. */
-	if (!lvds->panel)
-		return 0;
-
-	ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
-				 DRM_MODE_CONNECTOR_LVDS);
-	if (ret < 0)
-		return ret;
-
-	drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
-
-	ret = drm_connector_attach_encoder(connector, encoder);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
-static void rcar_lvds_detach(struct drm_bridge *bridge)
-{
+	return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
+				 flags);
 }
 
 static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
 	.attach = rcar_lvds_attach,
-	.detach = rcar_lvds_detach,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
@@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 		 * that we are expected to generate even pixels from the primary
 		 * encoder, and odd pixels from the companion encoder.
 		 */
-		if (lvds->next_bridge && lvds->next_bridge->timings &&
+		if (lvds->next_bridge->timings &&
 		    lvds->next_bridge->timings->dual_link)
 			lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
 		else
@@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 	if (ret)
 		goto done;
 
+	if (lvds->panel) {
+		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
+							      lvds->panel);
+		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
+			ret = -EINVAL;
+			goto done;
+		}
+	}
+
 	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
 		ret = rcar_lvds_parse_dt_companion(lvds);
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/4] drm: bridge: dw-hdmi: Attach to next bridge if available

[Laurent Pinchart]

On all platforms except i.MX and Rockchip, the dw-hdmi DT bindings
require a video output port connected to an HDMI sink (most likely an
HDMI connector, in rare cases another bridges converting HDMI to another
protocol). For those platforms, retrieve the next bridge and attach it
from the dw-hdmi bridge attach handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
Changes since v1:

- Make missing endpoint a fatal error
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 54 ++++++++++++++++++++++-
 include/drm/bridge/dw_hdmi.h              |  2 +
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 0c79a9ba48bb..a8393ecd3d19 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -143,6 +143,7 @@ struct dw_hdmi_phy_data {
 struct dw_hdmi {
 	struct drm_connector connector;
 	struct drm_bridge bridge;
+	struct drm_bridge *next_bridge;
 
 	unsigned int version;
 
@@ -2791,7 +2792,8 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
 	struct dw_hdmi *hdmi = bridge->driver_private;
 
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
-		return 0;
+		return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
+					 bridge, flags);
 
 	return dw_hdmi_connector_create(hdmi);
 }
@@ -3176,6 +3178,52 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi)
 /* -----------------------------------------------------------------------------
  * Probe/remove API, used from platforms based on the DRM bridge API.
  */
+
+static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
+{
+	struct device_node *endpoint;
+	struct device_node *remote;
+
+	if (!hdmi->plat_data->output_port)
+		return 0;
+
+	endpoint = of_graph_get_endpoint_by_regs(hdmi->dev->of_node,
+						 hdmi->plat_data->output_port,
+						 -1);
+	if (!endpoint) {
+		/*
+		 * On platforms whose bindings don't make the output port
+		 * mandatory (such as Rockchip) the plat_data->output_port
+		 * field isn't set, so it's safe to make this a fatal error.
+		 */
+		dev_err(hdmi->dev, "Missing endpoint in port@%u\n",
+			hdmi->plat_data->output_port);
+		return -ENODEV;
+	}
+
+	remote = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+	if (!remote) {
+		dev_err(hdmi->dev, "Endpoint in port@%u unconnected\n",
+			hdmi->plat_data->output_port);
+		return -ENODEV;
+	}
+
+	if (!of_device_is_available(remote)) {
+		dev_err(hdmi->dev, "port@%u remote device is disabled\n",
+			hdmi->plat_data->output_port);
+		of_node_put(remote);
+		return -ENODEV;
+	}
+
+	hdmi->next_bridge = of_drm_find_bridge(remote);
+	of_node_put(remote);
+	if (!hdmi->next_bridge)
+		return -EPROBE_DEFER;
+
+	return 0;
+}
+
 struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 			      const struct dw_hdmi_plat_data *plat_data)
 {
@@ -3212,6 +3260,10 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 	mutex_init(&hdmi->cec_notifier_mutex);
 	spin_lock_init(&hdmi->audio_lock);
 
+	ret = dw_hdmi_parse_dt(hdmi);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
 	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
 	if (ddc_node) {
 		hdmi->ddc = of_get_i2c_adapter_by_node(ddc_node);
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index ea34ca146b82..8ebeb65d6371 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -126,6 +126,8 @@ struct dw_hdmi_phy_ops {
 struct dw_hdmi_plat_data {
 	struct regmap *regm;
 
+	unsigned int output_port;
+
 	unsigned long input_bus_encoding;
 	bool use_drm_infoframe;
 	bool ycbcr_420_allowed;
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/4] drm: bridge: dw-hdmi: Attach to next bridge if available

[Laurent Pinchart]

On all platforms except i.MX and Rockchip, the dw-hdmi DT bindings
require a video output port connected to an HDMI sink (most likely an
HDMI connector, in rare cases another bridges converting HDMI to another
protocol). For those platforms, retrieve the next bridge and attach it
from the dw-hdmi bridge attach handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
Changes since v1:

- Make missing endpoint a fatal error
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 54 ++++++++++++++++++++++-
 include/drm/bridge/dw_hdmi.h              |  2 +
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 0c79a9ba48bb..a8393ecd3d19 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -143,6 +143,7 @@ struct dw_hdmi_phy_data {
 struct dw_hdmi {
 	struct drm_connector connector;
 	struct drm_bridge bridge;
+	struct drm_bridge *next_bridge;
 
 	unsigned int version;
 
@@ -2791,7 +2792,8 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge,
 	struct dw_hdmi *hdmi = bridge->driver_private;
 
 	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
-		return 0;
+		return drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
+					 bridge, flags);
 
 	return dw_hdmi_connector_create(hdmi);
 }
@@ -3176,6 +3178,52 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi)
 /* -----------------------------------------------------------------------------
  * Probe/remove API, used from platforms based on the DRM bridge API.
  */
+
+static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
+{
+	struct device_node *endpoint;
+	struct device_node *remote;
+
+	if (!hdmi->plat_data->output_port)
+		return 0;
+
+	endpoint = of_graph_get_endpoint_by_regs(hdmi->dev->of_node,
+						 hdmi->plat_data->output_port,
+						 -1);
+	if (!endpoint) {
+		/*
+		 * On platforms whose bindings don't make the output port
+		 * mandatory (such as Rockchip) the plat_data->output_port
+		 * field isn't set, so it's safe to make this a fatal error.
+		 */
+		dev_err(hdmi->dev, "Missing endpoint in port@%u\n",
+			hdmi->plat_data->output_port);
+		return -ENODEV;
+	}
+
+	remote = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+	if (!remote) {
+		dev_err(hdmi->dev, "Endpoint in port@%u unconnected\n",
+			hdmi->plat_data->output_port);
+		return -ENODEV;
+	}
+
+	if (!of_device_is_available(remote)) {
+		dev_err(hdmi->dev, "port@%u remote device is disabled\n",
+			hdmi->plat_data->output_port);
+		of_node_put(remote);
+		return -ENODEV;
+	}
+
+	hdmi->next_bridge = of_drm_find_bridge(remote);
+	of_node_put(remote);
+	if (!hdmi->next_bridge)
+		return -EPROBE_DEFER;
+
+	return 0;
+}
+
 struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 			      const struct dw_hdmi_plat_data *plat_data)
 {
@@ -3212,6 +3260,10 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 	mutex_init(&hdmi->cec_notifier_mutex);
 	spin_lock_init(&hdmi->audio_lock);
 
+	ret = dw_hdmi_parse_dt(hdmi);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
 	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
 	if (ddc_node) {
 		hdmi->ddc = of_get_i2c_adapter_by_node(ddc_node);
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index ea34ca146b82..8ebeb65d6371 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -126,6 +126,8 @@ struct dw_hdmi_phy_ops {
 struct dw_hdmi_plat_data {
 	struct regmap *regm;
 
+	unsigned int output_port;
+
 	unsigned long input_bus_encoding;
 	bool use_drm_infoframe;
 	bool ycbcr_420_allowed;
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/4] drm: rcar-du: dw-hdmi: Set output port number

[Laurent Pinchart]

Report the DT output port number in dw_hdmi_plat_data to connect to the
next bridge in the dw-hdmi driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index 7b8ec8310699..18ed14911b98 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -75,6 +75,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi, void *data,
 }
 
 static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
+	.output_port = 1,
 	.mode_valid = rcar_hdmi_mode_valid,
 	.configure_phy	= rcar_hdmi_phy_configure,
 };
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/4] drm: rcar-du: dw-hdmi: Set output port number

[Laurent Pinchart]

Report the DT output port number in dw_hdmi_plat_data to connect to the
next bridge in the dw-hdmi driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index 7b8ec8310699..18ed14911b98 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -75,6 +75,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi, void *data,
 }
 
 static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
+	.output_port = 1,
 	.mode_valid = rcar_hdmi_mode_valid,
 	.configure_phy	= rcar_hdmi_phy_configure,
 };
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm: rcar-du: Use drm_bridge_connector_init() helper

[Laurent Pinchart]

Use the drm_bridge_connector_init() helper to create a drm_connector for
each output, instead of relying on the bridge drivers doing so. Attach
the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct
them not to create a connector.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index ba8c6038cd63..10a66091391a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
@@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 			 struct device_node *enc_node)
 {
 	struct rcar_du_encoder *renc;
+	struct drm_connector *connector;
 	struct drm_bridge *bridge;
 	int ret;
 
@@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 	if (ret)
 		return ret;
 
-	/*
-	 * Attach the bridge to the encoder. The bridge will create the
-	 * connector.
-	 */
-	return drm_bridge_attach(&renc->base, bridge, NULL, 0);
+	/* Attach the bridge to the encoder. */
+	ret = drm_bridge_attach(&renc->base, bridge, NULL,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret) {
+		dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
+			output);
+		return ret;
+	}
+
+	/* Create the connector for the chain of bridges. */
+	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
+	if (IS_ERR(connector)) {
+		dev_err(rcdu->dev,
+			"failed to created connector for output %u\n", output);
+		return PTR_ERR(connector);
+	}
+
+	return drm_connector_attach_encoder(connector, &renc->base);
 }
-- 
Regards,

Laurent Pinchart

[PATCH v2 4/4] drm: rcar-du: Use drm_bridge_connector_init() helper

[Laurent Pinchart]

Use the drm_bridge_connector_init() helper to create a drm_connector for
each output, instead of relying on the bridge drivers doing so. Attach
the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct
them not to create a connector.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index ba8c6038cd63..10a66091391a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
@@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 			 struct device_node *enc_node)
 {
 	struct rcar_du_encoder *renc;
+	struct drm_connector *connector;
 	struct drm_bridge *bridge;
 	int ret;
 
@@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 	if (ret)
 		return ret;
 
-	/*
-	 * Attach the bridge to the encoder. The bridge will create the
-	 * connector.
-	 */
-	return drm_bridge_attach(&renc->base, bridge, NULL, 0);
+	/* Attach the bridge to the encoder. */
+	ret = drm_bridge_attach(&renc->base, bridge, NULL,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret) {
+		dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
+			output);
+		return ret;
+	}
+
+	/* Create the connector for the chain of bridges. */
+	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
+	if (IS_ERR(connector)) {
+		dev_err(rcdu->dev,
+			"failed to created connector for output %u\n", output);
+		return PTR_ERR(connector);
+	}
+
+	return drm_connector_attach_encoder(connector, &renc->base);
 }
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm: rcar-du: Use drm_bridge_connector_init() helper

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 02:50:21AM +0200, Laurent Pinchart wrote:
> Use the drm_bridge_connector_init() helper to create a drm_connector for
> each output, instead of relying on the bridge drivers doing so. Attach
> the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct
> them not to create a connector.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index ba8c6038cd63..10a66091391a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_modeset_helper_vtables.h>
> @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  			 struct device_node *enc_node)
>  {
>  	struct rcar_du_encoder *renc;
> +	struct drm_connector *connector;
>  	struct drm_bridge *bridge;
>  	int ret;
>
> @@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  	if (ret)
>  		return ret;
>
> -	/*
> -	 * Attach the bridge to the encoder. The bridge will create the
> -	 * connector.
> -	 */
> -	return drm_bridge_attach(&renc->base, bridge, NULL, 0);
> +	/* Attach the bridge to the encoder. */
> +	ret = drm_bridge_attach(&renc->base, bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret) {
> +		dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
> +			output);
> +		return ret;
> +	}
> +
> +	/* Create the connector for the chain of bridges. */
> +	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
> +	if (IS_ERR(connector)) {
> +		dev_err(rcdu->dev,
> +			"failed to created connector for output %u\n", output);
> +		return PTR_ERR(connector);
> +	}
> +
> +	return drm_connector_attach_encoder(connector, &renc->base);

Looks good
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I'm trying to figure out how deferred probe of a panel directly
connected to the lvds encoder work. I assume there's no risk of
creating the connector before the panel is probed, or this is not an
issue.

>  }
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 4/4] drm: rcar-du: Use drm_bridge_connector_init() helper

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 02:50:21AM +0200, Laurent Pinchart wrote:
> Use the drm_bridge_connector_init() helper to create a drm_connector for
> each output, instead of relying on the bridge drivers doing so. Attach
> the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct
> them not to create a connector.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index ba8c6038cd63..10a66091391a 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_modeset_helper_vtables.h>
> @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  			 struct device_node *enc_node)
>  {
>  	struct rcar_du_encoder *renc;
> +	struct drm_connector *connector;
>  	struct drm_bridge *bridge;
>  	int ret;
>
> @@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  	if (ret)
>  		return ret;
>
> -	/*
> -	 * Attach the bridge to the encoder. The bridge will create the
> -	 * connector.
> -	 */
> -	return drm_bridge_attach(&renc->base, bridge, NULL, 0);
> +	/* Attach the bridge to the encoder. */
> +	ret = drm_bridge_attach(&renc->base, bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret) {
> +		dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
> +			output);
> +		return ret;
> +	}
> +
> +	/* Create the connector for the chain of bridges. */
> +	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
> +	if (IS_ERR(connector)) {
> +		dev_err(rcdu->dev,
> +			"failed to created connector for output %u\n", output);
> +		return PTR_ERR(connector);
> +	}
> +
> +	return drm_connector_attach_encoder(connector, &renc->base);

Looks good
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I'm trying to figure out how deferred probe of a panel directly
connected to the lvds encoder work. I assume there's no risk of
creating the connector before the panel is probed, or this is not an
issue.

>  }
> --
> Regards,
>
> Laurent Pinchart
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 02:50:18AM +0200, Laurent Pinchart wrote:
> Replace the manual panel handling with usage of the DRM panel bridge
> helper. This simplifies the driver, and brings support for
> DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++-------------------------
>  1 file changed, 12 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 70dbbe44bb23..1b360e06658c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -63,7 +63,6 @@ struct rcar_lvds {
>  	struct drm_bridge bridge;
>
>  	struct drm_bridge *next_bridge;
> -	struct drm_connector connector;
>  	struct drm_panel *panel;
>
>  	void __iomem *mmio;
> @@ -80,73 +79,11 @@ struct rcar_lvds {
>  #define bridge_to_rcar_lvds(b) \
>  	container_of(b, struct rcar_lvds, bridge)
>
> -#define connector_to_rcar_lvds(c) \
> -	container_of(c, struct rcar_lvds, connector)
> -
>  static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
>  {
>  	iowrite32(data, lvds->mmio + reg);
>  }
>
> -/* -----------------------------------------------------------------------------
> - * Connector & Panel
> - */
> -
> -static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
> -{
> -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> -
> -	return drm_panel_get_modes(lvds->panel, connector);
> -}
> -
> -static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
> -					    struct drm_atomic_state *state)
> -{
> -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> -	const struct drm_display_mode *panel_mode;
> -	struct drm_connector_state *conn_state;
> -	struct drm_crtc_state *crtc_state;
> -
> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> -	if (!conn_state->crtc)
> -		return 0;
> -
> -	if (list_empty(&connector->modes)) {
> -		dev_dbg(lvds->dev, "connector: empty modes list\n");
> -		return -EINVAL;
> -	}
> -
> -	panel_mode = list_first_entry(&connector->modes,
> -				      struct drm_display_mode, head);
> -
> -	/* We're not allowed to modify the resolution. */
> -	crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> -	if (IS_ERR(crtc_state))
> -		return PTR_ERR(crtc_state);
> -
> -	if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
> -	    crtc_state->mode.vdisplay != panel_mode->vdisplay)
> -		return -EINVAL;
> -
> -	/* The flat panel mode is fixed, just copy it to the adjusted mode. */
> -	drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
> -
> -	return 0;
> -}
> -
> -static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
> -	.get_modes = rcar_lvds_connector_get_modes,
> -	.atomic_check = rcar_lvds_connector_atomic_check,
> -};
> -
> -static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
> -	.reset = drm_atomic_helper_connector_reset,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
>  /* -----------------------------------------------------------------------------
>   * PLL Setup
>   */
> @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>  	/* Turn the output on. */
>  	lvdcr0 |= LVDCR0_LVRES;
>  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> -
> -	if (lvds->panel) {
> -		drm_panel_prepare(lvds->panel);
> -		drm_panel_enable(lvds->panel);
> -	}
>  }
>
>  static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>
> -	if (lvds->panel) {
> -		drm_panel_disable(lvds->panel);
> -		drm_panel_unprepare(lvds->panel);
> -	}
> -
>  	rcar_lvds_write(lvds, LVDCR0, 0);
>  	rcar_lvds_write(lvds, LVDCR1, 0);
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
> @@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge,
>  			    enum drm_bridge_attach_flags flags)
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> -	struct drm_connector *connector = &lvds->connector;
> -	struct drm_encoder *encoder = bridge->encoder;
> -	int ret;
>
> -	/* If we have a next bridge just attach it. */
> -	if (lvds->next_bridge)
> -		return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
> -					 bridge, flags);
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
> -	/* Otherwise if we have a panel, create a connector. */
> -	if (!lvds->panel)
> -		return 0;
> -
> -	ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
> -				 DRM_MODE_CONNECTOR_LVDS);
> -	if (ret < 0)
> -		return ret;
> -
> -	drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
> -
> -	ret = drm_connector_attach_encoder(connector, encoder);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> -}
> -
> -static void rcar_lvds_detach(struct drm_bridge *bridge)
> -{
> +	return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
> +				 flags);
>  }
>
>  static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
>  	.attach = rcar_lvds_attach,
> -	.detach = rcar_lvds_detach,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
> @@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  		 * that we are expected to generate even pixels from the primary
>  		 * encoder, and odd pixels from the companion encoder.
>  		 */
> -		if (lvds->next_bridge && lvds->next_bridge->timings &&
> +		if (lvds->next_bridge->timings &&
>  		    lvds->next_bridge->timings->dual_link)
>  			lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
>  		else
> @@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  	if (ret)
>  		goto done;
>
> +	if (lvds->panel) {
> +		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
> +							      lvds->panel);

Reading the devm_drm_panel_bridge_add() function documentation:

 * devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector

Doesn't this conflict with the drm_bridge_connector_init() called by
the encoder in [4/4] ?


> +		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
> +			ret = -EINVAL;
> +			goto done;
> +		}
> +	}
> +
>  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
>  		ret = rcar_lvds_parse_dt_companion(lvds);
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 02:50:18AM +0200, Laurent Pinchart wrote:
> Replace the manual panel handling with usage of the DRM panel bridge
> helper. This simplifies the driver, and brings support for
> DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++-------------------------
>  1 file changed, 12 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 70dbbe44bb23..1b360e06658c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -63,7 +63,6 @@ struct rcar_lvds {
>  	struct drm_bridge bridge;
>
>  	struct drm_bridge *next_bridge;
> -	struct drm_connector connector;
>  	struct drm_panel *panel;
>
>  	void __iomem *mmio;
> @@ -80,73 +79,11 @@ struct rcar_lvds {
>  #define bridge_to_rcar_lvds(b) \
>  	container_of(b, struct rcar_lvds, bridge)
>
> -#define connector_to_rcar_lvds(c) \
> -	container_of(c, struct rcar_lvds, connector)
> -
>  static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
>  {
>  	iowrite32(data, lvds->mmio + reg);
>  }
>
> -/* -----------------------------------------------------------------------------
> - * Connector & Panel
> - */
> -
> -static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
> -{
> -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> -
> -	return drm_panel_get_modes(lvds->panel, connector);
> -}
> -
> -static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
> -					    struct drm_atomic_state *state)
> -{
> -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> -	const struct drm_display_mode *panel_mode;
> -	struct drm_connector_state *conn_state;
> -	struct drm_crtc_state *crtc_state;
> -
> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> -	if (!conn_state->crtc)
> -		return 0;
> -
> -	if (list_empty(&connector->modes)) {
> -		dev_dbg(lvds->dev, "connector: empty modes list\n");
> -		return -EINVAL;
> -	}
> -
> -	panel_mode = list_first_entry(&connector->modes,
> -				      struct drm_display_mode, head);
> -
> -	/* We're not allowed to modify the resolution. */
> -	crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> -	if (IS_ERR(crtc_state))
> -		return PTR_ERR(crtc_state);
> -
> -	if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
> -	    crtc_state->mode.vdisplay != panel_mode->vdisplay)
> -		return -EINVAL;
> -
> -	/* The flat panel mode is fixed, just copy it to the adjusted mode. */
> -	drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
> -
> -	return 0;
> -}
> -
> -static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
> -	.get_modes = rcar_lvds_connector_get_modes,
> -	.atomic_check = rcar_lvds_connector_atomic_check,
> -};
> -
> -static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
> -	.reset = drm_atomic_helper_connector_reset,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
>  /* -----------------------------------------------------------------------------
>   * PLL Setup
>   */
> @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
>  	/* Turn the output on. */
>  	lvdcr0 |= LVDCR0_LVRES;
>  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> -
> -	if (lvds->panel) {
> -		drm_panel_prepare(lvds->panel);
> -		drm_panel_enable(lvds->panel);
> -	}
>  }
>
>  static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>
> -	if (lvds->panel) {
> -		drm_panel_disable(lvds->panel);
> -		drm_panel_unprepare(lvds->panel);
> -	}
> -
>  	rcar_lvds_write(lvds, LVDCR0, 0);
>  	rcar_lvds_write(lvds, LVDCR1, 0);
>  	rcar_lvds_write(lvds, LVDPLLCR, 0);
> @@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge,
>  			    enum drm_bridge_attach_flags flags)
>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> -	struct drm_connector *connector = &lvds->connector;
> -	struct drm_encoder *encoder = bridge->encoder;
> -	int ret;
>
> -	/* If we have a next bridge just attach it. */
> -	if (lvds->next_bridge)
> -		return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
> -					 bridge, flags);
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
> -	/* Otherwise if we have a panel, create a connector. */
> -	if (!lvds->panel)
> -		return 0;
> -
> -	ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
> -				 DRM_MODE_CONNECTOR_LVDS);
> -	if (ret < 0)
> -		return ret;
> -
> -	drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
> -
> -	ret = drm_connector_attach_encoder(connector, encoder);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> -}
> -
> -static void rcar_lvds_detach(struct drm_bridge *bridge)
> -{
> +	return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
> +				 flags);
>  }
>
>  static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
>  	.attach = rcar_lvds_attach,
> -	.detach = rcar_lvds_detach,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
> @@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  		 * that we are expected to generate even pixels from the primary
>  		 * encoder, and odd pixels from the companion encoder.
>  		 */
> -		if (lvds->next_bridge && lvds->next_bridge->timings &&
> +		if (lvds->next_bridge->timings &&
>  		    lvds->next_bridge->timings->dual_link)
>  			lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
>  		else
> @@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  	if (ret)
>  		goto done;
>
> +	if (lvds->panel) {
> +		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
> +							      lvds->panel);

Reading the devm_drm_panel_bridge_add() function documentation:

 * devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector

Doesn't this conflict with the drm_bridge_connector_init() called by
the encoder in [4/4] ?


> +		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
> +			ret = -EINVAL;
> +			goto done;
> +		}
> +	}
> +
>  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
>  		ret = rcar_lvds_parse_dt_companion(lvds);
>
> --
> Regards,
>
> Laurent Pinchart
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Laurent Pinchart]

Hi Jacopo,

On Wed, Dec 16, 2020 at 02:16:56PM +0100, Jacopo Mondi wrote:
> On Wed, Dec 16, 2020 at 02:50:18AM +0200, Laurent Pinchart wrote:
> > Replace the manual panel handling with usage of the DRM panel bridge
> > helper. This simplifies the driver, and brings support for
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++-------------------------
> >  1 file changed, 12 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 70dbbe44bb23..1b360e06658c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -63,7 +63,6 @@ struct rcar_lvds {
> >  	struct drm_bridge bridge;
> >
> >  	struct drm_bridge *next_bridge;
> > -	struct drm_connector connector;
> >  	struct drm_panel *panel;
> >
> >  	void __iomem *mmio;
> > @@ -80,73 +79,11 @@ struct rcar_lvds {
> >  #define bridge_to_rcar_lvds(b) \
> >  	container_of(b, struct rcar_lvds, bridge)
> >
> > -#define connector_to_rcar_lvds(c) \
> > -	container_of(c, struct rcar_lvds, connector)
> > -
> >  static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
> >  {
> >  	iowrite32(data, lvds->mmio + reg);
> >  }
> >
> > -/* -----------------------------------------------------------------------------
> > - * Connector & Panel
> > - */
> > -
> > -static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
> > -{
> > -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> > -
> > -	return drm_panel_get_modes(lvds->panel, connector);
> > -}
> > -
> > -static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
> > -					    struct drm_atomic_state *state)
> > -{
> > -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> > -	const struct drm_display_mode *panel_mode;
> > -	struct drm_connector_state *conn_state;
> > -	struct drm_crtc_state *crtc_state;
> > -
> > -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> > -	if (!conn_state->crtc)
> > -		return 0;
> > -
> > -	if (list_empty(&connector->modes)) {
> > -		dev_dbg(lvds->dev, "connector: empty modes list\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	panel_mode = list_first_entry(&connector->modes,
> > -				      struct drm_display_mode, head);
> > -
> > -	/* We're not allowed to modify the resolution. */
> > -	crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> > -	if (IS_ERR(crtc_state))
> > -		return PTR_ERR(crtc_state);
> > -
> > -	if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
> > -	    crtc_state->mode.vdisplay != panel_mode->vdisplay)
> > -		return -EINVAL;
> > -
> > -	/* The flat panel mode is fixed, just copy it to the adjusted mode. */
> > -	drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
> > -
> > -	return 0;
> > -}
> > -
> > -static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
> > -	.get_modes = rcar_lvds_connector_get_modes,
> > -	.atomic_check = rcar_lvds_connector_atomic_check,
> > -};
> > -
> > -static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
> > -	.reset = drm_atomic_helper_connector_reset,
> > -	.fill_modes = drm_helper_probe_single_connector_modes,
> > -	.destroy = drm_connector_cleanup,
> > -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > -};
> > -
> >  /* -----------------------------------------------------------------------------
> >   * PLL Setup
> >   */
> > @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> >  	/* Turn the output on. */
> >  	lvdcr0 |= LVDCR0_LVRES;
> >  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> > -
> > -	if (lvds->panel) {
> > -		drm_panel_prepare(lvds->panel);
> > -		drm_panel_enable(lvds->panel);
> > -	}
> >  }
> >
> >  static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> > @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
> >  {
> >  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> >
> > -	if (lvds->panel) {
> > -		drm_panel_disable(lvds->panel);
> > -		drm_panel_unprepare(lvds->panel);
> > -	}
> > -
> >  	rcar_lvds_write(lvds, LVDCR0, 0);
> >  	rcar_lvds_write(lvds, LVDCR1, 0);
> >  	rcar_lvds_write(lvds, LVDPLLCR, 0);
> > @@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge,
> >  			    enum drm_bridge_attach_flags flags)
> >  {
> >  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > -	struct drm_connector *connector = &lvds->connector;
> > -	struct drm_encoder *encoder = bridge->encoder;
> > -	int ret;
> >
> > -	/* If we have a next bridge just attach it. */
> > -	if (lvds->next_bridge)
> > -		return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
> > -					 bridge, flags);
> > -
> > -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -		DRM_ERROR("Fix bridge driver to make connector optional!");
> > -		return -EINVAL;
> > -	}
> > -
> > -	/* Otherwise if we have a panel, create a connector. */
> > -	if (!lvds->panel)
> > -		return 0;
> > -
> > -	ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
> > -				 DRM_MODE_CONNECTOR_LVDS);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
> > -
> > -	ret = drm_connector_attach_encoder(connector, encoder);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	return 0;
> > -}
> > -
> > -static void rcar_lvds_detach(struct drm_bridge *bridge)
> > -{
> > +	return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
> > +				 flags);
> >  }
> >
> >  static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
> >  	.attach = rcar_lvds_attach,
> > -	.detach = rcar_lvds_detach,
> >  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >  	.atomic_reset = drm_atomic_helper_bridge_reset,
> > @@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  		 * that we are expected to generate even pixels from the primary
> >  		 * encoder, and odd pixels from the companion encoder.
> >  		 */
> > -		if (lvds->next_bridge && lvds->next_bridge->timings &&
> > +		if (lvds->next_bridge->timings &&
> >  		    lvds->next_bridge->timings->dual_link)
> >  			lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> >  		else
> > @@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  	if (ret)
> >  		goto done;
> >
> > +	if (lvds->panel) {
> > +		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
> > +							      lvds->panel);
> 
> Reading the devm_drm_panel_bridge_add() function documentation:
> 
>  * devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
> 
> Doesn't this conflict with the drm_bridge_connector_init() called by
> the encoder in [4/4] ?

It would, if the documentation was right :-) The function only creates a
bridge. A connector will only be created when the bridge is attached
without DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Would you like to send a patch to fix the documentation ?

> > +		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
> > +			ret = -EINVAL;
> > +			goto done;
> > +		}
> > +	}
> > +
> >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> >  		ret = rcar_lvds_parse_dt_companion(lvds);
> >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Laurent Pinchart]

Hi Jacopo,

On Wed, Dec 16, 2020 at 02:16:56PM +0100, Jacopo Mondi wrote:
> On Wed, Dec 16, 2020 at 02:50:18AM +0200, Laurent Pinchart wrote:
> > Replace the manual panel handling with usage of the DRM panel bridge
> > helper. This simplifies the driver, and brings support for
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR as an added bonus.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 120 +++-------------------------
> >  1 file changed, 12 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 70dbbe44bb23..1b360e06658c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -63,7 +63,6 @@ struct rcar_lvds {
> >  	struct drm_bridge bridge;
> >
> >  	struct drm_bridge *next_bridge;
> > -	struct drm_connector connector;
> >  	struct drm_panel *panel;
> >
> >  	void __iomem *mmio;
> > @@ -80,73 +79,11 @@ struct rcar_lvds {
> >  #define bridge_to_rcar_lvds(b) \
> >  	container_of(b, struct rcar_lvds, bridge)
> >
> > -#define connector_to_rcar_lvds(c) \
> > -	container_of(c, struct rcar_lvds, connector)
> > -
> >  static void rcar_lvds_write(struct rcar_lvds *lvds, u32 reg, u32 data)
> >  {
> >  	iowrite32(data, lvds->mmio + reg);
> >  }
> >
> > -/* -----------------------------------------------------------------------------
> > - * Connector & Panel
> > - */
> > -
> > -static int rcar_lvds_connector_get_modes(struct drm_connector *connector)
> > -{
> > -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> > -
> > -	return drm_panel_get_modes(lvds->panel, connector);
> > -}
> > -
> > -static int rcar_lvds_connector_atomic_check(struct drm_connector *connector,
> > -					    struct drm_atomic_state *state)
> > -{
> > -	struct rcar_lvds *lvds = connector_to_rcar_lvds(connector);
> > -	const struct drm_display_mode *panel_mode;
> > -	struct drm_connector_state *conn_state;
> > -	struct drm_crtc_state *crtc_state;
> > -
> > -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> > -	if (!conn_state->crtc)
> > -		return 0;
> > -
> > -	if (list_empty(&connector->modes)) {
> > -		dev_dbg(lvds->dev, "connector: empty modes list\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	panel_mode = list_first_entry(&connector->modes,
> > -				      struct drm_display_mode, head);
> > -
> > -	/* We're not allowed to modify the resolution. */
> > -	crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc);
> > -	if (IS_ERR(crtc_state))
> > -		return PTR_ERR(crtc_state);
> > -
> > -	if (crtc_state->mode.hdisplay != panel_mode->hdisplay ||
> > -	    crtc_state->mode.vdisplay != panel_mode->vdisplay)
> > -		return -EINVAL;
> > -
> > -	/* The flat panel mode is fixed, just copy it to the adjusted mode. */
> > -	drm_mode_copy(&crtc_state->adjusted_mode, panel_mode);
> > -
> > -	return 0;
> > -}
> > -
> > -static const struct drm_connector_helper_funcs rcar_lvds_conn_helper_funcs = {
> > -	.get_modes = rcar_lvds_connector_get_modes,
> > -	.atomic_check = rcar_lvds_connector_atomic_check,
> > -};
> > -
> > -static const struct drm_connector_funcs rcar_lvds_conn_funcs = {
> > -	.reset = drm_atomic_helper_connector_reset,
> > -	.fill_modes = drm_helper_probe_single_connector_modes,
> > -	.destroy = drm_connector_cleanup,
> > -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > -};
> > -
> >  /* -----------------------------------------------------------------------------
> >   * PLL Setup
> >   */
> > @@ -583,11 +520,6 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> >  	/* Turn the output on. */
> >  	lvdcr0 |= LVDCR0_LVRES;
> >  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> > -
> > -	if (lvds->panel) {
> > -		drm_panel_prepare(lvds->panel);
> > -		drm_panel_enable(lvds->panel);
> > -	}
> >  }
> >
> >  static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> > @@ -609,11 +541,6 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
> >  {
> >  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> >
> > -	if (lvds->panel) {
> > -		drm_panel_disable(lvds->panel);
> > -		drm_panel_unprepare(lvds->panel);
> > -	}
> > -
> >  	rcar_lvds_write(lvds, LVDCR0, 0);
> >  	rcar_lvds_write(lvds, LVDCR1, 0);
> >  	rcar_lvds_write(lvds, LVDPLLCR, 0);
> > @@ -648,45 +575,13 @@ static int rcar_lvds_attach(struct drm_bridge *bridge,
> >  			    enum drm_bridge_attach_flags flags)
> >  {
> >  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > -	struct drm_connector *connector = &lvds->connector;
> > -	struct drm_encoder *encoder = bridge->encoder;
> > -	int ret;
> >
> > -	/* If we have a next bridge just attach it. */
> > -	if (lvds->next_bridge)
> > -		return drm_bridge_attach(bridge->encoder, lvds->next_bridge,
> > -					 bridge, flags);
> > -
> > -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -		DRM_ERROR("Fix bridge driver to make connector optional!");
> > -		return -EINVAL;
> > -	}
> > -
> > -	/* Otherwise if we have a panel, create a connector. */
> > -	if (!lvds->panel)
> > -		return 0;
> > -
> > -	ret = drm_connector_init(bridge->dev, connector, &rcar_lvds_conn_funcs,
> > -				 DRM_MODE_CONNECTOR_LVDS);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	drm_connector_helper_add(connector, &rcar_lvds_conn_helper_funcs);
> > -
> > -	ret = drm_connector_attach_encoder(connector, encoder);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	return 0;
> > -}
> > -
> > -static void rcar_lvds_detach(struct drm_bridge *bridge)
> > -{
> > +	return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge,
> > +				 flags);
> >  }
> >
> >  static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
> >  	.attach = rcar_lvds_attach,
> > -	.detach = rcar_lvds_detach,
> >  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >  	.atomic_reset = drm_atomic_helper_bridge_reset,
> > @@ -759,7 +654,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  		 * that we are expected to generate even pixels from the primary
> >  		 * encoder, and odd pixels from the companion encoder.
> >  		 */
> > -		if (lvds->next_bridge && lvds->next_bridge->timings &&
> > +		if (lvds->next_bridge->timings &&
> >  		    lvds->next_bridge->timings->dual_link)
> >  			lvds->link_type = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS;
> >  		else
> > @@ -811,6 +706,15 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  	if (ret)
> >  		goto done;
> >
> > +	if (lvds->panel) {
> > +		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
> > +							      lvds->panel);
> 
> Reading the devm_drm_panel_bridge_add() function documentation:
> 
>  * devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
> 
> Doesn't this conflict with the drm_bridge_connector_init() called by
> the encoder in [4/4] ?

It would, if the documentation was right :-) The function only creates a
bridge. A connector will only be created when the bridge is attached
without DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Would you like to send a patch to fix the documentation ?

> > +		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
> > +			ret = -EINVAL;
> > +			goto done;
> > +		}
> > +	}
> > +
> >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> >  		ret = rcar_lvds_parse_dt_companion(lvds);
> >

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm: rcar-du: Use drm_bridge_connector_init() helper

[Laurent Pinchart]

Hi Jacopo,

On Wed, Dec 16, 2020 at 12:53:19PM +0100, Jacopo Mondi wrote:
> On Wed, Dec 16, 2020 at 02:50:21AM +0200, Laurent Pinchart wrote:
> > Use the drm_bridge_connector_init() helper to create a drm_connector for
> > each output, instead of relying on the bridge drivers doing so. Attach
> > the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct
> > them not to create a connector.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > index ba8c6038cd63..10a66091391a 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/slab.h>
> >
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_bridge_connector.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_managed.h>
> >  #include <drm/drm_modeset_helper_vtables.h>
> > @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  			 struct device_node *enc_node)
> >  {
> >  	struct rcar_du_encoder *renc;
> > +	struct drm_connector *connector;
> >  	struct drm_bridge *bridge;
> >  	int ret;
> >
> > @@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  	if (ret)
> >  		return ret;
> >
> > -	/*
> > -	 * Attach the bridge to the encoder. The bridge will create the
> > -	 * connector.
> > -	 */
> > -	return drm_bridge_attach(&renc->base, bridge, NULL, 0);
> > +	/* Attach the bridge to the encoder. */
> > +	ret = drm_bridge_attach(&renc->base, bridge, NULL,
> > +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +	if (ret) {
> > +		dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
> > +			output);
> > +		return ret;
> > +	}
> > +
> > +	/* Create the connector for the chain of bridges. */
> > +	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
> > +	if (IS_ERR(connector)) {
> > +		dev_err(rcdu->dev,
> > +			"failed to created connector for output %u\n", output);
> > +		return PTR_ERR(connector);
> > +	}
> > +
> > +	return drm_connector_attach_encoder(connector, &renc->base);
> 
> Looks good
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> I'm trying to figure out how deferred probe of a panel directly
> connected to the lvds encoder work. I assume there's no risk of
> creating the connector before the panel is probed, or this is not an
> issue.

If the panel isn't probed yet, the call to drm_of_find_panel_or_bridge()
in rcar_lvds_parse_dt() will defer probing of the LVDS encoder, which in
turn will defer probind of the DU as the LVDS bridge won't be
registered.

> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] drm: rcar-du: Use drm_bridge_connector_init() helper

[Laurent Pinchart]

Hi Jacopo,

On Wed, Dec 16, 2020 at 12:53:19PM +0100, Jacopo Mondi wrote:
> On Wed, Dec 16, 2020 at 02:50:21AM +0200, Laurent Pinchart wrote:
> > Use the drm_bridge_connector_init() helper to create a drm_connector for
> > each output, instead of relying on the bridge drivers doing so. Attach
> > the bridges with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to instruct
> > them not to create a connector.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 25 ++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > index ba8c6038cd63..10a66091391a 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/slab.h>
> >
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_bridge_connector.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_managed.h>
> >  #include <drm/drm_modeset_helper_vtables.h>
> > @@ -61,6 +62,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  			 struct device_node *enc_node)
> >  {
> >  	struct rcar_du_encoder *renc;
> > +	struct drm_connector *connector;
> >  	struct drm_bridge *bridge;
> >  	int ret;
> >
> > @@ -122,9 +124,22 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  	if (ret)
> >  		return ret;
> >
> > -	/*
> > -	 * Attach the bridge to the encoder. The bridge will create the
> > -	 * connector.
> > -	 */
> > -	return drm_bridge_attach(&renc->base, bridge, NULL, 0);
> > +	/* Attach the bridge to the encoder. */
> > +	ret = drm_bridge_attach(&renc->base, bridge, NULL,
> > +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +	if (ret) {
> > +		dev_err(rcdu->dev, "failed to attach bridge for output %u\n",
> > +			output);
> > +		return ret;
> > +	}
> > +
> > +	/* Create the connector for the chain of bridges. */
> > +	connector = drm_bridge_connector_init(&rcdu->ddev, &renc->base);
> > +	if (IS_ERR(connector)) {
> > +		dev_err(rcdu->dev,
> > +			"failed to created connector for output %u\n", output);
> > +		return PTR_ERR(connector);
> > +	}
> > +
> > +	return drm_connector_attach_encoder(connector, &renc->base);
> 
> Looks good
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> I'm trying to figure out how deferred probe of a panel directly
> connected to the lvds encoder work. I assume there's no risk of
> creating the connector before the panel is probed, or this is not an
> issue.

If the panel isn't probed yet, the call to drm_of_find_panel_or_bridge()
in rcar_lvds_parse_dt() will defer probing of the LVDS encoder, which in
turn will defer probind of the DU as the LVDS bridge won't be
registered.

> >  }

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 03:49:24PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> > > +	if (lvds->panel) {
> > > +		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
> > > +							      lvds->panel);
> >
> > Reading the devm_drm_panel_bridge_add() function documentation:
> >
> >  * devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
> >
> > Doesn't this conflict with the drm_bridge_connector_init() called by
> > the encoder in [4/4] ?
>
> It would, if the documentation was right :-) The function only creates a
> bridge. A connector will only be created when the bridge is attached
> without DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Well, reading it again, it is kind of implied that if NO_CONNECTOR is
given to the bridge, no connector will be registered at all.

>
> Would you like to send a patch to fix the documentation ?
>
> > > +		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
> > > +			ret = -EINVAL;
> > > +			goto done;
> > > +		}
> > > +	}
> > > +
> > >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > >  		ret = rcar_lvds_parse_dt_companion(lvds);
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/4] drm: rcar-du: lvds: Convert to DRM panel bridge helper

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 16, 2020 at 03:49:24PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> > > +	if (lvds->panel) {
> > > +		lvds->next_bridge = devm_drm_panel_bridge_add(lvds->dev,
> > > +							      lvds->panel);
> >
> > Reading the devm_drm_panel_bridge_add() function documentation:
> >
> >  * devm_drm_panel_bridge_add - Creates a managed &drm_bridge and &drm_connector
> >
> > Doesn't this conflict with the drm_bridge_connector_init() called by
> > the encoder in [4/4] ?
>
> It would, if the documentation was right :-) The function only creates a
> bridge. A connector will only be created when the bridge is attached
> without DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Well, reading it again, it is kind of implied that if NO_CONNECTOR is
given to the bridge, no connector will be registered at all.

>
> Would you like to send a patch to fix the documentation ?
>
> > > +		if (IS_ERR_OR_NULL(lvds->next_bridge)) {
> > > +			ret = -EINVAL;
> > > +			goto done;
> > > +		}
> > > +	}
> > > +
> > >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> > >  		ret = rcar_lvds_parse_dt_companion(lvds);
> > >
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel