[PATCH 00/24] device link, bridge supplier <-> drm device

[PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russel King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russel.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 23 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. That
said, I might have missed some subtlety.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (24):
  drm/bridge: allow optionally specifying an .owner device
  drm/bridge: adv7511: provide an .owner device
  drm/bridge/analogix: core: specify the .owner of the bridge
  drm/bridge: analogix-anx78xx: provide an .owner device
  drm/bridge: vga-dac: provide an .owner device
  drm/bridge: lvds-encoder: provide an .owner device
  drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an .owner device
  drm/bridge: nxp-ptn3460: provide an .owner device
  drm/bridge: panel: provide an .owner device
  drm/bridge: ps8622: provide an .owner device
  drm/bridge: sii902x: provide an .owner device
  drm/bridge: sii9234: provide an .owner device
  drm/bridge: sii8620: provide an .owner device
  drm/bridge: synopsys: provide an .owner device for the bridges
  drm/bridge: tc358767: provide an .owner device
  drm/bridge: ti-tfp410: provide an .owner device
  drm/exynos: mic: provide an .owner device for the bridge
  drm/mediatek: hdmi: provide an .owner device for the bridge
  drm/msm: specify the .owner of the bridges
  drm/rcar-du: lvds: provide an .owner device for the bridge
  drm/sti: provide an .owner device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the .owner to be filled in on drm_bridge_attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c          |  5 +----
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/dumb-vga-dac.c              |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c              |  2 +-
 .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c               |  2 +-
 drivers/gpu/drm/bridge/panel.c                     |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c             |  2 +-
 drivers/gpu/drm/bridge/sii902x.c                   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c                   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c               |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c      |  4 +---
 drivers/gpu/drm/bridge/tc358767.c                  |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
 drivers/gpu/drm/drm_bridge.c                       | 23 +++++++++++++++++++++-
 drivers/gpu/drm/exynos/exynos_drm_mic.c            |  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c                |  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c               |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c                |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c                      |  2 +-
 drivers/gpu/drm/sti/sti_hda.c                      |  1 +
 drivers/gpu/drm/sti/sti_hdmi.c                     |  1 +
 include/drm/drm_bridge.h                           |  8 ++++----
 27 files changed, 51 insertions(+), 33 deletions(-)

-- 
2.11.0

[PATCH 01/24] drm/bridge: allow optionally specifying an .owner device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner or keep just providing an of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 ++-
 include/drm/drm_bridge.h     | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..67147673fdeb 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	mutex_lock(&bridge_lock);
 
 	list_for_each_entry(bridge, &bridge_list, list) {
-		if (bridge->of_node == np) {
+		if ((bridge->owner && bridge->owner->of_node == np) ||
+		    bridge->of_node == np) {
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..c28a75ad0ae2 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @owner: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+	struct device *owner;
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;
-- 
2.11.0

[PATCH 02/24] drm/bridge: adv7511: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index efa29db5fc2b..866f3ebe9e60 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1195,8 +1195,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	if (ret)
 		goto err_unregister_cec;
 
+	adv7511->bridge.owner = dev;
 	adv7511->bridge.funcs = &adv7511_bridge_funcs;
-	adv7511->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&adv7511->bridge);
 
-- 
2.11.0

[PATCH 03/24] drm/bridge/analogix: core: specify the .owner of the bridge

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 5c52307146c7..5a9dbbd7897a 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1308,6 +1308,7 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev,
 
 	dp->bridge = bridge;
 
+	bridge->owner = dp->dev;
 	bridge->driver_private = dp;
 	bridge->funcs = &analogix_dp_bridge_funcs;
 
-- 
2.11.0

[PATCH 04/24] drm/bridge: analogix-anx78xx: provide an .owner device

[Peter Rosin]

It gets rid of an #if and the .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index b49043866be6..4b2eccdf88a3 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1332,10 +1332,6 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 
 	mutex_init(&anx78xx->lock);
 
-#if IS_ENABLED(CONFIG_OF)
-	anx78xx->bridge.of_node = client->dev.of_node;
-#endif
-
 	anx78xx->client = client;
 	i2c_set_clientdata(client, anx78xx);
 
@@ -1433,6 +1429,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 		goto err_poweroff;
 	}
 
+	anx78xx->bridge.owner = &client->dev;
 	anx78xx->bridge.funcs = &anx78xx_bridge_funcs;
 
 	drm_bridge_add(&anx78xx->bridge);
-- 
2.11.0

[PATCH 05/24] drm/bridge: vga-dac: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 498d5948d1a8..244f6bfb8967 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -203,8 +203,8 @@ static int dumb_vga_probe(struct platform_device *pdev)
 		}
 	}
 
+	vga->bridge.owner = &pdev->dev;
 	vga->bridge.funcs = &dumb_vga_bridge_funcs;
-	vga->bridge.of_node = pdev->dev.of_node;
 	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
 
 	drm_bridge_add(&vga->bridge);
-- 
2.11.0

[PATCH 06/24] drm/bridge: lvds-encoder: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/lvds-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..7aa12f4b5745 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -83,7 +83,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	 * but we need a bridge attached to our of_node for our user
 	 * to look up.
 	 */
-	lvds_encoder->bridge.of_node = pdev->dev.of_node;
+	lvds_encoder->bridge.owner = &pdev->dev;
 	lvds_encoder->bridge.funcs = &funcs;
 	drm_bridge_add(&lvds_encoder->bridge);
 
-- 
2.11.0

[PATCH 07/24] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 7ccadba7c98c..7b7531f75d41 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -313,8 +313,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 	i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
 
 	/* drm bridge initialization */
+	ge_b850v3_lvds_ptr->bridge.owner = dev;
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
-	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
 	/* Clear pending interrupts since power up. */
-- 
2.11.0

[PATCH 08/24] drm/bridge: nxp-ptn3460: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index d64a3283822a..e6a15ec60e12 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -329,8 +329,8 @@ static int ptn3460_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ptn_bridge->bridge.owner = dev;
 	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
-	ptn_bridge->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ptn_bridge->bridge);
 
 	i2c_set_clientdata(client, ptn_bridge);
-- 
2.11.0

[PATCH 09/24] drm/bridge: panel: provide an .owner device

[Peter Rosin]

It gets rid of an #ifdef and the .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 6d99d4a3beb3..279271a1d7e7 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
 	panel_bridge->connector_type = connector_type;
 	panel_bridge->panel = panel;
 
+	panel_bridge->bridge.owner = panel->dev;
 	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
-#ifdef CONFIG_OF
-	panel_bridge->bridge.of_node = panel->dev->of_node;
-#endif
 
 	drm_bridge_add(&panel_bridge->bridge);
 
-- 
2.11.0

[PATCH 10/24] drm/bridge: ps8622: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/parade-ps8622.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index 81198f5e9afa..6dd19a5fcebd 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -595,8 +595,8 @@ static int ps8622_probe(struct i2c_client *client,
 		ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
 	}
 
