[PATCH v4 0/15] drm/bridge: support chained bridges + panel updates

[PATCH v4 0/15] drm/bridge: support chained bridges + panel updates

[Sam Ravnborg]

The objective is that all bridge drivers shall support a chained setup
connector creation is moved to the display drivers.
This is just one step on this path.

The general approach for the bridge drivers:
- Introduce bridge operations
- Introduce use of panel bridge and make connector creation optional

v4:
  - Dropped patch for ti-sn65dsi86. Await full conversion.
  - Dropped patch for ti-tpd12s015. It was wrong (Laurent)
  - Drop boe,hv070wsa-100 patch, it was applied
  - Combined a few patches to fix connector created twice (Laurent)
  - Fix memory leak in get_edid in several drivers (Laurent)
  - Added patch to validate panel descriptions in panel-simple
  - Set bridge.type in relevant drivers
 
v3:
  - Rebase on top of drm-misc-next
  - Address kbuild test robot feedback
 
v2:
  - Updated bus_flags for boe,hv070wsa-100
  - Collected r-b, but did not apply patches yet
  - On the panel side the panel-simple driver gained a default
    connector type for all the dumb panels that do not
    include so in their description.
    With this change panels always provide a connector type,
    and we have the potential to drop most uses of
    devm_drm_panel_bridge_add_typed().
  - Added conversion of a few more bridge drivers

Patches can build but no run-time testing.
So both test and review feedback appreciated!

	Sam

Sam Ravnborg (15):
      drm/panel: panel-simple: validate panel description
      drm/panel: panel-simple: add default connector_type
      drm/bridge: tc358764: drop drm_connector_(un)register
      drm/bridge: tc358764: add drm_panel_bridge support
      drm/bridge: tc358767: add detect bridge operation
      drm/bridge: tc358767: add get_edid bridge operation
      drm/bridge: tc358767: add drm_panel_bridge support
      drm/bridge: parade-ps8622: add drm_panel_bridge support
      drm/bridge: megachips: add helper to create connector
      drm/bridge: megachips: get drm_device from bridge
      drm/bridge: megachips: enable detect bridge operation
      drm/bridge: megachips: add get_edid bridge operation
      drm/bridge: megachips: make connector creation optional
      drm/bridge: nxp-ptn3460: add get_edid bridge operation
      drm/bridge: nxp-ptn3460: add drm_panel_bridge support

 .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  97 +++++++++-------
 drivers/gpu/drm/bridge/nxp-ptn3460.c               | 103 ++++++++---------
 drivers/gpu/drm/bridge/parade-ps8622.c             | 100 +++-------------
 drivers/gpu/drm/bridge/tc358764.c                  | 110 +++---------------
 drivers/gpu/drm/bridge/tc358767.c                  | 126 +++++++++++----------
 drivers/gpu/drm/panel/panel-simple.c               |  48 +++++++-
 6 files changed, 242 insertions(+), 342 deletions(-)


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

[PATCH v4 01/15] drm/panel: panel-simple: validate panel description

[Sam Ravnborg]

Warn if we detect a panel with incomplete/wrong description.
This is inspired by a similar patch by Laurent that introduced checks
for LVDS panels - this extends the checks to the remaining type of
connectors.

This is known to warn for some of the existing panels but added
despite this as we need help from people using the panels to
add the missing info.
The checks are not complete but will catch the most common mistakes.

The checks at the same time serves as documentation for the minimum
required description for a panel.

The checks uses dev_warn() as we know this will hit. WARN() was
too noisy at the moment for anything else than LVDS.

v2:
  - Use dev_warn (Laurent)
  - Check for empty bus_flags

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 41 ++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 54323515ca2c..a8d68102931e 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	struct panel_simple *panel;
 	struct display_timing dt;
 	struct device_node *ddc;
+	u32 bus_flags;
 	int err;
 
 	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
@@ -549,8 +550,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 			panel_simple_parse_panel_timing_node(dev, panel, &dt);
 	}
 
-	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
-		/* Catch common mistakes for LVDS panels. */
+	/* Catch common mistakes for panels. */
+	switch (desc->connector_type) {
+	case 0:
+		dev_warn(dev, "Specify missing connector_type\n");
+		break;
+	case DRM_MODE_CONNECTOR_LVDS:
 		WARN_ON(desc->bus_flags &
 			~(DRM_BUS_FLAG_DE_LOW |
 			  DRM_BUS_FLAG_DE_HIGH |
@@ -564,6 +569,38 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
 			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
 			desc->bpc != 8);
+		break;
+	case DRM_MODE_CONNECTOR_eDP:
+		if (desc->bus_format == 0)
+			dev_warn(dev, "Specify missing bus_format\n");
+		if (desc->bpc != 6 && desc->bpc != 8)
+			dev_warn(dev, "Expected bpc in {6,8} but got: %d\n", desc->bpc);
+		break;
+	case DRM_MODE_CONNECTOR_DSI:
+		if (desc->bpc != 6 && desc->bpc != 8)
+			dev_warn(dev, "Expected bpc in {6,8} but got: %d\n", desc->bpc);
+		break;
+	case DRM_MODE_CONNECTOR_DPI:
+		bus_flags = DRM_BUS_FLAG_DE_LOW |
+			    DRM_BUS_FLAG_DE_HIGH |
+			    DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
+			    DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
+			    DRM_BUS_FLAG_DATA_MSB_TO_LSB |
+			    DRM_BUS_FLAG_DATA_LSB_TO_MSB |
+			    DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
+			    DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE;
+		if (desc->bus_flags & ~bus_flags)
+			dev_warn(dev, "Unexpected bus_flags(%d)\n", desc->bus_flags & ~bus_flags);
+		if (!(desc->bus_flags & bus_flags))
+			dev_warn(dev, "Specify missing bus_flags\n");
+		if (desc->bus_format == 0)
+			dev_warn(dev, "Specify missing bus_format\n");
+		if (desc->bpc != 6 && desc->bpc != 8)
+			dev_warn(dev, "Expected bpc in {6,8} but got: %d\n", desc->bpc);
+		break;
+	default:
+		dev_warn(dev, "Specify a valid connector_type: %d\n", desc->connector_type);
+		break;
 	}
 
 	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
-- 
2.25.1

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

[PATCH v4 02/15] drm/panel: panel-simple: add default connector_type

[Sam Ravnborg]

All panels shall report a connector type.
panel-simple has a lot of panels with no connector_type,
and for these fall back to DPI as the default.

v2:
  - Rebased on top of validation of panel description

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index a8d68102931e..56ab073e4e6e 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	struct panel_simple *panel;
 	struct display_timing dt;
 	struct device_node *ddc;
+	int connector_type;
 	u32 bus_flags;
 	int err;
 
@@ -550,10 +551,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 			panel_simple_parse_panel_timing_node(dev, panel, &dt);
 	}
 
+	connector_type = desc->connector_type;
 	/* Catch common mistakes for panels. */
-	switch (desc->connector_type) {
+	switch (connector_type) {
 	case 0:
 		dev_warn(dev, "Specify missing connector_type\n");
+		connector_type = DRM_MODE_CONNECTOR_DPI;
 		break;
 	case DRM_MODE_CONNECTOR_LVDS:
 		WARN_ON(desc->bus_flags &
@@ -600,11 +603,11 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 		break;
 	default:
 		dev_warn(dev, "Specify a valid connector_type: %d\n", desc->connector_type);
+		connector_type = DRM_MODE_CONNECTOR_DPI;
 		break;
 	}
 
-	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
-		       desc->connector_type);
+	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
 
 	err = drm_panel_of_backlight(&panel->base);
 	if (err)
-- 
2.25.1

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

[PATCH v4 03/15] drm/bridge: tc358764: drop drm_connector_(un)register

[Sam Ravnborg]

Drop drm_connector handling that is not needed:

- drm_dev_register() in the display controller driver takes
  care of registering connectors.
  So the call to drm_connector_register() call is not needed in the bridge
  driver.

- Use of drm_connector_unregister() is only required for drivers that
  explicit have called drm_dev_register.

- The reference counting using drm_connector_put() is likewise not needed.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/tc358764.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index 5ac1430fab04..a277739fab58 100644
--- a/drivers/gpu/drm/bridge/tc358764.c
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -375,7 +375,6 @@ static int tc358764_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
 	drm_panel_attach(ctx->panel, &ctx->connector);
 	ctx->connector.funcs->reset(&ctx->connector);
-	drm_connector_register(&ctx->connector);
 
 	return 0;
 }
@@ -384,10 +383,8 @@ static void tc358764_detach(struct drm_bridge *bridge)
 {
 	struct tc358764 *ctx = bridge_to_tc358764(bridge);
 
-	drm_connector_unregister(&ctx->connector);
 	drm_panel_detach(ctx->panel);
 	ctx->panel = NULL;
-	drm_connector_put(&ctx->connector);
 }
 
 static const struct drm_bridge_funcs tc358764_bridge_funcs = {
-- 
2.25.1

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

[PATCH v4 04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the tc358764 bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

The bridge panel will use the connector type reported by the panel,
where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.

The tc358764 did not any additional info the the connector so the
connector creation is passed to the bridge panel driver.

v3:
  - Merge with patch to make connector creation optional to avoid
    creating two connectors (Laurent)
  - Pass connector creation to bridge panel, as this bridge driver
    did not add any extra info to the connector.
  - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.

v2:
  - Use PTR_ERR_OR_ZERO() (kbuild test robot)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: kbuild test robot <lkp@intel.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/tc358764.c | 107 +++++-------------------------
 1 file changed, 16 insertions(+), 91 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index a277739fab58..fdde4cfdc724 100644
--- a/drivers/gpu/drm/bridge/tc358764.c
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = {
 struct tc358764 {
 	struct device *dev;
 	struct drm_bridge bridge;
-	struct drm_connector connector;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
 	struct gpio_desc *gpio_reset;
-	struct drm_panel *panel;
+	struct drm_bridge *panel_bridge;
 	int error;
 };
 
@@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
 	return container_of(bridge, struct tc358764, bridge);
 }
 
-static inline
-struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
-{
-	return container_of(connector, struct tc358764, connector);
-}
-
 static int tc358764_init(struct tc358764 *ctx)
 {
 	u32 v = 0;
@@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx)
 	usleep_range(1000, 2000);
 }
 
-static int tc358764_get_modes(struct drm_connector *connector)
-{
-	struct tc358764 *ctx = connector_to_tc358764(connector);
-
-	return drm_panel_get_modes(ctx->panel, connector);
-}
-
-static const
-struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
-	.get_modes = tc358764_get_modes,
-};
-
-static const struct drm_connector_funcs tc358764_connector_funcs = {
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static void tc358764_disable(struct drm_bridge *bridge)
-{
-	struct tc358764 *ctx = bridge_to_tc358764(bridge);
-	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
-
-	if (ret < 0)
-		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
-}
-
 static void tc358764_post_disable(struct drm_bridge *bridge)
 {
 	struct tc358764 *ctx = bridge_to_tc358764(bridge);
 	int ret;
 
-	ret = drm_panel_unprepare(ctx->panel);
-	if (ret < 0)
-		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
 	tc358764_reset(ctx);
 	usleep_range(10000, 15000);
 	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
@@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
 	ret = tc358764_init(ctx);
 	if (ret < 0)
 		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
-	ret = drm_panel_prepare(ctx->panel);
-	if (ret < 0)
-		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
-}
-
-static void tc358764_enable(struct drm_bridge *bridge)
-{
-	struct tc358764 *ctx = bridge_to_tc358764(bridge);
-	int ret = drm_panel_enable(ctx->panel);
-
-	if (ret < 0)
-		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
 }
 
 static int tc358764_attach(struct drm_bridge *bridge,
 			   enum drm_bridge_attach_flags flags)
-{
-	struct tc358764 *ctx = bridge_to_tc358764(bridge);
-	struct drm_device *drm = bridge->dev;
-	int ret;
-
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
-	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
-	ret = drm_connector_init(drm, &ctx->connector,
-				 &tc358764_connector_funcs,
-				 DRM_MODE_CONNECTOR_LVDS);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector\n");
-		return ret;
-	}
-
-	drm_connector_helper_add(&ctx->connector,
-				 &tc358764_connector_helper_funcs);
-	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
-	drm_panel_attach(ctx->panel, &ctx->connector);
-	ctx->connector.funcs->reset(&ctx->connector);
-
-	return 0;
-}
-
-static void tc358764_detach(struct drm_bridge *bridge)
 {
 	struct tc358764 *ctx = bridge_to_tc358764(bridge);
 
-	drm_panel_detach(ctx->panel);
-	ctx->panel = NULL;
+	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
+				 bridge, flags);
 }
 
 static const struct drm_bridge_funcs tc358764_bridge_funcs = {
-	.disable = tc358764_disable,
 	.post_disable = tc358764_post_disable,
-	.enable = tc358764_enable,
 	.pre_enable = tc358764_pre_enable,
 	.attach = tc358764_attach,
-	.detach = tc358764_detach,
 };
 
 static int tc358764_parse_dt(struct tc358764 *ctx)
 {
+	struct drm_bridge *panel_bridge;
 	struct device *dev = ctx->dev;
+	struct drm_panel *panel;
 	int ret;
 
 	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
@@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
 		return PTR_ERR(ctx->gpio_reset);
 	}
 
-	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
-					  NULL);
-	if (ret && ret != -EPROBE_DEFER)
-		dev_err(dev, "cannot find panel (%d)\n", ret);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
+	if (ret)
+		return ret;
 
-	return ret;
+	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+
+	if (IS_ERR(panel_bridge))
+		return PTR_ERR(panel_bridge);
+
+	ctx->panel_bridge = panel_bridge;
+	return 0;
 }
 
 static int tc358764_configure_regulators(struct tc358764 *ctx)
@@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
 		return ret;
 
 	ctx->bridge.funcs = &tc358764_bridge_funcs;
+	ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;
 	ctx->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&ctx->bridge);
-- 
2.25.1

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

[PATCH v4 05/15] drm/bridge: tc358767: add detect bridge operation

[Sam Ravnborg]

Prepare the bridge driver for chained operation by adding
support for the detect operation.

v2:
  - Do not announce detect operation if there is no hpd pin (Laurent)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/tc358767.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index c2777b226c75..e8ba713bedac 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1369,21 +1369,13 @@ static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
 };
 
-static enum drm_connector_status tc_connector_detect(struct drm_connector *connector,
-						     bool force)
+static enum drm_connector_status tc_bridge_detect(struct drm_bridge *bridge)
 {
-	struct tc_data *tc = connector_to_tc(connector);
+	struct tc_data *tc = bridge_to_tc(bridge);
 	bool conn;
 	u32 val;
 	int ret;
 
-	if (tc->hpd_pin < 0) {
-		if (tc->panel)
-			return connector_status_connected;
-		else
-			return connector_status_unknown;
-	}
-
 	ret = regmap_read(tc->regmap, GPIOI, &val);
 	if (ret)
 		return connector_status_unknown;
@@ -1396,6 +1388,20 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
 		return connector_status_disconnected;
 }
 
+static enum drm_connector_status
+tc_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tc_data *tc = connector_to_tc(connector);
+
+	if (tc->hpd_pin >= 0)
+		return tc_bridge_detect(&tc->bridge);
+	else
+		if (tc->panel)
+			return connector_status_connected;
+
+	return connector_status_unknown;
+}
+
 static const struct drm_connector_funcs tc_connector_funcs = {
 	.detect = tc_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -1458,6 +1464,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.disable = tc_bridge_disable,
 	.post_disable = tc_bridge_post_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
+	.detect = tc_bridge_detect,
 };
 
 static bool tc_readable_reg(struct device *dev, unsigned int reg)
@@ -1680,6 +1687,9 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 
 	tc->bridge.funcs = &tc_bridge_funcs;
+	if (tc->hpd_pin >= 0)
+		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
+
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
-- 
2.25.1

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

[PATCH v4 06/15] drm/bridge: tc358767: add get_edid bridge operation

[Sam Ravnborg]

Prepare for chained bridge with the addition of
get_edid support.

v2:
  - Fixed handling of edid storage (Laurent)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/tc358767.c | 34 ++++++++++++++++---------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index e8ba713bedac..d86d7f06bebb 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -250,8 +250,6 @@ struct tc_data {
 	/* link settings */
 	struct tc_edp_link	link;
 
-	/* display edid */
-	struct edid		*edid;
 	/* current mode */
 	struct drm_display_mode	mode;
 
@@ -1335,11 +1333,19 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
 	tc->mode = *mode;
 }
 
+static struct edid *tc_get_edid(struct drm_bridge *bridge,
+				struct drm_connector *connector)
+{
+	struct tc_data *tc = bridge_to_tc(bridge);
+
+	return drm_get_edid(connector, &tc->aux.ddc);
+}
+
 static int tc_connector_get_modes(struct drm_connector *connector)
 {
 	struct tc_data *tc = connector_to_tc(connector);
+	int num_modes;
 	struct edid *edid;
-	int count;
 	int ret;
 
 	ret = tc_get_display_props(tc);
@@ -1348,21 +1354,15 @@ static int tc_connector_get_modes(struct drm_connector *connector)
 		return 0;
 	}
 
-	count = drm_panel_get_modes(tc->panel, connector);
-	if (count > 0)
-		return count;
-
-	edid = drm_get_edid(connector, &tc->aux.ddc);
-
-	kfree(tc->edid);
-	tc->edid = edid;
-	if (!edid)
-		return 0;
+	num_modes = drm_panel_get_modes(tc->panel, connector);
+	if (num_modes > 0)
+		return num_modes;
 
-	drm_connector_update_edid_property(connector, edid);
-	count = drm_add_edid_modes(connector, edid);
+	edid = tc_get_edid(&tc->bridge, connector);
+	num_modes = drm_add_edid_modes(connector, edid);
+	kfree(edid);
 
-	return count;
+	return num_modes;
 }
 
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
@@ -1465,6 +1465,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.post_disable = tc_bridge_post_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
 	.detect = tc_bridge_detect,
+	.get_edid = tc_get_edid,
 };
 
 static bool tc_readable_reg(struct device *dev, unsigned int reg)
@@ -1689,6 +1690,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	tc->bridge.funcs = &tc_bridge_funcs;
 	if (tc->hpd_pin >= 0)
 		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
+	tc->bridge.ops |= DRM_BRIDGE_OP_EDID;
 
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
-- 
2.25.1

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

[PATCH v4 07/15] drm/bridge: tc358767: add drm_panel_bridge support

[Sam Ravnborg]

