linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/26] device link, bridge supplier <-> drm device
@ 2018-05-04 13:51 Peter Rosin
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
  2018-05-07 13:56 ` [PATCH v2 00/26] device link, bridge supplier <-> drm device Daniel Vetter
  0 siblings, 2 replies; 22+ messages in thread
From: Peter Rosin @ 2018-05-04 13:51 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Andrzej Hajda,
	Laurent Pinchart, Benjamin Gaignard, Heiko Stübner,
	Archit Taneja, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha

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 25 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. Jyri, are
you able to test? That said, I might have missed some subtlety.

Oh and the reason I'm pushing this is of course so that the issue
noted by Russel in [1] is addressed which in turn means that the
tda998x bridge driver can be patched according to that series without
objection (hopefully) and then used from the atmel-hlcdc driver (and
other drivers that are not componentized).

Changes since v1    https://lkml.org/lkml/2018/4/26/1018

- rename .owner to .odev to not get mixed up with the module owner.
- added patches for new recent drivers thc63lvd1024 and cdns-dsi
- fix for problem in the rockchip_lvds driver reported by 0day
- added a WARN in drm_bridge_add if there is no .odev owner device

I did *not*:
- add any ack from Daniel since he suggested "pdev", and I ended up
  with "odev" in the rename since I disliked "pdev" about as much
  as "owner".
- add any port id. The current .of_node (that this series removes)
  does not identify the port, so that problem seems orthogonal
  to me.

Cheers,
Peter

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

Peter Rosin (26):
  drm/bridge: allow optionally specifying an owner .odev device
  drm/bridge: adv7511: provide an owner .odev device
  drm/bridge/analogix: core: specify the owner .odev of the bridge
  drm/bridge: analogix-anx78xx: provide an owner .odev device
  drm/bridge: cdns-dsi: provide an owner .odev device
  drm/bridge: vga-dac: provide an owner .odev device
  drm/bridge: lvds-encoder: provide an owner .odev device
  drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev
    device
  drm/bridge: nxp-ptn3460: provide an owner .odev device
  drm/bridge: panel: provide an owner .odev device
  drm/bridge: ps8622: provide an owner .odev device
  drm/bridge: sii902x: provide an owner .odev device
  drm/bridge: sii9234: provide an owner .odev device
  drm/bridge: sii8620: provide an owner .odev device
  drm/bridge: synopsys: provide an owner .odev device for the bridges
  drm/bridge: tc358767: provide an owner .odev device
  drm/bridge: thc63lvd1024: provide an owner .odev device
  drm/bridge: ti-tfp410: provide an owner .odev device
  drm/exynos: mic: provide an owner .odev device for the bridge
  drm/mediatek: hdmi: provide an owner .odev device for the bridge
  drm/msm: specify the owner .odev of the bridges
  drm/rcar-du: lvds: provide an owner .odev device for the bridge
  drm/sti: provide an owner .odev device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the owner .odev to be filled in on
    drm_bridge_add/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/cdns-dsi.c                  |  2 +-
 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/thc63lvd1024.c              |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
 drivers/gpu/drm/drm_bridge.c                       | 26 +++++++++++++++++++++-
 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/rockchip/rockchip_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 +++----
 30 files changed, 57 insertions(+), 36 deletions(-)

-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
@ 2018-05-04 13:51   ` Peter Rosin
  2018-05-09 15:08     ` Andrzej Hajda
  2018-05-04 13:52   ` [PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges Peter Rosin
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-04 13:51 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Andrzej Hajda,
	Laurent Pinchart, Benjamin Gaignard, Heiko Stübner,
	Archit Taneja, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner device 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 ++-
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
 include/drm/drm_bridge.h                 | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3872f5379998 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->odev && bridge->odev->of_node == np) ||