+	ps8622->bridge.owner = dev;
 	ps8622->bridge.funcs = &ps8622_bridge_funcs;
-	ps8622->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ps8622->bridge);
 
 	i2c_set_clientdata(client, ps8622);
-- 
2.11.0

[PATCH 11/24] drm/bridge: sii902x: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/sii902x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..0127090f38b3 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -427,8 +427,8 @@ static int sii902x_probe(struct i2c_client *client,
 			return ret;
 	}
 
+	sii902x->bridge.owner = dev;
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
-	sii902x->bridge.of_node = dev->of_node;
 	drm_bridge_add(&sii902x->bridge);
 
 	i2c_set_clientdata(client, sii902x);
-- 
2.11.0

[PATCH 12/24] drm/bridge: sii9234: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/sii9234.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index c77000626c22..879d9b5ef4cf 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -948,8 +948,8 @@ static int sii9234_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ctx);
 
+	ctx->bridge.owner = dev;
 	ctx->bridge.funcs = &sii9234_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
 	sii9234_cable_in(ctx);
-- 
2.11.0

[PATCH 13/24] drm/bridge: sii8620: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 7ab36042a822..650f71003e45 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2387,8 +2387,8 @@ static int sii8620_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ctx);
 
+	ctx->bridge.owner = dev;
 	ctx->bridge.funcs = &sii8620_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
 	if (!ctx->extcon)
-- 
2.11.0

[PATCH 14/24] drm/bridge: synopsys: provide an .owner device for the bridges

[Peter Rosin]

It gets rid of two #ifdefs and the .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     | 4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ec8d0006ef7c..48ad6888c967 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2471,11 +2471,9 @@ __dw_hdmi_probe(struct platform_device *pdev,
 			hdmi->ddc = NULL;
 	}
 
+	hdmi->bridge.owner = dev;
 	hdmi->bridge.driver_private = hdmi;
 	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
-#ifdef CONFIG_OF
-	hdmi->bridge.of_node = pdev->dev.of_node;
-#endif
 
 	dw_hdmi_setup_i2c(hdmi);
 	if (hdmi->phy.ops->setup_hpd)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 226171a3ece1..91e2cc3da90a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -934,11 +934,9 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
 		return ERR_PTR(ret);
 	}
 
+	dsi->bridge.owner = dev;
 	dsi->bridge.driver_private = dsi;
 	dsi->bridge.funcs = &dw_mipi_dsi_bridge_funcs;
-#ifdef CONFIG_OF
-	dsi->bridge.of_node = pdev->dev.of_node;
-#endif
 
 	return dsi;
 }
-- 
2.11.0

[PATCH 15/24] drm/bridge: tc358767: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 08ab7d6aea65..a16573bb8715 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1331,8 +1331,8 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	tc_connector_set_polling(tc, &tc->connector);
 
+	tc->bridge.owner = dev;
 	tc->bridge.funcs = &tc_bridge_funcs;
-	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
 	i2c_set_clientdata(client, tc);
-- 
2.11.0

[PATCH 16/24] drm/bridge: ti-tfp410: provide an .owner device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index acb857030951..7e5938e988be 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -215,8 +215,8 @@ static int tfp410_init(struct device *dev)
 		return -ENOMEM;
 	dev_set_drvdata(dev, dvi);
 
+	dvi->bridge.owner = dev;
 	dvi->bridge.funcs = &tfp410_bridge_funcs;
-	dvi->bridge.of_node = dev->of_node;
 	dvi->dev = dev;
 
 	ret = tfp410_get_connector_properties(dvi);
-- 
2.11.0

[PATCH 17/24] drm/exynos: mic: provide an .owner device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index 2174814273e2..453716cb4d3d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -417,8 +417,8 @@ static int exynos_mic_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mic);
 
+	mic->bridge.owner = dev;
 	mic->bridge.funcs = &mic_bridge_funcs;
-	mic->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&mic->bridge);
 
-- 
2.11.0

[PATCH 18/24] drm/mediatek: hdmi: provide an .owner device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 59a11026dceb..b4a7908e0cc6 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1694,8 +1694,8 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
 
 	mtk_hdmi_register_audio_driver(dev);
 
+	hdmi->bridge.owner = &pdev->dev;
 	hdmi->bridge.funcs = &mtk_hdmi_bridge_funcs;
-	hdmi->bridge.of_node = pdev->dev.of_node;
 	drm_bridge_add(&hdmi->bridge);
 
 	ret = mtk_hdmi_clk_enable_audio(hdmi);
-- 
2.11.0

[PATCH 19/24] drm/msm: specify the .owner of the bridges

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   | 1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 4cb1cb68878b..b6c344bce75d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -710,6 +710,7 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
 	encoder = msm_dsi->encoder;
 
 	bridge = &dsi_bridge->base;
+	bridge->owner = msm_dsi->dev->dev;
 	bridge->funcs = &dsi_mgr_bridge_funcs;
 
 	ret = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c b/drivers/gpu/drm/msm/edp/edp_bridge.c
index 931a5c97cccf..30d9e0add9fe 100644
--- a/drivers/gpu/drm/msm/edp/edp_bridge.c
+++ b/drivers/gpu/drm/msm/edp/edp_bridge.c
@@ -104,6 +104,7 @@ struct drm_bridge *msm_edp_bridge_init(struct msm_edp *edp)
 	edp_bridge->edp = edp;
 
 	bridge = &edp_bridge->base;