With the bridge operations implemented the last step to prepare
this driver for a chained setup is the use of the bridge panel driver.

The bridge panel driver is only used when a prot@2 is present in
the DT. So when the display driver request a connector
support both situations:
- connector created by bridge panel driver
- connector created by this driver

And on top, support that the display driver creates the connector,
which is the preferred setup.

Note: the bridge panel will use the connector type from the panel.

v2:
  - Merge connector and drm_panel_bridge patches
    and fix so we do not create two connectors (Laurent)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/tc358767.c | 70 +++++++++++++++----------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index d86d7f06bebb..75a2cd792ccc 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -244,8 +244,8 @@ struct tc_data {
 	struct drm_dp_aux	aux;
 
 	struct drm_bridge	bridge;
+	struct drm_bridge	*panel_bridge;
 	struct drm_connector	connector;
-	struct drm_panel	*panel;
 
 	/* link settings */
 	struct tc_edp_link	link;
@@ -1234,13 +1234,6 @@ static int tc_stream_disable(struct tc_data *tc)
 	return 0;
 }
 
-static void tc_bridge_pre_enable(struct drm_bridge *bridge)
-{
-	struct tc_data *tc = bridge_to_tc(bridge);
-
-	drm_panel_prepare(tc->panel);
-}
-
 static void tc_bridge_enable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
@@ -1264,8 +1257,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 		tc_main_link_disable(tc);
 		return;
 	}
-
-	drm_panel_enable(tc->panel);
 }
 
 static void tc_bridge_disable(struct drm_bridge *bridge)
@@ -1273,8 +1264,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
-	drm_panel_disable(tc->panel);
-
 	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
@@ -1284,13 +1273,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 		dev_err(tc->dev, "main link disable error: %d\n", ret);
 }
 
-static void tc_bridge_post_disable(struct drm_bridge *bridge)
-{
-	struct tc_data *tc = bridge_to_tc(bridge);
-
-	drm_panel_unprepare(tc->panel);
-}
-
 static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
 				 const struct drm_display_mode *mode,
 				 struct drm_display_mode *adj)
@@ -1354,9 +1336,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
 		return 0;
 	}
 
-	num_modes = drm_panel_get_modes(tc->panel, connector);
-	if (num_modes > 0)
-		return num_modes;
+	if (tc->panel_bridge) {
+		num_modes = drm_bridge_get_modes(tc->panel_bridge, connector);
+		if (num_modes > 0)
+			return num_modes;
+	}
 
 	edid = tc_get_edid(&tc->bridge, connector);
 	num_modes = drm_add_edid_modes(connector, edid);
@@ -1396,7 +1380,7 @@ tc_connector_detect(struct drm_connector *connector, bool force)
 	if (tc->hpd_pin >= 0)
 		return tc_bridge_detect(&tc->bridge);
 	else
-		if (tc->panel)
+		if (tc->panel_bridge)
 			return connector_status_connected;
 
 	return connector_status_unknown;
@@ -1419,16 +1403,23 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
+	if (tc->panel_bridge) {
+		enum drm_bridge_attach_flags panel_flags;
+
+		/* If a connector is required then this driver shall create it */
+		panel_flags = flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
+					&tc->bridge, panel_flags);
+		if (ret)
+			return ret;
 	}
 
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
+
 	/* Create DP/eDP connector */
 	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
-	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
-				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
-				 DRM_MODE_CONNECTOR_DisplayPort);
+	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, tc->bridge.type);
 	if (ret)
 		return ret;
 
@@ -1441,9 +1432,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 					       DRM_CONNECTOR_POLL_DISCONNECT;
 	}
 
-	if (tc->panel)
-		drm_panel_attach(tc->panel, &tc->connector);
-
 	drm_display_info_set_bus_formats(&tc->connector.display_info,
 					 &bus_format, 1);
 	tc->connector.display_info.bus_flags =
@@ -1459,10 +1447,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.attach = tc_bridge_attach,
 	.mode_valid = tc_mode_valid,
 	.mode_set = tc_bridge_mode_set,
-	.pre_enable = tc_bridge_pre_enable,
 	.enable = tc_bridge_enable,
 	.disable = tc_bridge_disable,
-	.post_disable = tc_bridge_post_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
 	.detect = tc_bridge_detect,
 	.get_edid = tc_get_edid,
@@ -1555,6 +1541,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
+	struct drm_panel *panel;
 	struct tc_data *tc;
 	int ret;
 
@@ -1565,10 +1552,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	tc->dev = dev;
 
 	/* port@2 is the output port */
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
 	if (ret && ret != -ENODEV)
 		return ret;
 
+	if (panel) {
+		struct drm_bridge *panel_bridge;
+
+		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+		if (IS_ERR(panel_bridge))
+			return PTR_ERR(panel_bridge);
+
+		tc->panel_bridge = panel_bridge;
+		tc->bridge.type = DRM_MODE_CONNECTOR_eDP;
+	} else {
+		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+	}
+
 	/* Shut down GPIO is optional */
 	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
 	if (IS_ERR(tc->sd_gpio))
-- 
2.25.1

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

[PATCH v4 08/15] drm/bridge: parade-ps8622: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup by
replacing direct use of drm_panel with drm_panel_bridge support.

The connecter is now either created by the panel bridge or the display
driver. So all code for connector creation in this driver is no longer
relevant and thus dropped.

The connector code had some special polling handling:
    connector.polled = DRM_CONNECTOR_POLL_HPD;
    drm_helper_hpd_irq_event(ps8622->bridge.dev);

This code was most likely added to speed up detection of the connector.
If really needed then this functionality belongs somewhere else.

Note: the bridge panel will use the connector type from the panel.

v2:
  - Fix to avoid creating connector twice (Laurent)
  - Drop all connector code - defer to bridge panel
  - Use panel_bridge for local variable to align with other drivers
  - Set bridge.type to DRM_MODE_CONNECTOR_LVDS;

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/parade-ps8622.c | 100 ++++---------------------
 1 file changed, 13 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index d789ea2a7fb9..614b19f0f1b7 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -42,10 +42,9 @@
 #endif
 
 struct ps8622_bridge {
-	struct drm_connector connector;
 	struct i2c_client *client;
 	struct drm_bridge bridge;
-	struct drm_panel *panel;
+	struct drm_bridge *panel_bridge;
 	struct regulator *v12;
 	struct backlight_device *bl;
 
@@ -64,12 +63,6 @@ static inline struct ps8622_bridge *
 	return container_of(bridge, struct ps8622_bridge, bridge);
 }
 
-static inline struct ps8622_bridge *
-		connector_to_ps8622(struct drm_connector *connector)
-{
-	return container_of(connector, struct ps8622_bridge, connector);
-}
-
 static int ps8622_set(struct i2c_client *client, u8 page, u8 reg, u8 val)
 {
 	int ret;
@@ -365,11 +358,6 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
 			DRM_ERROR("fails to enable ps8622->v12");
 	}
 
-	if (drm_panel_prepare(ps8622->panel)) {
-		DRM_ERROR("failed to prepare panel\n");
-		return;
-	}
-
 	gpiod_set_value(ps8622->gpio_slp, 1);
 
 	/*
@@ -399,24 +387,9 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
 	ps8622->enabled = true;
 }
 
-static void ps8622_enable(struct drm_bridge *bridge)
-{
-	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
-
-	if (drm_panel_enable(ps8622->panel)) {
-		DRM_ERROR("failed to enable panel\n");
-		return;
-	}
-}
-
 static void ps8622_disable(struct drm_bridge *bridge)
 {
-	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
-
-	if (drm_panel_disable(ps8622->panel)) {
-		DRM_ERROR("failed to disable panel\n");
-		return;
-	}
+	/* Delay after panel is disabled */
 	msleep(PS8622_PWMO_END_T12_MS);
 }
 
@@ -436,11 +409,6 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
 	 */
 	gpiod_set_value(ps8622->gpio_slp, 0);
 
-	if (drm_panel_unprepare(ps8622->panel)) {
-		DRM_ERROR("failed to unprepare panel\n");
-		return;
-	}
-
 	if (ps8622->v12)
 		regulator_disable(ps8622->v12);
 
@@ -455,67 +423,17 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
 	msleep(PS8622_POWER_OFF_T17_MS);
 }
 
-static int ps8622_get_modes(struct drm_connector *connector)
-{
-	struct ps8622_bridge *ps8622;
-
-	ps8622 = connector_to_ps8622(connector);
-
-	return drm_panel_get_modes(ps8622->panel, connector);
-}
-
-static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = {
-	.get_modes = ps8622_get_modes,
-};
-
-static const struct drm_connector_funcs ps8622_connector_funcs = {
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
 static int ps8622_attach(struct drm_bridge *bridge,
 			 enum drm_bridge_attach_flags flags)
 {
 	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
-	int ret;
-
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
 
-	if (!bridge->encoder) {
-		DRM_ERROR("Parent encoder object not found");
-		return -ENODEV;
-	}
-
-	ps8622->connector.polled = DRM_CONNECTOR_POLL_HPD;
-	ret = drm_connector_init(bridge->dev, &ps8622->connector,
-			&ps8622_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
-		return ret;
-	}
-	drm_connector_helper_add(&ps8622->connector,
-					&ps8622_connector_helper_funcs);
-	drm_connector_register(&ps8622->connector);
-	drm_connector_attach_encoder(&ps8622->connector,
-							bridge->encoder);
-
-	if (ps8622->panel)
-		drm_panel_attach(ps8622->panel, &ps8622->connector);
-
-	drm_helper_hpd_irq_event(ps8622->connector.dev);
-
-	return ret;
+	return drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
+				 &ps8622->bridge, flags);
 }
 
 static const struct drm_bridge_funcs ps8622_bridge_funcs = {
 	.pre_enable = ps8622_pre_enable,
-	.enable = ps8622_enable,
 	.disable = ps8622_disable,
 	.post_disable = ps8622_post_disable,
 	.attach = ps8622_attach,
@@ -533,16 +451,23 @@ static int ps8622_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct ps8622_bridge *ps8622;
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
 	int ret;
 
 	ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL);
 	if (!ps8622)
 		return -ENOMEM;
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ps8622->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
 	if (ret)
 		return ret;
 
+	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+	if (IS_ERR(panel_bridge))
+		return PTR_ERR(panel_bridge);
+
+	ps8622->panel_bridge = panel_bridge;
 	ps8622->client = client;
 
 	ps8622->v12 = devm_regulator_get(dev, "vdd12");
@@ -595,6 +520,7 @@ static int ps8622_probe(struct i2c_client *client,
 	}
 
 	ps8622->bridge.funcs = &ps8622_bridge_funcs;
+	ps8622->bridge.type = DRM_MODE_CONNECTOR_LVDS;
 	ps8622->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ps8622->bridge);
 
-- 
2.25.1

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

[PATCH v4 09/15] drm/bridge: megachips: add helper to create connector

[Sam Ravnborg]

Factor out connector creation to a small helper function.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 47 +++++++++++--------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 6200f12a37e6..258e0525cdcc 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -191,6 +191,32 @@ static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static int ge_b850v3_lvds_create_connector(struct drm_bridge *bridge)
+{
+	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+	drm_connector_helper_add(connector,
+				 &ge_b850v3_lvds_connector_helper_funcs);
+
+	ret = drm_connector_init(bridge->dev, connector,
+				 &ge_b850v3_lvds_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	return drm_connector_attach_encoder(connector, bridge->encoder);
+}
+
 static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
 {
 	struct i2c_client *stdp4028_i2c
@@ -209,7 +235,6 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
 static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
-	struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
 	struct i2c_client *stdp4028_i2c
 			= ge_b850v3_lvds_ptr->stdp4028_i2c;
 	int ret;
@@ -219,25 +244,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
-	if (!bridge->encoder) {
-		DRM_ERROR("Parent encoder object not found");
-		return -ENODEV;
-	}
-
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
-
-	drm_connector_helper_add(connector,
-				 &ge_b850v3_lvds_connector_helper_funcs);
-
-	ret = drm_connector_init(bridge->dev, connector,
-				 &ge_b850v3_lvds_connector_funcs,
-				 DRM_MODE_CONNECTOR_DisplayPort);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
-		return ret;
-	}
-
-	ret = drm_connector_attach_encoder(connector, bridge->encoder);
+	ret = ge_b850v3_lvds_create_connector(bridge);
 	if (ret)
 		return ret;
 
-- 
2.25.1

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

[PATCH v4 10/15] drm/bridge: megachips: get drm_device from bridge

[Sam Ravnborg]

Fix so drm_device is read from the bridge.
This is a preparation for the connector being optional.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 258e0525cdcc..cf1dfbc88acf 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -226,8 +226,8 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id)
 				  STDP4028_DPTX_IRQ_STS_REG,
 				  STDP4028_DPTX_IRQ_CLEAR);
 
-	if (ge_b850v3_lvds_ptr->connector.dev)
-		drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->connector.dev);
+	if (ge_b850v3_lvds_ptr->bridge.dev)
+		drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->bridge.dev);
 
 	return IRQ_HANDLED;
 }
-- 
2.25.1

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

[PATCH v4 11/15] drm/bridge: megachips: enable detect bridge operation

[Sam Ravnborg]

To prepare for use in a chained bridge setup enable the
detect operation.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index cf1dfbc88acf..450dca33ea48 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -163,8 +163,7 @@ drm_connector_helper_funcs ge_b850v3_lvds_connector_helper_funcs = {
 	.mode_valid = ge_b850v3_lvds_mode_valid,
 };
 
-static enum drm_connector_status ge_b850v3_lvds_detect(
-		struct drm_connector *connector, bool force)
+static enum drm_connector_status ge_b850v3_lvds_bridge_detect(struct drm_bridge *bridge)
 {
 	struct i2c_client *stdp4028_i2c =
 			ge_b850v3_lvds_ptr->stdp4028_i2c;
@@ -182,6 +181,12 @@ static enum drm_connector_status ge_b850v3_lvds_detect(
 	return connector_status_unknown;
 }
 
+static enum drm_connector_status ge_b850v3_lvds_detect(struct drm_connector *connector,
+						       bool force)
+{
+	return ge_b850v3_lvds_bridge_detect(&ge_b850v3_lvds_ptr->bridge);
+}
+
 static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = ge_b850v3_lvds_detect,
@@ -263,6 +268,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
 	.attach = ge_b850v3_lvds_attach,
+	.detect = ge_b850v3_lvds_bridge_detect,
 };
 
 static int ge_b850v3_lvds_init(struct device *dev)
@@ -317,6 +323,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 
 	/* drm bridge initialization */
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
+	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
 	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
-- 
2.25.1

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

[PATCH v4 12/15] drm/bridge: megachips: add get_edid bridge operation

[Sam Ravnborg]

To prepare for a chained bridge setup add support for the
get_edid bridge operation.
There is no need for a copy of the edid - so drop
the pointer to the copy.

v2:
  - Fix so we do not leak memory (Laurent)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 31 ++++++++++---------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 450dca33ea48..f7b55dc374ac 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -61,7 +61,6 @@ struct ge_b850v3_lvds {
 	struct drm_bridge bridge;
 	struct i2c_client *stdp4028_i2c;
 	struct i2c_client *stdp2690_i2c;
-	struct edid *edid;
 };
 
 static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
@@ -131,22 +130,26 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
 	return NULL;
 }
 
-static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
+static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
+					    struct drm_connector *connector)
 {
 	struct i2c_client *client;
-	int num_modes = 0;
 
 	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
 
-	kfree(ge_b850v3_lvds_ptr->edid);
-	ge_b850v3_lvds_ptr->edid = (struct edid *)stdp2690_get_edid(client);
+	return (struct edid *)stdp2690_get_edid(client);
+}
 
-	if (ge_b850v3_lvds_ptr->edid) {
-		drm_connector_update_edid_property(connector,
-						      ge_b850v3_lvds_ptr->edid);
-		num_modes = drm_add_edid_modes(connector,
-					       ge_b850v3_lvds_ptr->edid);
-	}
+static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
+{
+	struct edid *edid;
+	int num_modes;
+
+	edid = ge_b850v3_lvds_get_edid(&ge_b850v3_lvds_ptr->bridge, connector);
+
+	drm_connector_update_edid_property(connector, edid);
+	num_modes = drm_add_edid_modes(connector, edid);
+	kfree(edid);
 
 	return num_modes;
 }
@@ -269,6 +272,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
 	.attach = ge_b850v3_lvds_attach,
 	.detect = ge_b850v3_lvds_bridge_detect,
+	.get_edid = ge_b850v3_lvds_get_edid,
 };
 
 static int ge_b850v3_lvds_init(struct device *dev)
@@ -304,8 +308,6 @@ static void ge_b850v3_lvds_remove(void)
 
 	drm_bridge_remove(&ge_b850v3_lvds_ptr->bridge);
 
-	kfree(ge_b850v3_lvds_ptr->edid);
-
 	ge_b850v3_lvds_ptr = NULL;
 out:
 	mutex_unlock(&ge_b850v3_lvds_dev_mutex);
@@ -323,7 +325,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 
 	/* drm bridge initialization */
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
-	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
+	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
+					 DRM_BRIDGE_OP_EDID;
 	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
-- 
2.25.1

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

[PATCH v4 13/15] drm/bridge: megachips: make connector creation optional

[Sam Ravnborg]

Make the connector creation optional to enable usage of the
megachips-stdpxxxx-ge-b850v3-fw bridge with the DRM bridge connector
helper.

v2:
  - Set bridge.type to DRM_MODE_CONNECTOR_DisplayPort

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index f7b55dc374ac..61a24f265c7a 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -245,16 +245,6 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 {
 	struct i2c_client *stdp4028_i2c
 			= ge_b850v3_lvds_ptr->stdp4028_i2c;
-	int ret;
-
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
-
-	ret = ge_b850v3_lvds_create_connector(bridge);
-	if (ret)
-		return ret;
 
 	/* Configures the bridge to re-enable interrupts after each ack. */
 	i2c_smbus_write_word_data(stdp4028_i2c,
@@ -266,7 +256,10 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
 				  STDP4028_DPTX_IRQ_EN_REG,
 				  STDP4028_DPTX_IRQ_CONFIG);
 
-	return 0;
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
+
+	return ge_b850v3_lvds_create_connector(bridge);
 }
 
 static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
@@ -327,6 +320,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
 	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
 					 DRM_BRIDGE_OP_EDID;
+	ge_b850v3_lvds_ptr->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
-- 
2.25.1

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

[PATCH v4 14/15] drm/bridge: nxp-ptn3460: add get_edid bridge operation

