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
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 <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .owner will be handy to have around.
Signed-off-by: Peter Rosin <[email protected]>
---
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
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 <[email protected]>
---
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
It is unused.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away and providing an .owner will become
mandatory.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
This will become mandatory.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
It gets rid of two #ifdefs and the .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
It gets rid of an #if and the .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
It gets rid of an #ifdef and the .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
The .of_node member is going away.
Signed-off-by: Peter Rosin <[email protected]>
---
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
This will become mandatory.
Signed-off-by: Peter Rosin <[email protected]>
---
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
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
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(-)
>
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.
>
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
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
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 <[email protected]>
:::::: CC: Mark Yao <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
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 <[email protected]>
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
>> ---
>> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
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 <[email protected]>
>
> 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 <[email protected]>
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