+	bridge->owner = edp->dev->dev;
 	bridge->funcs = &edp_bridge_funcs;
 
 	ret = drm_bridge_attach(edp->encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 7e357077ed26..6fb103d91956 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -293,6 +293,7 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
 	hdmi_bridge->hdmi = hdmi;
 
 	bridge = &hdmi_bridge->base;
+	bridge->owner = hdmi->dev->dev;
 	bridge->funcs = &msm_hdmi_bridge_funcs;
 
 	ret = drm_bridge_attach(hdmi->encoder, bridge, NULL);
-- 
2.11.0

[PATCH 20/24] drm/rcar-du: lvds: provide an .owner device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..5984c70b5590 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	lvds->bridge.owner = &pdev->dev;
 	lvds->bridge.driver_private = lvds;
 	lvds->bridge.funcs = &rcar_lvds_bridge_ops;
-	lvds->bridge.of_node = pdev->dev.of_node;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	lvds->mmio = devm_ioremap_resource(&pdev->dev, mem);
-- 
2.11.0

[PATCH 21/24] drm/sti: provide an .owner device for the bridges

[Peter Rosin]

The .of_node member is going away and providing an .owner will become
mandatory.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/sti/sti_dvo.c  | 2 +-
 drivers/gpu/drm/sti/sti_hda.c  | 1 +
 drivers/gpu/drm/sti/sti_hdmi.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index a5979cd25cc7..aa390053de16 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -460,9 +460,9 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -ENOMEM;
 
+	bridge->owner = &dvo->dev;
 	bridge->driver_private = dvo;
 	bridge->funcs = &sti_dvo_bridge_funcs;
-	bridge->of_node = dvo->dev.of_node;
 	drm_bridge_add(bridge);
 
 	err = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 67bbdb49fffc..f45623628c95 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -694,6 +694,7 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -ENOMEM;
 
+	bridge->owner = dev;
 	bridge->driver_private = hda;
 	bridge->funcs = &sti_hda_bridge_funcs;
 	drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 58f431102512..3989cafd1c23 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1270,6 +1270,7 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -EINVAL;
 
+	bridge->owner = dev;
 	bridge->driver_private = hdmi;
 	bridge->funcs = &sti_hdmi_bridge_funcs;
 	drm_bridge_attach(encoder, bridge, NULL);
-- 
2.11.0

[PATCH 22/24] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +--
 include/drm/drm_bridge.h     | 4 ----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 67147673fdeb..9f023bd84d56 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	mutex_lock(&bridge_lock);
 
 	list_for_each_entry(bridge, &bridge_list, list) {
-		if ((bridge->owner && bridge->owner->of_node == np) ||
-		    bridge->of_node == np) {
+		if (bridge->owner->of_node == np) {
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c28a75ad0ae2..3bc659f3e7d2 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;
-#ifdef CONFIG_OF
-	struct device_node *of_node;
-#endif
 	struct list_head list;
 	const struct drm_bridge_timings *timings;
 
-- 
2.11.0

[PATCH 23/24] drm/bridge: require the .owner to be filled in on drm_bridge_attach

[Peter Rosin]

The .owner will be handy to have around.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 9f023bd84d56..a038da696802 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -115,6 +115,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (!encoder || !bridge)
 		return -EINVAL;
 
+	if (WARN_ON(!bridge->owner))
+		return -EINVAL;
+
 	if (previous && (!previous->dev || previous->encoder != encoder))
 		return -EINVAL;
 
-- 
2.11.0

[PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
 include/drm/drm_bridge.h     |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index a038da696802..f0c79043ec43 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include <linux/mutex.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_device.h>
 #include <drm/drm_encoder.h>
 
 #include "drm_crtc_internal.h"
@@ -124,12 +125,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (bridge->dev)
 		return -EBUSY;
 
+	if (encoder->dev->dev != bridge->owner) {
+		bridge->link = device_link_add(encoder->dev->dev,
+					       bridge->owner, 0);
+		if (!bridge->link) {
+			dev_err(bridge->owner, "failed to link bridge to %s\n",
+				dev_name(encoder->dev->dev));
+			return -EINVAL;
+		}
+	}
+
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
 		if (ret < 0) {
+			if (bridge->link)
+				device_link_del(bridge->link);
+			bridge->link = NULL;
 			bridge->dev = NULL;
 			bridge->encoder = NULL;
 			return ret;
@@ -156,6 +170,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	if (bridge->link)
+		device_link_del(bridge->link);
+	bridge->link = NULL;
+
 	bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3bc659f3e7d2..9a386559a41a 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
 	struct drm_bridge *next;
 	struct list_head list;
 	const struct drm_bridge_timings *timings;
+	struct device_link *link;
 
 	const struct drm_bridge_funcs *funcs;
 	void *driver_private;
-- 
2.11.0

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 00:40, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patches.
> 
> On Friday, 27 April 2018 01:31:15 EEST Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 23 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. That
>> said, I might have missed some subtlety.
> 
> I don't think this is the way to go. We have a real lifetime management 
> problem with bridges, and device links are just trying to hide the problem 
> under the carpet. They will further corner us by making a real fix much more 
> difficult to implement. I'll try to comment further in the next few days on 
> what I think a better solution would be, but in a nutshell I believe that 
> drm_bridge objects need to be refcounted, with a .release() operation to free 
> the bridge resources when the reference count drops to zero. This shouldn't be 
> difficult to implement and I'm willing to help.

Ok, sp 24/24 is dead, and maybe 23/24 too. But how do you feel about dropping
.of_node in favour of .owner? That gets rid of ugly #ifdefs...

I also have the nagging feeling that .driver_private serves very little real
purpose if there is a .owner so that you can do

	dev_get_drvdata(bridge->owner)

for the cases where the container_of macro is not appropriate.

Cheers,
Peter

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Andrzej Hajda]

Hi Peter,

On 27.04.2018 00:31, Peter Rosin wrote:
> Hi!
>
> It was noted by Russel King [1] that bridges (not using components)
> might disappear unexpectedly if the owner of the bridge was unbound.
> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
> came up with using device links to resolve the panel issue, which
> was also my (independent) reaction to the note from Russel.
>
> This series builds up to the addition of that link in the last
> patch, but in my opinion the other 23 patches do have merit on their
> own.
>
> The last patch needs testing, while the others look trivial. That
> said, I might have missed some subtlety.

of_node is used as an identifier of the bridge in the kernel. If you
replace it with device pointer there will be potential problem with
devices having two or more bridges, how do you differentiate bridges if
the owner is the same? If I remember correctly current bridge code does
not allow to have multiple bridges in one device, but that should be
quite easy to fix if necessary. After this change it will become more
difficult.

Anyway I remember discussion that in DT world bridge should be
identified rather by of_graph port node, not by parent node as it is
now. If you want to translate this relation to device owner, you should
add also port number to have full identification of the bridge, ie pair
(owner, port_number) would be equivalent of port node.

Regards
Andrzej


>
> Cheers,
> Peter
>
> [1] https://lkml.org/lkml/2018/4/23/769
> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
>
> Peter Rosin (24):
>   drm/bridge: allow optionally specifying an .owner device
>   drm/bridge: adv7511: provide an .owner device
>   drm/bridge/analogix: core: specify the .owner of the bridge
>   drm/bridge: analogix-anx78xx: provide an .owner device
>   drm/bridge: vga-dac: provide an .owner device
>   drm/bridge: lvds-encoder: provide an .owner device
>   drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an .owner device
>   drm/bridge: nxp-ptn3460: provide an .owner device
>   drm/bridge: panel: provide an .owner device
>   drm/bridge: ps8622: provide an .owner device
>   drm/bridge: sii902x: provide an .owner device
>   drm/bridge: sii9234: provide an .owner device
>   drm/bridge: sii8620: provide an .owner device
>   drm/bridge: synopsys: provide an .owner device for the bridges
>   drm/bridge: tc358767: provide an .owner device
>   drm/bridge: ti-tfp410: provide an .owner device
>   drm/exynos: mic: provide an .owner device for the bridge
>   drm/mediatek: hdmi: provide an .owner device for the bridge
>   drm/msm: specify the .owner of the bridges
>   drm/rcar-du: lvds: provide an .owner device for the bridge
>   drm/sti: provide an .owner device for the bridges
>   drm/bridge: remove the .of_node member
>   drm/bridge: require the .owner to be filled in on drm_bridge_attach
>   drm/bridge: establish a link between the bridge supplier and consumer
>
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       |  2 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c          |  5 +----
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>  drivers/gpu/drm/bridge/dumb-vga-dac.c              |  2 +-
>  drivers/gpu/drm/bridge/lvds-encoder.c              |  2 +-
>  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  2 +-
>  drivers/gpu/drm/bridge/nxp-ptn3460.c               |  2 +-
>  drivers/gpu/drm/bridge/panel.c                     |  4 +---
>  drivers/gpu/drm/bridge/parade-ps8622.c             |  2 +-
>  drivers/gpu/drm/bridge/sii902x.c                   |  2 +-
>  drivers/gpu/drm/bridge/sii9234.c                   |  2 +-
>  drivers/gpu/drm/bridge/sil-sii8620.c               |  2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  4 +---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c      |  4 +---
>  drivers/gpu/drm/bridge/tc358767.c                  |  2 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
>  drivers/gpu/drm/drm_bridge.c                       | 23 +++++++++++++++++++++-
>  drivers/gpu/drm/exynos/exynos_drm_mic.c            |  2 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c                |  2 +-
>  drivers/gpu/drm/msm/dsi/dsi_manager.c              |  1 +
>  drivers/gpu/drm/msm/edp/edp_bridge.c               |  1 +
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  1 +
>  drivers/gpu/drm/rcar-du/rcar_lvds.c                |  2 +-
>  drivers/gpu/drm/sti/sti_dvo.c                      |  2 +-
>  drivers/gpu/drm/sti/sti_hda.c                      |  1 +
>  drivers/gpu/drm/sti/sti_hdmi.c                     |  1 +
>  include/drm/drm_bridge.h                           |  8 ++++----
>  27 files changed, 51 insertions(+), 33 deletions(-)
>

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 01:18, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Friday, 27 April 2018 02:09:14 EEST Peter Rosin wrote:
>> On 2018-04-27 00:40, Laurent Pinchart wrote:
>>> On Friday, 27 April 2018 01:31:15 EEST Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> It was noted by Russel King [1] that bridges (not using components)
>>>> might disappear unexpectedly if the owner of the bridge was unbound.
>>>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>>>> came up with using device links to resolve the panel issue, which
>>>> was also my (independent) reaction to the note from Russel.
>>>>
>>>> This series builds up to the addition of that link in the last
>>>> patch, but in my opinion the other 23 patches do have merit on their
>>>> own.
>>>>
>>>> The last patch needs testing, while the others look trivial. That
>>>> said, I might have missed some subtlety.
>>>
>>> I don't think this is the way to go. We have a real lifetime management
>>> problem with bridges, and device links are just trying to hide the problem
>>> under the carpet. They will further corner us by making a real fix much
>>> more difficult to implement. I'll try to comment further in the next few
>>> days on what I think a better solution would be, but in a nutshell I
>>> believe that drm_bridge objects need to be refcounted, with a .release()
>>> operation to free the bridge resources when the reference count drops to
>>> zero. This shouldn't be difficult to implement and I'm willing to help.
>>
>> Ok, sp 24/24 is dead, and maybe 23/24 too.
> 
> Not necessarily, maybe I'll get proven wrong :-) Let's discuss, but I first 
> need some sleep.

Ok, I'll add my view...

I don't see how a refcount is going to resolve anything. Let's take some
device that allocs a bridge that is later attached to some encoder. In doing
so it adds hooks to some of the drm_bridge_funcs. If you add a refcount to
the bridge itself then yes, the bridge object might remain but the code
backing the hook functions will go away the moment the owner device goes
away. So, you'd have to refcount the owner device itself to prevent it
from going away. But, you can't stop a device from going away IIUC, you can
only bring other stuff down with it in an orderly fashion.

Components, that some bridges use, takes care of bringing the whole cluster
down *before* any device goes away, so that is obviously a solution. But
that solution is not in place everywhere.

Now, my device-link is pretty light-weight. And it should only matter if
the owner goes away before the consuming drm device has brought down the
encoder properly. If that ever happens, we're in trouble. So, the link can
at worst only substitute one problem with another, and at best it prevents
the fireworks.

Sure, there's the reprobe problem if the bridge's owner device shows up
again, but that's pretty minor compared to a hard crash. And there was a
patch for that, so in the end that may be a non-issue.

If some other solution is found, removing the device-link is trivial. It is
localized to bridge attach/detach and nothing else is affected (in terms of
code).

Cheers,
Peter

>> But how do you feel about dropping .of_node in favour of .owner? That gets
>> rid of ugly #ifdefs...
> 
> I'll review that part and reply, I have nothing against it on principle at the 
> moment. The more generic the code is, the better in my opinion. We just need 
> to make sure that the OF node we're interested in will always be .owner-
>> of_node in all cases.
> 
>> I also have the nagging feeling that .driver_private serves very little real
>> purpose if there is a .owner so that you can do
>>
>> 	dev_get_drvdata(bridge->owner)
>>
>> for the cases where the container_of macro is not appropriate.
> 
> I'll review that too, it's a good point.
> 

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 09:11, Andrzej Hajda wrote:
> Hi Peter,
> 
> On 27.04.2018 00:31, Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 23 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. That
>> said, I might have missed some subtlety.
> 
> of_node is used as an identifier of the bridge in the kernel. If you
> replace it with device pointer there will be potential problem with
> devices having two or more bridges, how do you differentiate bridges if
> the owner is the same? If I remember correctly current bridge code does
> not allow to have multiple bridges in one device, but that should be
> quite easy to fix if necessary. After this change it will become more
> difficult.

I don't see how it will be more difficult?

> Anyway I remember discussion that in DT world bridge should be
> identified rather by of_graph port node, not by parent node as it is
> now. If you want to translate this relation to device owner, you should
> add also port number to have full identification of the bridge, ie pair
> (owner, port_number) would be equivalent of port node.

You even state the trivial solution here, just add the port/endpoint ID
when/if it is needed. So, what is the significant difference?

Cheers,
Peter

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-04-27 09:37, Peter Rosin wrote:
> On 2018-04-27 09:11, Andrzej Hajda wrote:
>> Hi Peter,
>>
>> On 27.04.2018 00:31, Peter Rosin wrote:
>>> Hi!
>>>
>>> It was noted by Russel King [1] that bridges (not using components)
>>> might disappear unexpectedly if the owner of the bridge was unbound.
>>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>>> came up with using device links to resolve the panel issue, which
>>> was also my (independent) reaction to the note from Russel.
>>>
>>> This series builds up to the addition of that link in the last
>>> patch, but in my opinion the other 23 patches do have merit on their
>>> own.
>>>
>>> The last patch needs testing, while the others look trivial. That
>>> said, I might have missed some subtlety.
>>
>> of_node is used as an identifier of the bridge in the kernel. If you
>> replace it with device pointer there will be potential problem with
>> devices having two or more bridges, how do you differentiate bridges if
>> the owner is the same? If I remember correctly current bridge code does
>> not allow to have multiple bridges in one device, but that should be
>> quite easy to fix if necessary. After this change it will become more
>> difficult.
> 
> I don't see how it will be more difficult?
> 
>> Anyway I remember discussion that in DT world bridge should be
>> identified rather by of_graph port node, not by parent node as it is
>> now. If you want to translate this relation to device owner, you should
>> add also port number to have full identification of the bridge, ie pair
>> (owner, port_number) would be equivalent of port node.
> 
> You even state the trivial solution here, just add the port/endpoint ID
> when/if it is needed. So, what is the significant difference?

Or, since this is apparently a rare requirement, you could make the owners
that do need it fix it themselves. E.g. by embedding the struct drm_bridge
in another struct that contains the needed ID, and use container_of to get
to that containing struct with the ID.

Cheers,
Peter

Re: [PATCH 22/24] drm/bridge: remove the .of_node member

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 10471 bytes --]

Hi Peter,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.17-rc2]
[also build test ERROR on next-20180426]
[cannot apply to drm/drm-next robclark/msm-next drm-exynos/exynos-drm/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Rosin/device-link-bridge-supplier-drm-device/20180428-135229
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/rockchip/rockchip_lvds.c: In function 'rockchip_lvds_bind':
>> drivers/gpu//drm/rockchip/rockchip_lvds.c:381:24: error: 'struct drm_bridge' has no member named 'of_node'
      remote = lvds->bridge->of_node;
                           ^~

vim +381 drivers/gpu//drm/rockchip/rockchip_lvds.c

34cc0aa25 Sandy Huang 2017-09-02  340  
34cc0aa25 Sandy Huang 2017-09-02  341  static int rockchip_lvds_bind(struct device *dev, struct device *master,
34cc0aa25 Sandy Huang 2017-09-02  342  			      void *data)
34cc0aa25 Sandy Huang 2017-09-02  343  {
34cc0aa25 Sandy Huang 2017-09-02  344  	struct rockchip_lvds *lvds = dev_get_drvdata(dev);
34cc0aa25 Sandy Huang 2017-09-02  345  	struct drm_device *drm_dev = data;
34cc0aa25 Sandy Huang 2017-09-02  346  	struct drm_encoder *encoder;
34cc0aa25 Sandy Huang 2017-09-02  347  	struct drm_connector *connector;
34cc0aa25 Sandy Huang 2017-09-02  348  	struct device_node *remote = NULL;
34cc0aa25 Sandy Huang 2017-09-02  349  	struct device_node  *port, *endpoint;
6bf2e0324 Sean Paul   2017-09-20  350  	int ret = 0, child_count = 0;
34cc0aa25 Sandy Huang 2017-09-02  351  	const char *name;
34cc0aa25 Sandy Huang 2017-09-02  352  	u32 endpoint_id;
34cc0aa25 Sandy Huang 2017-09-02  353  
34cc0aa25 Sandy Huang 2017-09-02  354  	lvds->drm_dev = drm_dev;
34cc0aa25 Sandy Huang 2017-09-02  355  	port = of_graph_get_port_by_id(dev->of_node, 1);
34cc0aa25 Sandy Huang 2017-09-02  356  	if (!port) {
34cc0aa25 Sandy Huang 2017-09-02  357  		DRM_DEV_ERROR(dev,
34cc0aa25 Sandy Huang 2017-09-02  358  			      "can't found port point, please init lvds panel port!\n");
34cc0aa25 Sandy Huang 2017-09-02  359  		return -EINVAL;
34cc0aa25 Sandy Huang 2017-09-02  360  	}
34cc0aa25 Sandy Huang 2017-09-02  361  	for_each_child_of_node(port, endpoint) {
6bf2e0324 Sean Paul   2017-09-20  362  		child_count++;
34cc0aa25 Sandy Huang 2017-09-02  363  		of_property_read_u32(endpoint, "reg", &endpoint_id);
34cc0aa25 Sandy Huang 2017-09-02  364  		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
34cc0aa25 Sandy Huang 2017-09-02  365  						  &lvds->panel, &lvds->bridge);
34cc0aa25 Sandy Huang 2017-09-02  366  		if (!ret)
34cc0aa25 Sandy Huang 2017-09-02  367  			break;
34cc0aa25 Sandy Huang 2017-09-02  368  	}
6bf2e0324 Sean Paul   2017-09-20  369  	if (!child_count) {
6bf2e0324 Sean Paul   2017-09-20  370  		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
6bf2e0324 Sean Paul   2017-09-20  371  		ret = -EINVAL;
6bf2e0324 Sean Paul   2017-09-20  372  		goto err_put_port;
6bf2e0324 Sean Paul   2017-09-20  373  	} else if (ret) {
34cc0aa25 Sandy Huang 2017-09-02  374  		DRM_DEV_ERROR(dev, "failed to find panel and bridge node\n");
34cc0aa25 Sandy Huang 2017-09-02  375  		ret = -EPROBE_DEFER;
34cc0aa25 Sandy Huang 2017-09-02  376  		goto err_put_port;
34cc0aa25 Sandy Huang 2017-09-02  377  	}
34cc0aa25 Sandy Huang 2017-09-02  378  	if (lvds->panel)
34cc0aa25 Sandy Huang 2017-09-02  379  		remote = lvds->panel->dev->of_node;
34cc0aa25 Sandy Huang 2017-09-02  380  	else
34cc0aa25 Sandy Huang 2017-09-02 @381  		remote = lvds->bridge->of_node;
34cc0aa25 Sandy Huang 2017-09-02  382  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
34cc0aa25 Sandy Huang 2017-09-02  383  		/* default set it as output rgb */
34cc0aa25 Sandy Huang 2017-09-02  384  		lvds->output = DISPLAY_OUTPUT_RGB;
34cc0aa25 Sandy Huang 2017-09-02  385  	else
34cc0aa25 Sandy Huang 2017-09-02  386  		lvds->output = lvds_name_to_output(name);
34cc0aa25 Sandy Huang 2017-09-02  387  
34cc0aa25 Sandy Huang 2017-09-02  388  	if (lvds->output < 0) {
34cc0aa25 Sandy Huang 2017-09-02  389  		DRM_DEV_ERROR(dev, "invalid output type [%s]\n", name);
34cc0aa25 Sandy Huang 2017-09-02  390  		ret = lvds->output;
34cc0aa25 Sandy Huang 2017-09-02  391  		goto err_put_remote;
34cc0aa25 Sandy Huang 2017-09-02  392  	}
34cc0aa25 Sandy Huang 2017-09-02  393  
34cc0aa25 Sandy Huang 2017-09-02  394  	if (of_property_read_string(remote, "data-mapping", &name))
34cc0aa25 Sandy Huang 2017-09-02  395  		/* default set it as format vesa 18 */
34cc0aa25 Sandy Huang 2017-09-02  396  		lvds->format = LVDS_VESA_18;
34cc0aa25 Sandy Huang 2017-09-02  397  	else
34cc0aa25 Sandy Huang 2017-09-02  398  		lvds->format = lvds_name_to_format(name);
34cc0aa25 Sandy Huang 2017-09-02  399  
34cc0aa25 Sandy Huang 2017-09-02  400  	if (lvds->format < 0) {
34cc0aa25 Sandy Huang 2017-09-02  401  		DRM_DEV_ERROR(dev, "invalid data-mapping format [%s]\n", name);
34cc0aa25 Sandy Huang 2017-09-02  402  		ret = lvds->format;
34cc0aa25 Sandy Huang 2017-09-02  403  		goto err_put_remote;
34cc0aa25 Sandy Huang 2017-09-02  404  	}
34cc0aa25 Sandy Huang 2017-09-02  405  
34cc0aa25 Sandy Huang 2017-09-02  406  	encoder = &lvds->encoder;
34cc0aa25 Sandy Huang 2017-09-02  407  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev,
34cc0aa25 Sandy Huang 2017-09-02  408  							     dev->of_node);
34cc0aa25 Sandy Huang 2017-09-02  409  
34cc0aa25 Sandy Huang 2017-09-02  410  	ret = drm_encoder_init(drm_dev, encoder, &rockchip_lvds_encoder_funcs,
34cc0aa25 Sandy Huang 2017-09-02  411  			       DRM_MODE_ENCODER_LVDS, NULL);
34cc0aa25 Sandy Huang 2017-09-02  412  	if (ret < 0) {
34cc0aa25 Sandy Huang 2017-09-02  413  		DRM_DEV_ERROR(drm_dev->dev,
34cc0aa25 Sandy Huang 2017-09-02  414  			      "failed to initialize encoder: %d\n", ret);
34cc0aa25 Sandy Huang 2017-09-02  415  		goto err_put_remote;
34cc0aa25 Sandy Huang 2017-09-02  416  	}
34cc0aa25 Sandy Huang 2017-09-02  417  
34cc0aa25 Sandy Huang 2017-09-02  418  	drm_encoder_helper_add(encoder, &rockchip_lvds_encoder_helper_funcs);
34cc0aa25 Sandy Huang 2017-09-02  419  
34cc0aa25 Sandy Huang 2017-09-02  420  	if (lvds->panel) {
34cc0aa25 Sandy Huang 2017-09-02  421  		connector = &lvds->connector;
34cc0aa25 Sandy Huang 2017-09-02  422  		connector->dpms = DRM_MODE_DPMS_OFF;
34cc0aa25 Sandy Huang 2017-09-02  423  		ret = drm_connector_init(drm_dev, connector,
34cc0aa25 Sandy Huang 2017-09-02  424  					 &rockchip_lvds_connector_funcs,
34cc0aa25 Sandy Huang 2017-09-02  425  					 DRM_MODE_CONNECTOR_LVDS);
34cc0aa25 Sandy Huang 2017-09-02  426  		if (ret < 0) {
34cc0aa25 Sandy Huang 2017-09-02  427  			DRM_DEV_ERROR(drm_dev->dev,
34cc0aa25 Sandy Huang 2017-09-02  428  				      "failed to initialize connector: %d\n", ret);
34cc0aa25 Sandy Huang 2017-09-02  429  			goto err_free_encoder;
34cc0aa25 Sandy Huang 2017-09-02  430  		}
34cc0aa25 Sandy Huang 2017-09-02  431  
34cc0aa25 Sandy Huang 2017-09-02  432  		drm_connector_helper_add(connector,
34cc0aa25 Sandy Huang 2017-09-02  433  					 &rockchip_lvds_connector_helper_funcs);
34cc0aa25 Sandy Huang 2017-09-02  434  
34cc0aa25 Sandy Huang 2017-09-02  435  		ret = drm_mode_connector_attach_encoder(connector, encoder);
34cc0aa25 Sandy Huang 2017-09-02  436  		if (ret < 0) {
34cc0aa25 Sandy Huang 2017-09-02  437  			DRM_DEV_ERROR(drm_dev->dev,
34cc0aa25 Sandy Huang 2017-09-02  438  				      "failed to attach encoder: %d\n", ret);
34cc0aa25 Sandy Huang 2017-09-02  439  			goto err_free_connector;
34cc0aa25 Sandy Huang 2017-09-02  440  		}
34cc0aa25 Sandy Huang 2017-09-02  441  
34cc0aa25 Sandy Huang 2017-09-02  442  		ret = drm_panel_attach(lvds->panel, connector);
34cc0aa25 Sandy Huang 2017-09-02  443  		if (ret < 0) {
34cc0aa25 Sandy Huang 2017-09-02  444  			DRM_DEV_ERROR(drm_dev->dev,
34cc0aa25 Sandy Huang 2017-09-02  445  				      "failed to attach panel: %d\n", ret);
34cc0aa25 Sandy Huang 2017-09-02  446  			goto err_free_connector;
34cc0aa25 Sandy Huang 2017-09-02  447  		}
34cc0aa25 Sandy Huang 2017-09-02  448  	} else {
34cc0aa25 Sandy Huang 2017-09-02  449  		lvds->bridge->encoder = encoder;
34cc0aa25 Sandy Huang 2017-09-02  450  		ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
34cc0aa25 Sandy Huang 2017-09-02  451  		if (ret) {
34cc0aa25 Sandy Huang 2017-09-02  452  			DRM_DEV_ERROR(drm_dev->dev,
34cc0aa25 Sandy Huang 2017-09-02  453  				      "failed to attach bridge: %d\n", ret);
34cc0aa25 Sandy Huang 2017-09-02  454  			goto err_free_encoder;
34cc0aa25 Sandy Huang 2017-09-02  455  		}
34cc0aa25 Sandy Huang 2017-09-02  456  		encoder->bridge = lvds->bridge;
34cc0aa25 Sandy Huang 2017-09-02  457  	}
34cc0aa25 Sandy Huang 2017-09-02  458  
34cc0aa25 Sandy Huang 2017-09-02  459  	pm_runtime_enable(dev);
34cc0aa25 Sandy Huang 2017-09-02  460  	of_node_put(remote);
34cc0aa25 Sandy Huang 2017-09-02  461  	of_node_put(port);
34cc0aa25 Sandy Huang 2017-09-02  462  
34cc0aa25 Sandy Huang 2017-09-02  463  	return 0;
34cc0aa25 Sandy Huang 2017-09-02  464  
34cc0aa25 Sandy Huang 2017-09-02  465  err_free_connector:
34cc0aa25 Sandy Huang 2017-09-02  466  	drm_connector_cleanup(connector);
34cc0aa25 Sandy Huang 2017-09-02  467  err_free_encoder:
34cc0aa25 Sandy Huang 2017-09-02  468  	drm_encoder_cleanup(encoder);
34cc0aa25 Sandy Huang 2017-09-02  469  err_put_remote:
34cc0aa25 Sandy Huang 2017-09-02  470  	of_node_put(remote);
34cc0aa25 Sandy Huang 2017-09-02  471  err_put_port:
34cc0aa25 Sandy Huang 2017-09-02  472  	of_node_put(port);
34cc0aa25 Sandy Huang 2017-09-02  473  
34cc0aa25 Sandy Huang 2017-09-02  474  	return ret;
34cc0aa25 Sandy Huang 2017-09-02  475  }
34cc0aa25 Sandy Huang 2017-09-02  476  

:::::: The code at line 381 was first introduced by commit
:::::: 34cc0aa2545603560c79aaea3340d8ff3a71bd10 drm/rockchip: Add support for Rockchip Soc LVDS

:::::: TO: Sandy Huang <hjc@rock-chips.com>
:::::: CC: Mark Yao <mark.yao@rock-chips.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65228 bytes --]

Re: [PATCH 22/24] drm/bridge: remove the .of_node member

[Peter Rosin]

On 2018-04-28 10:09, kbuild test robot wrote:
> Hi Peter,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on v4.17-rc2]
> [also build test ERROR on next-20180426]
> [cannot apply to drm/drm-next robclark/msm-next drm-exynos/exynos-drm/for-next]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Peter-Rosin/device-link-bridge-supplier-drm-device/20180428-135229
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/gpu//drm/rockchip/rockchip_lvds.c: In function 'rockchip_lvds_bind':
>>> drivers/gpu//drm/rockchip/rockchip_lvds.c:381:24: error: 'struct drm_bridge' has no member named 'of_node'
>       remote = lvds->bridge->of_node;
>                            ^~
> 
> vim +381 drivers/gpu//drm/rockchip/rockchip_lvds.c