[Sam Ravnborg]

Add the get_edid() bridge operation to prepare for
use as a chained bridge.
Add helper function that is also used by the connector.

v2:
  - Fix memory leak (Laurent)
  - Do not save a copy of edid, read it when needed

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 43 ++++++++++++++++------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 438e566ce0a4..2805c8938f98 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -29,7 +29,6 @@ struct ptn3460_bridge {
 	struct drm_connector connector;
 	struct i2c_client *client;
 	struct drm_bridge bridge;
-	struct edid *edid;
 	struct drm_panel *panel;
 	struct gpio_desc *gpio_pd_n;
 	struct gpio_desc *gpio_rst_n;
@@ -184,17 +183,13 @@ static void ptn3460_post_disable(struct drm_bridge *bridge)
 	}
 }
 
-static int ptn3460_get_modes(struct drm_connector *connector)
+static struct edid *ptn3460_get_edid(struct drm_bridge *bridge,
+				     struct drm_connector *connector)
 {
-	struct ptn3460_bridge *ptn_bridge;
-	u8 *edid;
-	int ret, num_modes = 0;
+	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
 	bool power_off;
-
-	ptn_bridge = connector_to_ptn3460(connector);
-
-	if (ptn_bridge->edid)
-		return drm_add_edid_modes(connector, ptn_bridge->edid);
+	u8 *edid;
+	int ret;
 
 	power_off = !ptn_bridge->enabled;
 	ptn3460_pre_enable(&ptn_bridge->bridge);
@@ -202,30 +197,40 @@ static int ptn3460_get_modes(struct drm_connector *connector)
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid) {
 		DRM_ERROR("Failed to allocate EDID\n");
-		return 0;
+		goto out;
 	}
 
 	ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,
-			EDID_LENGTH);
+				 EDID_LENGTH);
 	if (ret) {
 		kfree(edid);
+		edid = NULL;
 		goto out;
 	}
 
-	ptn_bridge->edid = (struct edid *)edid;
-	drm_connector_update_edid_property(connector, ptn_bridge->edid);
-
-	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
-
 out:
 	if (power_off)
 		ptn3460_disable(&ptn_bridge->bridge);
 
+	return (struct edid *)edid;
+}
+
+static int ptn3460_connector_get_modes(struct drm_connector *connector)
+{
+	struct ptn3460_bridge *ptn_bridge = connector_to_ptn3460(connector);
+	struct edid *edid;
+	int num_modes;
+
+	edid = ptn3460_get_edid(&ptn_bridge->bridge, connector);
+	drm_connector_update_edid_property(connector, edid);
+	num_modes = drm_add_edid_modes(connector, edid);
+	kfree(edid);
+
 	return num_modes;
 }
 
 static const struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = {
-	.get_modes = ptn3460_get_modes,
+	.get_modes = ptn3460_connector_get_modes,
 };
 
 static const struct drm_connector_funcs ptn3460_connector_funcs = {
@@ -279,6 +284,7 @@ static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
 	.disable = ptn3460_disable,
 	.post_disable = ptn3460_post_disable,
 	.attach = ptn3460_bridge_attach,
+	.get_edid = ptn3460_get_edid,
 };
 
 static int ptn3460_probe(struct i2c_client *client,
@@ -327,6 +333,7 @@ static int ptn3460_probe(struct i2c_client *client,
 	}
 
 	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
+	ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
 	ptn_bridge->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ptn_bridge->bridge);
 
-- 
2.25.1

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

[PATCH v4 15/15] drm/bridge: nxp-ptn3460: add drm_panel_bridge support

[Sam Ravnborg]

Prepare the bridge driver for use in a chained setup.

- Replacing direct use of drm_panel with drm_panel_bridge support.
- Make the connector creation optional

Note: the bridge panel will use the connector type from the panel.

v2:
  - Use panel_bridge for local variable name to align with other drivers
  - Fix that connector was created twice (Laurent)
  - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 60 ++++++++++------------------
 1 file changed, 20 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 2805c8938f98..a49616855dd3 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -29,7 +29,7 @@ struct ptn3460_bridge {
 	struct drm_connector connector;
 	struct i2c_client *client;
 	struct drm_bridge bridge;
-	struct drm_panel *panel;
+	struct drm_bridge *panel_bridge;
 	struct gpio_desc *gpio_pd_n;
 	struct gpio_desc *gpio_rst_n;
 	u32 edid_emulation;
@@ -126,11 +126,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 	usleep_range(10, 20);
 	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
 
-	if (drm_panel_prepare(ptn_bridge->panel)) {
-		DRM_ERROR("failed to prepare panel\n");
-		return;
-	}
-
 	/*
 	 * There's a bug in the PTN chip where it falsely asserts hotplug before
 	 * it is fully functional. We're forced to wait for the maximum start up
@@ -145,16 +140,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 	ptn_bridge->enabled = true;
 }
 
-static void ptn3460_enable(struct drm_bridge *bridge)
-{
-	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
-
-	if (drm_panel_enable(ptn_bridge->panel)) {
-		DRM_ERROR("failed to enable panel\n");
-		return;
-	}
-}
-
 static void ptn3460_disable(struct drm_bridge *bridge)
 {
 	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
@@ -164,24 +149,10 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 	ptn_bridge->enabled = false;
 
-	if (drm_panel_disable(ptn_bridge->panel)) {
-		DRM_ERROR("failed to disable panel\n");
-		return;
-	}
-
 	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
 	gpiod_set_value(ptn_bridge->gpio_pd_n, 0);
 }
 
-static void ptn3460_post_disable(struct drm_bridge *bridge)
-{
-	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
-
-	if (drm_panel_unprepare(ptn_bridge->panel)) {
-		DRM_ERROR("failed to unprepare panel\n");
-		return;
-	}
-}
 
 static struct edid *ptn3460_get_edid(struct drm_bridge *bridge,
 				     struct drm_connector *connector)
@@ -245,12 +216,18 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
 	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
+	enum drm_bridge_attach_flags panel_flags;
 	int ret;
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		DRM_ERROR("Fix bridge driver to make connector optional!");
-		return -EINVAL;
-	}
+	/* Let this driver create connector if requested */
+	panel_flags = flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+	ret = drm_bridge_attach(bridge->encoder, ptn_bridge->panel_bridge,
+				bridge, panel_flags);
+	if (ret < 0)
+		return ret;
+
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+		return 0;
 
 	if (!bridge->encoder) {
 		DRM_ERROR("Parent encoder object not found");
@@ -270,9 +247,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 	drm_connector_attach_encoder(&ptn_bridge->connector,
 							bridge->encoder);
 
-	if (ptn_bridge->panel)
-		drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
-
 	drm_helper_hpd_irq_event(ptn_bridge->connector.dev);
 
 	return ret;
@@ -280,9 +254,7 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
 	.pre_enable = ptn3460_pre_enable,
-	.enable = ptn3460_enable,
 	.disable = ptn3460_disable,
-	.post_disable = ptn3460_post_disable,
 	.attach = ptn3460_bridge_attach,
 	.get_edid = ptn3460_get_edid,
 };
@@ -292,6 +264,8 @@ static int ptn3460_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct ptn3460_bridge *ptn_bridge;
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
 	int ret;
 
 	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
@@ -299,10 +273,15 @@ static int ptn3460_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ptn_bridge->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
 	if (ret)
 		return ret;
 
+	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+	if (IS_ERR(panel_bridge))
+		return PTR_ERR(panel_bridge);
+
+	ptn_bridge->panel_bridge = panel_bridge;
 	ptn_bridge->client = client;
 
 	ptn_bridge->gpio_pd_n = devm_gpiod_get(&client->dev, "powerdown",
@@ -334,6 +313,7 @@ static int ptn3460_probe(struct i2c_client *client,
 
 	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
 	ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
+	ptn_bridge->bridge.type = DRM_MODE_CONNECTOR_LVDS;
 	ptn_bridge->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ptn_bridge->bridge);
 
-- 
2.25.1

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

Re: [PATCH v4 01/15] drm/panel: panel-simple: validate panel description

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:10PM +0200, Sam Ravnborg wrote:
> Warn if we detect a panel with incomplete/wrong description.
> This is inspired by a similar patch by Laurent that introduced checks
> for LVDS panels - this extends the checks to the remaining type of
> connectors.
> 
> This is known to warn for some of the existing panels but added
> despite this as we need help from people using the panels to
> add the missing info.
> The checks are not complete but will catch the most common mistakes.
> 
> The checks at the same time serves as documentation for the minimum

s/serves/serve/

> required description for a panel.
> 
> The checks uses dev_warn() as we know this will hit. WARN() was
> too noisy at the moment for anything else than LVDS.
> 
> v2:
>   - Use dev_warn (Laurent)
>   - Check for empty bus_flags
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 41 ++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 54323515ca2c..a8d68102931e 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  	struct panel_simple *panel;
>  	struct display_timing dt;
>  	struct device_node *ddc;
> +	u32 bus_flags;
>  	int err;
>  
>  	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> @@ -549,8 +550,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  			panel_simple_parse_panel_timing_node(dev, panel, &dt);
>  	}
>  
> -	if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> -		/* Catch common mistakes for LVDS panels. */
> +	/* Catch common mistakes for panels. */
> +	switch (desc->connector_type) {
> +	case 0:
> +		dev_warn(dev, "Specify missing connector_type\n");
> +		break;
> +	case DRM_MODE_CONNECTOR_LVDS:
>  		WARN_ON(desc->bus_flags &
>  			~(DRM_BUS_FLAG_DE_LOW |
>  			  DRM_BUS_FLAG_DE_HIGH |
> @@ -564,6 +569,38 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
>  			 desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) &&
>  			desc->bpc != 8);
> +		break;
> +	case DRM_MODE_CONNECTOR_eDP:
> +		if (desc->bus_format == 0)
> +			dev_warn(dev, "Specify missing bus_format\n");
> +		if (desc->bpc != 6 && desc->bpc != 8)
> +			dev_warn(dev, "Expected bpc in {6,8} but got: %d\n", desc->bpc);

bpc is unsigned, so s/%d/%u/

> +		break;
> +	case DRM_MODE_CONNECTOR_DSI:
> +		if (desc->bpc != 6 && desc->bpc != 8)
> +			dev_warn(dev, "Expected bpc in {6,8} but got: %d\n", desc->bpc);

Same here.

> +		break;
> +	case DRM_MODE_CONNECTOR_DPI:
> +		bus_flags = DRM_BUS_FLAG_DE_LOW |
> +			    DRM_BUS_FLAG_DE_HIGH |
> +			    DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> +			    DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +			    DRM_BUS_FLAG_DATA_MSB_TO_LSB |
> +			    DRM_BUS_FLAG_DATA_LSB_TO_MSB |
> +			    DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> +			    DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE;
> +		if (desc->bus_flags & ~bus_flags)
> +			dev_warn(dev, "Unexpected bus_flags(%d)\n", desc->bus_flags & ~bus_flags);
> +		if (!(desc->bus_flags & bus_flags))
> +			dev_warn(dev, "Specify missing bus_flags\n");
> +		if (desc->bus_format == 0)
> +			dev_warn(dev, "Specify missing bus_format\n");
> +		if (desc->bpc != 6 && desc->bpc != 8)
> +			dev_warn(dev, "Expected bpc in {6,8} but got: %d\n", desc->bpc);

And here too.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		break;
> +	default:
> +		dev_warn(dev, "Specify a valid connector_type: %d\n", desc->connector_type);
> +		break;
>  	}
>  
>  	drm_panel_init(&panel->base, dev, &panel_simple_funcs,

-- 
Regards,

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

Re: [PATCH v4 02/15] drm/panel: panel-simple: add default connector_type

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:11PM +0200, Sam Ravnborg wrote:
> All panels shall report a connector type.
> panel-simple has a lot of panels with no connector_type,
> and for these fall back to DPI as the default.
> 
> v2:
>   - Rebased on top of validation of panel description
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/panel/panel-simple.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index a8d68102931e..56ab073e4e6e 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  	struct panel_simple *panel;
>  	struct display_timing dt;
>  	struct device_node *ddc;
> +	int connector_type;
>  	u32 bus_flags;
>  	int err;
>  
> @@ -550,10 +551,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  			panel_simple_parse_panel_timing_node(dev, panel, &dt);
>  	}
>  
> +	connector_type = desc->connector_type;
>  	/* Catch common mistakes for panels. */
> -	switch (desc->connector_type) {
> +	switch (connector_type) {
>  	case 0:
>  		dev_warn(dev, "Specify missing connector_type\n");
> +		connector_type = DRM_MODE_CONNECTOR_DPI;
>  		break;
>  	case DRM_MODE_CONNECTOR_LVDS:
>  		WARN_ON(desc->bus_flags &
> @@ -600,11 +603,11 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		break;
>  	default:
>  		dev_warn(dev, "Specify a valid connector_type: %d\n", desc->connector_type);
> +		connector_type = DRM_MODE_CONNECTOR_DPI;
>  		break;
>  	}
>  
> -	drm_panel_init(&panel->base, dev, &panel_simple_funcs,
> -		       desc->connector_type);
> +	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
>  
>  	err = drm_panel_of_backlight(&panel->base);
>  	if (err)

-- 
Regards,

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

Re: [PATCH v4 05/15] drm/bridge: tc358767: add detect bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:14PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for chained operation by adding
> support for the detect operation.
> 
> v2:
>   - Do not announce detect operation if there is no hpd pin (Laurent)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index c2777b226c75..e8ba713bedac 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1369,21 +1369,13 @@ static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
>  };
>  
> -static enum drm_connector_status tc_connector_detect(struct drm_connector *connector,
> -						     bool force)
> +static enum drm_connector_status tc_bridge_detect(struct drm_bridge *bridge)
>  {
> -	struct tc_data *tc = connector_to_tc(connector);
> +	struct tc_data *tc = bridge_to_tc(bridge);
>  	bool conn;
>  	u32 val;
>  	int ret;
>  
> -	if (tc->hpd_pin < 0) {
> -		if (tc->panel)
> -			return connector_status_connected;
> -		else
> -			return connector_status_unknown;
> -	}
> -
>  	ret = regmap_read(tc->regmap, GPIOI, &val);
>  	if (ret)
>  		return connector_status_unknown;
> @@ -1396,6 +1388,20 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
>  		return connector_status_disconnected;
>  }
>  
> +static enum drm_connector_status
> +tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct tc_data *tc = connector_to_tc(connector);
> +
> +	if (tc->hpd_pin >= 0)
> +		return tc_bridge_detect(&tc->bridge);
> +	else
> +		if (tc->panel)
> +			return connector_status_connected;
> +
> +	return connector_status_unknown;

I'd write this

	if (tc->hpd_pin >= 0)
		return tc_bridge_detect(&tc->bridge);

	if (tc->panel)
		return connector_status_connected;
	else
		return connector_status_unknown;

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +}
> +
>  static const struct drm_connector_funcs tc_connector_funcs = {
>  	.detect = tc_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> @@ -1458,6 +1464,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.disable = tc_bridge_disable,
>  	.post_disable = tc_bridge_post_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
> +	.detect = tc_bridge_detect,
>  };
>  
>  static bool tc_readable_reg(struct device *dev, unsigned int reg)
> @@ -1680,6 +1687,9 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  
>  	tc->bridge.funcs = &tc_bridge_funcs;
> +	if (tc->hpd_pin >= 0)
> +		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> +
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
>  

-- 
Regards,

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

Re: [PATCH v4 04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:13PM +0200, Sam Ravnborg wrote:
> Prepare the tc358764 bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
> 
> The bridge panel will use the connector type reported by the panel,
> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
> 
> The tc358764 did not any additional info the the connector so the

Did you mean s/did not any/did not add any/ ?
s/the the/to the/

> connector creation is passed to the bridge panel driver.
> 
> v3:
>   - Merge with patch to make connector creation optional to avoid
>     creating two connectors (Laurent)
>   - Pass connector creation to bridge panel, as this bridge driver
>     did not add any extra info to the connector.
>   - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
> 
> v2:
>   - Use PTR_ERR_OR_ZERO() (kbuild test robot)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: kbuild test robot <lkp@intel.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/tc358764.c | 107 +++++-------------------------
>  1 file changed, 16 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
> index a277739fab58..fdde4cfdc724 100644
> --- a/drivers/gpu/drm/bridge/tc358764.c
> +++ b/drivers/gpu/drm/bridge/tc358764.c
> @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = {
>  struct tc358764 {
>  	struct device *dev;
>  	struct drm_bridge bridge;
> -	struct drm_connector connector;
>  	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
>  	struct gpio_desc *gpio_reset;
> -	struct drm_panel *panel;
> +	struct drm_bridge *panel_bridge;
>  	int error;
>  };
>  
> @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
>  	return container_of(bridge, struct tc358764, bridge);
>  }
>  
> -static inline
> -struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct tc358764, connector);
> -}
> -
>  static int tc358764_init(struct tc358764 *ctx)
>  {
>  	u32 v = 0;
> @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx)
>  	usleep_range(1000, 2000);
>  }
>  
> -static int tc358764_get_modes(struct drm_connector *connector)
> -{
> -	struct tc358764 *ctx = connector_to_tc358764(connector);
> -
> -	return drm_panel_get_modes(ctx->panel, connector);
> -}
> -
> -static const
> -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
> -	.get_modes = tc358764_get_modes,
> -};
> -
> -static const struct drm_connector_funcs tc358764_connector_funcs = {
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static void tc358764_disable(struct drm_bridge *bridge)
> -{
> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> -	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
> -
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
> -}
> -
>  static void tc358764_post_disable(struct drm_bridge *bridge)
>  {
>  	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>  	int ret;
>  
> -	ret = drm_panel_unprepare(ctx->panel);
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
>  	tc358764_reset(ctx);
>  	usleep_range(10000, 15000);
>  	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
>  	ret = tc358764_init(ctx);
>  	if (ret < 0)
>  		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> -	ret = drm_panel_prepare(ctx->panel);
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
> -}
> -
> -static void tc358764_enable(struct drm_bridge *bridge)
> -{
> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> -	int ret = drm_panel_enable(ctx->panel);
> -
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
>  }
>  
>  static int tc358764_attach(struct drm_bridge *bridge,
>  			   enum drm_bridge_attach_flags flags)
> -{
> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> -	struct drm_device *drm = bridge->dev;
> -	int ret;
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
> -	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
> -	ret = drm_connector_init(drm, &ctx->connector,
> -				 &tc358764_connector_funcs,
> -				 DRM_MODE_CONNECTOR_LVDS);
> -	if (ret) {
> -		DRM_ERROR("Failed to initialize connector\n");
> -		return ret;
> -	}
> -
> -	drm_connector_helper_add(&ctx->connector,
> -				 &tc358764_connector_helper_funcs);
> -	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
> -	drm_panel_attach(ctx->panel, &ctx->connector);
> -	ctx->connector.funcs->reset(&ctx->connector);
> -
> -	return 0;
> -}
> -
> -static void tc358764_detach(struct drm_bridge *bridge)
>  {
>  	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>  
> -	drm_panel_detach(ctx->panel);
> -	ctx->panel = NULL;
> +	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
> +				 bridge, flags);
>  }
>  
>  static const struct drm_bridge_funcs tc358764_bridge_funcs = {
> -	.disable = tc358764_disable,
>  	.post_disable = tc358764_post_disable,
> -	.enable = tc358764_enable,
>  	.pre_enable = tc358764_pre_enable,
>  	.attach = tc358764_attach,
> -	.detach = tc358764_detach,
>  };
>  
>  static int tc358764_parse_dt(struct tc358764 *ctx)
>  {
> +	struct drm_bridge *panel_bridge;
>  	struct device *dev = ctx->dev;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
>  		return PTR_ERR(ctx->gpio_reset);
>  	}
>  
> -	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
> -					  NULL);
> -	if (ret && ret != -EPROBE_DEFER)
> -		dev_err(dev, "cannot find panel (%d)\n", ret);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +

You can drop this blank line.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (IS_ERR(panel_bridge))
> +		return PTR_ERR(panel_bridge);
> +
> +	ctx->panel_bridge = panel_bridge;
> +	return 0;
>  }
>  
>  static int tc358764_configure_regulators(struct tc358764 *ctx)
> @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
>  		return ret;
>  
>  	ctx->bridge.funcs = &tc358764_bridge_funcs;
> +	ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>  	ctx->bridge.of_node = dev->of_node;
>  
>  	drm_bridge_add(&ctx->bridge);

-- 
Regards,

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

Re: [PATCH v4 06/15] drm/bridge: tc358767: add get_edid bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:15PM +0200, Sam Ravnborg wrote:
> Prepare for chained bridge with the addition of
> get_edid support.
> 
> v2:
>   - Fixed handling of edid storage (Laurent)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 34 ++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index e8ba713bedac..d86d7f06bebb 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -250,8 +250,6 @@ struct tc_data {
>  	/* link settings */
>  	struct tc_edp_link	link;
>  
> -	/* display edid */
> -	struct edid		*edid;
>  	/* current mode */
>  	struct drm_display_mode	mode;
>  
> @@ -1335,11 +1333,19 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
>  	tc->mode = *mode;
>  }
>  
> +static struct edid *tc_get_edid(struct drm_bridge *bridge,
> +				struct drm_connector *connector)
> +{
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +
> +	return drm_get_edid(connector, &tc->aux.ddc);
> +}
> +
>  static int tc_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct tc_data *tc = connector_to_tc(connector);
> +	int num_modes;
>  	struct edid *edid;
> -	int count;
>  	int ret;
>  
>  	ret = tc_get_display_props(tc);
> @@ -1348,21 +1354,15 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  		return 0;
>  	}
>  
> -	count = drm_panel_get_modes(tc->panel, connector);
> -	if (count > 0)
> -		return count;
> -
> -	edid = drm_get_edid(connector, &tc->aux.ddc);
> -
> -	kfree(tc->edid);
> -	tc->edid = edid;
> -	if (!edid)
> -		return 0;
> +	num_modes = drm_panel_get_modes(tc->panel, connector);
> +	if (num_modes > 0)
> +		return num_modes;
>  
> -	drm_connector_update_edid_property(connector, edid);
> -	count = drm_add_edid_modes(connector, edid);
> +	edid = tc_get_edid(&tc->bridge, connector);
> +	num_modes = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
>  
> -	return count;
> +	return num_modes;
>  }
>  
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
> @@ -1465,6 +1465,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.post_disable = tc_bridge_post_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
>  	.detect = tc_bridge_detect,
> +	.get_edid = tc_get_edid,
>  };
>  
>  static bool tc_readable_reg(struct device *dev, unsigned int reg)
> @@ -1689,6 +1690,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	tc->bridge.funcs = &tc_bridge_funcs;
>  	if (tc->hpd_pin >= 0)
>  		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> +	tc->bridge.ops |= DRM_BRIDGE_OP_EDID;
>  
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);

-- 
Regards,

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

Re: [PATCH v4 07/15] drm/bridge: tc358767: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:16PM +0200, Sam Ravnborg wrote:
> With the bridge operations implemented the last step to prepare
> this driver for a chained setup is the use of the bridge panel driver.
> 
> The bridge panel driver is only used when a prot@2 is present in

s/prot/port/

> the DT. So when the display driver request a connector

s/request/requests/

> support both situations:
> - connector created by bridge panel driver
> - connector created by this driver
> 
> And on top, support that the display driver creates the connector,
> which is the preferred setup.
> 
> Note: the bridge panel will use the connector type from the panel.
> 
> v2:
>   - Merge connector and drm_panel_bridge patches
>     and fix so we do not create two connectors (Laurent)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 70 +++++++++++++++----------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index d86d7f06bebb..75a2cd792ccc 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -244,8 +244,8 @@ struct tc_data {
>  	struct drm_dp_aux	aux;
>  
>  	struct drm_bridge	bridge;
> +	struct drm_bridge	*panel_bridge;
>  	struct drm_connector	connector;
> -	struct drm_panel	*panel;
>  
>  	/* link settings */
>  	struct tc_edp_link	link;
> @@ -1234,13 +1234,6 @@ static int tc_stream_disable(struct tc_data *tc)
>  	return 0;
>  }
>  
> -static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> -{
> -	struct tc_data *tc = bridge_to_tc(bridge);
> -
> -	drm_panel_prepare(tc->panel);
> -}
> -
>  static void tc_bridge_enable(struct drm_bridge *bridge)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
> @@ -1264,8 +1257,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  		tc_main_link_disable(tc);
>  		return;
>  	}
> -
> -	drm_panel_enable(tc->panel);
>  }
>  
>  static void tc_bridge_disable(struct drm_bridge *bridge)
> @@ -1273,8 +1264,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> -	drm_panel_disable(tc->panel);
> -
>  	ret = tc_stream_disable(tc);
>  	if (ret < 0)
>  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> @@ -1284,13 +1273,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  		dev_err(tc->dev, "main link disable error: %d\n", ret);
>  }
>  
> -static void tc_bridge_post_disable(struct drm_bridge *bridge)
> -{
> -	struct tc_data *tc = bridge_to_tc(bridge);
> -
> -	drm_panel_unprepare(tc->panel);
> -}
> -
>  static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>  				 const struct drm_display_mode *mode,
>  				 struct drm_display_mode *adj)
> @@ -1354,9 +1336,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  		return 0;
>  	}
>  
> -	num_modes = drm_panel_get_modes(tc->panel, connector);
> -	if (num_modes > 0)
> -		return num_modes;
> +	if (tc->panel_bridge) {
> +		num_modes = drm_bridge_get_modes(tc->panel_bridge, connector);
> +		if (num_modes > 0)
> +			return num_modes;
> +	}
>  
>  	edid = tc_get_edid(&tc->bridge, connector);
>  	num_modes = drm_add_edid_modes(connector, edid);
> @@ -1396,7 +1380,7 @@ tc_connector_detect(struct drm_connector *connector, bool force)
>  	if (tc->hpd_pin >= 0)
>  		return tc_bridge_detect(&tc->bridge);
>  	else
> -		if (tc->panel)
> +		if (tc->panel_bridge)
>  			return connector_status_connected;
>  
>  	return connector_status_unknown;
> @@ -1419,16 +1403,23 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  	struct drm_device *drm = bridge->dev;
>  	int ret;
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> +	if (tc->panel_bridge) {
> +		enum drm_bridge_attach_flags panel_flags;
> +
> +		/* If a connector is required then this driver shall create it */
> +		panel_flags = flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR;

Shouldn't this be

		panel_flags = flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR;

to ensure that the panel driver will not create a connector ?

> +		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> +					&tc->bridge, panel_flags);
> +		if (ret)
> +			return ret;
>  	}
>  
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +		return 0;
> +
>  	/* Create DP/eDP connector */
>  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
> -	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
> -				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> -				 DRM_MODE_CONNECTOR_DisplayPort);
> +	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, tc->bridge.type);
>  	if (ret)
>  		return ret;
>  
> @@ -1441,9 +1432,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  					       DRM_CONNECTOR_POLL_DISCONNECT;
>  	}
>  
> -	if (tc->panel)
> -		drm_panel_attach(tc->panel, &tc->connector);
> -
>  	drm_display_info_set_bus_formats(&tc->connector.display_info,
>  					 &bus_format, 1);
>  	tc->connector.display_info.bus_flags =
> @@ -1459,10 +1447,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.attach = tc_bridge_attach,
>  	.mode_valid = tc_mode_valid,
>  	.mode_set = tc_bridge_mode_set,
> -	.pre_enable = tc_bridge_pre_enable,
>  	.enable = tc_bridge_enable,
>  	.disable = tc_bridge_disable,
> -	.post_disable = tc_bridge_post_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
>  	.detect = tc_bridge_detect,
>  	.get_edid = tc_get_edid,
> @@ -1555,6 +1541,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
>  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> +	struct drm_panel *panel;
>  	struct tc_data *tc;
>  	int ret;
>  
> @@ -1565,10 +1552,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	tc->dev = dev;
>  
>  	/* port@2 is the output port */
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
>  	if (ret && ret != -ENODEV)
>  		return ret;
>  
> +	if (panel) {
> +		struct drm_bridge *panel_bridge;
> +
> +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(panel_bridge))
> +			return PTR_ERR(panel_bridge);
> +
> +		tc->panel_bridge = panel_bridge;
> +		tc->bridge.type = DRM_MODE_CONNECTOR_eDP;
> +	} else {
> +		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> +	}
> +
>  	/* Shut down GPIO is optional */
>  	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
>  	if (IS_ERR(tc->sd_gpio))

-- 
Regards,

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

Re: [PATCH v4 08/15] drm/bridge: parade-ps8622: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:17PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
> 
> The connecter is now either created by the panel bridge or the display
> driver. So all code for connector creation in this driver is no longer
> relevant and thus dropped.
> 
> The connector code had some special polling handling:
>     connector.polled = DRM_CONNECTOR_POLL_HPD;
>     drm_helper_hpd_irq_event(ps8622->bridge.dev);
> 
> This code was most likely added to speed up detection of the connector.
> If really needed then this functionality belongs somewhere else.
> 
> Note: the bridge panel will use the connector type from the panel.
> 
> v2:
>   - Fix to avoid creating connector twice (Laurent)
>   - Drop all connector code - defer to bridge panel
>   - Use panel_bridge for local variable to align with other drivers
>   - Set bridge.type to DRM_MODE_CONNECTOR_LVDS;
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/parade-ps8622.c | 100 ++++---------------------
>  1 file changed, 13 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> index d789ea2a7fb9..614b19f0f1b7 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8622.c
> @@ -42,10 +42,9 @@
>  #endif
>  
>  struct ps8622_bridge {
> -	struct drm_connector connector;
>  	struct i2c_client *client;
>  	struct drm_bridge bridge;
> -	struct drm_panel *panel;
> +	struct drm_bridge *panel_bridge;
>  	struct regulator *v12;
>  	struct backlight_device *bl;
>  
> @@ -64,12 +63,6 @@ static inline struct ps8622_bridge *
>  	return container_of(bridge, struct ps8622_bridge, bridge);
>  }
>  
> -static inline struct ps8622_bridge *
> -		connector_to_ps8622(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct ps8622_bridge, connector);
> -}
> -
>  static int ps8622_set(struct i2c_client *client, u8 page, u8 reg, u8 val)
>  {
>  	int ret;
> @@ -365,11 +358,6 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
>  			DRM_ERROR("fails to enable ps8622->v12");
>  	}
>  
> -	if (drm_panel_prepare(ps8622->panel)) {
> -		DRM_ERROR("failed to prepare panel\n");
> -		return;
> -	}
> -
>  	gpiod_set_value(ps8622->gpio_slp, 1);
>  
>  	/*
> @@ -399,24 +387,9 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
>  	ps8622->enabled = true;
>  }
>  
> -static void ps8622_enable(struct drm_bridge *bridge)
> -{
> -	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> -
> -	if (drm_panel_enable(ps8622->panel)) {
> -		DRM_ERROR("failed to enable panel\n");
> -		return;
> -	}
> -}
> -
>  static void ps8622_disable(struct drm_bridge *bridge)
>  {
> -	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> -
> -	if (drm_panel_disable(ps8622->panel)) {
> -		DRM_ERROR("failed to disable panel\n");
> -		return;
> -	}
> +	/* Delay after panel is disabled */
>  	msleep(PS8622_PWMO_END_T12_MS);

I really don't understand why a delay would be needed here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
> @@ -436,11 +409,6 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
>  	 */
>  	gpiod_set_value(ps8622->gpio_slp, 0);
>  
> -	if (drm_panel_unprepare(ps8622->panel)) {
> -		DRM_ERROR("failed to unprepare panel\n");
> -		return;
> -	}
> -
>  	if (ps8622->v12)
>  		regulator_disable(ps8622->v12);
>  
> @@ -455,67 +423,17 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
>  	msleep(PS8622_POWER_OFF_T17_MS);
>  }
>  
> -static int ps8622_get_modes(struct drm_connector *connector)
> -{
> -	struct ps8622_bridge *ps8622;
> -
> -	ps8622 = connector_to_ps8622(connector);
> -
> -	return drm_panel_get_modes(ps8622->panel, connector);
> -}
> -
> -static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = {
> -	.get_modes = ps8622_get_modes,
> -};
> -
> -static const struct drm_connector_funcs ps8622_connector_funcs = {
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
>  static int ps8622_attach(struct drm_bridge *bridge,
>  			 enum drm_bridge_attach_flags flags)
>  {
>  	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> -	int ret;
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
>  
> -	if (!bridge->encoder) {
> -		DRM_ERROR("Parent encoder object not found");
> -		return -ENODEV;
> -	}
> -
> -	ps8622->connector.polled = DRM_CONNECTOR_POLL_HPD;
> -	ret = drm_connector_init(bridge->dev, &ps8622->connector,
> -			&ps8622_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
> -	if (ret) {
> -		DRM_ERROR("Failed to initialize connector with drm\n");
> -		return ret;
> -	}
> -	drm_connector_helper_add(&ps8622->connector,
> -					&ps8622_connector_helper_funcs);
> -	drm_connector_register(&ps8622->connector);
> -	drm_connector_attach_encoder(&ps8622->connector,
> -							bridge->encoder);
> -
> -	if (ps8622->panel)
> -		drm_panel_attach(ps8622->panel, &ps8622->connector);
> -
> -	drm_helper_hpd_irq_event(ps8622->connector.dev);
> -
> -	return ret;
> +	return drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
> +				 &ps8622->bridge, flags);
>  }
>  
>  static const struct drm_bridge_funcs ps8622_bridge_funcs = {
>  	.pre_enable = ps8622_pre_enable,
> -	.enable = ps8622_enable,
>  	.disable = ps8622_disable,
>  	.post_disable = ps8622_post_disable,
>  	.attach = ps8622_attach,
> @@ -533,16 +451,23 @@ static int ps8622_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct ps8622_bridge *ps8622;
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL);
>  	if (!ps8622)
>  		return -ENOMEM;
>  
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ps8622->panel, NULL);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
>  	if (ret)
>  		return ret;
>  
> +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +	if (IS_ERR(panel_bridge))
> +		return PTR_ERR(panel_bridge);
> +
> +	ps8622->panel_bridge = panel_bridge;
>  	ps8622->client = client;
>  
>  	ps8622->v12 = devm_regulator_get(dev, "vdd12");
> @@ -595,6 +520,7 @@ static int ps8622_probe(struct i2c_client *client,
>  	}
>  
>  	ps8622->bridge.funcs = &ps8622_bridge_funcs;
> +	ps8622->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>  	ps8622->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ps8622->bridge);
>  

-- 
Regards,

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

Re: [PATCH v4 12/15] drm/bridge: megachips: add get_edid bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:21PM +0200, Sam Ravnborg wrote:
> To prepare for a chained bridge setup add support for the
> get_edid bridge operation.
> There is no need for a copy of the edid - so drop
> the pointer to the copy.
> 
> v2:
>   - Fix so we do not leak memory (Laurent)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 31 ++++++++++---------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 450dca33ea48..f7b55dc374ac 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -61,7 +61,6 @@ struct ge_b850v3_lvds {
>  	struct drm_bridge bridge;
>  	struct i2c_client *stdp4028_i2c;
>  	struct i2c_client *stdp2690_i2c;
> -	struct edid *edid;
>  };
>  
>  static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
> @@ -131,22 +130,26 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
>  	return NULL;
>  }
>  
> -static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
> +static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
> +					    struct drm_connector *connector)
>  {
>  	struct i2c_client *client;
> -	int num_modes = 0;
>  
>  	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
>  
> -	kfree(ge_b850v3_lvds_ptr->edid);
> -	ge_b850v3_lvds_ptr->edid = (struct edid *)stdp2690_get_edid(client);
> +	return (struct edid *)stdp2690_get_edid(client);
> +}
>  
> -	if (ge_b850v3_lvds_ptr->edid) {
> -		drm_connector_update_edid_property(connector,
> -						      ge_b850v3_lvds_ptr->edid);
> -		num_modes = drm_add_edid_modes(connector,
> -					       ge_b850v3_lvds_ptr->edid);
> -	}
> +static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +	int num_modes;
> +
> +	edid = ge_b850v3_lvds_get_edid(&ge_b850v3_lvds_ptr->bridge, connector);
> +
> +	drm_connector_update_edid_property(connector, edid);
> +	num_modes = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
>  
>  	return num_modes;
>  }
> @@ -269,6 +272,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
>  	.attach = ge_b850v3_lvds_attach,
>  	.detect = ge_b850v3_lvds_bridge_detect,
> +	.get_edid = ge_b850v3_lvds_get_edid,
>  };
>  
>  static int ge_b850v3_lvds_init(struct device *dev)
> @@ -304,8 +308,6 @@ static void ge_b850v3_lvds_remove(void)
>  
>  	drm_bridge_remove(&ge_b850v3_lvds_ptr->bridge);
>  
> -	kfree(ge_b850v3_lvds_ptr->edid);
> -
>  	ge_b850v3_lvds_ptr = NULL;
>  out:
>  	mutex_unlock(&ge_b850v3_lvds_dev_mutex);
> @@ -323,7 +325,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
>  
>  	/* drm bridge initialization */
>  	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
> -	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT;
> +	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
> +					 DRM_BRIDGE_OP_EDID;
>  	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
>  