+		    bridge->of_node == np) {
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..557e0079c98d 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->odev->of_node;
 	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
 		/* default set it as output rgb */
 		lvds->output = DISPLAY_OUTPUT_RGB;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7c17977c3537 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
+ * @odev: 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 *odev;
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
  2018-05-04 13:51   ` [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device Peter Rosin
@ 2018-05-04 13:52   ` Peter Rosin
  2018-05-04 13:52   ` [PATCH v2 24/26] drm/bridge: remove the .of_node member Peter Rosin
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Peter Rosin @ 2018-05-04 13:52 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: David Airlie, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Clark,
	Jyri Sarha, Daniel Vetter,
	freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, 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..1668e8abe5c1 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->odev = 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..4c56e29c57b7 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->odev = 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..aa6dd1bc5dc0 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->odev = hdmi->dev->dev;
 	bridge->funcs = &msm_hdmi_bridge_funcs;
 
 	ret = drm_bridge_attach(hdmi->encoder, bridge, NULL);
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 24/26] drm/bridge: remove the .of_node member
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
  2018-05-04 13:51   ` [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device Peter Rosin
  2018-05-04 13:52   ` [PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges Peter Rosin
@ 2018-05-04 13:52   ` Peter Rosin
  2018-05-04 13:52   ` [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach Peter Rosin
  2018-05-04 13:52   ` [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Peter Rosin
  4 siblings, 0 replies; 22+ messages in thread
From: Peter Rosin @ 2018-05-04 13:52 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Andrzej Hajda,
	Laurent Pinchart, Benjamin Gaignard, Heiko Stübner,
	Archit Taneja, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha

It is unused.

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

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3872f5379998..df084db33494 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->odev && bridge->odev->of_node == np) ||
-		    bridge->of_node == np) {
+		if (bridge->odev->of_node == np) {
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 557e0079c98d..e77d4c909582 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->odev->of_node;
 	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7c17977c3537..b656e505d11e 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

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
                     ` (2 preceding siblings ...)
  2018-05-04 13:52   ` [PATCH v2 24/26] drm/bridge: remove the .of_node member Peter Rosin
@ 2018-05-04 13:52   ` Peter Rosin
  2018-05-10  7:13     ` Andrzej Hajda
  2018-05-04 13:52   ` [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Peter Rosin
  4 siblings, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-04 13:52 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Andrzej Hajda,
	Laurent Pinchart, Benjamin Gaignard, Heiko Stübner,
	Archit Taneja, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha

The .odev owner device will be handy to have around.

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

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index df084db33494..78d186b6831b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+	if (WARN_ON(!bridge->odev))
+		return;
+
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
@@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (!encoder || !bridge)
 		return -EINVAL;
 
+	if (WARN_ON(!bridge->odev))
+		return -EINVAL;
+
 	if (previous && (!previous->dev || previous->encoder != encoder))
 		return -EINVAL;
 
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
                     ` (3 preceding siblings ...)
  2018-05-04 13:52   ` [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach Peter Rosin
@ 2018-05-04 13:52   ` Peter Rosin
  2018-05-07 12:59     ` Andrzej Hajda
  2018-05-10  8:10     ` Andrzej Hajda
  4 siblings, 2 replies; 22+ messages in thread
From: Peter Rosin @ 2018-05-04 13:52 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Andrzej Hajda,
	Laurent Pinchart, Benjamin Gaignard, Heiko Stübner,
	Archit Taneja, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha

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 78d186b6831b..0259f0a3ff27 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"
@@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (bridge->dev)
 		return -EBUSY;
 
+	if (encoder->dev->dev != bridge->odev) {
+		bridge->link = device_link_add(encoder->dev->dev,
+					       bridge->odev, 0);
+		if (!bridge->link) {
+			dev_err(bridge->odev, "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;
@@ -159,6 +173,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 b656e505d11e..804189c63a4c 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

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
  2018-05-04 13:52   ` [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Peter Rosin
@ 2018-05-07 12:59     ` Andrzej Hajda
       [not found]       ` <4cdcd215-8caf-e045-a478-f438f128c9f2-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  2018-05-10  8:10     ` Andrzej Hajda
  1 sibling, 1 reply; 22+ messages in thread
From: Andrzej Hajda @ 2018-05-07 12:59 UTC (permalink / raw)
  To: Peter Rosin, linux-kernel
  Cc: Archit Taneja, Laurent Pinchart, David Airlie,
	Peter Senna Tschudin, Martin Donnelly, Martyn Welch,
	Gustavo Padovan, Maarten Lankhorst, Sean Paul, Inki Dae,
	Joonyoung Shim, Seung-Woo Kim, Kyungmin Park, Kukjin Kim,
	Krzysztof Kozlowski, CK Hu, Philipp Zabel, Matthias Brugger,
	Rob Clark

On 04.05.2018 15:52, 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.

I understand rationales behind this patch, but it is another step into
making drm_dev one big driver with subcomponents, where drm will work
only if every subcomponent is working/loaded. Do we need to go this way?
In case of many platforms such approach results in display turned on
very late on boot for example due to late initialization of some
regulator exposed by some i2c device, which is used by hdmi bridge. And
this hdmi bridge is just to provide alternative(rarely used) display
path, the main display path would work anyway.

So the main question to drm maintainers is about evolution of bridges,
if drm_bridges should become mandatory components of drm device or they
could be added/removed dynamically?

Regards
Andrzej


>
> 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 78d186b6831b..0259f0a3ff27 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"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->odev) {
> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->odev, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->odev, "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;
> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
       [not found]       ` <4cdcd215-8caf-e045-a478-f438f128c9f2-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2018-05-07 13:43         ` Peter Rosin
  2018-05-08  9:03           ` Andrzej Hajda
  2018-05-07 13:53         ` Daniel Vetter
  1 sibling, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-07 13:43 UTC (permalink / raw)
  To: Andrzej Hajda, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
	Benjamin Gaignard, Heiko Stübner, Archit Taneja,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Joonyoung Shim,
	Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha, Inki Dae

On 2018-05-07 14:59, Andrzej Hajda wrote:
> On 04.05.2018 15:52, 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.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded.

The step is very small IMHO. Just a device-link, which is very easy to
remove once whatever other solution is ready.

>                                               Do we need to go this way?

If the drivers expect the parts to be there, and there is no other safety
net in place if they are not, what is the (short-term) alternative?

> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.

This patch does not contribute to any late init and any such delay is not
affected by this. At all.

> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

That is a much bigger question than this patch/series. Conflating the
two is not fair IMHO. You could run this very same argument for every
driver that gets added, since any additional driver will just make it
harder to make everything dynamic. Should we stop development right
away?

Besides, as long as the drm devices are in fact acting as big static
drivers (built from smaller parts), this should be considered a bug-fix
that will prevent dereference of stale pointers.

Or will some other solution appear and magically make all bridges and
drm drivers capable of dynamic reconfiguration in the next few weeks?
Yeah, right...

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> 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 78d186b6831b..0259f0a3ff27 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"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (bridge->dev)
>>  		return -EBUSY;
>>  
>> +	if (encoder->dev->dev != bridge->odev) {
>> +		bridge->link = device_link_add(encoder->dev->dev,
>> +					       bridge->odev, 0);
>> +		if (!bridge->link) {
>> +			dev_err(bridge->odev, "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;
>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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;
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
       [not found]       ` <4cdcd215-8caf-e045-a478-f438f128c9f2-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  2018-05-07 13:43         ` Peter Rosin
@ 2018-05-07 13:53         ` Daniel Vetter
  1 sibling, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2018-05-07 13:53 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Sandy Huang,
	Laurent Pinchart, Benjamin Gaignard, Heiko Stübner,
	Archit Taneja, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha

On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote:
> On 04.05.2018 15:52, 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.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded. Do we need to go this way?
> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.
> 
> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

This is already the case. You currently cannot hotplug a drm_bridge,
everything must be present. I don't think it makes sense to change that
until we have physically hotpluggable drm_bridges in real hardware. I
definitely don't want to refcount stuff to work around driver load
bonghits on DT platforms, because refcounting is way too hard to get right
:-)

Afaik there's out-of-tree patches to solve 99% of the driver load fun on
DT platforms, but because it's not a 100% solution it's blocked since
forever.
-Daniel

> 
> Regards
> Andrzej
> 
> 
> >
> > 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 78d186b6831b..0259f0a3ff27 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"
> > @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  	if (bridge->dev)
> >  		return -EBUSY;
> >  
> > +	if (encoder->dev->dev != bridge->odev) {
> > +		bridge->link = device_link_add(encoder->dev->dev,
> > +					       bridge->odev, 0);
> > +		if (!bridge->link) {
> > +			dev_err(bridge->odev, "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;
> > @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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;
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 00/26] device link, bridge supplier <-> drm device
  2018-05-04 13:51 [PATCH v2 00/26] device link, bridge supplier <-> drm device Peter Rosin
       [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
@ 2018-05-07 13:56 ` Daniel Vetter
  1 sibling, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2018-05-07 13:56 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim, linux-kernel,
	linux-renesas-soc, linux-mediatek, freedreno

On Fri, May 04, 2018 at 03:51:46PM +0200, 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 25 patches do have merit on their
> own.
> 
> The last patch needs testing, while the others look trivial. Jyri, are
> you able to test? That said, I might have missed some subtlety.
> 
> Oh and the reason I'm pushing this is of course so that the issue
> noted by Russel in [1] is addressed which in turn means that the
> tda998x bridge driver can be patched according to that series without
> objection (hopefully) and then used from the atmel-hlcdc driver (and
> other drivers that are not componentized).
> 
> Changes since v1    https://lkml.org/lkml/2018/4/26/1018
> 
> - rename .owner to .odev to not get mixed up with the module owner.
> - added patches for new recent drivers thc63lvd1024 and cdns-dsi
> - fix for problem in the rockchip_lvds driver reported by 0day
> - added a WARN in drm_bridge_add if there is no .odev owner device
> 
> I did *not*:
> - add any ack from Daniel since he suggested "pdev", and I ended up
>   with "odev" in the rename since I disliked "pdev" about as much
>   as "owner".

As long as it's not owner, I'm fine :-) Ack on the idea still holds.

> - add any port id. The current .of_node (that this series removes)
>   does not identify the port, so that problem seems orthogonal
>   to me.

Hm, from my cursory DT/of code reading last week I thought the port is
used to lookup the right node, but there's no port thing on the target for
a phandle? At least that's how current drm_of_find_panel_or_bridge seems
to work ...
-Daniel
> 
> Cheers,
> Peter
> 
> [1] https://lkml.org/lkml/2018/4/23/769
> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
> 
> Peter Rosin (26):
>   drm/bridge: allow optionally specifying an owner .odev device
>   drm/bridge: adv7511: provide an owner .odev device
>   drm/bridge/analogix: core: specify the owner .odev of the bridge
>   drm/bridge: analogix-anx78xx: provide an owner .odev device
>   drm/bridge: cdns-dsi: provide an owner .odev device
>   drm/bridge: vga-dac: provide an owner .odev device
>   drm/bridge: lvds-encoder: provide an owner .odev device
>   drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev
>     device
>   drm/bridge: nxp-ptn3460: provide an owner .odev device
>   drm/bridge: panel: provide an owner .odev device
>   drm/bridge: ps8622: provide an owner .odev device
>   drm/bridge: sii902x: provide an owner .odev device
>   drm/bridge: sii9234: provide an owner .odev device
>   drm/bridge: sii8620: provide an owner .odev device
>   drm/bridge: synopsys: provide an owner .odev device for the bridges
>   drm/bridge: tc358767: provide an owner .odev device
>   drm/bridge: thc63lvd1024: provide an owner .odev device
>   drm/bridge: ti-tfp410: provide an owner .odev device
>   drm/exynos: mic: provide an owner .odev device for the bridge
>   drm/mediatek: hdmi: provide an owner .odev device for the bridge
>   drm/msm: specify the owner .odev of the bridges
>   drm/rcar-du: lvds: provide an owner .odev device for the bridge
>   drm/sti: provide an owner .odev device for the bridges
>   drm/bridge: remove the .of_node member
>   drm/bridge: require the owner .odev to be filled in on
>     drm_bridge_add/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/cdns-dsi.c                  |  2 +-
>  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/thc63lvd1024.c              |  2 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
>  drivers/gpu/drm/drm_bridge.c                       | 26 +++++++++++++++++++++-
>  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/rockchip/rockchip_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 +++----
>  30 files changed, 57 insertions(+), 36 deletions(-)
> 
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
  2018-05-07 13:43         ` Peter Rosin
@ 2018-05-08  9:03           ` Andrzej Hajda
  0 siblings, 0 replies; 22+ messages in thread
From: Andrzej Hajda @ 2018-05-08  9:03 UTC (permalink / raw)
  To: Peter Rosin, linux-kernel
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim,
	linux-renesas-soc, linux-mediatek, freedreno

On 07.05.2018 15:43, Peter Rosin wrote:
> On 2018-05-07 14:59, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, 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.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded.
> The step is very small IMHO. Just a device-link, which is very easy to
> remove once whatever other solution is ready.
>
>>                                               Do we need to go this way?
> If the drivers expect the parts to be there, and there is no other safety
> net in place if they are not, what is the (short-term) alternative?
>
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
> This patch does not contribute to any late init and any such delay is not
> affected by this. At all.
>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> That is a much bigger question than this patch/series. Conflating the
> two is not fair IMHO. You could run this very same argument for every
> driver that gets added, since any additional driver will just make it
> harder to make everything dynamic. Should we stop development right
> away?
>
> Besides, as long as the drm devices are in fact acting as big static
> drivers (built from smaller parts), 

not true

> this should be considered a bug-fix
> that will prevent dereference of stale pointers.
>
> Or will some other solution appear and magically make all bridges and
> drm drivers capable of dynamic reconfiguration in the next few weeks?
> Yeah, right...

You are not changing single driver, you are changing framework and it
affects all the drivers using it, being more cautious about such patches
seems quite natural.

Anyway, I have realized that since drm_bridge_detach will remove the
link, so with properly written dynamic bridge removal, your patch should
not be a blocker.

Regards
Andrzej

>
> Cheers,
> Peter
>
>> Regards
>> Andrzej
>>
>>
>>> 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 78d186b6831b..0259f0a3ff27 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"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>  	if (bridge->dev)
>>>  		return -EBUSY;
>>>  
>>> +	if (encoder->dev->dev != bridge->odev) {
>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>> +					       bridge->odev, 0);
>>> +		if (!bridge->link) {
>>> +			dev_err(bridge->odev, "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;
>>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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;
>>
>
>
>

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device
  2018-05-04 13:51   ` [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device Peter Rosin
@ 2018-05-09 15:08     ` Andrzej Hajda
       [not found]       ` <4e92fdea-0609-0fff-0e3f-d9f78f596eb7-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 22+ messages in thread
From: Andrzej Hajda @ 2018-05-09 15:08 UTC (permalink / raw)
  To: Peter Rosin, linux-kernel
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim,
	linux-renesas-soc, linux-mediatek, freedreno

On 04.05.2018 15:51, Peter Rosin wrote:
> Bridge drivers can now (temporarily, in a transition phase) select if
> they want to provide a full owner device 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 ++-
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-

What is the reason to put rockchip here? Shouldn't be in separate patch?

>  include/drm/drm_bridge.h                 | 2 ++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..3872f5379998 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->odev && bridge->odev->of_node == np) ||
> +		    bridge->of_node == np) {
>  			mutex_unlock(&bridge_lock);
>  			return bridge;
>  		}
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 4bd94b167d2c..557e0079c98d 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->odev->of_node;

I guess odev should be NULL here, or have I missed something.

Regards
Andrzej

>  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
>  		/* default set it as output rgb */
>  		lvds->output = DISPLAY_OUTPUT_RGB;
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..7c17977c3537 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
> + * @odev: 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 *odev;
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *next;


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device
       [not found]       ` <4e92fdea-0609-0fff-0e3f-d9f78f596eb7-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2018-05-09 15:53         ` Peter Rosin
       [not found]           ` <4be4448e-763c-4832-f194-6b79afe87d08-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-09 15:53 UTC (permalink / raw)
  To: Andrzej Hajda, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
	Benjamin Gaignard, Heiko Stübner, Archit Taneja,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Joonyoung Shim,
	Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha, Inki Dae

On 2018-05-09 17:08, Andrzej Hajda wrote:
> On 04.05.2018 15:51, Peter Rosin wrote:
>> Bridge drivers can now (temporarily, in a transition phase) select if
>> they want to provide a full owner device 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 ++-
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
> 
> What is the reason to put rockchip here? Shouldn't be in separate patch?

Because the rockchip driver peeks into the bridge struct and all the
changes in this patch relate to making the new .odev member optional in
the transition phase, when the bridge can have either a new-style odev
or an old style of_node.

I guess this rockchip change could be patch 2, but it has to come first
after this patch and it makes no sense on its own. Hence, one patch.

This rockchip_lvds interaction is also present in patch 24/26
drm/bridge: remove the .of_node member

I can split them if you want, but I personally don't see the point.

>>  include/drm/drm_bridge.h                 | 2 ++
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 1638bfe9627c..3872f5379998 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->odev && bridge->odev->of_node == np) ||
>> +		    bridge->of_node == np) {
>>  			mutex_unlock(&bridge_lock);
>>  			return bridge;
>>  		}
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 4bd94b167d2c..557e0079c98d 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->odev->of_node;
> 
> I guess odev should be NULL here, or have I missed something.

s/should/could/ ???

Assuming that was what you meant and answering accordingly...

No, .odev cannot be NULL in that else branch. drm_of_find_panel_or_bridge
either found a panel or a bridge (or it failed). If it found a bridge
(which is the relevant branch for this question) that bridge would have
to have either an of_node (in the transition phase) or a valid .odev.
Otherwise the bridge could not have been found by
drm_of_find_panel_or_bridge.

*time passes*

Ahh, yes, .odev is always NULL here so you probably did mean "should".
But after patches 2-23 when bridges start populating .odev *instead*
of .of_node, .odev will not remain NULL. But as I said above, while
.odev is NULL, .of_node will never be NULL and vice versa (or
drm_of_find_panel_or_bridge could not have found the bridge) so there
is no risk of a NULL dereference.

Cheers,
Peter

> 
> Regards
> Andrzej
> 
>>  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
>>  		/* default set it as output rgb */
>>  		lvds->output = DISPLAY_OUTPUT_RGB;
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..7c17977c3537 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
>> + * @odev: 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 *odev;
>>  	struct drm_device *dev;
>>  	struct drm_encoder *encoder;
>>  	struct drm_bridge *next;
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device
       [not found]           ` <4be4448e-763c-4832-f194-6b79afe87d08-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
@ 2018-05-09 22:21             ` Peter Rosin
  2018-05-10  7:00               ` Andrzej Hajda
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-09 22:21 UTC (permalink / raw)
  To: Andrzej Hajda, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
	Benjamin Gaignard, Heiko Stübner, Archit Taneja,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Joonyoung Shim,
	Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha, Inki Dae

On 2018-05-09 17:53, Peter Rosin wrote:
> On 2018-05-09 17:08, Andrzej Hajda wrote:
>> On 04.05.2018 15:51, Peter Rosin wrote:
>>> Bridge drivers can now (temporarily, in a transition phase) select if
>>> they want to provide a full owner device 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 ++-
>>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
>>
>> What is the reason to put rockchip here? Shouldn't be in separate patch?
> 
> Because the rockchip driver peeks into the bridge struct and all the
> changes in this patch relate to making the new .odev member optional in
> the transition phase, when the bridge can have either a new-style odev
> or an old style of_node.
> 
> I guess this rockchip change could be patch 2, but it has to come first
> after this patch and it makes no sense on its own. Hence, one patch.
> 
> This rockchip_lvds interaction is also present in patch 24/26
> drm/bridge: remove the .of_node member
> 
> I can split them if you want, but I personally don't see the point.

I had a second look, and maybe the series should start with a patch like
this instead, so that the rockchip_lvds.c hunks can be removed from
patch 1/26 and 24/26. That would perhaps be slightly cleaner?

On the other hand, it's orthogonal and this series can be left as is
(with the benefit of me not having to do another iteration and you all
not having another bunch of messages to sift through). The below
patch could easily be (adjusted and) applied later instead. Or not,
since the right fix is to do this with the newfangled static image
format mechanism from Jacopo Mondi, and it might be easier to just do
it right.

State your preference.

Cheers,
Peter

From dee27b36a36acd271459d1489336b56132097425 Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@axentia.se>
Date: Wed, 9 May 2018 23:58:24 +0200
Subject: [PATCH] drm/rockchip: lvds: do not dig into the DT of remote bridges

The driver is trying to find the needed "data-mapping" for
interacting with the following bridge in the graphics chain.
But, doing so is bad since it is done w/o regard of the
compatible of the remote bridge, so the value of "data-mapping"
might not mean what this driver assumes. It is also pointless
since no bridge has any documented "data-mapping" DT property
and no dts file show any undocumented use.

Just remove the inappropriate code.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..fa3f4bf9712f 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
-		remote = lvds->bridge->of_node;
 	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
 		/* default set it as output rgb */
 		lvds->output = DISPLAY_OUTPUT_RGB;
-- 
2.11.0


_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device
  2018-05-09 22:21             ` Peter Rosin
@ 2018-05-10  7:00               ` Andrzej Hajda
  0 siblings, 0 replies; 22+ messages in thread
From: Andrzej Hajda @ 2018-05-10  7:00 UTC (permalink / raw)
  To: Peter Rosin, linux-kernel
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim,
	linux-renesas-soc, linux-mediatek, freedreno

On 10.05.2018 00:21, Peter Rosin wrote:
> On 2018-05-09 17:53, Peter Rosin wrote:
>> On 2018-05-09 17:08, Andrzej Hajda wrote:
>>> On 04.05.2018 15:51, Peter Rosin wrote:
>>>> Bridge drivers can now (temporarily, in a transition phase) select if
>>>> they want to provide a full owner device 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 ++-
>>>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
>>> What is the reason to put rockchip here? Shouldn't be in separate patch?
>> Because the rockchip driver peeks into the bridge struct and all the
>> changes in this patch relate to making the new .odev member optional in
>> the transition phase, when the bridge can have either a new-style odev
>> or an old style of_node.
>>
>> I guess this rockchip change could be patch 2, but it has to come first
>> after this patch and it makes no sense on its own. Hence, one patch.
>>
>> This rockchip_lvds interaction is also present in patch 24/26
>> drm/bridge: remove the .of_node member
>>
>> I can split them if you want, but I personally don't see the point.
> I had a second look, and maybe the series should start with a patch like
> this instead, so that the rockchip_lvds.c hunks can be removed from
> patch 1/26 and 24/26. That would perhaps be slightly cleaner?
>
> On the other hand, it's orthogonal and this series can be left as is
> (with the benefit of me not having to do another iteration and you all
> not having another bunch of messages to sift through). The below
> patch could easily be (adjusted and) applied later instead. Or not,
> since the right fix is to do this with the newfangled static image
> format mechanism from Jacopo Mondi, and it might be easier to just do
> it right.
>
> State your preference.

For me the current version is OK, it maybe lacks explanation why do you
need to touch rockchip, from my PoV it did not seem so obvious.
Somebody should fix rockchip to use Jacopo's approach instead of
violating abstractions, but this is another story.

With or without added missing explanation:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


>
> Cheers,
> Peter
>
> >From dee27b36a36acd271459d1489336b56132097425 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <peda@axentia.se>
> Date: Wed, 9 May 2018 23:58:24 +0200
> Subject: [PATCH] drm/rockchip: lvds: do not dig into the DT of remote bridges
>
> The driver is trying to find the needed "data-mapping" for
> interacting with the following bridge in the graphics chain.
> But, doing so is bad since it is done w/o regard of the
> compatible of the remote bridge, so the value of "data-mapping"
> might not mean what this driver assumes. It is also pointless
> since no bridge has any documented "data-mapping" DT property
> and no dts file show any undocumented use.
>
> Just remove the inappropriate code.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 4bd94b167d2c..fa3f4bf9712f 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
> -		remote = lvds->bridge->of_node;
>  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
>  		/* default set it as output rgb */
>  		lvds->output = DISPLAY_OUTPUT_RGB;


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach
  2018-05-04 13:52   ` [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach Peter Rosin
@ 2018-05-10  7:13     ` Andrzej Hajda
  0 siblings, 0 replies; 22+ messages in thread
From: Andrzej Hajda @ 2018-05-10  7:13 UTC (permalink / raw)
  To: Peter Rosin, linux-kernel
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim,
	linux-renesas-soc, linux-mediatek, freedreno

On 04.05.2018 15:52, Peter Rosin wrote:
> The .odev owner device will be handy to have around.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
> ---
>  drivers/gpu/drm/drm_bridge.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index df084db33494..78d186b6831b 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
>   */
>  void drm_bridge_add(struct drm_bridge *bridge)
>  {
> +	if (WARN_ON(!bridge->odev))
> +		return;
> +
>  	mutex_lock(&bridge_lock);
>  	list_add_tail(&bridge->list, &bridge_list);
>  	mutex_unlock(&bridge_lock);
> @@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (!encoder || !bridge)
>  		return -EINVAL;
>  
> +	if (WARN_ON(!bridge->odev))
> +		return -EINVAL;
> +
>  	if (previous && (!previous->dev || previous->encoder != encoder))
>  		return -EINVAL;
>  


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
  2018-05-04 13:52   ` [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Peter Rosin
  2018-05-07 12:59     ` Andrzej Hajda
@ 2018-05-10  8:10     ` Andrzej Hajda
       [not found]       ` <a723ad4a-8caa-4ff5-d39d-52db98a56d7b-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  1 sibling, 1 reply; 22+ messages in thread
From: Andrzej Hajda @ 2018-05-10  8:10 UTC (permalink / raw)
  To: Peter Rosin, linux-kernel
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim,
	linux-renesas-soc, linux-mediatek, freedreno

On 04.05.2018 15:52, 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>
> ---
>  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 78d186b6831b..0259f0a3ff27 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"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->odev) {

I wonder why device_link_add does not handle this case (self dependency)
silently as noop, as it seems to be a correct behavior.

> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->odev, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->odev, "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;
> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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

Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
to the bridge" would be better.

Anyway:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

>   * @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;


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
       [not found]       ` <a723ad4a-8caa-4ff5-d39d-52db98a56d7b-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2018-05-11  7:37         ` Peter Rosin
  2018-05-14 16:28           ` Daniel Vetter
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-11  7:37 UTC (permalink / raw)
  To: Andrzej Hajda, linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Martyn Welch, David Airlie, Gustavo Padovan,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
	Benjamin Gaignard, Heiko Stübner, Archit Taneja,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Joonyoung Shim,
	Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Kukjin Kim,
	Peter Senna Tschudin, CK Hu, Martin Donnelly, Daniel Vetter,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Maarten Lankhorst,
	Jyri Sarha, Inki Dae

On 2018-05-10 10:10, Andrzej Hajda wrote:
> On 04.05.2018 15:52, 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>
>> ---
>>  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 78d186b6831b..0259f0a3ff27 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"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (bridge->dev)
>>  		return -EBUSY;
>>  
>> +	if (encoder->dev->dev != bridge->odev) {
> 
> I wonder why device_link_add does not handle this case (self dependency)
> silently as noop, as it seems to be a correct behavior.

It's kind-of a silly corner-case though, so perfectly understandable
that it isn't handled.

>> +		bridge->link = device_link_add(encoder->dev->dev,
>> +					       bridge->odev, 0);
>> +		if (!bridge->link) {
>> +			dev_err(bridge->odev, "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;
>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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
> 
> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> to the bridge" would be better.

I meant "<->" to indicate that the link is bidirectional, not that the
relationship is in any way symmetric. I wasn't aware of any implication
of a symmetric relationship when using "<->", do you have a reference?
But I guess the different arrow notations in math are somewhat overloaded
and that someone at some point must have used "<->" to indicate a
symmetric relationship...

> Anyway:
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Thanks!

Cheers,
Peter

>  --
> Regards
> Andrzej
> 
>>   * @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;
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
  2018-05-11  7:37         ` Peter Rosin
@ 2018-05-14 16:28           ` Daniel Vetter
       [not found]             ` <73fa1ca3-28e4-96c5-1fc6-23e9c0cebb49@axentia.se>
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel Vetter @ 2018-05-14 16:28 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	linux-rockchip, Kukjin Kim, Peter Senna Tschudin,
	Martin Donnelly, linux-arm-msm, Jyri Sarha, Matthias Brugger,
	Vincent Abriou, linux-arm-kernel, Seung-Woo Kim, linux-kernel,
	linux-renesas-soc, linux-mediatek, freedreno

On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
> On 2018-05-10 10:10, Andrzej Hajda wrote:
> > On 04.05.2018 15:52, 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>
> >> ---
> >>  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 78d186b6831b..0259f0a3ff27 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"
> >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >>  	if (bridge->dev)
> >>  		return -EBUSY;
> >>  
> >> +	if (encoder->dev->dev != bridge->odev) {
> > 
> > I wonder why device_link_add does not handle this case (self dependency)
> > silently as noop, as it seems to be a correct behavior.
> 
> It's kind-of a silly corner-case though, so perfectly understandable
> that it isn't handled.
> 
> >> +		bridge->link = device_link_add(encoder->dev->dev,
> >> +					       bridge->odev, 0);
> >> +		if (!bridge->link) {
> >> +			dev_err(bridge->odev, "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;
> >> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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
> > 
> > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> > to the bridge" would be better.
> 
> I meant "<->" to indicate that the link is bidirectional, not that the
> relationship is in any way symmetric. I wasn't aware of any implication
> of a symmetric relationship when using "<->", do you have a reference?
> But I guess the different arrow notations in math are somewhat overloaded
> and that someone at some point must have used "<->" to indicate a
> symmetric relationship...

Yeah I agree with Andrzej here, for me <-> implies a symmetric
relationship. Spelling it out like Andrzej suggested sounds like the
better idea.
-Daniel

> 
> > Anyway:
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> Thanks!
> 
> Cheers,
> Peter
> 
> >  --
> > Regards
> > Andrzej
> > 
> >>   * @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;
> > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
       [not found]             ` <73fa1ca3-28e4-96c5-1fc6-23e9c0cebb49@axentia.se>
@ 2018-05-15 10:22               ` Daniel Vetter
       [not found]                 ` <CAKMK7uECSUo5k6uG3-y+yKQTGxB3FfGcwzMT+ZP5uux2SbpfUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 22+ messages in thread
From: Daniel Vetter @ 2018-05-15 10:22 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	open list:ARM/Rockchip SoC...,
	Kukjin Kim, Peter Senna Tschudin, Martin Donnelly, linux-arm-msm,
	Jyri Sarha, Matthias Brugger, Vincent Abriou, Linux ARM,
	Seung-Woo Kim, Linux Kernel Mailing List,
	open list:DRM DRIVERS FOR RENESAS

On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-14 18:28, Daniel Vetter wrote:
>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>> On 04.05.2018 15:52, 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>
>>>>> ---
>>>>>  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 78d186b6831b..0259f0a3ff27 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"
>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>>    if (bridge->dev)
>>>>>            return -EBUSY;
>>>>>
>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>
>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>> silently as noop, as it seems to be a correct behavior.
>>>
>>> It's kind-of a silly corner-case though, so perfectly understandable
>>> that it isn't handled.
>>>
>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
>>>>> +                                         bridge->odev, 0);
>>>>> +          if (!bridge->link) {
>>>>> +                  dev_err(bridge->odev, "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;
>>>>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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
>>>>
>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>>> to the bridge" would be better.
>>>
>>> I meant "<->" to indicate that the link is bidirectional, not that the
>>> relationship is in any way symmetric. I wasn't aware of any implication
>>> of a symmetric relationship when using "<->", do you have a reference?
>>> But I guess the different arrow notations in math are somewhat overloaded
>>> and that someone at some point must have used "<->" to indicate a
>>> symmetric relationship...
>>
>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
>> relationship. Spelling it out like Andrzej suggested sounds like the
>> better idea.
>> -Daniel
>
> Ok, I guess that means I have to do a v3 after all. Or can this
> trivial documentation update be done by the committer? I hate to
> spam everyone with another volley...
>
> Or perhaps I should squash patches 2-23 that are all rather similar
> and mechanic? I separated them to allow for easier review from
> individual driver maintainers, but that didn't seem to happen
> anyway...

Do another volley of the full set, or in-reply-to to just replace the
patch that needs to be respun (but some people don't like that).

When resending just make sure you're picking up all the acks/r-bs you
have already.
-Daniel
> Cheers,
> Peter
>
>>
>>>
>>>> Anyway:
>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>
>>> Thanks!
>>>
>>> Cheers,
>>> Peter
>>>
>>>>  --
>>>> Regards
>>>> Andrzej
>>>>
>>>>>   * @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;
>>>>
>>>>
>>>
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
       [not found]                 ` <CAKMK7uECSUo5k6uG3-y+yKQTGxB3FfGcwzMT+ZP5uux2SbpfUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-05-15 11:09                   ` Peter Rosin
  2018-05-16  9:31                     ` Daniel Vetter
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Rosin @ 2018-05-15 11:09 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Martyn Welch, David Airlie, Gustavo Padovan, dri-devel,
	Sandy Huang, Andrzej Hajda, Laurent Pinchart, Benjamin Gaignard,
	Heiko Stübner, Archit Taneja, linux-samsung-soc,
	Joonyoung Shim, Kyungmin Park, Krzysztof Kozlowski,
	open list:ARM/Rockchip SoC...,
	Kukjin Kim, Peter Senna Tschudin, CK Hu, Martin Donnelly,
	linux-arm-msm

On 2018-05-15 12:22, Daniel Vetter wrote:
> On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-14 18:28, Daniel Vetter wrote:
>>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>>> On 04.05.2018 15:52, 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>
>>>>>> ---
>>>>>>  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 78d186b6831b..0259f0a3ff27 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"
>>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>>>    if (bridge->dev)
>>>>>>            return -EBUSY;
>>>>>>
>>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>>
>>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>>> silently as noop, as it seems to be a correct behavior.
>>>>
>>>> It's kind-of a silly corner-case though, so perfectly understandable
>>>> that it isn't handled.
>>>>
>>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
>>>>>> +                                         bridge->odev, 0);
>>>>>> +          if (!bridge->link) {
>>>>>> +                  dev_err(bridge->odev, "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;
>>>>>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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
>>>>>
>>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>>>> to the bridge" would be better.
>>>>
>>>> I meant "<->" to indicate that the link is bidirectional, not that the
>>>> relationship is in any way symmetric. I wasn't aware of any implication
>>>> of a symmetric relationship when using "<->", do you have a reference?
>>>> But I guess the different arrow notations in math are somewhat overloaded
>>>> and that someone at some point must have used "<->" to indicate a
>>>> symmetric relationship...
>>>
>>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
>>> relationship. Spelling it out like Andrzej suggested sounds like the
>>> better idea.
>>> -Daniel
>>
>> Ok, I guess that means I have to do a v3 after all. Or can this
>> trivial documentation update be done by the committer? I hate to
>> spam everyone with another volley...
>>
>> Or perhaps I should squash patches 2-23 that are all rather similar
>> and mechanic? I separated them to allow for easier review from
>> individual driver maintainers, but that didn't seem to happen
>> anyway...
> 
> Do another volley of the full set, or in-reply-to to just replace the
> patch that needs to be respun (but some people don't like that).
> 
> When resending just make sure you're picking up all the acks/r-bs you
> have already.

Right, I always try to do that. One Ack that I did not include in v2
was the one you had on v1 24/24 (i.e. this patch). The reason I did
not add your Ack for v2 even on the patch where it obviously applied
was that I didn't know if you'd barf on the odev name.

But it was (and still is) a bit unclear if that was on Ack on the
last patch only, or if it was for the whole series? I think it might
have been for the whole series, but I'm not sure and I hate to be a
presumptuous idiot...

Cheers,
Peter

> -Daniel
>> Cheers,
>> Peter
>>
>>>
>>>>
>>>>> Anyway:
>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>
>>>> Thanks!
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>  --
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>>   * @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;
>>>>>
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
  2018-05-15 11:09                   ` Peter Rosin
@ 2018-05-16  9:31                     ` Daniel Vetter
  0 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2018-05-16  9:31 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Martyn Welch, David Airlie, dri-devel, Laurent Pinchart,
	linux-samsung-soc, Kyungmin Park, Krzysztof Kozlowski,
	open list:ARM/Rockchip SoC...,
	Kukjin Kim, Peter Senna Tschudin, Martin Donnelly, linux-arm-msm,
	Jyri Sarha, Matthias Brugger, Vincent Abriou, Linux ARM,
	Seung-Woo Kim, Linux Kernel Mailing List,
	open list:DRM DRIVERS FOR RENESAS

On Tue, May 15, 2018 at 01:09:59PM +0200, Peter Rosin wrote:
> On 2018-05-15 12:22, Daniel Vetter wrote:
> > On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
> >> On 2018-05-14 18:28, Daniel Vetter wrote:
> >>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
> >>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
> >>>>> On 04.05.2018 15:52, 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>
> >>>>>> ---
> >>>>>>  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 78d186b6831b..0259f0a3ff27 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"
> >>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >>>>>>    if (bridge->dev)
> >>>>>>            return -EBUSY;
> >>>>>>
> >>>>>> +  if (encoder->dev->dev != bridge->odev) {
> >>>>>
> >>>>> I wonder why device_link_add does not handle this case (self dependency)
> >>>>> silently as noop, as it seems to be a correct behavior.
> >>>>
> >>>> It's kind-of a silly corner-case though, so perfectly understandable
> >>>> that it isn't handled.
> >>>>
> >>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
> >>>>>> +                                         bridge->odev, 0);
> >>>>>> +          if (!bridge->link) {
> >>>>>> +                  dev_err(bridge->odev, "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;
> >>>>>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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
> >>>>>
> >>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> >>>>> to the bridge" would be better.
> >>>>
> >>>> I meant "<->" to indicate that the link is bidirectional, not that the
> >>>> relationship is in any way symmetric. I wasn't aware of any implication
> >>>> of a symmetric relationship when using "<->", do you have a reference?
> >>>> But I guess the different arrow notations in math are somewhat overloaded
> >>>> and that someone at some point must have used "<->" to indicate a
> >>>> symmetric relationship...
> >>>
> >>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
> >>> relationship. Spelling it out like Andrzej suggested sounds like the
> >>> better idea.
> >>> -Daniel
> >>
> >> Ok, I guess that means I have to do a v3 after all. Or can this
> >> trivial documentation update be done by the committer? I hate to
> >> spam everyone with another volley...
> >>
> >> Or perhaps I should squash patches 2-23 that are all rather similar
> >> and mechanic? I separated them to allow for easier review from
> >> individual driver maintainers, but that didn't seem to happen
> >> anyway...
> > 
> > Do another volley of the full set, or in-reply-to to just replace the
> > patch that needs to be respun (but some people don't like that).
> > 
> > When resending just make sure you're picking up all the acks/r-bs you
> > have already.
> 
> Right, I always try to do that. One Ack that I did not include in v2
> was the one you had on v1 24/24 (i.e. this patch). The reason I did
> not add your Ack for v2 even on the patch where it obviously applied
> was that I didn't know if you'd barf on the odev name.
> 
> But it was (and still is) a bit unclear if that was on Ack on the
> last patch only, or if it was for the whole series? I think it might
> have been for the whole series, but I'm not sure and I hate to be a
> presumptuous idiot...

Ack on the overall concept, and I'm ok with odev too. But definitely get
acks from relevant people (bridge/driver maintainers). In terms of
patches, I'd say patch 1 + 24-26 have my

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

-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >> Cheers,
> >> Peter
> >>
> >>>
> >>>>
> >>>>> Anyway:
> >>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> >>>>
> >>>> Thanks!
> >>>>
> >>>> Cheers,
> >>>> Peter
> >>>>
> >>>>>  --
> >>>>> Regards
> >>>>> Andrzej
> >>>>>
> >>>>>>   * @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;
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >> _______________________________________________
> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2018-05-16  9:31 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-04 13:51 [PATCH v2 00/26] device link, bridge supplier <-> drm device Peter Rosin
     [not found] ` <20180504135212.26977-1-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
2018-05-04 13:51   ` [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device Peter Rosin
2018-05-09 15:08     ` Andrzej Hajda
     [not found]       ` <4e92fdea-0609-0fff-0e3f-d9f78f596eb7-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2018-05-09 15:53         ` Peter Rosin
     [not found]           ` <4be4448e-763c-4832-f194-6b79afe87d08-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
2018-05-09 22:21             ` Peter Rosin
2018-05-10  7:00               ` Andrzej Hajda
2018-05-04 13:52   ` [PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges Peter Rosin
2018-05-04 13:52   ` [PATCH v2 24/26] drm/bridge: remove the .of_node member Peter Rosin
2018-05-04 13:52   ` [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach Peter Rosin
2018-05-10  7:13     ` Andrzej Hajda
2018-05-04 13:52   ` [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer Peter Rosin
2018-05-07 12:59     ` Andrzej Hajda
     [not found]       ` <4cdcd215-8caf-e045-a478-f438f128c9f2-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2018-05-07 13:43         ` Peter Rosin
2018-05-08  9:03           ` Andrzej Hajda
2018-05-07 13:53         ` Daniel Vetter
2018-05-10  8:10     ` Andrzej Hajda
     [not found]       ` <a723ad4a-8caa-4ff5-d39d-52db98a56d7b-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2018-05-11  7:37         ` Peter Rosin
2018-05-14 16:28           ` Daniel Vetter
     [not found]             ` <73fa1ca3-28e4-96c5-1fc6-23e9c0cebb49@axentia.se>
2018-05-15 10:22               ` Daniel Vetter
     [not found]                 ` <CAKMK7uECSUo5k6uG3-y+yKQTGxB3FfGcwzMT+ZP5uux2SbpfUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-05-15 11:09                   ` Peter Rosin
2018-05-16  9:31                     ` Daniel Vetter
2018-05-07 13:56 ` [PATCH v2 00/26] device link, bridge supplier <-> drm device Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).