Ugh.

So, patch 1/24 needs to be amended with this

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..3f33034b3f58 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
        }
        if (lvds->panel)
                remote = lvds->panel->dev->of_node;
-       else
+       else if (lvds->bridge->of_node)
                remote = lvds->bridge->of_node;
+       else
+               remote = lvds->bridge->owner->of_node;
        if (of_property_read_string(dev->of_node, "rockchip,output", &name))
                /* default set it as output rgb */
                lvds->output = DISPLAY_OUTPUT_RGB;


and patch 22/24 with this

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 3f33034b3f58..8c82fa647536 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
        }
        if (lvds->panel)
                remote = lvds->panel->dev->of_node;
-       else if (lvds->bridge->of_node)
-               remote = lvds->bridge->of_node;
        else
                remote = lvds->bridge->owner->of_node;
        if (of_property_read_string(dev->of_node, "rockchip,output", &name))


But that is of course just a stop-gap. The real fix is to adapt to the
"drm: bridge: Add support for static image formats​" series from Jacopo.
But that's orthogonal.

Cheers,
Peter

Re: [PATCH 00/24] device link, bridge supplier <-> drm device

[Daniel Vetter]

On Fri, Apr 27, 2018 at 01:40:21AM +0300, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patches.
> 
> On Friday, 27 April 2018 01:31:15 EEST Peter Rosin wrote:
> > Hi!
> > 
> > It was noted by Russel King [1] that bridges (not using components)
> > might disappear unexpectedly if the owner of the bridge was unbound.
> > Jyri Sarha had previously noted the same thing with panels [2]. Jyri
> > came up with using device links to resolve the panel issue, which
> > was also my (independent) reaction to the note from Russel.
> > 
> > This series builds up to the addition of that link in the last
> > patch, but in my opinion the other 23 patches do have merit on their
> > own.
> > 
> > The last patch needs testing, while the others look trivial. That
> > said, I might have missed some subtlety.
> 
> I don't think this is the way to go. We have a real lifetime management 
> problem with bridges, and device links are just trying to hide the problem 
> under the carpet. They will further corner us by making a real fix much more 
> difficult to implement. I'll try to comment further in the next few days on 
> what I think a better solution would be, but in a nutshell I believe that 
> drm_bridge objects need to be refcounted, with a .release() operation to free 
> the bridge resources when the reference count drops to zero. This shouldn't be 
> difficult to implement and I'm willing to help.