-- 
Regards,

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

Re: [PATCH v4 13/15] drm/bridge: megachips: make connector creation optional

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:22PM +0200, Sam Ravnborg wrote:
> Make the connector creation optional to enable usage of the
> megachips-stdpxxxx-ge-b850v3-fw bridge with the DRM bridge connector
> helper.
> 
> v2:
>   - Set bridge.type to DRM_MODE_CONNECTOR_DisplayPort
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index f7b55dc374ac..61a24f265c7a 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -245,16 +245,6 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  {
>  	struct i2c_client *stdp4028_i2c
>  			= ge_b850v3_lvds_ptr->stdp4028_i2c;
> -	int ret;
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
> -	ret = ge_b850v3_lvds_create_connector(bridge);
> -	if (ret)
> -		return ret;
>  
>  	/* Configures the bridge to re-enable interrupts after each ack. */
>  	i2c_smbus_write_word_data(stdp4028_i2c,
> @@ -266,7 +256,10 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
>  				  STDP4028_DPTX_IRQ_EN_REG,
>  				  STDP4028_DPTX_IRQ_CONFIG);
>  
> -	return 0;
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +		return 0;
> +
> +	return ge_b850v3_lvds_create_connector(bridge);
>  }
>  
>  static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
> @@ -327,6 +320,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
>  	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
>  	ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT |
>  					 DRM_BRIDGE_OP_EDID;
> +	ge_b850v3_lvds_ptr->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
>  

-- 
Regards,

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

Re: [PATCH v4 14/15] drm/bridge: nxp-ptn3460: add get_edid bridge operation

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:23PM +0200, Sam Ravnborg wrote:
> Add the get_edid() bridge operation to prepare for
> use as a chained bridge.
> Add helper function that is also used by the connector.
> 
> v2:
>   - Fix memory leak (Laurent)
>   - Do not save a copy of edid, read it when needed
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/nxp-ptn3460.c | 43 ++++++++++++++++------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index 438e566ce0a4..2805c8938f98 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -29,7 +29,6 @@ struct ptn3460_bridge {
>  	struct drm_connector connector;
>  	struct i2c_client *client;
>  	struct drm_bridge bridge;
> -	struct edid *edid;
>  	struct drm_panel *panel;
>  	struct gpio_desc *gpio_pd_n;
>  	struct gpio_desc *gpio_rst_n;
> @@ -184,17 +183,13 @@ static void ptn3460_post_disable(struct drm_bridge *bridge)
>  	}
>  }
>  
> -static int ptn3460_get_modes(struct drm_connector *connector)
> +static struct edid *ptn3460_get_edid(struct drm_bridge *bridge,
> +				     struct drm_connector *connector)
>  {
> -	struct ptn3460_bridge *ptn_bridge;
> -	u8 *edid;
> -	int ret, num_modes = 0;
> +	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
>  	bool power_off;
> -
> -	ptn_bridge = connector_to_ptn3460(connector);
> -
> -	if (ptn_bridge->edid)
> -		return drm_add_edid_modes(connector, ptn_bridge->edid);
> +	u8 *edid;
> +	int ret;
>  
>  	power_off = !ptn_bridge->enabled;
>  	ptn3460_pre_enable(&ptn_bridge->bridge);
> @@ -202,30 +197,40 @@ static int ptn3460_get_modes(struct drm_connector *connector)
>  	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>  	if (!edid) {
>  		DRM_ERROR("Failed to allocate EDID\n");
> -		return 0;
> +		goto out;
>  	}
>  
>  	ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,
> -			EDID_LENGTH);
> +				 EDID_LENGTH);
>  	if (ret) {
>  		kfree(edid);
> +		edid = NULL;
>  		goto out;
>  	}
>  
> -	ptn_bridge->edid = (struct edid *)edid;
> -	drm_connector_update_edid_property(connector, ptn_bridge->edid);
> -
> -	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
> -
>  out:
>  	if (power_off)
>  		ptn3460_disable(&ptn_bridge->bridge);
>  
> +	return (struct edid *)edid;
> +}
> +
> +static int ptn3460_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct ptn3460_bridge *ptn_bridge = connector_to_ptn3460(connector);
> +	struct edid *edid;
> +	int num_modes;
> +
> +	edid = ptn3460_get_edid(&ptn_bridge->bridge, connector);
> +	drm_connector_update_edid_property(connector, edid);
> +	num_modes = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
> +
>  	return num_modes;
>  }
>  
>  static const struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = {
> -	.get_modes = ptn3460_get_modes,
> +	.get_modes = ptn3460_connector_get_modes,
>  };
>  
>  static const struct drm_connector_funcs ptn3460_connector_funcs = {
> @@ -279,6 +284,7 @@ static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
>  	.disable = ptn3460_disable,
>  	.post_disable = ptn3460_post_disable,
>  	.attach = ptn3460_bridge_attach,
> +	.get_edid = ptn3460_get_edid,
>  };
>  
>  static int ptn3460_probe(struct i2c_client *client,
> @@ -327,6 +333,7 @@ static int ptn3460_probe(struct i2c_client *client,
>  	}
>  
>  	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
> +	ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
>  	ptn_bridge->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ptn_bridge->bridge);
>  

-- 
Regards,

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

Re: [PATCH v4 15/15] drm/bridge: nxp-ptn3460: add drm_panel_bridge support

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 26, 2020 at 10:33:24PM +0200, Sam Ravnborg wrote:
> Prepare the bridge driver for use in a chained setup.
> 
> - Replacing direct use of drm_panel with drm_panel_bridge support.
> - Make the connector creation optional
> 
> Note: the bridge panel will use the connector type from the panel.
> 
> v2:
>   - Use panel_bridge for local variable name to align with other drivers
>   - Fix that connector was created twice (Laurent)
>   - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/nxp-ptn3460.c | 60 ++++++++++------------------
>  1 file changed, 20 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index 2805c8938f98..a49616855dd3 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -29,7 +29,7 @@ struct ptn3460_bridge {
>  	struct drm_connector connector;
>  	struct i2c_client *client;
>  	struct drm_bridge bridge;
> -	struct drm_panel *panel;
> +	struct drm_bridge *panel_bridge;
>  	struct gpio_desc *gpio_pd_n;
>  	struct gpio_desc *gpio_rst_n;
>  	u32 edid_emulation;
> @@ -126,11 +126,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>  	usleep_range(10, 20);
>  	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
>  
> -	if (drm_panel_prepare(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to prepare panel\n");
> -		return;
> -	}
> -
>  	/*
>  	 * There's a bug in the PTN chip where it falsely asserts hotplug before
>  	 * it is fully functional. We're forced to wait for the maximum start up
> @@ -145,16 +140,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>  	ptn_bridge->enabled = true;
>  }
>  
> -static void ptn3460_enable(struct drm_bridge *bridge)
> -{
> -	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> -
> -	if (drm_panel_enable(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to enable panel\n");
> -		return;
> -	}
> -}
> -
>  static void ptn3460_disable(struct drm_bridge *bridge)
>  {
>  	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> @@ -164,24 +149,10 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>  
>  	ptn_bridge->enabled = false;
>  
> -	if (drm_panel_disable(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to disable panel\n");
> -		return;
> -	}
> -
>  	gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
>  	gpiod_set_value(ptn_bridge->gpio_pd_n, 0);
>  }
>  
> -static void ptn3460_post_disable(struct drm_bridge *bridge)
> -{
> -	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> -
> -	if (drm_panel_unprepare(ptn_bridge->panel)) {
> -		DRM_ERROR("failed to unprepare panel\n");
> -		return;
> -	}
> -}
>  

Extra blank line.

>  static struct edid *ptn3460_get_edid(struct drm_bridge *bridge,
>  				     struct drm_connector *connector)
> @@ -245,12 +216,18 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  				 enum drm_bridge_attach_flags flags)
>  {
>  	struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge);
> +	enum drm_bridge_attach_flags panel_flags;
>  	int ret;
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> +	/* Let this driver create connector if requested */
> +	panel_flags = flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR;

Same as in a previous patch, should this be

	panel_flags = flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR;

?

> +	ret = drm_bridge_attach(bridge->encoder, ptn_bridge->panel_bridge,
> +				bridge, panel_flags);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +		return 0;
>  
>  	if (!bridge->encoder) {
>  		DRM_ERROR("Parent encoder object not found");
> @@ -270,9 +247,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  	drm_connector_attach_encoder(&ptn_bridge->connector,
>  							bridge->encoder);
>  
> -	if (ptn_bridge->panel)
> -		drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
> -
>  	drm_helper_hpd_irq_event(ptn_bridge->connector.dev);
>  
>  	return ret;
> @@ -280,9 +254,7 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs ptn3460_bridge_funcs = {
>  	.pre_enable = ptn3460_pre_enable,
> -	.enable = ptn3460_enable,
>  	.disable = ptn3460_disable,
> -	.post_disable = ptn3460_post_disable,
>  	.attach = ptn3460_bridge_attach,
>  	.get_edid = ptn3460_get_edid,
>  };
> @@ -292,6 +264,8 @@ static int ptn3460_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct ptn3460_bridge *ptn_bridge;
> +	struct drm_bridge *panel_bridge;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
> @@ -299,10 +273,15 @@ static int ptn3460_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	}
>  
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ptn_bridge->panel, NULL);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
>  	if (ret)
>  		return ret;
>  
> +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +	if (IS_ERR(panel_bridge))
> +		return PTR_ERR(panel_bridge);
> +
> +	ptn_bridge->panel_bridge = panel_bridge;
>  	ptn_bridge->client = client;
>  
>  	ptn_bridge->gpio_pd_n = devm_gpiod_get(&client->dev, "powerdown",
> @@ -334,6 +313,7 @@ static int ptn3460_probe(struct i2c_client *client,
>  
>  	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
>  	ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
> +	ptn_bridge->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>  	ptn_bridge->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ptn_bridge->bridge);
>  

-- 
Regards,

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

Re: [PATCH v4 07/15] drm/bridge: tc358767: add drm_panel_bridge support

[Sam Ravnborg]

Hi Laurent.

Thanks for the prompt review.

On Mon, Jul 27, 2020 at 12:48:32AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Sun, Jul 26, 2020 at 10:33:16PM +0200, Sam Ravnborg wrote:
> > With the bridge operations implemented the last step to prepare
> > this driver for a chained setup is the use of the bridge panel driver.
> > 
> > The bridge panel driver is only used when a prot@2 is present in
> 
> s/prot/port/
> 
> > the DT. So when the display driver request a connector
> 
> s/request/requests/
> 
> > support both situations:
> > - connector created by bridge panel driver
> > - connector created by this driver
> > 
> > And on top, support that the display driver creates the connector,
> > which is the preferred setup.
> > 
> > Note: the bridge panel will use the connector type from the panel.
> > 
> > v2:
> >   - Merge connector and drm_panel_bridge patches
> >     and fix so we do not create two connectors (Laurent)
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 70 +++++++++++++++----------------
> >  1 file changed, 35 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index d86d7f06bebb..75a2cd792ccc 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -244,8 +244,8 @@ struct tc_data {
> >  	struct drm_dp_aux	aux;
> >  
> >  	struct drm_bridge	bridge;
> > +	struct drm_bridge	*panel_bridge;
> >  	struct drm_connector	connector;
> > -	struct drm_panel	*panel;
> >  
> >  	/* link settings */
> >  	struct tc_edp_link	link;
> > @@ -1234,13 +1234,6 @@ static int tc_stream_disable(struct tc_data *tc)
> >  	return 0;
> >  }
> >  
> > -static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> > -{
> > -	struct tc_data *tc = bridge_to_tc(bridge);
> > -
> > -	drm_panel_prepare(tc->panel);
> > -}
> > -
> >  static void tc_bridge_enable(struct drm_bridge *bridge)
> >  {
> >  	struct tc_data *tc = bridge_to_tc(bridge);
> > @@ -1264,8 +1257,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> >  		tc_main_link_disable(tc);
> >  		return;
> >  	}
> > -
> > -	drm_panel_enable(tc->panel);
> >  }
> >  
> >  static void tc_bridge_disable(struct drm_bridge *bridge)
> > @@ -1273,8 +1264,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> >  	struct tc_data *tc = bridge_to_tc(bridge);
> >  	int ret;
> >  
> > -	drm_panel_disable(tc->panel);
> > -
> >  	ret = tc_stream_disable(tc);
> >  	if (ret < 0)
> >  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> > @@ -1284,13 +1273,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> >  		dev_err(tc->dev, "main link disable error: %d\n", ret);
> >  }
> >  
> > -static void tc_bridge_post_disable(struct drm_bridge *bridge)
> > -{
> > -	struct tc_data *tc = bridge_to_tc(bridge);
> > -
> > -	drm_panel_unprepare(tc->panel);
> > -}
> > -
> >  static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> >  				 const struct drm_display_mode *mode,
> >  				 struct drm_display_mode *adj)
> > @@ -1354,9 +1336,11 @@ static int tc_connector_get_modes(struct drm_connector *connector)
> >  		return 0;
> >  	}
> >  
> > -	num_modes = drm_panel_get_modes(tc->panel, connector);
> > -	if (num_modes > 0)
> > -		return num_modes;
> > +	if (tc->panel_bridge) {
> > +		num_modes = drm_bridge_get_modes(tc->panel_bridge, connector);
> > +		if (num_modes > 0)
> > +			return num_modes;
> > +	}
> >  
> >  	edid = tc_get_edid(&tc->bridge, connector);
> >  	num_modes = drm_add_edid_modes(connector, edid);
> > @@ -1396,7 +1380,7 @@ tc_connector_detect(struct drm_connector *connector, bool force)
> >  	if (tc->hpd_pin >= 0)
> >  		return tc_bridge_detect(&tc->bridge);
> >  	else
> > -		if (tc->panel)
> > +		if (tc->panel_bridge)
> >  			return connector_status_connected;
> >  
> >  	return connector_status_unknown;
> > @@ -1419,16 +1403,23 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> >  	struct drm_device *drm = bridge->dev;
> >  	int ret;
> >  
> > -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -		DRM_ERROR("Fix bridge driver to make connector optional!");
> > -		return -EINVAL;
> > +	if (tc->panel_bridge) {
> > +		enum drm_bridge_attach_flags panel_flags;
> > +
> > +		/* If a connector is required then this driver shall create it */
> > +		panel_flags = flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> 
> Shouldn't this be
> 
> 		panel_flags = flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> 
> to ensure that the panel driver will not create a connector ?

Brown paper bag time so I can hide myself.
You are ofc right. Will fix and send a v5.
Same for other patch were the same pattern is used.

	Sam

> 
> > +		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> > +					&tc->bridge, panel_flags);
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> > +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> > +		return 0;
> > +
> >  	/* Create DP/eDP connector */
> >  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
> > -	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
> > -				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> > -				 DRM_MODE_CONNECTOR_DisplayPort);
> > +	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, tc->bridge.type);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1441,9 +1432,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
> >  					       DRM_CONNECTOR_POLL_DISCONNECT;
> >  	}
> >  
> > -	if (tc->panel)
> > -		drm_panel_attach(tc->panel, &tc->connector);
> > -
> >  	drm_display_info_set_bus_formats(&tc->connector.display_info,
> >  					 &bus_format, 1);
> >  	tc->connector.display_info.bus_flags =
> > @@ -1459,10 +1447,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
> >  	.attach = tc_bridge_attach,
> >  	.mode_valid = tc_mode_valid,
> >  	.mode_set = tc_bridge_mode_set,
> > -	.pre_enable = tc_bridge_pre_enable,
> >  	.enable = tc_bridge_enable,
> >  	.disable = tc_bridge_disable,
> > -	.post_disable = tc_bridge_post_disable,
> >  	.mode_fixup = tc_bridge_mode_fixup,
> >  	.detect = tc_bridge_detect,
> >  	.get_edid = tc_get_edid,
> > @@ -1555,6 +1541,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
> >  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  {
> >  	struct device *dev = &client->dev;
> > +	struct drm_panel *panel;
> >  	struct tc_data *tc;
> >  	int ret;
> >  
> > @@ -1565,10 +1552,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  	tc->dev = dev;
> >  
> >  	/* port@2 is the output port */
> > -	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
> > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
> >  	if (ret && ret != -ENODEV)
> >  		return ret;
> >  
> > +	if (panel) {
> > +		struct drm_bridge *panel_bridge;
> > +
> > +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +		if (IS_ERR(panel_bridge))
> > +			return PTR_ERR(panel_bridge);
> > +
> > +		tc->panel_bridge = panel_bridge;
> > +		tc->bridge.type = DRM_MODE_CONNECTOR_eDP;
> > +	} else {
> > +		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> > +	}
> > +
> >  	/* Shut down GPIO is optional */
> >  	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
> >  	if (IS_ERR(tc->sd_gpio))
> 
> -- 
> Regards,
> 
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 08/15] drm/bridge: parade-ps8622: add drm_panel_bridge support

[Sam Ravnborg]

Hi Laurent.