I agree that refcounting is the proper fix if bridges can be
hot-unplugged, independently of the overall drm_device.

And yes retro-shoehorning refcounting is "fun", but it's also not
impossible. I've done it a few times by now in drm.

Otoh, refcounting has a pretty serious cost - we're still blowing up
drm_framebuffer refcounting in corner cases at shocking regularity, and
that's something that's been refcounted for years by now. As long as we
don't have a clearly defined need to make bridges hotpluggable then I
think not paying the hefty bill just yet is the correct technical
decision. This patch series here looks like a reasonable way to tie the
lifetimes together a bit more closely and should fix the bugs we have
right now.

Cheers, Daniel

PS: Yes there's cases where you can hotplug display blocks on a SoC. To my
knowledge they're all covered by hotplugging the entire drm_device (plus
all the statically associated bits like drm_bridge/crtc/plane/encoder).
And we're already working on fixing up drm_device to at least make it
possible to write a correct hot-unpluggable driver in theory. We'll see
whether anyone manages to pull that off in practice :-)

> 
> > [1] https://lkml.org/lkml/2018/4/23/769
> > [2] https://www.spinics.net/lists/dri-devel/msg174275.html
> > 
> > Peter Rosin (24):
> >   drm/bridge: allow optionally specifying an .owner device
> >   drm/bridge: adv7511: provide an .owner device
> >   drm/bridge/analogix: core: specify the .owner of the bridge
> >   drm/bridge: analogix-anx78xx: provide an .owner device
> >   drm/bridge: vga-dac: provide an .owner device
> >   drm/bridge: lvds-encoder: provide an .owner device
> >   drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an .owner device
> >   drm/bridge: nxp-ptn3460: provide an .owner device
> >   drm/bridge: panel: provide an .owner device
> >   drm/bridge: ps8622: provide an .owner device
> >   drm/bridge: sii902x: provide an .owner device
> >   drm/bridge: sii9234: provide an .owner device
> >   drm/bridge: sii8620: provide an .owner device
> >   drm/bridge: synopsys: provide an .owner device for the bridges
> >   drm/bridge: tc358767: provide an .owner device
> >   drm/bridge: ti-tfp410: provide an .owner device
> >   drm/exynos: mic: provide an .owner device for the bridge
> >   drm/mediatek: hdmi: provide an .owner device for the bridge
> >   drm/msm: specify the .owner of the bridges
> >   drm/rcar-du: lvds: provide an .owner device for the bridge
> >   drm/sti: provide an .owner device for the bridges
> >   drm/bridge: remove the .of_node member
> >   drm/bridge: require the .owner to be filled in on drm_bridge_attach
> >   drm/bridge: establish a link between the bridge supplier and consumer
> > 
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       |  2 +-
> >  drivers/gpu/drm/bridge/analogix-anx78xx.c          |  5 +----
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
> >  drivers/gpu/drm/bridge/dumb-vga-dac.c              |  2 +-
> >  drivers/gpu/drm/bridge/lvds-encoder.c              |  2 +-
> >  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  2 +-
> >  drivers/gpu/drm/bridge/nxp-ptn3460.c               |  2 +-
> >  drivers/gpu/drm/bridge/panel.c                     |  4 +---
> >  drivers/gpu/drm/bridge/parade-ps8622.c             |  2 +-
> >  drivers/gpu/drm/bridge/sii902x.c                   |  2 +-
> >  drivers/gpu/drm/bridge/sii9234.c                   |  2 +-
> >  drivers/gpu/drm/bridge/sil-sii8620.c               |  2 +-
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  4 +---
> >  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c      |  4 +---
> >  drivers/gpu/drm/bridge/tc358767.c                  |  2 +-
> >  drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
> >  drivers/gpu/drm/drm_bridge.c                       | 23 ++++++++++++++++++-
> >  drivers/gpu/drm/exynos/exynos_drm_mic.c            |  2 +-
> >  drivers/gpu/drm/mediatek/mtk_hdmi.c                |  2 +-
> >  drivers/gpu/drm/msm/dsi/dsi_manager.c              |  1 +
> >  drivers/gpu/drm/msm/edp/edp_bridge.c               |  1 +
> >  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  1 +
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c                |  2 +-
> >  drivers/gpu/drm/sti/sti_dvo.c                      |  2 +-
> >  drivers/gpu/drm/sti/sti_hda.c                      |  1 +
> >  drivers/gpu/drm/sti/sti_hdmi.c                     |  1 +
> >  include/drm/drm_bridge.h                           |  8 ++++----
> >  27 files changed, 51 insertions(+), 33 deletions(-)
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 23/24] drm/bridge: require the .owner to be filled in on drm_bridge_attach

[Daniel Vetter]

On Fri, Apr 27, 2018 at 12:31:38AM +0200, Peter Rosin wrote:
> The .owner will be handy to have around.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 9f023bd84d56..a038da696802 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -115,6 +115,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (!encoder || !bridge)
>  		return -EINVAL;
>  
> +	if (WARN_ON(!bridge->owner))
> +		return -EINVAL;

I think conceptually this is checked at the wrong place, and I think also misnamed
a bit. The ->owner is essentially the struct device (and its associated
driver) that provides the drm_bridge. As such it should be filled out
already at drm_bridge_add() time, and I think the check should be in
there. For driver-internal bridges it might make sense to also check this
here, not sure. Or just require all bridges get added.

Wrt the name, I think we should call this pdev or something. ->owner
usually means the module owner. I think in other subsystems ->dev is used,
but in drm we use ->dev for the drm_device pointer, so totally different
thing. pdev = physical device is the best I came up with. Better
suggestions very much welcome.
-Daniel

> +
>  	if (previous && (!previous->dev || previous->encoder != encoder))
>  		return -EINVAL;
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer

[Daniel Vetter]

On Fri, Apr 27, 2018 at 12:31:39AM +0200, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Minus the ->owner bikeshed I brought up in the previous patch I agree with
this approach as the best way to move forward for now.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

One small suggestion below, for merging I'd say pls get Jyri's
review/tested-by too, since you're both working on the same problem it
seems.

Aside: Do you want commit rights to drm-misc to be able to push work like
this?

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>  include/drm/drm_bridge.h     |  2 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index a038da696802..f0c79043ec43 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
>  #include <drm/drm_encoder.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -124,12 +125,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->owner) {

You might end up with a NULL encoder->dev->dev. Perhaps check that and
bail with a WARN_ON?

> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->owner, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->owner, "failed to link bridge to %s\n",
> +				dev_name(encoder->dev->dev));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			if (bridge->link)
> +				device_link_del(bridge->link);
> +			bridge->link = NULL;
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -156,6 +170,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	if (bridge->link)
> +		device_link_del(bridge->link);
> +	bridge->link = NULL;
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3bc659f3e7d2..9a386559a41a 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier
>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>  	struct drm_bridge *next;
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> +	struct device_link *link;
>  
>  	const struct drm_bridge_funcs *funcs;
>  	void *driver_private;
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 23/24] drm/bridge: require the .owner to be filled in on drm_bridge_attach

[Peter Rosin]

On 2018-04-30 17:24, Daniel Vetter wrote:
> On Fri, Apr 27, 2018 at 12:31:38AM +0200, Peter Rosin wrote:
>> The .owner will be handy to have around.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 9f023bd84d56..a038da696802 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -115,6 +115,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (!encoder || !bridge)
>>  		return -EINVAL;
>>  
>> +	if (WARN_ON(!bridge->owner))
>> +		return -EINVAL;
> 
> I think conceptually this is checked at the wrong place, and I think also misnamed
> a bit. The ->owner is essentially the struct device (and its associated
> driver) that provides the drm_bridge. As such it should be filled out
> already at drm_bridge_add() time, and I think the check should be in
> there. For driver-internal bridges it might make sense to also check this
> here, not sure. Or just require all bridges get added.

The reason for the position is that while I originally had the WARN in
drm_bridge_add, I found that quite a few bridges never call drm_bridge_add.
So I moved it. Other options are to start requiring all bridge suppliers to
call drm_bridge_add or to have the WARN in both function. Too me, it would
make sense to require all bridge suppliers to call drm_bridge_add, as that
enables other init stuff later, when needed. But that is a hairy patch to
get right, and is probably best left as a separate series.

> Wrt the name, I think we should call this pdev or something. ->owner
> usually means the module owner. I think in other subsystems ->dev is used,
> but in drm we use ->dev for the drm_device pointer, so totally different
> thing. pdev = physical device is the best I came up with. Better
> suggestions very much welcome.

pdev is about as problematic as owner. To me it reads "platform device".
And dev for a drm_device is also somewhat problematic, and I think that
drm would have been better, but dev for drm_device is probably quite
common. But one way to go is to rename the current dev to drm, so that
dev is freed up for the owner/supplier device. But that is a tedious
patch to write (I don't do the cocci thing).

Other suggestions I can think of: odev for owner device, sdev for supplier
device or just plain supplier.

Cheers,
Peter

> -Daniel
> 
>> +
>>  	if (previous && (!previous->dev || previous->encoder != encoder))
>>  		return -EINVAL;
>>  
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-04-30 17:32, Daniel Vetter wrote:
> On Fri, Apr 27, 2018 at 12:31:39AM +0200, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> Minus the ->owner bikeshed I brought up in the previous patch I agree with
> this approach as the best way to move forward for now.
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, let's see if Laurent is also on-board...

> One small suggestion below, for merging I'd say pls get Jyri's
> review/tested-by too, since you're both working on the same problem it
> seems.

Yes, I too would be very happy to see a tested-by from someone.

> Aside: Do you want commit rights to drm-misc to be able to push work like
> this?

If that makes life easier, sure.

Cheers,
Peter