On Mon, Jul 27, 2020 at 12:54:08AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Sun, Jul 26, 2020 at 10:33:17PM +0200, Sam Ravnborg wrote:
> > Prepare the bridge driver for use in a chained setup by
> > replacing direct use of drm_panel with drm_panel_bridge support.
> > 
> > The connecter is now either created by the panel bridge or the display
> > driver. So all code for connector creation in this driver is no longer
> > relevant and thus dropped.
> > 
> > The connector code had some special polling handling:
> >     connector.polled = DRM_CONNECTOR_POLL_HPD;
> >     drm_helper_hpd_irq_event(ps8622->bridge.dev);
> > 
> > This code was most likely added to speed up detection of the connector.
> > If really needed then this functionality belongs somewhere else.
> > 
> > Note: the bridge panel will use the connector type from the panel.
> > 
> > v2:
> >   - Fix to avoid creating connector twice (Laurent)
> >   - Drop all connector code - defer to bridge panel
> >   - Use panel_bridge for local variable to align with other drivers
> >   - Set bridge.type to DRM_MODE_CONNECTOR_LVDS;
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >  drivers/gpu/drm/bridge/parade-ps8622.c | 100 ++++---------------------
> >  1 file changed, 13 insertions(+), 87 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> > index d789ea2a7fb9..614b19f0f1b7 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8622.c
> > @@ -42,10 +42,9 @@
> >  #endif
> >  
> >  struct ps8622_bridge {
> > -	struct drm_connector connector;
> >  	struct i2c_client *client;
> >  	struct drm_bridge bridge;
> > -	struct drm_panel *panel;
> > +	struct drm_bridge *panel_bridge;
> >  	struct regulator *v12;
> >  	struct backlight_device *bl;
> >  
> > @@ -64,12 +63,6 @@ static inline struct ps8622_bridge *
> >  	return container_of(bridge, struct ps8622_bridge, bridge);
> >  }
> >  
> > -static inline struct ps8622_bridge *
> > -		connector_to_ps8622(struct drm_connector *connector)
> > -{
> > -	return container_of(connector, struct ps8622_bridge, connector);
> > -}
> > -
> >  static int ps8622_set(struct i2c_client *client, u8 page, u8 reg, u8 val)
> >  {
> >  	int ret;
> > @@ -365,11 +358,6 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
> >  			DRM_ERROR("fails to enable ps8622->v12");
> >  	}
> >  
> > -	if (drm_panel_prepare(ps8622->panel)) {
> > -		DRM_ERROR("failed to prepare panel\n");
> > -		return;
> > -	}
> > -
> >  	gpiod_set_value(ps8622->gpio_slp, 1);
> >  
> >  	/*
> > @@ -399,24 +387,9 @@ static void ps8622_pre_enable(struct drm_bridge *bridge)
> >  	ps8622->enabled = true;
> >  }
> >  
> > -static void ps8622_enable(struct drm_bridge *bridge)
> > -{
> > -	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> > -
> > -	if (drm_panel_enable(ps8622->panel)) {
> > -		DRM_ERROR("failed to enable panel\n");
> > -		return;
> > -	}
> > -}
> > -
> >  static void ps8622_disable(struct drm_bridge *bridge)
> >  {
> > -	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> > -
> > -	if (drm_panel_disable(ps8622->panel)) {
> > -		DRM_ERROR("failed to disable panel\n");
> > -		return;
> > -	}
> > +	/* Delay after panel is disabled */
> >  	msleep(PS8622_PWMO_END_T12_MS);
> 
> I really don't understand why a delay would be needed here.
>
Neither do I. I was there in the original code and I have kept it.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thanks.

	Sam

> 
> >  }
> >  
> > @@ -436,11 +409,6 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
> >  	 */
> >  	gpiod_set_value(ps8622->gpio_slp, 0);
> >  
> > -	if (drm_panel_unprepare(ps8622->panel)) {
> > -		DRM_ERROR("failed to unprepare panel\n");
> > -		return;
> > -	}
> > -
> >  	if (ps8622->v12)
> >  		regulator_disable(ps8622->v12);
> >  
> > @@ -455,67 +423,17 @@ static void ps8622_post_disable(struct drm_bridge *bridge)
> >  	msleep(PS8622_POWER_OFF_T17_MS);
> >  }
> >  
> > -static int ps8622_get_modes(struct drm_connector *connector)
> > -{
> > -	struct ps8622_bridge *ps8622;
> > -
> > -	ps8622 = connector_to_ps8622(connector);
> > -
> > -	return drm_panel_get_modes(ps8622->panel, connector);
> > -}
> > -
> > -static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = {
> > -	.get_modes = ps8622_get_modes,
> > -};
> > -
> > -static const struct drm_connector_funcs ps8622_connector_funcs = {
> > -	.fill_modes = drm_helper_probe_single_connector_modes,
> > -	.destroy = drm_connector_cleanup,
> > -	.reset = drm_atomic_helper_connector_reset,
> > -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > -};
> > -
> >  static int ps8622_attach(struct drm_bridge *bridge,
> >  			 enum drm_bridge_attach_flags flags)
> >  {
> >  	struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge);
> > -	int ret;
> > -
> > -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -		DRM_ERROR("Fix bridge driver to make connector optional!");
> > -		return -EINVAL;
> > -	}
> >  
> > -	if (!bridge->encoder) {
> > -		DRM_ERROR("Parent encoder object not found");
> > -		return -ENODEV;
> > -	}
> > -
> > -	ps8622->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > -	ret = drm_connector_init(bridge->dev, &ps8622->connector,
> > -			&ps8622_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to initialize connector with drm\n");
> > -		return ret;
> > -	}
> > -	drm_connector_helper_add(&ps8622->connector,
> > -					&ps8622_connector_helper_funcs);
> > -	drm_connector_register(&ps8622->connector);
> > -	drm_connector_attach_encoder(&ps8622->connector,
> > -							bridge->encoder);
> > -
> > -	if (ps8622->panel)
> > -		drm_panel_attach(ps8622->panel, &ps8622->connector);
> > -
> > -	drm_helper_hpd_irq_event(ps8622->connector.dev);
> > -
> > -	return ret;
> > +	return drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge,
> > +				 &ps8622->bridge, flags);
> >  }
> >  
> >  static const struct drm_bridge_funcs ps8622_bridge_funcs = {
> >  	.pre_enable = ps8622_pre_enable,
> > -	.enable = ps8622_enable,
> >  	.disable = ps8622_disable,
> >  	.post_disable = ps8622_post_disable,
> >  	.attach = ps8622_attach,
> > @@ -533,16 +451,23 @@ static int ps8622_probe(struct i2c_client *client,
> >  {
> >  	struct device *dev = &client->dev;
> >  	struct ps8622_bridge *ps8622;
> > +	struct drm_bridge *panel_bridge;
> > +	struct drm_panel *panel;
> >  	int ret;
> >  
> >  	ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL);
> >  	if (!ps8622)
> >  		return -ENOMEM;
> >  
> > -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ps8622->panel, NULL);
> > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL);
> >  	if (ret)
> >  		return ret;
> >  
> > +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +	if (IS_ERR(panel_bridge))
> > +		return PTR_ERR(panel_bridge);
> > +
> > +	ps8622->panel_bridge = panel_bridge;
> >  	ps8622->client = client;
> >  
> >  	ps8622->v12 = devm_regulator_get(dev, "vdd12");
> > @@ -595,6 +520,7 @@ static int ps8622_probe(struct i2c_client *client,
> >  	}
> >  
> >  	ps8622->bridge.funcs = &ps8622_bridge_funcs;
> > +	ps8622->bridge.type = DRM_MODE_CONNECTOR_LVDS;
> >  	ps8622->bridge.of_node = dev->of_node;
> >  	drm_bridge_add(&ps8622->bridge);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 0/15] drm/bridge: support chained bridges + panel updates

[Sam Ravnborg]

Hi Laurent/all.

On Sun, Jul 26, 2020 at 10:33:09PM +0200, Sam Ravnborg wrote:
> The objective is that all bridge drivers shall support a chained setup
> connector creation is moved to the display drivers.
> This is just one step on this path.
> 
> The general approach for the bridge drivers:
> - Introduce bridge operations
> - Introduce use of panel bridge and make connector creation optional
> 
> v4:
>   - Dropped patch for ti-sn65dsi86. Await full conversion.
>   - Dropped patch for ti-tpd12s015. It was wrong (Laurent)
>   - Drop boe,hv070wsa-100 patch, it was applied
>   - Combined a few patches to fix connector created twice (Laurent)
>   - Fix memory leak in get_edid in several drivers (Laurent)
>   - Added patch to validate panel descriptions in panel-simple
>   - Set bridge.type in relevant drivers
>  
> v3:
>   - Rebase on top of drm-misc-next
>   - Address kbuild test robot feedback
>  
> v2:
>   - Updated bus_flags for boe,hv070wsa-100
>   - Collected r-b, but did not apply patches yet
>   - On the panel side the panel-simple driver gained a default
>     connector type for all the dumb panels that do not
>     include so in their description.
>     With this change panels always provide a connector type,
>     and we have the potential to drop most uses of
>     devm_drm_panel_bridge_add_typed().
>   - Added conversion of a few more bridge drivers
> 
> Patches can build but no run-time testing.
> So both test and review feedback appreciated!
> 
> 	Sam
> 
> Sam Ravnborg (15):
>       drm/panel: panel-simple: validate panel description
>       drm/panel: panel-simple: add default connector_type
>       drm/bridge: tc358764: drop drm_connector_(un)register
>       drm/bridge: tc358764: add drm_panel_bridge support
>       drm/bridge: tc358767: add detect bridge operation
>       drm/bridge: tc358767: add get_edid bridge operation
>       drm/bridge: tc358767: add drm_panel_bridge support
>       drm/bridge: parade-ps8622: add drm_panel_bridge support
>       drm/bridge: megachips: add helper to create connector
>       drm/bridge: megachips: get drm_device from bridge
>       drm/bridge: megachips: enable detect bridge operation
>       drm/bridge: megachips: add get_edid bridge operation
>       drm/bridge: megachips: make connector creation optional
>       drm/bridge: nxp-ptn3460: add get_edid bridge operation
>       drm/bridge: nxp-ptn3460: add drm_panel_bridge support

Laurent reviewed the full series - thanks!
I went ahead and applied the patches for drivers where all
patches was reviewed.

I will send a v5 soon for tc358767 and nxp-ptn3460 where I have fixed
my brown paper bag bugs
.
I am very happy Laurent spotted these before we applied the patches.
This also gives a good indication of the quality of the review.

	Sam

> 
>  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  97 +++++++++-------
>  drivers/gpu/drm/bridge/nxp-ptn3460.c               | 103 ++++++++---------
>  drivers/gpu/drm/bridge/parade-ps8622.c             | 100 +++-------------
>  drivers/gpu/drm/bridge/tc358764.c                  | 110 +++---------------
>  drivers/gpu/drm/bridge/tc358767.c                  | 126 +++++++++++----------
>  drivers/gpu/drm/panel/panel-simple.c               |  48 +++++++-
>  6 files changed, 242 insertions(+), 342 deletions(-)
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v4,04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Marek Szyprowski]

Hi Sam,

On 26.07.2020 22:33, Sam Ravnborg wrote:
> Prepare the tc358764 bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
>
> The bridge panel will use the connector type reported by the panel,
> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
>
> The tc358764 did not any additional info the the connector so the
> connector creation is passed to the bridge panel driver.
>
> v3:
>    - Merge with patch to make connector creation optional to avoid
>      creating two connectors (Laurent)
>    - Pass connector creation to bridge panel, as this bridge driver
>      did not add any extra info to the connector.
>    - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
>
> v2:
>    - Use PTR_ERR_OR_ZERO() (kbuild test robot)
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: kbuild test robot <lkp@intel.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I've noticed that this patch has been merged recently to linux-next. 
Sadly it causes regression on Samsung Exynos5250-based Arndale board.

It can be observed by the following warning during boot:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494 
drm_atomic_helper_connector_duplicate_state+0x60/0x68
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc2-00501-g1644127f83bc 
#1526
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c011184c>] (unwind_backtrace) from [<c010d250>] (show_stack+0x10/0x14)
[<c010d250>] (show_stack) from [<c0517ce4>] (dump_stack+0xbc/0xe8)
[<c0517ce4>] (dump_stack) from [<c01270a8>] (__warn+0xf0/0x108)
[<c01270a8>] (__warn) from [<c0127170>] (warn_slowpath_fmt+0xb0/0xb8)
[<c0127170>] (warn_slowpath_fmt) from [<c05e81f0>] 
(drm_atomic_helper_connector_duplicate_state+0x60/0x68)
[<c05e81f0>] (drm_atomic_helper_connector_duplicate_state) from 
[<c06014b8>] (drm_atomic_get_connector_state+0xfc/0x184)
[<c06014b8>] (drm_atomic_get_connector_state) from [<c0602238>] 
(__drm_atomic_helper_set_config+0x2a0/0x368)
[<c0602238>] (__drm_atomic_helper_set_config) from [<c06183b8>] 
(drm_client_modeset_commit_atomic+0x180/0x284)
[<c06183b8>] (drm_client_modeset_commit_atomic) from [<c061859c>] 
(drm_client_modeset_commit_locked+0x64/0x1cc)
[<c061859c>] (drm_client_modeset_commit_locked) from [<c0618728>] 
(drm_client_modeset_commit+0x24/0x40)
[<c0618728>] (drm_client_modeset_commit) from [<c05eb6b4>] 
(drm_fb_helper_restore_fbdev_mode_unlocked+0x50/0x94)
[<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from 
[<c05eb728>] (drm_fb_helper_set_par+0x30/0x5c)
[<c05eb728>] (drm_fb_helper_set_par) from [<c055dedc>] 
(fbcon_init+0x5c8/0x65c)
[<c055dedc>] (fbcon_init) from [<c05a8530>] (visual_init+0xc0/0x108)
[<c05a8530>] (visual_init) from [<c05aaca4>] 
(do_bind_con_driver+0x180/0x39c)
[<c05aaca4>] (do_bind_con_driver) from [<c05ab244>] 
(do_take_over_console+0x140/0x1cc)
[<c05ab244>] (do_take_over_console) from [<c055ac04>] 
(do_fbcon_takeover+0x84/0xe0)
[<c055ac04>] (do_fbcon_takeover) from [<c0553820>] 
(register_framebuffer+0x1cc/0x2dc)
[<c0553820>] (register_framebuffer) from [<c05eb19c>] 
(__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5e8)
[<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock) from 
[<c05d941c>] (drm_kms_helper_hotplug_event+0x24/0x30)
[<c05d941c>] (drm_kms_helper_hotplug_event) from [<c0628f74>] 
(exynos_dsi_host_attach+0x184/0x2d8)
[<c0628f74>] (exynos_dsi_host_attach) from [<c0634120>] 
(tc358764_probe+0x13c/0x1ac)
[<c0634120>] (tc358764_probe) from [<c064cce4>] (really_probe+0x200/0x48c)
[<c064cce4>] (really_probe) from [<c064d0d8>] 
(driver_probe_device+0x78/0x1fc)
[<c064d0d8>] (driver_probe_device) from [<c064d4c0>] 
(device_driver_attach+0x58/0x60)
[<c064d4c0>] (device_driver_attach) from [<c064d5a4>] 
(__driver_attach+0xdc/0x174)
[<c064d5a4>] (__driver_attach) from [<c064aaf0>] 
(bus_for_each_dev+0x68/0xb4)
[<c064aaf0>] (bus_for_each_dev) from [<c064be24>] 
(bus_add_driver+0x158/0x214)
[<c064be24>] (bus_add_driver) from [<c064e478>] (driver_register+0x78/0x110)
[<c064e478>] (driver_register) from [<c0102378>] 
(do_one_initcall+0x8c/0x424)
[<c0102378>] (do_one_initcall) from [<c1001158>] 
(kernel_init_freeable+0x190/0x204)
[<c1001158>] (kernel_init_freeable) from [<c0ab835c>] 
(kernel_init+0x8/0x118)
[<c0ab835c>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20)
Exception stack(0xee8ddfb0 to 0xee8ddff8)
dfa0:                                     00000000 00000000 00000000 
00000000
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
irq event stamp: 171647
hardirqs last  enabled at (171653): [<c019ec00>] vprintk_emit+0x2ac/0x2ec
hardirqs last disabled at (171658): [<c019eab8>] vprintk_emit+0x164/0x2ec
softirqs last  enabled at (171486): [<c010174c>] __do_softirq+0x50c/0x608
softirqs last disabled at (171473): [<c0130340>] irq_exit+0x168/0x16c
---[ end trace 33117a16f066466a ]---

Then calling modetest end with segmentation fault. I'm not able to check 
currently if there is anything on the display because of having only 
remote access to the board. If this is important I will try to ask 
someone to help checking at the board's display at the office.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: [v4,04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Sam Ravnborg]

Hi Marek.

On Thu, Aug 27, 2020 at 01:39:06PM +0200, Marek Szyprowski wrote:
> Hi Sam,
> 
> On 26.07.2020 22:33, Sam Ravnborg wrote:
> > Prepare the tc358764 bridge driver for use in a chained setup by
> > replacing direct use of drm_panel with drm_panel_bridge support.
> >
> > The bridge panel will use the connector type reported by the panel,
> > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
> >
> > The tc358764 did not any additional info the the connector so the
> > connector creation is passed to the bridge panel driver.
> >
> > v3:
> >    - Merge with patch to make connector creation optional to avoid
> >      creating two connectors (Laurent)
> >    - Pass connector creation to bridge panel, as this bridge driver
> >      did not add any extra info to the connector.
> >    - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
> >
> > v2:
> >    - Use PTR_ERR_OR_ZERO() (kbuild test robot)
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: kbuild test robot <lkp@intel.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I've noticed that this patch has been merged recently to linux-next. 
> Sadly it causes regression on Samsung Exynos5250-based Arndale board.

Thanks for reporting this!

I did not find time to focus on this bug this weekend. It is on my todo
list for the coming weekend.

Anything you could do to narrow down this a bit to help finding the root
cause?

Ideas:
- Trying to find out what part of the connector that cuases troubles
- Posting the full kernel boot log, to help identifying something.
  Bonus if we get a working and non-working log - so we can compare.
- Migrate exonys to the new model
  That would not fix the bug, so that would be a natural step 2
- Identify the exact code-patch in the exonys driver that is used.
  drm_bridge_attach() is called in several places
- And likely much more that I just forgot

Any help would be appreciated - I did not find the culprint from first
glance. I may still be obvious but I just failed to spot it.

	Sam

> 
> It can be observed by the following warning during boot:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494 
> drm_atomic_helper_connector_duplicate_state+0x60/0x68
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc2-00501-g1644127f83bc 
> #1526
> Hardware name: Samsung Exynos (Flattened Device Tree)
> [<c011184c>] (unwind_backtrace) from [<c010d250>] (show_stack+0x10/0x14)
> [<c010d250>] (show_stack) from [<c0517ce4>] (dump_stack+0xbc/0xe8)
> [<c0517ce4>] (dump_stack) from [<c01270a8>] (__warn+0xf0/0x108)
> [<c01270a8>] (__warn) from [<c0127170>] (warn_slowpath_fmt+0xb0/0xb8)
> [<c0127170>] (warn_slowpath_fmt) from [<c05e81f0>] 
> (drm_atomic_helper_connector_duplicate_state+0x60/0x68)
> [<c05e81f0>] (drm_atomic_helper_connector_duplicate_state) from 
> [<c06014b8>] (drm_atomic_get_connector_state+0xfc/0x184)
> [<c06014b8>] (drm_atomic_get_connector_state) from [<c0602238>] 
> (__drm_atomic_helper_set_config+0x2a0/0x368)
> [<c0602238>] (__drm_atomic_helper_set_config) from [<c06183b8>] 
> (drm_client_modeset_commit_atomic+0x180/0x284)
> [<c06183b8>] (drm_client_modeset_commit_atomic) from [<c061859c>] 
> (drm_client_modeset_commit_locked+0x64/0x1cc)
> [<c061859c>] (drm_client_modeset_commit_locked) from [<c0618728>] 
> (drm_client_modeset_commit+0x24/0x40)
> [<c0618728>] (drm_client_modeset_commit) from [<c05eb6b4>] 
> (drm_fb_helper_restore_fbdev_mode_unlocked+0x50/0x94)
> [<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from 
> [<c05eb728>] (drm_fb_helper_set_par+0x30/0x5c)
> [<c05eb728>] (drm_fb_helper_set_par) from [<c055dedc>] 
> (fbcon_init+0x5c8/0x65c)
> [<c055dedc>] (fbcon_init) from [<c05a8530>] (visual_init+0xc0/0x108)
> [<c05a8530>] (visual_init) from [<c05aaca4>] 
> (do_bind_con_driver+0x180/0x39c)
> [<c05aaca4>] (do_bind_con_driver) from [<c05ab244>] 
> (do_take_over_console+0x140/0x1cc)
> [<c05ab244>] (do_take_over_console) from [<c055ac04>] 
> (do_fbcon_takeover+0x84/0xe0)
> [<c055ac04>] (do_fbcon_takeover) from [<c0553820>] 
> (register_framebuffer+0x1cc/0x2dc)
> [<c0553820>] (register_framebuffer) from [<c05eb19c>] 
> (__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5e8)
> [<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock) from 
> [<c05d941c>] (drm_kms_helper_hotplug_event+0x24/0x30)
> [<c05d941c>] (drm_kms_helper_hotplug_event) from [<c0628f74>] 
> (exynos_dsi_host_attach+0x184/0x2d8)
> [<c0628f74>] (exynos_dsi_host_attach) from [<c0634120>] 
> (tc358764_probe+0x13c/0x1ac)
> [<c0634120>] (tc358764_probe) from [<c064cce4>] (really_probe+0x200/0x48c)
> [<c064cce4>] (really_probe) from [<c064d0d8>] 
> (driver_probe_device+0x78/0x1fc)
> [<c064d0d8>] (driver_probe_device) from [<c064d4c0>] 
> (device_driver_attach+0x58/0x60)
> [<c064d4c0>] (device_driver_attach) from [<c064d5a4>] 
> (__driver_attach+0xdc/0x174)
> [<c064d5a4>] (__driver_attach) from [<c064aaf0>] 
> (bus_for_each_dev+0x68/0xb4)
> [<c064aaf0>] (bus_for_each_dev) from [<c064be24>] 
> (bus_add_driver+0x158/0x214)
> [<c064be24>] (bus_add_driver) from [<c064e478>] (driver_register+0x78/0x110)
> [<c064e478>] (driver_register) from [<c0102378>] 
> (do_one_initcall+0x8c/0x424)
> [<c0102378>] (do_one_initcall) from [<c1001158>] 
> (kernel_init_freeable+0x190/0x204)
> [<c1001158>] (kernel_init_freeable) from [<c0ab835c>] 
> (kernel_init+0x8/0x118)
> [<c0ab835c>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20)
> Exception stack(0xee8ddfb0 to 0xee8ddff8)
> dfa0:                                     00000000 00000000 00000000 
> 00000000
> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
> 00000000
> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> irq event stamp: 171647
> hardirqs last  enabled at (171653): [<c019ec00>] vprintk_emit+0x2ac/0x2ec
> hardirqs last disabled at (171658): [<c019eab8>] vprintk_emit+0x164/0x2ec
> softirqs last  enabled at (171486): [<c010174c>] __do_softirq+0x50c/0x608
> softirqs last disabled at (171473): [<c0130340>] irq_exit+0x168/0x16c
> ---[ end trace 33117a16f066466a ]---
> 
> Then calling modetest end with segmentation fault. I'm not able to check 
> currently if there is anything on the display because of having only 
> remote access to the board. If this is important I will try to ask 
> someone to help checking at the board's display at the office.
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 03/15] drm/bridge: tc358764: drop drm_connector_(un)register

[Andrzej Hajda]

Hi Sam,

On 26.07.2020 22:33, Sam Ravnborg wrote:
> Drop drm_connector handling that is not needed:
>
> - drm_dev_register() in the display controller driver takes
>    care of registering connectors.
>    So the call to drm_connector_register() call is not needed in the bridge
>    driver.
>
> - Use of drm_connector_unregister() is only required for drivers that
>    explicit have called drm_dev_register.
>
> - The reference counting using drm_connector_put() is likewise not needed.


All three statements above are true in case of statically allocated 
connectors.

Here the connector is allocated dynamically, so it must be 
registered/deregistered by the driver itself.


Regards

Andrzej


>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>   drivers/gpu/drm/bridge/tc358764.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
> index 5ac1430fab04..a277739fab58 100644
> --- a/drivers/gpu/drm/bridge/tc358764.c
> +++ b/drivers/gpu/drm/bridge/tc358764.c
> @@ -375,7 +375,6 @@ static int tc358764_attach(struct drm_bridge *bridge,
>   	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
>   	drm_panel_attach(ctx->panel, &ctx->connector);
>   	ctx->connector.funcs->reset(&ctx->connector);
> -	drm_connector_register(&ctx->connector);
>   	return 0;
>   }
> @@ -384,10 +383,8 @@ static void tc358764_detach(struct drm_bridge *bridge)
>   {
>   	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>   
> -	drm_connector_unregister(&ctx->connector);
>   	drm_panel_detach(ctx->panel);
>   	ctx->panel = NULL;
> -	drm_connector_put(&ctx->connector);
>   }
>   
>   static const struct drm_bridge_funcs tc358764_bridge_funcs = {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v4,04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Andrzej Hajda]

Hi Sam,

On 30.08.2020 22:42, Sam Ravnborg wrote:
> Hi Marek.
>
> On Thu, Aug 27, 2020 at 01:39:06PM +0200, Marek Szyprowski wrote:
>> Hi Sam,
>>
>> On 26.07.2020 22:33, Sam Ravnborg wrote:
>>> Prepare the tc358764 bridge driver for use in a chained setup by
>>> replacing direct use of drm_panel with drm_panel_bridge support.
>>>
>>> The bridge panel will use the connector type reported by the panel,
>>> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
>>>
>>> The tc358764 did not any additional info the the connector so the
>>> connector creation is passed to the bridge panel driver.
>>>
>>> v3:
>>>     - Merge with patch to make connector creation optional to avoid
>>>       creating two connectors (Laurent)
>>>     - Pass connector creation to bridge panel, as this bridge driver
>>>       did not add any extra info to the connector.
>>>     - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
>>>
>>> v2:
>>>     - Use PTR_ERR_OR_ZERO() (kbuild test robot)
>>>
>>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: kbuild test robot <lkp@intel.com>
>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> I've noticed that this patch has been merged recently to linux-next.
>> Sadly it causes regression on Samsung Exynos5250-based Arndale board.
> Thanks for reporting this!
>
> I did not find time to focus on this bug this weekend. It is on my todo
> list for the coming weekend.
>
> Anything you could do to narrow down this a bit to help finding the root
> cause?
>
> Ideas:
> - Trying to find out what part of the connector that cuases troubles
> - Posting the full kernel boot log, to help identifying something.
>    Bonus if we get a working and non-working log - so we can compare.
> - Migrate exonys to the new model
>    That would not fix the bug, so that would be a natural step 2
> - Identify the exact code-patch in the exonys driver that is used.
>    drm_bridge_attach() is called in several places
> - And likely much more that I just forgot


dmesg WARN suggest there is no connector registered and it can be true.

I guess drm_panel_bridge does not support dynamic connector registration,

so if the bridge appears after drm dev is created it will not be 
properly registered.

I will try look closer at the patch but I guess the above can be the 
main reason.


Regards

Andrzej


>
> Any help would be appreciated - I did not find the culprint from first
> glance. I may still be obvious but I just failed to spot it.
>
> 	Sam
>
>> It can be observed by the following warning during boot:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494
>> drm_atomic_helper_connector_duplicate_state+0x60/0x68
>> Modules linked in:
>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc2-00501-g1644127f83bc
>> #1526
>> Hardware name: Samsung Exynos (Flattened Device Tree)
>> [<c011184c>] (unwind_backtrace) from [<c010d250>] (show_stack+0x10/0x14)
>> [<c010d250>] (show_stack) from [<c0517ce4>] (dump_stack+0xbc/0xe8)
>> [<c0517ce4>] (dump_stack) from [<c01270a8>] (__warn+0xf0/0x108)
>> [<c01270a8>] (__warn) from [<c0127170>] (warn_slowpath_fmt+0xb0/0xb8)
>> [<c0127170>] (warn_slowpath_fmt) from [<c05e81f0>]
>> (drm_atomic_helper_connector_duplicate_state+0x60/0x68)
>> [<c05e81f0>] (drm_atomic_helper_connector_duplicate_state) from
>> [<c06014b8>] (drm_atomic_get_connector_state+0xfc/0x184)
>> [<c06014b8>] (drm_atomic_get_connector_state) from [<c0602238>]
>> (__drm_atomic_helper_set_config+0x2a0/0x368)
>> [<c0602238>] (__drm_atomic_helper_set_config) from [<c06183b8>]
>> (drm_client_modeset_commit_atomic+0x180/0x284)
>> [<c06183b8>] (drm_client_modeset_commit_atomic) from [<c061859c>]
>> (drm_client_modeset_commit_locked+0x64/0x1cc)
>> [<c061859c>] (drm_client_modeset_commit_locked) from [<c0618728>]
>> (drm_client_modeset_commit+0x24/0x40)
>> [<c0618728>] (drm_client_modeset_commit) from [<c05eb6b4>]
>> (drm_fb_helper_restore_fbdev_mode_unlocked+0x50/0x94)
>> [<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from
>> [<c05eb728>] (drm_fb_helper_set_par+0x30/0x5c)
>> [<c05eb728>] (drm_fb_helper_set_par) from [<c055dedc>]
>> (fbcon_init+0x5c8/0x65c)
>> [<c055dedc>] (fbcon_init) from [<c05a8530>] (visual_init+0xc0/0x108)
>> [<c05a8530>] (visual_init) from [<c05aaca4>]
>> (do_bind_con_driver+0x180/0x39c)
>> [<c05aaca4>] (do_bind_con_driver) from [<c05ab244>]
>> (do_take_over_console+0x140/0x1cc)
>> [<c05ab244>] (do_take_over_console) from [<c055ac04>]
>> (do_fbcon_takeover+0x84/0xe0)
>> [<c055ac04>] (do_fbcon_takeover) from [<c0553820>]
>> (register_framebuffer+0x1cc/0x2dc)
>> [<c0553820>] (register_framebuffer) from [<c05eb19c>]
>> (__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5e8)
>> [<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock) from
>> [<c05d941c>] (drm_kms_helper_hotplug_event+0x24/0x30)
>> [<c05d941c>] (drm_kms_helper_hotplug_event) from [<c0628f74>]
>> (exynos_dsi_host_attach+0x184/0x2d8)
>> [<c0628f74>] (exynos_dsi_host_attach) from [<c0634120>]
>> (tc358764_probe+0x13c/0x1ac)
>> [<c0634120>] (tc358764_probe) from [<c064cce4>] (really_probe+0x200/0x48c)
>> [<c064cce4>] (really_probe) from [<c064d0d8>]
>> (driver_probe_device+0x78/0x1fc)
>> [<c064d0d8>] (driver_probe_device) from [<c064d4c0>]
>> (device_driver_attach+0x58/0x60)
>> [<c064d4c0>] (device_driver_attach) from [<c064d5a4>]
>> (__driver_attach+0xdc/0x174)
>> [<c064d5a4>] (__driver_attach) from [<c064aaf0>]
>> (bus_for_each_dev+0x68/0xb4)
>> [<c064aaf0>] (bus_for_each_dev) from [<c064be24>]
>> (bus_add_driver+0x158/0x214)
>> [<c064be24>] (bus_add_driver) from [<c064e478>] (driver_register+0x78/0x110)
>> [<c064e478>] (driver_register) from [<c0102378>]
>> (do_one_initcall+0x8c/0x424)
>> [<c0102378>] (do_one_initcall) from [<c1001158>]
>> (kernel_init_freeable+0x190/0x204)
>> [<c1001158>] (kernel_init_freeable) from [<c0ab835c>]
>> (kernel_init+0x8/0x118)
>> [<c0ab835c>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20)
>> Exception stack(0xee8ddfb0 to 0xee8ddff8)
>> dfa0:                                     00000000 00000000 00000000
>> 00000000
>> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 00000000
>> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> irq event stamp: 171647
>> hardirqs last  enabled at (171653): [<c019ec00>] vprintk_emit+0x2ac/0x2ec
>> hardirqs last disabled at (171658): [<c019eab8>] vprintk_emit+0x164/0x2ec
>> softirqs last  enabled at (171486): [<c010174c>] __do_softirq+0x50c/0x608
>> softirqs last disabled at (171473): [<c0130340>] irq_exit+0x168/0x16c
>> ---[ end trace 33117a16f066466a ]---
>>
>> Then calling modetest end with segmentation fault. I'm not able to check
>> currently if there is anything on the display because of having only
>> remote access to the board. If this is important I will try to ask
>> someone to help checking at the board's display at the office.
>>
>> Best regards
>> -- 
>> Marek Szyprowski, PhD
>> Samsung R&D Institute Poland
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=4965500c-14b1ec64-4964db43-0cc47a3356b2-f40ef8e76ffb9eeb&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Andrzej Hajda]

On 26.07.2020 22:33, Sam Ravnborg wrote:
> Prepare the tc358764 bridge driver for use in a chained setup by
> replacing direct use of drm_panel with drm_panel_bridge support.
>
> The bridge panel will use the connector type reported by the panel,
> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
>
> The tc358764 did not any additional info the the connector so the
> connector creation is passed to the bridge panel driver.
>
> v3:
>    - Merge with patch to make connector creation optional to avoid
>      creating two connectors (Laurent)
>    - Pass connector creation to bridge panel, as this bridge driver
>      did not add any extra info to the connector.
>    - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
>
> v2:
>    - Use PTR_ERR_OR_ZERO() (kbuild test robot)
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: kbuild test robot <lkp@intel.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>   drivers/gpu/drm/bridge/tc358764.c | 107 +++++-------------------------
>   1 file changed, 16 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
> index a277739fab58..fdde4cfdc724 100644
> --- a/drivers/gpu/drm/bridge/tc358764.c
> +++ b/drivers/gpu/drm/bridge/tc358764.c
> @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = {
>   struct tc358764 {
>   	struct device *dev;
>   	struct drm_bridge bridge;
> -	struct drm_connector connector;
>   	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
>   	struct gpio_desc *gpio_reset;
> -	struct drm_panel *panel;
> +	struct drm_bridge *panel_bridge;
>   	int error;
>   };
>   
> @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
>   	return container_of(bridge, struct tc358764, bridge);
>   }
>   
> -static inline
> -struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct tc358764, connector);
> -}
> -
>   static int tc358764_init(struct tc358764 *ctx)
>   {
>   	u32 v = 0;
> @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx)
>   	usleep_range(1000, 2000);
>   }
>   
> -static int tc358764_get_modes(struct drm_connector *connector)
> -{
> -	struct tc358764 *ctx = connector_to_tc358764(connector);
> -
> -	return drm_panel_get_modes(ctx->panel, connector);
> -}
> -
> -static const
> -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
> -	.get_modes = tc358764_get_modes,
> -};
> -
> -static const struct drm_connector_funcs tc358764_connector_funcs = {
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static void tc358764_disable(struct drm_bridge *bridge)
> -{
> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> -	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
> -
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
> -}
> -
>   static void tc358764_post_disable(struct drm_bridge *bridge)
>   {
>   	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>   	int ret;
>   
> -	ret = drm_panel_unprepare(ctx->panel);
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);


Using this bridge_panel thing you reverse order of hw 
initialization/de-initialization, this is incorrect.

For example:

- panel_unprepare should be called before tc35* turn off,

- panel_prepare should be called after tc35* on.

This is why I avoid the whole "bridge chaining" - it enforces ridiculous 
order of initialization.


>   	tc358764_reset(ctx);
>   	usleep_range(10000, 15000);
>   	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
>   	ret = tc358764_init(ctx);
>   	if (ret < 0)
>   		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> -	ret = drm_panel_prepare(ctx->panel);
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
> -}
> -
> -static void tc358764_enable(struct drm_bridge *bridge)
> -{
> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> -	int ret = drm_panel_enable(ctx->panel);
> -
> -	if (ret < 0)
> -		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
>   }
>   
>   static int tc358764_attach(struct drm_bridge *bridge,
>   			   enum drm_bridge_attach_flags flags)
> -{
> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> -	struct drm_device *drm = bridge->dev;
> -	int ret;
> -
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		DRM_ERROR("Fix bridge driver to make connector optional!");
> -		return -EINVAL;
> -	}
> -
> -	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
> -	ret = drm_connector_init(drm, &ctx->connector,
> -				 &tc358764_connector_funcs,
> -				 DRM_MODE_CONNECTOR_LVDS);
> -	if (ret) {
> -		DRM_ERROR("Failed to initialize connector\n");
> -		return ret;
> -	}
> -
> -	drm_connector_helper_add(&ctx->connector,
> -				 &tc358764_connector_helper_funcs);
> -	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
> -	drm_panel_attach(ctx->panel, &ctx->connector);
> -	ctx->connector.funcs->reset(&ctx->connector);


I guess lack of calling .reset here is direct cause of WARN reported by 
Marek.


Summarizing my findings:

1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge 
chaining" which has crazy assumption that order of hw initalization in 
the display chain follows the same fixed order of calls for all hw.

2. tc35* bridge allocates/deallocates connector dynamically - to safely 
handle drivers load/unload, and to avoid multiple deferred probe issues 
, drm_panel_bridge does not support it.


This and previous patch violates both points.


Regards

Andrzej


> -
> -	return 0;
> -}
> -
> -static void tc358764_detach(struct drm_bridge *bridge)
>   {
>   	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>   
> -	drm_panel_detach(ctx->panel);
> -	ctx->panel = NULL;
> +	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
> +				 bridge, flags);
>   }
>   
>   static const struct drm_bridge_funcs tc358764_bridge_funcs = {
> -	.disable = tc358764_disable,
>   	.post_disable = tc358764_post_disable,
> -	.enable = tc358764_enable,
>   	.pre_enable = tc358764_pre_enable,
>   	.attach = tc358764_attach,
> -	.detach = tc358764_detach,
>   };
>   
>   static int tc358764_parse_dt(struct tc358764 *ctx)
>   {
> +	struct drm_bridge *panel_bridge;
>   	struct device *dev = ctx->dev;
> +	struct drm_panel *panel;
>   	int ret;
>   
>   	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
>   		return PTR_ERR(ctx->gpio_reset);
>   	}
>   
> -	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
> -					  NULL);
> -	if (ret && ret != -EPROBE_DEFER)
> -		dev_err(dev, "cannot find panel (%d)\n", ret);
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> +	if (ret)
> +		return ret;
>   
> -	return ret;
> +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +
> +	if (IS_ERR(panel_bridge))
> +		return PTR_ERR(panel_bridge);
> +
> +	ctx->panel_bridge = panel_bridge;
> +	return 0;
>   }
>   
>   static int tc358764_configure_regulators(struct tc358764 *ctx)
> @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
>   		return ret;
>   
>   	ctx->bridge.funcs = &tc358764_bridge_funcs;
> +	ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>   	ctx->bridge.of_node = dev->of_node;
>   
>   	drm_bridge_add(&ctx->bridge);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Laurent Pinchart]

Hi Andrzej,

On Thu, Sep 03, 2020 at 11:40:58AM +0200, Andrzej Hajda wrote:
> On 26.07.2020 22:33, Sam Ravnborg wrote:
> > Prepare the tc358764 bridge driver for use in a chained setup by
> > replacing direct use of drm_panel with drm_panel_bridge support.
> >
> > The bridge panel will use the connector type reported by the panel,
> > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
> >
> > The tc358764 did not any additional info the the connector so the
> > connector creation is passed to the bridge panel driver.
> >
> > v3:
> >    - Merge with patch to make connector creation optional to avoid
> >      creating two connectors (Laurent)
> >    - Pass connector creation to bridge panel, as this bridge driver
> >      did not add any extra info to the connector.
> >    - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
> >
> > v2:
> >    - Use PTR_ERR_OR_ZERO() (kbuild test robot)
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: kbuild test robot <lkp@intel.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >   drivers/gpu/drm/bridge/tc358764.c | 107 +++++-------------------------
> >   1 file changed, 16 insertions(+), 91 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
> > index a277739fab58..fdde4cfdc724 100644
> > --- a/drivers/gpu/drm/bridge/tc358764.c
> > +++ b/drivers/gpu/drm/bridge/tc358764.c
> > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = {
> >   struct tc358764 {
> >   	struct device *dev;
> >   	struct drm_bridge bridge;
> > -	struct drm_connector connector;
> >   	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
> >   	struct gpio_desc *gpio_reset;
> > -	struct drm_panel *panel;
> > +	struct drm_bridge *panel_bridge;
> >   	int error;
> >   };
> >   
> > @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
> >   	return container_of(bridge, struct tc358764, bridge);
> >   }
> >   
> > -static inline
> > -struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
> > -{
> > -	return container_of(connector, struct tc358764, connector);
> > -}
> > -
> >   static int tc358764_init(struct tc358764 *ctx)
> >   {
> >   	u32 v = 0;
> > @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx)
> >   	usleep_range(1000, 2000);
> >   }
> >   
> > -static int tc358764_get_modes(struct drm_connector *connector)
> > -{
> > -	struct tc358764 *ctx = connector_to_tc358764(connector);
> > -
> > -	return drm_panel_get_modes(ctx->panel, connector);
> > -}
> > -
> > -static const
> > -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
> > -	.get_modes = tc358764_get_modes,
> > -};
> > -
> > -static const struct drm_connector_funcs tc358764_connector_funcs = {
> > -	.fill_modes = drm_helper_probe_single_connector_modes,
> > -	.destroy = drm_connector_cleanup,
> > -	.reset = drm_atomic_helper_connector_reset,
> > -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > -};
> > -
> > -static void tc358764_disable(struct drm_bridge *bridge)
> > -{
> > -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> > -	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
> > -
> > -	if (ret < 0)
> > -		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
> > -}
> > -
> >   static void tc358764_post_disable(struct drm_bridge *bridge)
> >   {
> >   	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >   	int ret;
> >   
> > -	ret = drm_panel_unprepare(ctx->panel);
> > -	if (ret < 0)
> > -		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
> 
> 
> Using this bridge_panel thing you reverse order of hw 
> initialization/de-initialization, this is incorrect.
> 
> For example:
> 
> - panel_unprepare should be called before tc35* turn off,
> 
> - panel_prepare should be called after tc35* on.
> 
> This is why I avoid the whole "bridge chaining" - it enforces ridiculous 
> order of initialization.
> 
> 
> >   	tc358764_reset(ctx);
> >   	usleep_range(10000, 15000);
> >   	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
> >   	ret = tc358764_init(ctx);
> >   	if (ret < 0)
> >   		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> > -	ret = drm_panel_prepare(ctx->panel);
> > -	if (ret < 0)
> > -		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
> > -}
> > -
> > -static void tc358764_enable(struct drm_bridge *bridge)
> > -{
> > -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> > -	int ret = drm_panel_enable(ctx->panel);
> > -
> > -	if (ret < 0)
> > -		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
> >   }
> >   
> >   static int tc358764_attach(struct drm_bridge *bridge,
> >   			   enum drm_bridge_attach_flags flags)
> > -{
> > -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> > -	struct drm_device *drm = bridge->dev;
> > -	int ret;
> > -
> > -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -		DRM_ERROR("Fix bridge driver to make connector optional!");
> > -		return -EINVAL;
> > -	}
> > -
> > -	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > -	ret = drm_connector_init(drm, &ctx->connector,
> > -				 &tc358764_connector_funcs,
> > -				 DRM_MODE_CONNECTOR_LVDS);
> > -	if (ret) {
> > -		DRM_ERROR("Failed to initialize connector\n");
> > -		return ret;
> > -	}
> > -
> > -	drm_connector_helper_add(&ctx->connector,
> > -				 &tc358764_connector_helper_funcs);
> > -	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
> > -	drm_panel_attach(ctx->panel, &ctx->connector);
> > -	ctx->connector.funcs->reset(&ctx->connector);
> 
> 
> I guess lack of calling .reset here is direct cause of WARN reported by 
> Marek.
> 
> 
> Summarizing my findings:
> 
> 1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge 
> chaining" which has crazy assumption that order of hw initalization in 
> the display chain follows the same fixed order of calls for all hw.

This would need to be addressed in the bridge core. I don't want to go
back to manual chaining of operations, that opens the door to creating
incompatibilities between bridges and display controllers. The pre/post
enable/disable operations probably need to be better defined, and if a
sink requires a smaller granularity, then new operations need to be
added.

> 2. tc35* bridge allocates/deallocates connector dynamically - to safely 
> handle drivers load/unload, and to avoid multiple deferred probe issues 
> , drm_panel_bridge does not support it.
> 
> This and previous patch violates both points.
> 
> > -
> > -	return 0;
> > -}
> > -
> > -static void tc358764_detach(struct drm_bridge *bridge)
> >   {
> >   	struct tc358764 *ctx = bridge_to_tc358764(bridge);
> >   
> > -	drm_panel_detach(ctx->panel);
> > -	ctx->panel = NULL;
> > +	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
> > +				 bridge, flags);
> >   }
> >   
> >   static const struct drm_bridge_funcs tc358764_bridge_funcs = {
> > -	.disable = tc358764_disable,
> >   	.post_disable = tc358764_post_disable,
> > -	.enable = tc358764_enable,
> >   	.pre_enable = tc358764_pre_enable,
> >   	.attach = tc358764_attach,
> > -	.detach = tc358764_detach,
> >   };
> >   
> >   static int tc358764_parse_dt(struct tc358764 *ctx)
> >   {
> > +	struct drm_bridge *panel_bridge;
> >   	struct device *dev = ctx->dev;
> > +	struct drm_panel *panel;
> >   	int ret;
> >   
> >   	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
> >   		return PTR_ERR(ctx->gpio_reset);
> >   	}
> >   
> > -	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
> > -					  NULL);
> > -	if (ret && ret != -EPROBE_DEFER)
> > -		dev_err(dev, "cannot find panel (%d)\n", ret);
> > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > +	if (ret)
> > +		return ret;
> >   
> > -	return ret;
> > +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +
> > +	if (IS_ERR(panel_bridge))
> > +		return PTR_ERR(panel_bridge);
> > +
> > +	ctx->panel_bridge = panel_bridge;
> > +	return 0;
> >   }
> >   
> >   static int tc358764_configure_regulators(struct tc358764 *ctx)
> > @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
> >   		return ret;
> >   
> >   	ctx->bridge.funcs = &tc358764_bridge_funcs;
> > +	ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;
> >   	ctx->bridge.of_node = dev->of_node;
> >   
> >   	drm_bridge_add(&ctx->bridge);

-- 
Regards,

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

Re: [PATCH v4 04/15] drm/bridge: tc358764: add drm_panel_bridge support

[Andrzej Hajda]

Hi Laurent,

On 03.09.2020 11:59, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Thu, Sep 03, 2020 at 11:40:58AM +0200, Andrzej Hajda wrote:
>> On 26.07.2020 22:33, Sam Ravnborg wrote:
>>> Prepare the tc358764 bridge driver for use in a chained setup by
>>> replacing direct use of drm_panel with drm_panel_bridge support.
>>>
>>> The bridge panel will use the connector type reported by the panel,
>>> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS.
>>>
>>> The tc358764 did not any additional info the the connector so the
>>> connector creation is passed to the bridge panel driver.
>>>
>>> v3:
>>>     - Merge with patch to make connector creation optional to avoid
>>>       creating two connectors (Laurent)
>>>     - Pass connector creation to bridge panel, as this bridge driver
>>>       did not add any extra info to the connector.
>>>     - Set bridge.type to DRM_MODE_CONNECTOR_LVDS.
>>>
>>> v2:
>>>     - Use PTR_ERR_OR_ZERO() (kbuild test robot)
>>>
>>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: kbuild test robot <lkp@intel.com>
>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
>>> ---
>>>    drivers/gpu/drm/bridge/tc358764.c | 107 +++++-------------------------
>>>    1 file changed, 16 insertions(+), 91 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
>>> index a277739fab58..fdde4cfdc724 100644
>>> --- a/drivers/gpu/drm/bridge/tc358764.c
>>> +++ b/drivers/gpu/drm/bridge/tc358764.c
>>> @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = {
>>>    struct tc358764 {
>>>    	struct device *dev;
>>>    	struct drm_bridge bridge;
>>> -	struct drm_connector connector;
>>>    	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
>>>    	struct gpio_desc *gpio_reset;
>>> -	struct drm_panel *panel;
>>> +	struct drm_bridge *panel_bridge;
>>>    	int error;
>>>    };
>>>    
>>> @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge)
>>>    	return container_of(bridge, struct tc358764, bridge);
>>>    }
>>>    
>>> -static inline
>>> -struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
>>> -{
>>> -	return container_of(connector, struct tc358764, connector);
>>> -}
>>> -
>>>    static int tc358764_init(struct tc358764 *ctx)
>>>    {
>>>    	u32 v = 0;
>>> @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx)
>>>    	usleep_range(1000, 2000);
>>>    }
>>>    
>>> -static int tc358764_get_modes(struct drm_connector *connector)
>>> -{
>>> -	struct tc358764 *ctx = connector_to_tc358764(connector);
>>> -
>>> -	return drm_panel_get_modes(ctx->panel, connector);
>>> -}
>>> -
>>> -static const
>>> -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
>>> -	.get_modes = tc358764_get_modes,
>>> -};
>>> -
>>> -static const struct drm_connector_funcs tc358764_connector_funcs = {
>>> -	.fill_modes = drm_helper_probe_single_connector_modes,
>>> -	.destroy = drm_connector_cleanup,
>>> -	.reset = drm_atomic_helper_connector_reset,
>>> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> -};
>>> -
>>> -static void tc358764_disable(struct drm_bridge *bridge)
>>> -{
>>> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> -	int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
>>> -
>>> -	if (ret < 0)
>>> -		dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
>>> -}
>>> -
>>>    static void tc358764_post_disable(struct drm_bridge *bridge)
>>>    {
>>>    	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>    	int ret;
>>>    
>>> -	ret = drm_panel_unprepare(ctx->panel);
>>> -	if (ret < 0)
>>> -		dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
>>
>> Using this bridge_panel thing you reverse order of hw
>> initialization/de-initialization, this is incorrect.
>>
>> For example:
>>
>> - panel_unprepare should be called before tc35* turn off,
>>
>> - panel_prepare should be called after tc35* on.
>>
>> This is why I avoid the whole "bridge chaining" - it enforces ridiculous
>> order of initialization.
>>
>>
>>>    	tc358764_reset(ctx);
>>>    	usleep_range(10000, 15000);
>>>    	ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>> @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge)
>>>    	ret = tc358764_init(ctx);
>>>    	if (ret < 0)
>>>    		dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
>>> -	ret = drm_panel_prepare(ctx->panel);
>>> -	if (ret < 0)
>>> -		dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
>>> -}
>>> -
>>> -static void tc358764_enable(struct drm_bridge *bridge)
>>> -{
>>> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> -	int ret = drm_panel_enable(ctx->panel);
>>> -
>>> -	if (ret < 0)
>>> -		dev_err(ctx->dev, "error enabling panel (%d)\n", ret);
>>>    }
>>>    
>>>    static int tc358764_attach(struct drm_bridge *bridge,
>>>    			   enum drm_bridge_attach_flags flags)
>>> -{
>>> -	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>> -	struct drm_device *drm = bridge->dev;
>>> -	int ret;
>>> -
>>> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>>> -		DRM_ERROR("Fix bridge driver to make connector optional!");
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	ctx->connector.polled = DRM_CONNECTOR_POLL_HPD;
>>> -	ret = drm_connector_init(drm, &ctx->connector,
>>> -				 &tc358764_connector_funcs,
>>> -				 DRM_MODE_CONNECTOR_LVDS);
>>> -	if (ret) {
>>> -		DRM_ERROR("Failed to initialize connector\n");
>>> -		return ret;
>>> -	}
>>> -
>>> -	drm_connector_helper_add(&ctx->connector,
>>> -				 &tc358764_connector_helper_funcs);
>>> -	drm_connector_attach_encoder(&ctx->connector, bridge->encoder);
>>> -	drm_panel_attach(ctx->panel, &ctx->connector);
>>> -	ctx->connector.funcs->reset(&ctx->connector);
>>
>> I guess lack of calling .reset here is direct cause of WARN reported by
>> Marek.
>>
>>
>> Summarizing my findings:
>>
>> 1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge
>> chaining" which has crazy assumption that order of hw initalization in
>> the display chain follows the same fixed order of calls for all hw.
> This would need to be addressed in the bridge core. I don't want to go
> back to manual chaining of operations, that opens the door to creating
> incompatibilities between bridges and display controllers. The pre/post
> enable/disable operations probably need to be better defined, and if a
> sink requires a smaller granularity, then new operations need to be
> added.


Bigger granularity of operations is source of incompatibilities. We have 
already multiple issues with only two operations - pre_enable, 
post_enable, developers are confused what put where, especially if they 
can test bridge driver only with only fixed chain determined by the 
platform they are working on.

Adding new operations will make things worse. On the other hand explicit 
calling ops of downstream device has following advantages:

- we see explicitly how the bridge and its sink is initialized, it is 
even easier to compare it with docs,

- in case the stream is split or duplicated to two or more sinks, and/or 
eventually joined then later, it is much easier and straightforward to 
program it with explicit ops. With bridge chaining it is impossible 
without workarounds - all the cases with dual DSI etc.


Regards

Andrzej


>> 2. tc35* bridge allocates/deallocates connector dynamically - to safely
>> handle drivers load/unload, and to avoid multiple deferred probe issues
>> , drm_panel_bridge does not support it.
>>
>> This and previous patch violates both points.
>>
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static void tc358764_detach(struct drm_bridge *bridge)
>>>    {
>>>    	struct tc358764 *ctx = bridge_to_tc358764(bridge);
>>>    
>>> -	drm_panel_detach(ctx->panel);
>>> -	ctx->panel = NULL;
>>> +	return drm_bridge_attach(bridge->encoder, ctx->panel_bridge,
>>> +				 bridge, flags);
>>>    }
>>>    
>>>    static const struct drm_bridge_funcs tc358764_bridge_funcs = {
>>> -	.disable = tc358764_disable,
>>>    	.post_disable = tc358764_post_disable,
>>> -	.enable = tc358764_enable,
>>>    	.pre_enable = tc358764_pre_enable,
>>>    	.attach = tc358764_attach,
>>> -	.detach = tc358764_detach,
>>>    };
>>>    
>>>    static int tc358764_parse_dt(struct tc358764 *ctx)
>>>    {
>>> +	struct drm_bridge *panel_bridge;
>>>    	struct device *dev = ctx->dev;
>>> +	struct drm_panel *panel;
>>>    	int ret;
>>>    
>>>    	ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>>> @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx)
>>>    		return PTR_ERR(ctx->gpio_reset);
>>>    	}
>>>    
>>> -	ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel,
>>> -					  NULL);
>>> -	if (ret && ret != -EPROBE_DEFER)
>>> -		dev_err(dev, "cannot find panel (%d)\n", ret);
>>> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
>>> +	if (ret)
>>> +		return ret;
>>>    
>>> -	return ret;
>>> +	panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>>> +
>>> +	if (IS_ERR(panel_bridge))
>>> +		return PTR_ERR(panel_bridge);
>>> +
>>> +	ctx->panel_bridge = panel_bridge;
>>> +	return 0;
>>>    }
>>>    
>>>    static int tc358764_configure_regulators(struct tc358764 *ctx)
>>> @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
>>>    		return ret;
>>>    
>>>    	ctx->bridge.funcs = &tc358764_bridge_funcs;
>>> +	ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>>>    	ctx->bridge.of_node = dev->of_node;
>>>    
>>>    	drm_bridge_add(&ctx->bridge);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel