[PATCH 0/3] drm/komeda: Support for drm_bridge endpoints

[PATCH 0/3] drm/komeda: Support for drm_bridge endpoints

[Mihail Atanassov]

Greetings,

This series attempts to add support for endpoints (in the DT sense)
whose drivers only have a drm_bridge interface, unlike the tda998x,
which uses the component framework and provides all of drm_encoder,
drm_bridge, drm_connector.

1 & 2 should be fairly non-contentious, and I believe are valuable
enough on their own as they remove some pointer chasing and a few
allocations at the top level of komeda.

3 is the meat of this series, where I add the drm_bridge endpoint code.
This was tested with ti_tfp410 on the back end of a D71. I've tagged it
with an RFC since I drew inspiration from tilcdc, which does similar
shenanigans to detect tda998x vs non-tda998x, and is hence a precedent
for including similar handling, but I wasn't sure if there's a more well
established pattern.

Note that I opted not to remove the previous way of doing things for
tda998x, even though it could work with the drm_bridge handling
directly, since AFAIUI, device links haven't been implemented for
drm_bridge (see [1] for an attempt at doing that that never landed, and
I'm not aware of any refcounting having been added since), and therefore
getting a drm_bridge driver rmmod'ed while in use would be Bad(tm).

[1] https://lore.kernel.org/lkml/20180426223139.16740-1-peda@axentia.se/

Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Cc: James (Qian) Wang <james.qian.wang@arm.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <sean@poorly.run>


Mihail Atanassov (3):
  drm/komeda: Consolidate struct komeda_drv allocations
  drm/komeda: Memory manage struct komeda_drv in probe/remove
  drm/komeda: Allow non-component drm_bridge only endpoints

 .../gpu/drm/arm/display/komeda/komeda_dev.c   |  21 +--
 .../gpu/drm/arm/display/komeda/komeda_dev.h   |   9 +-
 .../gpu/drm/arm/display/komeda/komeda_drv.c   | 118 ++++++++-----
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 159 ++++++++++++++++--
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |   9 +-
 5 files changed, 243 insertions(+), 73 deletions(-)

-- 
2.23.0

[PATCH 1/3] drm/komeda: Consolidate struct komeda_drv allocations

[Mihail Atanassov]

struct komeda_drv has two pointer members only, and both
it and its members get allocated separately. Since both
members' types are in scope, there's not a lot to gain by
keeping the indirection around.

To avoid double-free issues, provide a barebones ->release()
hook for the driver.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_dev.c   | 21 ++++-------
 .../gpu/drm/arm/display/komeda/komeda_dev.h   |  4 +--
 .../gpu/drm/arm/display/komeda/komeda_drv.c   | 36 +++++++++----------
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 26 ++++++++------
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  4 +--
 5 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index 937a6d4c4865..c84f978cacc3 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -179,28 +179,23 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
 	return ret;
 }
 
-struct komeda_dev *komeda_dev_create(struct device *dev)
+int komeda_dev_init(struct komeda_dev *mdev, struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	const struct komeda_product_data *product;
-	struct komeda_dev *mdev;
 	struct resource *io_res;
 	int err = 0;
 
 	product = of_device_get_match_data(dev);
 	if (!product)
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 
 	io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!io_res) {
 		DRM_ERROR("No registers defined.\n");
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 	}
 
-	mdev = devm_kzalloc(dev, sizeof(*mdev), GFP_KERNEL);
-	if (!mdev)
-		return ERR_PTR(-ENOMEM);
-
 	mutex_init(&mdev->lock);
 
 	mdev->dev = dev;
@@ -284,16 +279,16 @@ struct komeda_dev *komeda_dev_create(struct device *dev)
 	komeda_debugfs_init(mdev);
 #endif
 
-	return mdev;
+	return 0;
 
 disable_clk:
 	clk_disable_unprepare(mdev->aclk);
 err_cleanup:
-	komeda_dev_destroy(mdev);
-	return ERR_PTR(err);
+	komeda_dev_fini(mdev);
+	return err;
 }
 
-void komeda_dev_destroy(struct komeda_dev *mdev)
+void komeda_dev_fini(struct komeda_dev *mdev)
 {
 	struct device *dev = mdev->dev;
 	const struct komeda_dev_funcs *funcs = mdev->funcs;
@@ -335,8 +330,6 @@ void komeda_dev_destroy(struct komeda_dev *mdev)
 		devm_clk_put(dev, mdev->aclk);
 		mdev->aclk = NULL;
 	}
-
-	devm_kfree(dev, mdev);
 }
 
 int komeda_dev_resume(struct komeda_dev *mdev)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
index 414200233b64..e392b8ffc372 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
@@ -213,8 +213,8 @@ komeda_product_match(struct komeda_dev *mdev, u32 target)
 const struct komeda_dev_funcs *
 d71_identify(u32 __iomem *reg, struct komeda_chip_info *chip);
 
-struct komeda_dev *komeda_dev_create(struct device *dev);
-void komeda_dev_destroy(struct komeda_dev *mdev);
+int komeda_dev_init(struct komeda_dev *mdev, struct device *dev);
+void komeda_dev_fini(struct komeda_dev *mdev);
 
 struct komeda_dev *dev_to_mdev(struct device *dev);
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index d6c2222c5d33..660181bdc008 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -14,15 +14,15 @@
 #include "komeda_kms.h"
 
 struct komeda_drv {
-	struct komeda_dev *mdev;
-	struct komeda_kms_dev *kms;
+	struct komeda_dev mdev;
+	struct komeda_kms_dev kms;
 };
 
 struct komeda_dev *dev_to_mdev(struct device *dev)
 {
 	struct komeda_drv *mdrv = dev_get_drvdata(dev);
 
-	return mdrv ? mdrv->mdev : NULL;
+	return mdrv ? &mdrv->mdev : NULL;
 }
 
 static void komeda_unbind(struct device *dev)
@@ -32,8 +32,8 @@ static void komeda_unbind(struct device *dev)
 	if (!mdrv)
 		return;
 
-	komeda_kms_detach(mdrv->kms);
-	komeda_dev_destroy(mdrv->mdev);
+	komeda_kms_fini(mdrv->kms);
+	komeda_dev_fini(mdrv->mdev);
 
 	dev_set_drvdata(dev, NULL);
 	devm_kfree(dev, mdrv);
@@ -48,24 +48,20 @@ static int komeda_bind(struct device *dev)
 	if (!mdrv)
 		return -ENOMEM;
 
-	mdrv->mdev = komeda_dev_create(dev);
-	if (IS_ERR(mdrv->mdev)) {
-		err = PTR_ERR(mdrv->mdev);
+	err = komeda_dev_init(&mdrv->mdev, dev);
+	if (err)
 		goto free_mdrv;
-	}
 
-	mdrv->kms = komeda_kms_attach(mdrv->mdev);
-	if (IS_ERR(mdrv->kms)) {
-		err = PTR_ERR(mdrv->kms);
-		goto destroy_mdev;
-	}
+	err = komeda_kms_init(&mdrv->kms, &mdrv->mdev);
+	if (err)
+		goto fini_mdev;
 
 	dev_set_drvdata(dev, mdrv);
 
 	return 0;
 
-destroy_mdev:
-	komeda_dev_destroy(mdrv->mdev);
+fini_mdev:
+	komeda_dev_fini(&mdrv->mdev);
 
 free_mdrv:
 	devm_kfree(dev, mdrv);
@@ -140,12 +136,12 @@ MODULE_DEVICE_TABLE(of, komeda_of_match);
 static int __maybe_unused komeda_pm_suspend(struct device *dev)
 {
 	struct komeda_drv *mdrv = dev_get_drvdata(dev);
-	struct drm_device *drm = &mdrv->kms->base;
+	struct drm_device *drm = &mdrv->kms.base;
 	int res;
 
 	res = drm_mode_config_helper_suspend(drm);
 
-	komeda_dev_suspend(mdrv->mdev);
+	komeda_dev_suspend(&mdrv->mdev);
 
 	return res;
 }
@@ -153,9 +149,9 @@ static int __maybe_unused komeda_pm_suspend(struct device *dev)
 static int __maybe_unused komeda_pm_resume(struct device *dev)
 {
 	struct komeda_drv *mdrv = dev_get_drvdata(dev);
-	struct drm_device *drm = &mdrv->kms->base;
+	struct drm_device *drm = &mdrv->kms.base;
 
-	komeda_dev_resume(mdrv->mdev);
+	komeda_dev_resume(&mdrv->mdev);
 
 	return drm_mode_config_helper_resume(drm);
 }
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index d49772de93e0..03dcf1d7688f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -23,6 +23,15 @@
 
 DEFINE_DRM_GEM_CMA_FOPS(komeda_cma_fops);
 
+static void komeda_kms_release(struct drm_device *dev)
+{
+	drm_dev_fini(dev);
+	/*
+	 * No need to kfree(dev) since we're stored in an aggregate struct
+	 * that's managed separately.
+	 */
+}
+
 static int komeda_gem_cma_dumb_create(struct drm_file *file,
 				      struct drm_device *dev,
 				      struct drm_mode_create_dumb *args)
@@ -60,6 +69,7 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data)
 static struct drm_driver komeda_kms_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
 	.lastclose			= drm_fb_helper_lastclose,
+	.release			= komeda_kms_release,
 	.gem_free_object_unlocked	= drm_gem_cma_free_object,
 	.gem_vm_ops			= &drm_gem_cma_vm_ops,
 	.dumb_create			= komeda_gem_cma_dumb_create,
@@ -257,19 +267,15 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
 	config->helper_private = &komeda_mode_config_helpers;
 }
 
-struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
+int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
 {
-	struct komeda_kms_dev *kms = kzalloc(sizeof(*kms), GFP_KERNEL);
 	struct drm_device *drm;
 	int err;
 
-	if (!kms)
-		return ERR_PTR(-ENOMEM);
-
 	drm = &kms->base;
 	err = drm_dev_init(drm, &komeda_kms_driver, mdev->dev);
 	if (err)
-		goto free_kms;
+		return err;
 
 	drm->dev_private = mdev;
 
@@ -319,7 +325,7 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
 	if (err)
 		goto free_interrupts;
 
-	return kms;
+	return 0;
 
 free_interrupts:
 	drm_kms_helper_poll_fini(drm);
@@ -332,12 +338,10 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
 	komeda_kms_cleanup_private_objs(kms);
 	drm->dev_private = NULL;
 	drm_dev_put(drm);
-free_kms:
-	kfree(kms);
-	return ERR_PTR(err);
+	return err;
 }
 
-void komeda_kms_detach(struct komeda_kms_dev *kms)
+void komeda_kms_fini(struct komeda_kms_dev *kms)
 {
 	struct drm_device *drm = &kms->base;
 	struct komeda_dev *mdev = drm->dev_private;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 45c498e15e7a..e81ceb298034 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -181,7 +181,7 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev *kms);
 void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 			      struct komeda_events *evts);
 
-struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
-void komeda_kms_detach(struct komeda_kms_dev *kms);
+int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev);
+void komeda_kms_fini(struct komeda_kms_dev *kms);
 
 #endif /*_KOMEDA_KMS_H_*/
-- 
2.23.0

[PATCH 2/3] drm/komeda: Memory manage struct komeda_drv in probe/remove

[Mihail Atanassov]

Some fields of komeda_drv members will be useful very early
in probe code, so make sure an instance is available.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_drv.c   | 30 +++++++++++--------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index 660181bdc008..9ed25ffe0e22 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -32,22 +32,15 @@ static void komeda_unbind(struct device *dev)
 	if (!mdrv)
 		return;
 
-	komeda_kms_fini(mdrv->kms);
-	komeda_dev_fini(mdrv->mdev);
-
-	dev_set_drvdata(dev, NULL);
-	devm_kfree(dev, mdrv);
+	komeda_kms_fini(&mdrv->kms);
+	komeda_dev_fini(&mdrv->mdev);
 }
 
 static int komeda_bind(struct device *dev)
 {
-	struct komeda_drv *mdrv;
+	struct komeda_drv *mdrv = dev_get_drvdata(dev);
 	int err;
 
-	mdrv = devm_kzalloc(dev, sizeof(*mdrv), GFP_KERNEL);
-	if (!mdrv)
-		return -ENOMEM;
-
 	err = komeda_dev_init(&mdrv->mdev, dev);
 	if (err)
 		goto free_mdrv;
@@ -56,8 +49,6 @@ static int komeda_bind(struct device *dev)
 	if (err)
 		goto fini_mdev;
 
-	dev_set_drvdata(dev, mdrv);
-
 	return 0;
 
 fini_mdev:
@@ -97,10 +88,15 @@ static int komeda_platform_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct component_match *match = NULL;
 	struct device_node *child;
+	struct komeda_drv *mdrv;
 
 	if (!dev->of_node)
 		return -ENODEV;
 
+	mdrv = devm_kzalloc(dev, sizeof(*mdrv), GFP_KERNEL);
+	if (!mdrv)
+		return -ENOMEM;
+
 	for_each_available_child_of_node(dev->of_node, child) {
 		if (of_node_cmp(child->name, "pipeline") != 0)
 			continue;
@@ -110,12 +106,20 @@ static int komeda_platform_probe(struct platform_device *pdev)
 		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
 	}
 
+	dev_set_drvdata(dev, mdrv);
+
 	return component_master_add_with_match(dev, &komeda_master_ops, match);
 }
 
 static int komeda_platform_remove(struct platform_device *pdev)
 {
-	component_master_del(&pdev->dev, &komeda_master_ops);
+	struct device *dev = &pdev->dev;
+	struct komeda_drv *mdrv = dev_get_drvdata(dev);
+
+	component_master_del(dev, &komeda_master_ops);
+
+	dev_set_drvdata(dev, NULL);
+	devm_kfree(dev, mdrv);
 	return 0;
 }
 
-- 
2.23.0

[RFC PATCH 3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Mihail Atanassov]

To support transmitters other than the tda998x, we need to allow
non-component framework bridges to be attached to a dummy drm_encoder in
our driver.

For the existing supported encoder (tda998x), keep the behaviour as-is,
since there's no way to unbind if a drm_bridge module goes away under
our feet.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
 .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
 4 files changed, 187 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
index e392b8ffc372..64d2902e2e0c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
@@ -176,6 +176,11 @@ struct komeda_dev {
 
 	/** @irq: irq number */
 	int irq;
+	/** @has_components:
+	 *
+	 * if true, use the component framework to bind/unbind driver
+	 */
+	bool has_components;
 
 	/** @lock: used to protect dpmode */
 	struct mutex lock;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index 9ed25ffe0e22..34cfc6a4c3e4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -10,6 +10,8 @@
 #include <linux/component.h>
 #include <linux/pm_runtime.h>
 #include <drm/drm_of.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_encoder.h>
 #include "komeda_dev.h"
 #include "komeda_kms.h"
 
@@ -69,18 +71,35 @@ static int compare_of(struct device *dev, void *data)
 	return dev->of_node == data;
 }
 
-static void komeda_add_slave(struct device *master,
-			     struct component_match **match,
-			     struct device_node *np,
-			     u32 port, u32 endpoint)
+static int komeda_add_slave(struct device *master,
+			    struct komeda_drv *mdrv,
+			    struct component_match **match,
+			    struct device_node *np,
+			    u32 port, u32 endpoint)
 {
 	struct device_node *remote;
+	struct drm_bridge *bridge;
+	int ret = 0;
 
 	remote = of_graph_get_remote_node(np, port, endpoint);
-	if (remote) {
+	if (!remote)
+		return 0;
+
+	if (komeda_remote_device_is_component(np, port, endpoint)) {
+		mdrv->mdev.has_components = true;
 		drm_of_component_match_add(master, match, compare_of, remote);
-		of_node_put(remote);
+		goto exit;
+	}
+
+	bridge = of_drm_find_bridge(remote);
+	if (!bridge) {
+		ret = -EPROBE_DEFER;
+		goto exit;
 	}
+
+exit:
+	of_node_put(remote);
+	return ret;
 }
 
 static int komeda_platform_probe(struct platform_device *pdev)
@@ -89,6 +108,7 @@ static int komeda_platform_probe(struct platform_device *pdev)
 	struct component_match *match = NULL;
 	struct device_node *child;
 	struct komeda_drv *mdrv;
+	int ret;
 
 	if (!dev->of_node)
 		return -ENODEV;
@@ -101,14 +121,27 @@ static int komeda_platform_probe(struct platform_device *pdev)
 		if (of_node_cmp(child->name, "pipeline") != 0)
 			continue;
 
-		/* add connector */
-		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 0);
-		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
+		/* attach any remote devices if present */
+		ret = komeda_add_slave(dev, mdrv, &match, child,
+				       KOMEDA_OF_PORT_OUTPUT, 0);
+		if (ret)
+			goto free_mdrv;
+		ret = komeda_add_slave(dev, mdrv, &match, child,
+				       KOMEDA_OF_PORT_OUTPUT, 1);
+		if (ret)
+			goto free_mdrv;
 	}
 
 	dev_set_drvdata(dev, mdrv);
 
-	return component_master_add_with_match(dev, &komeda_master_ops, match);
+	return mdrv->mdev.has_components
+		? component_master_add_with_match(
+			dev, &komeda_master_ops, match)
+		: komeda_bind(dev);
+
+free_mdrv:
+	devm_kfree(dev, mdrv);
+	return ret;
 }
 
 static int komeda_platform_remove(struct platform_device *pdev)
@@ -116,7 +149,10 @@ static int komeda_platform_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct komeda_drv *mdrv = dev_get_drvdata(dev);
 
-	component_master_del(dev, &komeda_master_ops);
+	if (mdrv->mdev.has_components)
+		component_master_del(dev, &komeda_master_ops);
+	else
+		komeda_unbind(dev);
 
 	dev_set_drvdata(dev, NULL);
 	devm_kfree(dev, mdrv);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 03dcf1d7688f..6eb52d1b20a0 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -6,6 +6,7 @@
  */
 #include <linux/component.h>
 #include <linux/interrupt.h>
+#include <linux/of_graph.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -14,6 +15,8 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_irq.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_of.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
@@ -267,6 +270,130 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
 	config->helper_private = &komeda_mode_config_helpers;
 }
 
+static void komeda_encoder_destroy(struct drm_encoder *encoder)
+{
+	drm_encoder_cleanup(encoder);
+}
+
+static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
+	.destroy = komeda_encoder_destroy,
+};
+
+bool komeda_remote_device_is_component(struct device_node *local,
+				       u32 port, u32 endpoint)
+{
+	struct device_node *remote;
+	char const * const component_devices[] = {
+		"nxp,tda998x",
+	};
+	int i;
+	bool ret = false;
+
+	remote = of_graph_get_remote_node(local, port, endpoint);
+	if (!remote)
+		return false;
+
+	/* Coprocessor endpoints are always component based. */
+	if (port != KOMEDA_OF_PORT_OUTPUT) {
+		ret = true;
+		goto exit;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(component_devices); ++i) {
+		if (of_device_is_compatible(remote, component_devices[i])) {
+			ret = true;
+			goto exit;
+		}
+	}
+
+exit:
+	of_node_put(remote);
+	return ret;
+}
+
+static int komeda_encoder_attach_bridge(struct komeda_dev *mdev,
+					struct komeda_kms_dev *kms,
+					struct drm_encoder *encoder,
+					struct device_node *np)
+{
+	struct device_node *remote;
+	struct drm_bridge *bridge;
+	u32 crtcs = 0;
+	int ret = 0;
+
+	if (komeda_remote_device_is_component(np, KOMEDA_OF_PORT_OUTPUT, 0))
+		return 0;
+
+	remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0);
+	if (!remote)
+		return 0;
+
+	bridge = of_drm_find_bridge(remote);
+	if (!bridge) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	crtcs = drm_of_find_possible_crtcs(&kms->base, remote);
+
+	encoder->possible_crtcs = crtcs ? crtcs : 1;
+
+	ret = drm_encoder_init(&kms->base, encoder,
+			       &komeda_dummy_enc_funcs, DRM_MODE_ENCODER_TMDS,
+			       NULL);
+	if (ret)
+		goto exit;
+
+	ret = drm_bridge_attach(encoder, bridge, NULL);
+	if (ret)
+		goto exit;
+
+exit:
+	of_node_put(remote);
+	return ret;
+}
+
+static int komeda_encoder_attach_bridges(struct komeda_kms_dev *kms,
+					 struct komeda_dev *mdev)
+{
+	int i, err;
+
+	for (i = 0; i < kms->n_crtcs; i++) {
+		err = komeda_encoder_attach_bridge(
+			mdev, kms,
+			&kms->encoders[i], mdev->pipelines[i]->of_node);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int komeda_kms_attach_external(struct komeda_kms_dev *kms,
+				      struct komeda_dev *mdev)
+{
+	int err;
+
+	if (mdev->has_components) {
+		err = component_bind_all(mdev->dev, kms);
+		if (err)
+			return err;
+	}
+
+	err = komeda_encoder_attach_bridges(kms, mdev);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static void komeda_kms_detach_external(struct komeda_dev *mdev,
+				       struct drm_device *drm)
+{
+	if (mdev->has_components)
+		component_unbind_all(mdev->dev, drm);
+}
+
 int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
 {
 	struct drm_device *drm;
@@ -301,7 +428,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
 	if (err)
 		goto cleanup_mode_config;
 
-	err = component_bind_all(mdev->dev, kms);
+	err = komeda_kms_attach_external(kms, mdev);
 	if (err)
 		goto cleanup_mode_config;
 
@@ -332,7 +459,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
 	drm->irq_enabled = false;
 	mdev->funcs->disable_irq(mdev);
 free_component_binding:
-	component_unbind_all(mdev->dev, drm);
+	komeda_kms_detach_external(mdev, drm);
 cleanup_mode_config:
 	drm_mode_config_cleanup(drm);
 	komeda_kms_cleanup_private_objs(kms);
@@ -351,7 +478,7 @@ void komeda_kms_fini(struct komeda_kms_dev *kms)
 	drm_atomic_helper_shutdown(drm);
 	drm->irq_enabled = false;
 	mdev->funcs->disable_irq(mdev);
-	component_unbind_all(mdev->dev, drm);
+	komeda_kms_detach_external(mdev, drm);
 	drm_mode_config_cleanup(drm);
 	komeda_kms_cleanup_private_objs(kms);
 	drm->dev_private = NULL;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index e81ceb298034..c2856e19d092 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -12,6 +12,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_device.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_writeback.h>
 #include <drm/drm_print.h>
 
@@ -123,6 +124,7 @@ struct komeda_kms_dev {
 	int n_crtcs;
 	/** @crtcs: crtcs list */
 	struct komeda_crtc crtcs[KOMEDA_MAX_PIPELINES];
+	struct drm_encoder encoders[KOMEDA_MAX_PIPELINES];
 };
 
 #define to_kplane(p)	container_of(p, struct komeda_plane, base)
@@ -184,4 +186,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev);
 void komeda_kms_fini(struct komeda_kms_dev *kms);
 
+bool komeda_remote_device_is_component(struct device_node *local,
+				       u32 port, u32 endpoint);
+
 #endif /*_KOMEDA_KMS_H_*/
-- 
2.23.0

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[james qian wang (Arm Technology China)]

On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote:
> To support transmitters other than the tda998x, we need to allow
> non-component framework bridges to be attached to a dummy drm_encoder in
> our driver.
> 
> For the existing supported encoder (tda998x), keep the behaviour as-is,
> since there's no way to unbind if a drm_bridge module goes away under
> our feet.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> ---
>  .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
>  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
>  4 files changed, 187 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index e392b8ffc372..64d2902e2e0c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -176,6 +176,11 @@ struct komeda_dev {
>  
>  	/** @irq: irq number */
>  	int irq;
> +	/** @has_components:
> +	 *
> +	 * if true, use the component framework to bind/unbind driver
> +	 */
> +	bool has_components;
>  
>  	/** @lock: used to protect dpmode */
>  	struct mutex lock;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 9ed25ffe0e22..34cfc6a4c3e4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -10,6 +10,8 @@
>  #include <linux/component.h>
>  #include <linux/pm_runtime.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_encoder.h>
>  #include "komeda_dev.h"
>  #include "komeda_kms.h"
>  
> @@ -69,18 +71,35 @@ static int compare_of(struct device *dev, void *data)
>  	return dev->of_node == data;
>  }
>  
> -static void komeda_add_slave(struct device *master,
> -			     struct component_match **match,
> -			     struct device_node *np,
> -			     u32 port, u32 endpoint)
> +static int komeda_add_slave(struct device *master,
> +			    struct komeda_drv *mdrv,
> +			    struct component_match **match,
> +			    struct device_node *np,
> +			    u32 port, u32 endpoint)
>  {
>  	struct device_node *remote;
> +	struct drm_bridge *bridge;
> +	int ret = 0;
>  
>  	remote = of_graph_get_remote_node(np, port, endpoint);
> -	if (remote) {
> +	if (!remote)
> +		return 0;
> +
> +	if (komeda_remote_device_is_component(np, port, endpoint)) {
> +		mdrv->mdev.has_components = true;
>  		drm_of_component_match_add(master, match, compare_of, remote);
> -		of_node_put(remote);
> +		goto exit;
> +	}
> +
> +	bridge = of_drm_find_bridge(remote);
> +	if (!bridge) {
> +		ret = -EPROBE_DEFER;
> +		goto exit;
>  	}
> +
> +exit:
> +	of_node_put(remote);
> +	return ret;
>  }
>  
>  static int komeda_platform_probe(struct platform_device *pdev)
> @@ -89,6 +108,7 @@ static int komeda_platform_probe(struct platform_device *pdev)
>  	struct component_match *match = NULL;
>  	struct device_node *child;
>  	struct komeda_drv *mdrv;
> +	int ret;
>  
>  	if (!dev->of_node)
>  		return -ENODEV;
> @@ -101,14 +121,27 @@ static int komeda_platform_probe(struct platform_device *pdev)
>  		if (of_node_cmp(child->name, "pipeline") != 0)
>  			continue;
>  
> -		/* add connector */
> -		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 0);
> -		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
> +		/* attach any remote devices if present */
> +		ret = komeda_add_slave(dev, mdrv, &match, child,
> +				       KOMEDA_OF_PORT_OUTPUT, 0);
> +		if (ret)
> +			goto free_mdrv;
> +		ret = komeda_add_slave(dev, mdrv, &match, child,
> +				       KOMEDA_OF_PORT_OUTPUT, 1);
> +		if (ret)
> +			goto free_mdrv;
>  	}
>  
>  	dev_set_drvdata(dev, mdrv);
>  
> -	return component_master_add_with_match(dev, &komeda_master_ops, match);
> +	return mdrv->mdev.has_components
> +		? component_master_add_with_match(
> +			dev, &komeda_master_ops, match)
> +		: komeda_bind(dev);
> +
> +free_mdrv:
> +	devm_kfree(dev, mdrv);
> +	return ret;
>  }
>  
>  static int komeda_platform_remove(struct platform_device *pdev)
> @@ -116,7 +149,10 @@ static int komeda_platform_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct komeda_drv *mdrv = dev_get_drvdata(dev);
>  
> -	component_master_del(dev, &komeda_master_ops);
> +	if (mdrv->mdev.has_components)
> +		component_master_del(dev, &komeda_master_ops);
> +	else
> +		komeda_unbind(dev);
>  
>  	dev_set_drvdata(dev, NULL);
>  	devm_kfree(dev, mdrv);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 03dcf1d7688f..6eb52d1b20a0 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -6,6 +6,7 @@
>   */
>  #include <linux/component.h>
>  #include <linux/interrupt.h>
> +#include <linux/of_graph.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -14,6 +15,8 @@
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_irq.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -267,6 +270,130 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
>  	config->helper_private = &komeda_mode_config_helpers;
>  }
>  
> +static void komeda_encoder_destroy(struct drm_encoder *encoder)
> +{
> +	drm_encoder_cleanup(encoder);
> +}
> +
> +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
> +	.destroy = komeda_encoder_destroy,
> +};
> +
> +bool komeda_remote_device_is_component(struct device_node *local,
> +				       u32 port, u32 endpoint)
> +{
> +	struct device_node *remote;
> +	char const * const component_devices[] = {
> +		"nxp,tda998x",

I really don't think put this dummy_encoder into komeda is a good
idea.

And I suggest to seperate this dummy_encoder to a individual module
which will build the drm_ridge to a standard drm encoder and component
based module, which will be enable by DT, totally transparent for komeda.

BTW:
I really don't like such logic: distingush the SYSTEM configuration
by check the connected device name, it's hard to maintain and code
sharing, and that's why NOW we have the device-tree.

Thanks
James

> +	};
> +	int i;
> +	bool ret = false;
> +
> +	remote = of_graph_get_remote_node(local, port, endpoint);
> +	if (!remote)
> +		return false;
> +
> +	/* Coprocessor endpoints are always component based. */
> +	if (port != KOMEDA_OF_PORT_OUTPUT) {
> +		ret = true;
> +		goto exit;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(component_devices); ++i) {
> +		if (of_device_is_compatible(remote, component_devices[i])) {
> +			ret = true;
> +			goto exit;
> +		}
> +	}
> +
> +exit:
> +	of_node_put(remote);
> +	return ret;
> +}
> +
> +static int komeda_encoder_attach_bridge(struct komeda_dev *mdev,
> +					struct komeda_kms_dev *kms,
> +					struct drm_encoder *encoder,
> +					struct device_node *np)
> +{
> +	struct device_node *remote;
> +	struct drm_bridge *bridge;
> +	u32 crtcs = 0;
> +	int ret = 0;
> +
> +	if (komeda_remote_device_is_component(np, KOMEDA_OF_PORT_OUTPUT, 0))
> +		return 0;
> +
> +	remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0);
> +	if (!remote)
> +		return 0;
> +
> +	bridge = of_drm_find_bridge(remote);
> +	if (!bridge) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	crtcs = drm_of_find_possible_crtcs(&kms->base, remote);
> +
> +	encoder->possible_crtcs = crtcs ? crtcs : 1;
> +
> +	ret = drm_encoder_init(&kms->base, encoder,
> +			       &komeda_dummy_enc_funcs, DRM_MODE_ENCODER_TMDS,
> +			       NULL);
> +	if (ret)
> +		goto exit;
> +
> +	ret = drm_bridge_attach(encoder, bridge, NULL);
> +	if (ret)
> +		goto exit;
> +
> +exit:
> +	of_node_put(remote);
> +	return ret;
> +}
> +
> +static int komeda_encoder_attach_bridges(struct komeda_kms_dev *kms,
> +					 struct komeda_dev *mdev)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < kms->n_crtcs; i++) {
> +		err = komeda_encoder_attach_bridge(
> +			mdev, kms,
> +			&kms->encoders[i], mdev->pipelines[i]->of_node);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int komeda_kms_attach_external(struct komeda_kms_dev *kms,
> +				      struct komeda_dev *mdev)
> +{
> +	int err;
> +
> +	if (mdev->has_components) {
> +		err = component_bind_all(mdev->dev, kms);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = komeda_encoder_attach_bridges(kms, mdev);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void komeda_kms_detach_external(struct komeda_dev *mdev,
> +				       struct drm_device *drm)
> +{
> +	if (mdev->has_components)
> +		component_unbind_all(mdev->dev, drm);
> +}
> +
>  int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
>  {
>  	struct drm_device *drm;
> @@ -301,7 +428,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
>  	if (err)
>  		goto cleanup_mode_config;
>  
> -	err = component_bind_all(mdev->dev, kms);
> +	err = komeda_kms_attach_external(kms, mdev);
>  	if (err)
>  		goto cleanup_mode_config;
>  
> @@ -332,7 +459,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
>  	drm->irq_enabled = false;
>  	mdev->funcs->disable_irq(mdev);
>  free_component_binding:
> -	component_unbind_all(mdev->dev, drm);
> +	komeda_kms_detach_external(mdev, drm);
>  cleanup_mode_config:
>  	drm_mode_config_cleanup(drm);
>  	komeda_kms_cleanup_private_objs(kms);
> @@ -351,7 +478,7 @@ void komeda_kms_fini(struct komeda_kms_dev *kms)
>  	drm_atomic_helper_shutdown(drm);
>  	drm->irq_enabled = false;
>  	mdev->funcs->disable_irq(mdev);
> -	component_unbind_all(mdev->dev, drm);
> +	komeda_kms_detach_external(mdev, drm);
>  	drm_mode_config_cleanup(drm);
>  	komeda_kms_cleanup_private_objs(kms);
>  	drm->dev_private = NULL;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index e81ceb298034..c2856e19d092 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -12,6 +12,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_encoder.h>
>  #include <drm/drm_writeback.h>
>  #include <drm/drm_print.h>
>  
> @@ -123,6 +124,7 @@ struct komeda_kms_dev {
>  	int n_crtcs;
>  	/** @crtcs: crtcs list */
>  	struct komeda_crtc crtcs[KOMEDA_MAX_PIPELINES];
> +	struct drm_encoder encoders[KOMEDA_MAX_PIPELINES];
>  };
>  
>  #define to_kplane(p)	container_of(p, struct komeda_plane, base)
> @@ -184,4 +186,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev);
>  void komeda_kms_fini(struct komeda_kms_dev *kms);
>  
> +bool komeda_remote_device_is_component(struct device_node *local,
> +				       u32 port, u32 endpoint);
> +
>  #endif /*_KOMEDA_KMS_H_*/

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Mihail Atanassov]

Hi James,

On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote:
> On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote:
> > To support transmitters other than the tda998x, we need to allow
> > non-component framework bridges to be attached to a dummy drm_encoder in
> > our driver.
> > 
> > For the existing supported encoder (tda998x), keep the behaviour as-is,
> > since there's no way to unbind if a drm_bridge module goes away under
> > our feet.
> > 
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > ---
> >  .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
> >  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
> >  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
> >  .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
> >  4 files changed, 187 insertions(+), 14 deletions(-)
> > 
> > [snip]
> >  
> > +static void komeda_encoder_destroy(struct drm_encoder *encoder)
> > +{
> > +	drm_encoder_cleanup(encoder);
> > +}
> > +
> > +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
> > +	.destroy = komeda_encoder_destroy,
> > +};
> > +
> > +bool komeda_remote_device_is_component(struct device_node *local,
> > +				       u32 port, u32 endpoint)
> > +{
> > +	struct device_node *remote;
> > +	char const * const component_devices[] = {
> > +		"nxp,tda998x",
> 
> I really don't think put this dummy_encoder into komeda is a good
> idea.
> 
> And I suggest to seperate this dummy_encoder to a individual module
> which will build the drm_ridge to a standard drm encoder and component
> based module, which will be enable by DT, totally transparent for komeda.
> 
> BTW:
> I really don't like such logic: distingush the SYSTEM configuration
> by check the connected device name, it's hard to maintain and code
> sharing, and that's why NOW we have the device-tree.

+Cc Brian

I didn't think DT is the right place for pseudo-devices. The tda998x
looks to be in a small group of drivers that contain encoder +
bridge + connector; my impression of the current state of affairs is
that the drm_encoder tends to live where the CRTC provider is rather
than representing a HW entity (hence why drm_bridge based drivers
exist in drivers/gpu/drm/bridge). See the overview DOC comment in
drm_encoder.c ("drivers are free to use [drm_encoder] however they
wish"). I may be completely wrong, so would appreciate some
context and comment from others on the Cc list.

In any case, converting a do-nothing dummy encoder into its own
component-module will add a lot more bloat compared to the current
~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind,
a few extra structs here and there, yet another object file, DT
bindings, docs for the same, and maintaining all of that? It's a hard
sell for me. I'd prefer if we went ahead with the code as-is and fix it
up later if it really proves unwieldy and too hard to maintain. Could
this patch be improved? Sure! Can we improve it in follow-up work
though, as I think this is valuable enough on its own without any major
drawbacks?

As per my cover letter, in an ideal world we'd have the encoder locally
and do drm_bridge_attach() even for tda998x, but the lifetime issues
around bridges inside modules mean that doing that now is a bit of a
step back for this specific case.

> 
> Thanks
> James
> 
> > [snip]
> 

-- 
Mihail

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Brian Starkey]

Hi,

On Wed, Oct 16, 2019 at 03:51:39PM +0000, Mihail Atanassov wrote:
> Hi James,
> 
> On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote:
> > On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote:
> > > To support transmitters other than the tda998x, we need to allow
> > > non-component framework bridges to be attached to a dummy drm_encoder in
> > > our driver.
> > > 
> > > For the existing supported encoder (tda998x), keep the behaviour as-is,
> > > since there's no way to unbind if a drm_bridge module goes away under
> > > our feet.
> > > 
> > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > ---
> > >  .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
> > >  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
> > >  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
> > >  .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
> > >  4 files changed, 187 insertions(+), 14 deletions(-)
> > > 
> > > [snip]
> > >  
> > > +static void komeda_encoder_destroy(struct drm_encoder *encoder)
> > > +{
> > > +	drm_encoder_cleanup(encoder);
> > > +}
> > > +
> > > +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
> > > +	.destroy = komeda_encoder_destroy,
> > > +};
> > > +
> > > +bool komeda_remote_device_is_component(struct device_node *local,
> > > +				       u32 port, u32 endpoint)
> > > +{
> > > +	struct device_node *remote;
> > > +	char const * const component_devices[] = {
> > > +		"nxp,tda998x",
> > 
> > I really don't think put this dummy_encoder into komeda is a good
> > idea.
> > 
> > And I suggest to seperate this dummy_encoder to a individual module
> > which will build the drm_ridge to a standard drm encoder and component
> > based module, which will be enable by DT, totally transparent for komeda.
> > 
> > BTW:
> > I really don't like such logic: distingush the SYSTEM configuration
> > by check the connected device name, it's hard to maintain and code
> > sharing, and that's why NOW we have the device-tree.

It's not ideal to have such special cases, for sure. However, I don't
see this approach causing us any issues. tda998x really is "special",
and as far as I can see the code here scales to other devices pretty
easily.

> 
> +Cc Brian
> 
> I didn't think DT is the right place for pseudo-devices.

I agree. DT should represent the HW, not the structure of the
linux kernel subsystem.

> The tda998x
> looks to be in a small group of drivers that contain encoder +
> bridge + connector; my impression of the current state of affairs is
> that the drm_encoder tends to live where the CRTC provider is rather
> than representing a HW entity (hence why drm_bridge based drivers
> exist in drivers/gpu/drm/bridge). See the overview DOC comment in
> drm_encoder.c ("drivers are free to use [drm_encoder] however they
> wish"). I may be completely wrong, so would appreciate some
> context and comment from others on the Cc list.
> 
> In any case, converting a do-nothing dummy encoder into its own
> component-module will add a lot more bloat compared to the current
> ~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind,
> a few extra structs here and there, yet another object file, DT
> bindings, docs for the same, and maintaining all of that? It's a hard
> sell for me. I'd prefer if we went ahead with the code as-is and fix it
> up later if it really proves unwieldy and too hard to maintain. Could
> this patch be improved? Sure! Can we improve it in follow-up work
> though, as I think this is valuable enough on its own without any major
> drawbacks?
> 

Even if we implemented a separate component encoder, as far as I can
tell there's no way to use it without either:

a) sticking it in DT
b) invoking it from komeda based on a "of_device_is_compatible" list

IMO a) isn't acceptable, and b) doesn't have any advantages over this
approach.

> As per my cover letter, in an ideal world we'd have the encoder locally
> and do drm_bridge_attach() even for tda998x, but the lifetime issues
> around bridges inside modules mean that doing that now is a bit of a
> step back for this specific case.
> 

Yeah, my feeling is that being able to keep tda998x as a component
(for the superior bind/unbind behaviour) is worth the slight ugliness,
at least until bridges get the same functionality.

If James is strongly against merging this, maybe we just swap
wholesale to bridge? But for me, the pragmatic approach would be this
stop-gap.

Cheers,
-Brian

> > 
> > Thanks
> > James
> > 
> > > [snip]
> > 
> 
> -- 
> Mihail
> 
> 
> 

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[james qian wang (Arm Technology China)]

On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> Hi,
> 
> On Wed, Oct 16, 2019 at 03:51:39PM +0000, Mihail Atanassov wrote:
> > Hi James,
> > 
> > On Wednesday, 9 October 2019 06:54:15 BST james qian wang (Arm Technology China) wrote:
> > > On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote:
> > > > To support transmitters other than the tda998x, we need to allow
> > > > non-component framework bridges to be attached to a dummy drm_encoder in
> > > > our driver.
> > > > 
> > > > For the existing supported encoder (tda998x), keep the behaviour as-is,
> > > > since there's no way to unbind if a drm_bridge module goes away under
> > > > our feet.
> > > > 
> > > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > > ---
> > > >  .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
> > > >  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
> > > >  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
> > > >  .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
> > > >  4 files changed, 187 insertions(+), 14 deletions(-)
> > > > 
> > > > [snip]
> > > >  
> > > > +static void komeda_encoder_destroy(struct drm_encoder *encoder)
> > > > +{
> > > > +	drm_encoder_cleanup(encoder);
> > > > +}
> > > > +
> > > > +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
> > > > +	.destroy = komeda_encoder_destroy,
> > > > +};
> > > > +
> > > > +bool komeda_remote_device_is_component(struct device_node *local,
> > > > +				       u32 port, u32 endpoint)
> > > > +{
> > > > +	struct device_node *remote;
> > > > +	char const * const component_devices[] = {
> > > > +		"nxp,tda998x",
> > > 
> > > I really don't think put this dummy_encoder into komeda is a good
> > > idea.
> > > 
> > > And I suggest to seperate this dummy_encoder to a individual module
> > > which will build the drm_ridge to a standard drm encoder and component
> > > based module, which will be enable by DT, totally transparent for komeda.
> > > 
> > > BTW:
> > > I really don't like such logic: distingush the SYSTEM configuration
> > > by check the connected device name, it's hard to maintain and code
> > > sharing, and that's why NOW we have the device-tree.
> 
> It's not ideal to have such special cases, for sure. However, I don't
> see this approach causing us any issues. tda998x really is "special",
> and as far as I can see the code here scales to other devices pretty
> easily.
> 
> > 
> > +Cc Brian
> > 
> > I didn't think DT is the right place for pseudo-devices.
> 
> I agree. DT should represent the HW, not the structure of the
> linux kernel subsystem.
> 
> > The tda998x
> > looks to be in a small group of drivers that contain encoder +
> > bridge + connector; my impression of the current state of affairs is
> > that the drm_encoder tends to live where the CRTC provider is rather
> > than representing a HW entity (hence why drm_bridge based drivers
> > exist in drivers/gpu/drm/bridge). See the overview DOC comment in
> > drm_encoder.c ("drivers are free to use [drm_encoder] however they
> > wish"). I may be completely wrong, so would appreciate some
> > context and comment from others on the Cc list.
> > 
> > In any case, converting a do-nothing dummy encoder into its own
> > component-module will add a lot more bloat compared to the current
> > ~10 SLoC implementation of the drm_encoder. probe/remove/bind/unbind,
> > a few extra structs here and there, yet another object file, DT
> > bindings, docs for the same, and maintaining all of that? It's a hard
> > sell for me. I'd prefer if we went ahead with the code as-is and fix it
> > up later if it really proves unwieldy and too hard to maintain. Could
> > this patch be improved? Sure! Can we improve it in follow-up work
> > though, as I think this is valuable enough on its own without any major
> > drawbacks?
> > 
> 
> Even if we implemented a separate component encoder, as far as I can
> tell there's no way to use it without either:
> 
> a) sticking it in DT
> b) invoking it from komeda based on a "of_device_is_compatible" list
> 
> IMO a) isn't acceptable, and b) doesn't have any advantages over this
> approach.
> 
> > As per my cover letter, in an ideal world we'd have the encoder locally
> > and do drm_bridge_attach() even for tda998x, but the lifetime issues
> > around bridges inside modules mean that doing that now is a bit of a
> > step back for this specific case.
> > 
> 
> Yeah, my feeling is that being able to keep tda998x as a component
> (for the superior bind/unbind behaviour) is worth the slight ugliness,
> at least until bridges get the same functionality.
> 
> If James is strongly against merging this, maybe we just swap
> wholesale to bridge? But for me, the pragmatic approach would be this
> stop-gap.
>

This is a good idea, and I vote +ULONG_MAX :)

and I also checked tda998x driver, it supports bridge. so swap the
wholesale to brige is perfect. :)

Thanks
James.

> Cheers,
> -Brian
> 
> > > 
> > > Thanks
> > > James
> > > 
> > > > [snip]
> > > 
> > 
> > -- 
> > Mihail
> > 
> > 
> > 

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Brian Starkey]

On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > 
> > If James is strongly against merging this, maybe we just swap
> > wholesale to bridge? But for me, the pragmatic approach would be this
> > stop-gap.
> >
> 
> This is a good idea, and I vote +ULONG_MAX :)
> 
> and I also checked tda998x driver, it supports bridge. so swap the
> wholesale to brige is perfect. :)
> 

Well, as Mihail wrote, it's definitely not perfect.

Today, if you rmmod tda998x with the DPU driver still loaded,
everything will be unbound gracefully.

If we swap to bridge, then rmmod'ing tda998x (or any other bridge
driver the DPU is using) with the DPU driver still loaded will result
in a crash.

So, there really are proper benefits to sticking with the component
code for tda998x, which is why I'd like to understand why you're so
against this patch?

Thanks,
-Brian

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[james qian wang (Arm Technology China)]

On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > 
> > > If James is strongly against merging this, maybe we just swap
> > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > stop-gap.
> > >
> > 
> > This is a good idea, and I vote +ULONG_MAX :)
> > 
> > and I also checked tda998x driver, it supports bridge. so swap the
> > wholesale to brige is perfect. :)
> > 
> 
> Well, as Mihail wrote, it's definitely not perfect.
> 
> Today, if you rmmod tda998x with the DPU driver still loaded,
> everything will be unbound gracefully.
> 
> If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> driver the DPU is using) with the DPU driver still loaded will result
> in a crash.

I haven't read the bridge code, but seems this is a bug of drm_bridge,
since if the bridge is still in using by others, the rmmod should fail

And personally opinion, if the bridge doesn't handle the dependence.
for us:

- add such support to bridge
  or
- just do the insmod/rmmod in correct order.

> So, there really are proper benefits to sticking with the component
> code for tda998x, which is why I'd like to understand why you're so
> against this patch?
>

This change handles two different connectors in komeda internally, compare
with one interface, it increases the complexity, more risk of bug and more
cost of maintainance.

So my suggestion is keeping on one single interface in komeda, no
matter it is bridge or component, but I'd like it only one, but not
them both in komeda.

Thanks
James

> Thanks,
> -Brian

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Brian Starkey]

On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > 
> > > > If James is strongly against merging this, maybe we just swap
> > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > stop-gap.
> > > >
> > > 
> > > This is a good idea, and I vote +ULONG_MAX :)
> > > 
> > > and I also checked tda998x driver, it supports bridge. so swap the
> > > wholesale to brige is perfect. :)
> > > 
> > 
> > Well, as Mihail wrote, it's definitely not perfect.
> > 
> > Today, if you rmmod tda998x with the DPU driver still loaded,
> > everything will be unbound gracefully.
> > 
> > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > driver the DPU is using) with the DPU driver still loaded will result
> > in a crash.
> 
> I haven't read the bridge code, but seems this is a bug of drm_bridge,
> since if the bridge is still in using by others, the rmmod should fail
> 

Correct, but there's no fix for that today. You can also take a look
at the thread linked from Mihail's cover letter.

> And personally opinion, if the bridge doesn't handle the dependence.
> for us:
> 
> - add such support to bridge

That would certainly be helpful. I don't know if there's consensus on
how to do that.

>   or
> - just do the insmod/rmmod in correct order.
> 
> > So, there really are proper benefits to sticking with the component
> > code for tda998x, which is why I'd like to understand why you're so
> > against this patch?
> >
> 
> This change handles two different connectors in komeda internally, compare
> with one interface, it increases the complexity, more risk of bug and more
> cost of maintainance.
> 

Well, it's only about how to bind the drivers - two different methods
of binding, not two different connectors. I would argue that carrying
our out-of-tree patches to support both platforms is a larger
maintenance burden.

Honestly this looks like a win-win to me. We get the superior approach
when its supported, and still get to support bridges which are more
common.

As/when improvements are made to the bridge code we can remove the
component bits and not lose anything.

> So my suggestion is keeping on one single interface in komeda, no
> matter it is bridge or component, but I'd like it only one, but not
> them both in komeda.

If we can put the effort into fixing bridges then I guess that's the
best approach for everyone :-) Might not be easy though!

-Brian

> 
> Thanks
> James
> 
> > Thanks,
> > -Brian

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Russell King - ARM Linux admin]

On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > 
> > > > > If James is strongly against merging this, maybe we just swap
> > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > stop-gap.
> > > > >
> > > > 
> > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > 
> > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > wholesale to brige is perfect. :)
> > > > 
> > > 
> > > Well, as Mihail wrote, it's definitely not perfect.
> > > 
> > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > everything will be unbound gracefully.
> > > 
> > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > driver the DPU is using) with the DPU driver still loaded will result
> > > in a crash.
> > 
> > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > since if the bridge is still in using by others, the rmmod should fail
> > 
> 
> Correct, but there's no fix for that today. You can also take a look
> at the thread linked from Mihail's cover letter.
> 
> > And personally opinion, if the bridge doesn't handle the dependence.
> > for us:
> > 
> > - add such support to bridge
> 
> That would certainly be helpful. I don't know if there's consensus on
> how to do that.
> 
> >   or
> > - just do the insmod/rmmod in correct order.
> > 
> > > So, there really are proper benefits to sticking with the component
> > > code for tda998x, which is why I'd like to understand why you're so
> > > against this patch?
> > >
> > 
> > This change handles two different connectors in komeda internally, compare
> > with one interface, it increases the complexity, more risk of bug and more
> > cost of maintainance.
> > 
> 
> Well, it's only about how to bind the drivers - two different methods
> of binding, not two different connectors. I would argue that carrying
> our out-of-tree patches to support both platforms is a larger
> maintenance burden.
> 
> Honestly this looks like a win-win to me. We get the superior approach
> when its supported, and still get to support bridges which are more
> common.
> 
> As/when improvements are made to the bridge code we can remove the
> component bits and not lose anything.

There was an idea a while back about using the device links code to
solve the bridge issue - but at the time the device links code wasn't
up to the job.  I think that's been resolved now, but I haven't been
able to confirm it.  I did propose some patches for bridge at the
time but they probably need updating.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[james qian wang (Arm Technology China)]

On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > > 
> > > > > > If James is strongly against merging this, maybe we just swap
> > > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > > stop-gap.
> > > > > >
> > > > > 
> > > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > > 
> > > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > > wholesale to brige is perfect. :)
> > > > > 
> > > > 
> > > > Well, as Mihail wrote, it's definitely not perfect.
> > > > 
> > > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > > everything will be unbound gracefully.
> > > > 
> > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > > driver the DPU is using) with the DPU driver still loaded will result
> > > > in a crash.
> > > 
> > > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > > since if the bridge is still in using by others, the rmmod should fail
> > > 
> > 
> > Correct, but there's no fix for that today. You can also take a look
> > at the thread linked from Mihail's cover letter.
> > 
> > > And personally opinion, if the bridge doesn't handle the dependence.
> > > for us:
> > > 
> > > - add such support to bridge
> > 
> > That would certainly be helpful. I don't know if there's consensus on
> > how to do that.
> > 
> > >   or
> > > - just do the insmod/rmmod in correct order.
> > > 
> > > > So, there really are proper benefits to sticking with the component
> > > > code for tda998x, which is why I'd like to understand why you're so
> > > > against this patch?
> > > >
> > > 
> > > This change handles two different connectors in komeda internally, compare
> > > with one interface, it increases the complexity, more risk of bug and more
> > > cost of maintainance.
> > > 
> > 
> > Well, it's only about how to bind the drivers - two different methods
> > of binding, not two different connectors. I would argue that carrying
> > our out-of-tree patches to support both platforms is a larger
> > maintenance burden.
> > 
> > Honestly this looks like a win-win to me. We get the superior approach
> > when its supported, and still get to support bridges which are more
> > common.
> > 
> > As/when improvements are made to the bridge code we can remove the
> > component bits and not lose anything.
> 
> There was an idea a while back about using the device links code to
> solve the bridge issue - but at the time the device links code wasn't
> up to the job.  I think that's been resolved now, but I haven't been
> able to confirm it.  I did propose some patches for bridge at the
> time but they probably need updating.

Hi Russell:

Thank you, that's a good news.

Hi Brian:

Can this convince you to fully swap to bridge ?

Actually even there is no fix, we won't real encounter such rmmod problem,
since we always build the bridge/tda998 (by Y) into the image.

Thanks
James
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Brian Starkey]

On Fri, Oct 18, 2019 at 06:57:05AM +0000, james qian wang (Arm Technology China) wrote:
> 
> Hi Brian:
> 
> Can this convince you to fully swap to bridge ?

Not until those patches materialise and land, no :-)

> 
> Actually even there is no fix, we won't real encounter such rmmod problem,
> since we always build the bridge/tda998 (by Y) into the image.
> 

If you say so. I think the folks here like having drm as a module to
make it easy to patch things without a reboot.

-Brian

> Thanks
> James
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Daniel Vetter]

On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > > 
> > > > > > If James is strongly against merging this, maybe we just swap
> > > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > > stop-gap.
> > > > > >
> > > > > 
> > > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > > 
> > > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > > wholesale to brige is perfect. :)
> > > > > 
> > > > 
> > > > Well, as Mihail wrote, it's definitely not perfect.
> > > > 
> > > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > > everything will be unbound gracefully.
> > > > 
> > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > > driver the DPU is using) with the DPU driver still loaded will result
> > > > in a crash.
> > > 
> > > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > > since if the bridge is still in using by others, the rmmod should fail
> > > 
> > 
> > Correct, but there's no fix for that today. You can also take a look
> > at the thread linked from Mihail's cover letter.
> > 
> > > And personally opinion, if the bridge doesn't handle the dependence.
> > > for us:
> > > 
> > > - add such support to bridge
> > 
> > That would certainly be helpful. I don't know if there's consensus on
> > how to do that.
> > 
> > >   or
> > > - just do the insmod/rmmod in correct order.
> > > 
> > > > So, there really are proper benefits to sticking with the component
> > > > code for tda998x, which is why I'd like to understand why you're so
> > > > against this patch?
> > > >
> > > 
> > > This change handles two different connectors in komeda internally, compare
> > > with one interface, it increases the complexity, more risk of bug and more
> > > cost of maintainance.
> > > 
> > 
> > Well, it's only about how to bind the drivers - two different methods
> > of binding, not two different connectors. I would argue that carrying
> > our out-of-tree patches to support both platforms is a larger
> > maintenance burden.
> > 
> > Honestly this looks like a win-win to me. We get the superior approach
> > when its supported, and still get to support bridges which are more
> > common.
> > 
> > As/when improvements are made to the bridge code we can remove the
> > component bits and not lose anything.
> 
> There was an idea a while back about using the device links code to
> solve the bridge issue - but at the time the device links code wasn't
> up to the job.  I think that's been resolved now, but I haven't been
> able to confirm it.  I did propose some patches for bridge at the
> time but they probably need updating.

I think the only patches that existed where for panel, and we only
discussed the bridge case. At least I can only find patches for panel,not
bridge, but might be missing something.

Either way I think device core is fixed now, so would be really great if
someone can give this another stab, and make drm_bridge/panel easier to
use without fireworks on unload.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Russell King - ARM Linux admin]

On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote:
> On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote:
> > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > > > 
> > > > > > > If James is strongly against merging this, maybe we just swap
> > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > > > stop-gap.
> > > > > > >
> > > > > > 
> > > > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > > > 
> > > > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > > > wholesale to brige is perfect. :)
> > > > > > 
> > > > > 
> > > > > Well, as Mihail wrote, it's definitely not perfect.
> > > > > 
> > > > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > > > everything will be unbound gracefully.
> > > > > 
> > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > > > driver the DPU is using) with the DPU driver still loaded will result
> > > > > in a crash.
> > > > 
> > > > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > > > since if the bridge is still in using by others, the rmmod should fail
> > > > 
> > > 
> > > Correct, but there's no fix for that today. You can also take a look
> > > at the thread linked from Mihail's cover letter.
> > > 
> > > > And personally opinion, if the bridge doesn't handle the dependence.
> > > > for us:
> > > > 
> > > > - add such support to bridge
> > > 
> > > That would certainly be helpful. I don't know if there's consensus on
> > > how to do that.
> > > 
> > > >   or
> > > > - just do the insmod/rmmod in correct order.
> > > > 
> > > > > So, there really are proper benefits to sticking with the component
> > > > > code for tda998x, which is why I'd like to understand why you're so
> > > > > against this patch?
> > > > >
> > > > 
> > > > This change handles two different connectors in komeda internally, compare
> > > > with one interface, it increases the complexity, more risk of bug and more
> > > > cost of maintainance.
> > > > 
> > > 
> > > Well, it's only about how to bind the drivers - two different methods
> > > of binding, not two different connectors. I would argue that carrying
> > > our out-of-tree patches to support both platforms is a larger
> > > maintenance burden.
> > > 
> > > Honestly this looks like a win-win to me. We get the superior approach
> > > when its supported, and still get to support bridges which are more
> > > common.
> > > 
> > > As/when improvements are made to the bridge code we can remove the
> > > component bits and not lose anything.
> > 
> > There was an idea a while back about using the device links code to
> > solve the bridge issue - but at the time the device links code wasn't
> > up to the job.  I think that's been resolved now, but I haven't been
> > able to confirm it.  I did propose some patches for bridge at the
> > time but they probably need updating.
> 
> I think the only patches that existed where for panel, and we only
> discussed the bridge case. At least I can only find patches for panel,not
> bridge, but might be missing something.

I had a patches, which is why I raised the problem with the core:

6961edfee26d bridge hacks using device links

but it never went further than an experiment at the time because of the
problems in the core.  As it was a hack, it never got posted.  Seems
that kernel tree (for the cubox) is still 5.2 based, so has a lot of
patches and might need updating to a more recent base before anything
can be tested.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Daniel Vetter]

On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote:
> > On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote:
> > > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> > > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > > > >
> > > > > > > > If James is strongly against merging this, maybe we just swap
> > > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > > > > stop-gap.
> > > > > > > >
> > > > > > >
> > > > > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > > > >
> > > > > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > > > > wholesale to brige is perfect. :)
> > > > > > >
> > > > > >
> > > > > > Well, as Mihail wrote, it's definitely not perfect.
> > > > > >
> > > > > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > > > > everything will be unbound gracefully.
> > > > > >
> > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > > > > driver the DPU is using) with the DPU driver still loaded will result
> > > > > > in a crash.
> > > > >
> > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > > > > since if the bridge is still in using by others, the rmmod should fail
> > > > >
> > > >
> > > > Correct, but there's no fix for that today. You can also take a look
> > > > at the thread linked from Mihail's cover letter.
> > > >
> > > > > And personally opinion, if the bridge doesn't handle the dependence.
> > > > > for us:
> > > > >
> > > > > - add such support to bridge
> > > >
> > > > That would certainly be helpful. I don't know if there's consensus on
> > > > how to do that.
> > > >
> > > > >   or
> > > > > - just do the insmod/rmmod in correct order.
> > > > >
> > > > > > So, there really are proper benefits to sticking with the component
> > > > > > code for tda998x, which is why I'd like to understand why you're so
> > > > > > against this patch?
> > > > > >
> > > > >
> > > > > This change handles two different connectors in komeda internally, compare
> > > > > with one interface, it increases the complexity, more risk of bug and more
> > > > > cost of maintainance.
> > > > >
> > > >
> > > > Well, it's only about how to bind the drivers - two different methods
> > > > of binding, not two different connectors. I would argue that carrying
> > > > our out-of-tree patches to support both platforms is a larger
> > > > maintenance burden.
> > > >
> > > > Honestly this looks like a win-win to me. We get the superior approach
> > > > when its supported, and still get to support bridges which are more
> > > > common.
> > > >
> > > > As/when improvements are made to the bridge code we can remove the
> > > > component bits and not lose anything.
> > >
> > > There was an idea a while back about using the device links code to
> > > solve the bridge issue - but at the time the device links code wasn't
> > > up to the job.  I think that's been resolved now, but I haven't been
> > > able to confirm it.  I did propose some patches for bridge at the
> > > time but they probably need updating.
> >
> > I think the only patches that existed where for panel, and we only
> > discussed the bridge case. At least I can only find patches for panel,not
> > bridge, but might be missing something.
>
> I had a patches, which is why I raised the problem with the core:
>
> 6961edfee26d bridge hacks using device links
>
> but it never went further than an experiment at the time because of the
> problems in the core.  As it was a hack, it never got posted.  Seems
> that kernel tree (for the cubox) is still 5.2 based, so has a lot of
> patches and might need updating to a more recent base before anything
> can be tested.


For reference, the panel patch:

https://patchwork.kernel.org/patch/10364873/

And the huge discussion around bridges, that resulted in Rafael
Wyzocki fixing all the core issues:

https://www.spinics.net/lists/dri-devel/msg201927.html

James, do you want to look into this for bridges?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Russell King - ARM Linux admin]

On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote:
> On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > I had a patches, which is why I raised the problem with the core:
> >
> > 6961edfee26d bridge hacks using device links
> >
> > but it never went further than an experiment at the time because of the
> > problems in the core.  As it was a hack, it never got posted.  Seems
> > that kernel tree (for the cubox) is still 5.2 based, so has a lot of
> > patches and might need updating to a more recent base before anything
> > can be tested.
> 
> 
> For reference, the panel patch:
> 
> https://patchwork.kernel.org/patch/10364873/
> 
> And the huge discussion around bridges, that resulted in Rafael
> Wyzocki fixing all the core issues:
> 
> https://www.spinics.net/lists/dri-devel/msg201927.html
> 
> James, do you want to look into this for bridges?

I can now confirm that it does work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Russell King - ARM Linux admin]

On Tue, Oct 22, 2019 at 03:42:07PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote:
> > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > I had a patches, which is why I raised the problem with the core:
> > >
> > > 6961edfee26d bridge hacks using device links
> > >
> > > but it never went further than an experiment at the time because of the
> > > problems in the core.  As it was a hack, it never got posted.  Seems
> > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of
> > > patches and might need updating to a more recent base before anything
> > > can be tested.
> > 
> > 
> > For reference, the panel patch:
> > 
> > https://patchwork.kernel.org/patch/10364873/
> > 
> > And the huge discussion around bridges, that resulted in Rafael
> > Wyzocki fixing all the core issues:
> > 
> > https://www.spinics.net/lists/dri-devel/msg201927.html
> > 
> > James, do you want to look into this for bridges?
> 
> I can now confirm that it does work.

Something like this - it's based off an older kernel, so may be missing
some of the bridge drivers, but should be sufficient for people to test
with.

8<====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] drm/bridge: add support for device links to bridge

Bridge devices have been a potential for kernel oops as their lifetime
is independent of the DRM device that they are bound to.  Hence, if a
bridge device is unbound while the parent DRM device is using them, the
parent happily continues to use the bridge device, calling the driver
and accessing its objects that have been freed.

This can cause kernel memory corruption and kernel oops.

To control this, use device links to ensure that the parent DRM device
is unbound when the bridge device is unbound, and when the bridge
device is re-bound, automatically rebind the parent DRM device.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  |  1 +
 drivers/gpu/drm/bridge/analogix-anx78xx.c     |  1 +
 drivers/gpu/drm/bridge/dumb-vga-dac.c         |  1 +
 drivers/gpu/drm/bridge/lvds-encoder.c         |  1 +
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  |  1 +
 drivers/gpu/drm/bridge/nxp-ptn3460.c          |  1 +
 drivers/gpu/drm/bridge/panel.c                |  1 +
 drivers/gpu/drm/bridge/parade-ps8622.c        |  1 +
 drivers/gpu/drm/bridge/sii902x.c              |  1 +
 drivers/gpu/drm/bridge/sii9234.c              |  1 +
 drivers/gpu/drm/bridge/sil-sii8620.c          |  1 +
 drivers/gpu/drm/bridge/tc358767.c             |  1 +
 drivers/gpu/drm/bridge/ti-tfp410.c            |  1 +
 drivers/gpu/drm/drm_bridge.c                  | 48 ++++++++++++++-----
 drivers/gpu/drm/i2c/tda998x_drv.c             |  1 +
 include/drm/drm_bridge.h                      |  4 ++
 16 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index f6d2681f6927..6a5906da58ea 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1217,6 +1217,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 		goto err_unregister_cec;
 
 	adv7511->bridge.funcs = &adv7511_bridge_funcs;
+	adv7511->bridge.device = dev;
 	adv7511->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&adv7511->bridge);
diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index 3c7cc5af735c..77ff17c38037 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1323,6 +1323,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 
 	mutex_init(&anx78xx->lock);
 
+	anx78xx->bridge.device = &client->dev;
 #if IS_ENABLED(CONFIG_OF)
 	anx78xx->bridge.of_node = client->dev.of_node;
 #endif
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index d32885b906ae..40169920560e 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -202,6 +202,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
 	}
 
 	vga->bridge.funcs = &dumb_vga_bridge_funcs;
+	vga->bridge.device = &pdev->dev;
 	vga->bridge.of_node = pdev->dev.of_node;
 	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
 
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 2ab2c234f26c..5012be35a5fb 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -115,6 +115,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	 * to look up.
 	 */
 	lvds_encoder->bridge.of_node = dev->of_node;
+	lvds_encoder->bridge.device = dev;
 	lvds_encoder->bridge.funcs = &funcs;
 	drm_bridge_add(&lvds_encoder->bridge);
 
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 79311f8354bd..e211c57f6f56 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -304,6 +304,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 
 	/* drm bridge initialization */
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
+	ge_b850v3_lvds_ptr->bridge.device = dev;
 	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 98bc650b8c95..00097e314575 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -323,6 +323,7 @@ static int ptn3460_probe(struct i2c_client *client,
 	}
 
 	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
+	ptn_bridge->bridge.device = dev;
 	ptn_bridge->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ptn_bridge->bridge);
 
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index b12ae3a4c5f1..eab7126f0d61 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -168,6 +168,7 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
 	panel_bridge->panel = panel;
 
 	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
+	panel_bridge->bridge.device = panel->dev;
 #ifdef CONFIG_OF
 	panel_bridge->bridge.of_node = panel->dev->of_node;
 #endif
diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index 2d88146e4836..ff79df0ff183 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -589,6 +589,7 @@ static int ps8622_probe(struct i2c_client *client,
 	}
 
 	ps8622->bridge.funcs = &ps8622_bridge_funcs;
+	ps8622->bridge.device = dev;
 	ps8622->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ps8622->bridge);
 
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index dd7aa466b280..ef768b149bee 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -991,6 +991,7 @@ static int sii902x_probe(struct i2c_client *client,
 	}
 
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
+	sii902x->bridge.device = dev;
 	sii902x->bridge.of_node = dev->of_node;
 	sii902x->bridge.timings = &default_sii902x_timings;
 	drm_bridge_add(&sii902x->bridge);
diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index 25d4ad8c7ad6..824ffaeff16e 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -936,6 +936,7 @@ static int sii9234_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, ctx);
 
 	ctx->bridge.funcs = &sii9234_bridge_funcs;
+	ctx->bridge.device = dev;
 	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index bd3165ee5354..5bc56c5f6826 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2333,6 +2333,7 @@ static int sii8620_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, ctx);
 
 	ctx->bridge.funcs = &sii8620_bridge_funcs;
+	ctx->bridge.device = dev;
 	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 13ade28a36a8..d62c6925c5fe 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1526,6 +1526,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 
 	tc->bridge.funcs = &tc_bridge_funcs;
+	tc->bridge.device = dev;
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index dbf35c7bc85e..2f9899d7d4b4 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -326,6 +326,7 @@ static int tfp410_init(struct device *dev, bool i2c)
 	dev_set_drvdata(dev, dvi);
 
 	dvi->bridge.funcs = &tfp410_bridge_funcs;
+	dvi->bridge.device = dev;
 	dvi->bridge.of_node = dev->of_node;
 	dvi->bridge.timings = &dvi->timings;
 	dvi->dev = dev;
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cba537c99e43..b4561ce63a49 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"
@@ -463,6 +464,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
 EXPORT_SYMBOL(drm_atomic_bridge_enable);
 
 #ifdef CONFIG_OF
+static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
+					  struct device_node *np, bool link)
+{
+	struct drm_bridge *bridge, *found = NULL;
+	struct device_link *dl;
+
+	mutex_lock(&bridge_lock);
+
+	list_for_each_entry(bridge, &bridge_list, list)
+		if (bridge->of_node == np) {
+			found = bridge;
+			break;
+		}
+
+	if (found && link) {
+		dl = device_link_add(dev->dev, found->device,
+				     DL_FLAG_AUTOPROBE_CONSUMER);
+		if (!dl)
+			found = NULL;
+	}
+
+	mutex_unlock(&bridge_lock);
+
+	return found;
+}
+
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
  *			the global bridge list
@@ -474,21 +501,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
  */
 struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 {
-	struct drm_bridge *bridge;
-
-	mutex_lock(&bridge_lock);
-
-	list_for_each_entry(bridge, &bridge_list, list) {
-		if (bridge->of_node == np) {
-			mutex_unlock(&bridge_lock);
-			return bridge;
-		}
-	}
-
-	mutex_unlock(&bridge_lock);
-	return NULL;
+	return drm_bridge_find(NULL, np, false);
 }
 EXPORT_SYMBOL(of_drm_find_bridge);
+
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
+					      struct device_node *np)
+{
+	return drm_bridge_find(dev, np, true);
+}
+EXPORT_SYMBOL(of_drm_find_bridge_devlink);
 #endif
 
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index e79507fb225f..5d4122bcf7ff 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -2201,6 +2201,7 @@ static int tda998x_create(struct device *dev)
 	}
 
 	priv->bridge.funcs = &tda998x_bridge_funcs;
+	priv->bridge.device = dev;
 #ifdef CONFIG_OF
 	priv->bridge.of_node = dev->of_node;
 #endif
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7616f6562fe4..f8a3af42a382 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -382,6 +382,8 @@ struct drm_bridge {
 	struct drm_encoder *encoder;
 	/** @next: the next bridge in the encoder chain */
 	struct drm_bridge *next;
+	/** @device: Linux driver model device */
+	struct device *device;
 #ifdef CONFIG_OF
 	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
@@ -403,6 +405,8 @@ struct drm_bridge {
 void drm_bridge_add(struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
+					      struct device_node *np);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous);
 
-- 
2.20.1


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Mihail Atanassov]

Hi Russell,

On Tuesday, 22 October 2019 15:53:29 BST Russell King - ARM Linux admin wrote:
> On Tue, Oct 22, 2019 at 03:42:07PM +0100, Russell King - ARM Linux admin wrote:
> > On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote:
> > > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > > I had a patches, which is why I raised the problem with the core:
> > > >
> > > > 6961edfee26d bridge hacks using device links
> > > >
> > > > but it never went further than an experiment at the time because of the
> > > > problems in the core.  As it was a hack, it never got posted.  Seems
> > > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of
> > > > patches and might need updating to a more recent base before anything
> > > > can be tested.
> > > 
> > > 
> > > For reference, the panel patch:
> > > 
> > > https://patchwork.kernel.org/patch/10364873/
> > > 
> > > And the huge discussion around bridges, that resulted in Rafael
> > > Wyzocki fixing all the core issues:
> > > 
> > > https://www.spinics.net/lists/dri-devel/msg201927.html
> > > 
> > > James, do you want to look into this for bridges?
> > 
> > I can now confirm that it does work.
> 
> Something like this - it's based off an older kernel, so may be missing
> some of the bridge drivers, but should be sufficient for people to test
> with.

Thanks for the patch, I tested to the limit that our driver allows at
the moment -- rmmod'ing the bridge while the driver is not in use --
and I don't see any issues there. komeda successfully gets removed then
re-probed once the bridge reappears. For that part,

Tested-by: Mihail Atanassov <mihail.atanassov@arm.com>

I couldn't sadly check a hot unplug since we have an mm bug or two that
I need to sort out first. If anyone else has a hot-unplug capable
driver and can test, it'd be good to ensure that also functions
properly.

> 
> 8<====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] drm/bridge: add support for device links to bridge
> 
> Bridge devices have been a potential for kernel oops as their lifetime
> is independent of the DRM device that they are bound to.  Hence, if a
> bridge device is unbound while the parent DRM device is using them, the
> parent happily continues to use the bridge device, calling the driver
> and accessing its objects that have been freed.
> 
> This can cause kernel memory corruption and kernel oops.
> 
> To control this, use device links to ensure that the parent DRM device
> is unbound when the bridge device is unbound, and when the bridge
> device is re-bound, automatically rebind the parent DRM device.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  |  1 +
>  drivers/gpu/drm/bridge/analogix-anx78xx.c     |  1 +
>  drivers/gpu/drm/bridge/dumb-vga-dac.c         |  1 +
>  drivers/gpu/drm/bridge/lvds-encoder.c         |  1 +
>  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  |  1 +
>  drivers/gpu/drm/bridge/nxp-ptn3460.c          |  1 +
>  drivers/gpu/drm/bridge/panel.c                |  1 +
>  drivers/gpu/drm/bridge/parade-ps8622.c        |  1 +
>  drivers/gpu/drm/bridge/sii902x.c              |  1 +
>  drivers/gpu/drm/bridge/sii9234.c              |  1 +
>  drivers/gpu/drm/bridge/sil-sii8620.c          |  1 +
>  drivers/gpu/drm/bridge/tc358767.c             |  1 +
>  drivers/gpu/drm/bridge/ti-tfp410.c            |  1 +
>  drivers/gpu/drm/drm_bridge.c                  | 48 ++++++++++++++-----
>  drivers/gpu/drm/i2c/tda998x_drv.c             |  1 +
>  include/drm/drm_bridge.h                      |  4 ++
>  16 files changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index f6d2681f6927..6a5906da58ea 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1217,6 +1217,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  		goto err_unregister_cec;
>  
>  	adv7511->bridge.funcs = &adv7511_bridge_funcs;
> +	adv7511->bridge.device = dev;
>  	adv7511->bridge.of_node = dev->of_node;
>  
>  	drm_bridge_add(&adv7511->bridge);
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> index 3c7cc5af735c..77ff17c38037 100644
> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> @@ -1323,6 +1323,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
>  
>  	mutex_init(&anx78xx->lock);
>  
> +	anx78xx->bridge.device = &client->dev;
>  #if IS_ENABLED(CONFIG_OF)
>  	anx78xx->bridge.of_node = client->dev.of_node;
>  #endif
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> index d32885b906ae..40169920560e 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -202,6 +202,7 @@ static int dumb_vga_probe(struct platform_device *pdev)
>  	}
>  
>  	vga->bridge.funcs = &dumb_vga_bridge_funcs;
> +	vga->bridge.device = &pdev->dev;
>  	vga->bridge.of_node = pdev->dev.of_node;
>  	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
>  
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index 2ab2c234f26c..5012be35a5fb 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -115,6 +115,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>  	 * to look up.
>  	 */
>  	lvds_encoder->bridge.of_node = dev->of_node;
> +	lvds_encoder->bridge.device = dev;
>  	lvds_encoder->bridge.funcs = &funcs;
>  	drm_bridge_add(&lvds_encoder->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 79311f8354bd..e211c57f6f56 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -304,6 +304,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
>  
>  	/* drm bridge initialization */
>  	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
> +	ge_b850v3_lvds_ptr->bridge.device = dev;
>  	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index 98bc650b8c95..00097e314575 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -323,6 +323,7 @@ static int ptn3460_probe(struct i2c_client *client,
>  	}
>  
>  	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
> +	ptn_bridge->bridge.device = dev;
>  	ptn_bridge->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ptn_bridge->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index b12ae3a4c5f1..eab7126f0d61 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -168,6 +168,7 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>  	panel_bridge->panel = panel;
>  
>  	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
> +	panel_bridge->bridge.device = panel->dev;
>  #ifdef CONFIG_OF
>  	panel_bridge->bridge.of_node = panel->dev->of_node;
>  #endif
> diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
> index 2d88146e4836..ff79df0ff183 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8622.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8622.c
> @@ -589,6 +589,7 @@ static int ps8622_probe(struct i2c_client *client,
>  	}
>  
>  	ps8622->bridge.funcs = &ps8622_bridge_funcs;
> +	ps8622->bridge.device = dev;
>  	ps8622->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ps8622->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index dd7aa466b280..ef768b149bee 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -991,6 +991,7 @@ static int sii902x_probe(struct i2c_client *client,
>  	}
>  
>  	sii902x->bridge.funcs = &sii902x_bridge_funcs;
> +	sii902x->bridge.device = dev;
>  	sii902x->bridge.of_node = dev->of_node;
>  	sii902x->bridge.timings = &default_sii902x_timings;
>  	drm_bridge_add(&sii902x->bridge);
> diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
> index 25d4ad8c7ad6..824ffaeff16e 100644
> --- a/drivers/gpu/drm/bridge/sii9234.c
> +++ b/drivers/gpu/drm/bridge/sii9234.c
> @@ -936,6 +936,7 @@ static int sii9234_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, ctx);
>  
>  	ctx->bridge.funcs = &sii9234_bridge_funcs;
> +	ctx->bridge.device = dev;
>  	ctx->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ctx->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index bd3165ee5354..5bc56c5f6826 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -2333,6 +2333,7 @@ static int sii8620_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, ctx);
>  
>  	ctx->bridge.funcs = &sii8620_bridge_funcs;
> +	ctx->bridge.device = dev;
>  	ctx->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ctx->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 13ade28a36a8..d62c6925c5fe 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1526,6 +1526,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  
>  	tc->bridge.funcs = &tc_bridge_funcs;
> +	tc->bridge.device = dev;
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
>  
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index dbf35c7bc85e..2f9899d7d4b4 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -326,6 +326,7 @@ static int tfp410_init(struct device *dev, bool i2c)
>  	dev_set_drvdata(dev, dvi);
>  
>  	dvi->bridge.funcs = &tfp410_bridge_funcs;
> +	dvi->bridge.device = dev;
>  	dvi->bridge.of_node = dev->of_node;
>  	dvi->bridge.timings = &dvi->timings;
>  	dvi->dev = dev;
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cba537c99e43..b4561ce63a49 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"
> @@ -463,6 +464,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
>  EXPORT_SYMBOL(drm_atomic_bridge_enable);
>  
>  #ifdef CONFIG_OF
> +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
> +					  struct device_node *np, bool link)
> +{
> +	struct drm_bridge *bridge, *found = NULL;
> +	struct device_link *dl;
> +
> +	mutex_lock(&bridge_lock);
> +
> +	list_for_each_entry(bridge, &bridge_list, list)
> +		if (bridge->of_node == np) {
> +			found = bridge;
> +			break;
> +		}
> +
> +	if (found && link) {
> +		dl = device_link_add(dev->dev, found->device,
> +				     DL_FLAG_AUTOPROBE_CONSUMER);
> +		if (!dl)
> +			found = NULL;
> +	}
> +
> +	mutex_unlock(&bridge_lock);
> +
> +	return found;
> +}
> +
>  /**
>   * of_drm_find_bridge - find the bridge corresponding to the device node in
>   *			the global bridge list
> @@ -474,21 +501,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
>   */
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  {
> -	struct drm_bridge *bridge;
> -
> -	mutex_lock(&bridge_lock);
> -
> -	list_for_each_entry(bridge, &bridge_list, list) {
> -		if (bridge->of_node == np) {
> -			mutex_unlock(&bridge_lock);
> -			return bridge;
> -		}
> -	}
> -
> -	mutex_unlock(&bridge_lock);
> -	return NULL;
> +	return drm_bridge_find(NULL, np, false);
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> +					      struct device_node *np)
> +{
> +	return drm_bridge_find(dev, np, true);
> +}
> +EXPORT_SYMBOL(of_drm_find_bridge_devlink);
>  #endif
>  
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index e79507fb225f..5d4122bcf7ff 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -2201,6 +2201,7 @@ static int tda998x_create(struct device *dev)
>  	}
>  
>  	priv->bridge.funcs = &tda998x_bridge_funcs;
> +	priv->bridge.device = dev;
>  #ifdef CONFIG_OF
>  	priv->bridge.of_node = dev->of_node;
>  #endif
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 7616f6562fe4..f8a3af42a382 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -382,6 +382,8 @@ struct drm_bridge {
>  	struct drm_encoder *encoder;
>  	/** @next: the next bridge in the encoder chain */
>  	struct drm_bridge *next;
> +	/** @device: Linux driver model device */
> +	struct device *device;
>  #ifdef CONFIG_OF
>  	/** @of_node: device node pointer to the bridge */
>  	struct device_node *of_node;
> @@ -403,6 +405,8 @@ struct drm_bridge {
>  void drm_bridge_add(struct drm_bridge *bridge);
>  void drm_bridge_remove(struct drm_bridge *bridge);
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> +					      struct device_node *np);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous);
>  
> 


-- 
Mihail

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[james qian wang (Arm Technology China)]

On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote:
> On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote:
> > > On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin wrote:
> > > > On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> > > > > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > > > > >
> > > > > > > > > If James is strongly against merging this, maybe we just swap
> > > > > > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > > > > > stop-gap.
> > > > > > > > >
> > > > > > > >
> > > > > > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > > > > >
> > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > > > > > wholesale to brige is perfect. :)
> > > > > > > >
> > > > > > >
> > > > > > > Well, as Mihail wrote, it's definitely not perfect.
> > > > > > >
> > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > > > > > everything will be unbound gracefully.
> > > > > > >
> > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > > > > > driver the DPU is using) with the DPU driver still loaded will result
> > > > > > > in a crash.
> > > > > >
> > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > > > > > since if the bridge is still in using by others, the rmmod should fail
> > > > > >
> > > > >
> > > > > Correct, but there's no fix for that today. You can also take a look
> > > > > at the thread linked from Mihail's cover letter.
> > > > >
> > > > > > And personally opinion, if the bridge doesn't handle the dependence.
> > > > > > for us:
> > > > > >
> > > > > > - add such support to bridge
> > > > >
> > > > > That would certainly be helpful. I don't know if there's consensus on
> > > > > how to do that.
> > > > >
> > > > > >   or
> > > > > > - just do the insmod/rmmod in correct order.
> > > > > >
> > > > > > > So, there really are proper benefits to sticking with the component
> > > > > > > code for tda998x, which is why I'd like to understand why you're so
> > > > > > > against this patch?
> > > > > > >
> > > > > >
> > > > > > This change handles two different connectors in komeda internally, compare
> > > > > > with one interface, it increases the complexity, more risk of bug and more
> > > > > > cost of maintainance.
> > > > > >
> > > > >
> > > > > Well, it's only about how to bind the drivers - two different methods
> > > > > of binding, not two different connectors. I would argue that carrying
> > > > > our out-of-tree patches to support both platforms is a larger
> > > > > maintenance burden.
> > > > >
> > > > > Honestly this looks like a win-win to me. We get the superior approach
> > > > > when its supported, and still get to support bridges which are more
> > > > > common.
> > > > >
> > > > > As/when improvements are made to the bridge code we can remove the
> > > > > component bits and not lose anything.
> > > >
> > > > There was an idea a while back about using the device links code to
> > > > solve the bridge issue - but at the time the device links code wasn't
> > > > up to the job.  I think that's been resolved now, but I haven't been
> > > > able to confirm it.  I did propose some patches for bridge at the
> > > > time but they probably need updating.
> > >
> > > I think the only patches that existed where for panel, and we only
> > > discussed the bridge case. At least I can only find patches for panel,not
> > > bridge, but might be missing something.
> >
> > I had a patches, which is why I raised the problem with the core:
> >
> > 6961edfee26d bridge hacks using device links
> >
> > but it never went further than an experiment at the time because of the
> > problems in the core.  As it was a hack, it never got posted.  Seems
> > that kernel tree (for the cubox) is still 5.2 based, so has a lot of
> > patches and might need updating to a more recent base before anything
> > can be tested.
> 
> 
> For reference, the panel patch:
> 
> https://patchwork.kernel.org/patch/10364873/
> 
> And the huge discussion around bridges, that resulted in Rafael
> Wyzocki fixing all the core issues:
> 
> https://www.spinics.net/lists/dri-devel/msg201927.html
> 
> James, do you want to look into this for bridges?
> 

Hi Daniel:

It's my honour. but I don't have much time in the next 3 weeks.

And I talked with Mihail, he will help to check this problem.

Thanks
James.
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[james qian wang (Arm Technology China)]

On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > 
> > > > > If James is strongly against merging this, maybe we just swap
> > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > stop-gap.
> > > > >
> > > > 
> > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > 
> > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > wholesale to brige is perfect. :)
> > > > 
> > > 
> > > Well, as Mihail wrote, it's definitely not perfect.
> > > 
> > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > everything will be unbound gracefully.
> > > 
> > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > driver the DPU is using) with the DPU driver still loaded will result
> > > in a crash.
> > 
> > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > since if the bridge is still in using by others, the rmmod should fail
> > 
> 
> Correct, but there's no fix for that today. You can also take a look
> at the thread linked from Mihail's cover letter.
> 
> > And personally opinion, if the bridge doesn't handle the dependence.
> > for us:
> > 
> > - add such support to bridge
> 
> That would certainly be helpful. I don't know if there's consensus on
> how to do that.
> 
> >   or
> > - just do the insmod/rmmod in correct order.
> > 
> > > So, there really are proper benefits to sticking with the component
> > > code for tda998x, which is why I'd like to understand why you're so
> > > against this patch?
> > >
> > 
> > This change handles two different connectors in komeda internally, compare
> > with one interface, it increases the complexity, more risk of bug and more
> > cost of maintainance.
> > 
> 
> Well, it's only about how to bind the drivers - two different methods
> of binding, not two different connectors. I would argue that carrying
> our out-of-tree patches to support both platforms is a larger
> maintenance burden.
> 
> Honestly this looks like a win-win to me. We get the superior approach
> when its supported, and still get to support bridges which are more
> common.
>

My consideration is: if we support both link methods, we may suffering

- 1. bridge reference cnt problem
- 2. maintance two link methods.

the 1) seems unavoidable, so swap all to bridage at least can avoid
the pain of 2). that's why I thought your idea "swap all to bridage"
is good.

Thanks
James.

> As/when improvements are made to the bridge code we can remove the
> component bits and not lose anything.
> 
> > So my suggestion is keeping on one single interface in komeda, no
> > matter it is bridge or component, but I'd like it only one, but not
> > them both in komeda.
> 
> If we can put the effort into fixing bridges then I guess that's the
> best approach for everyone :-) Might not be easy though!
> 
> -Brian
> 
> > 
> > Thanks
> > James
> > 
> > > Thanks,
> > > -Brian

Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

[Mihail Atanassov]

On Friday, 18 October 2019 07:38:59 BST james qian wang (Arm Technology China) wrote:
> On Thu, Oct 17, 2019 at 10:48:12AM +0000, Brian Starkey wrote:
> > On Thu, Oct 17, 2019 at 10:21:03AM +0000, james qian wang (Arm Technology China) wrote:
> > > On Thu, Oct 17, 2019 at 08:20:56AM +0000, Brian Starkey wrote:
> > > > On Thu, Oct 17, 2019 at 03:07:59AM +0000, james qian wang (Arm Technology China) wrote:
> > > > > On Wed, Oct 16, 2019 at 04:22:07PM +0000, Brian Starkey wrote:
> > > > > > 
> > > > > > If James is strongly against merging this, maybe we just swap
> > > > > > wholesale to bridge? But for me, the pragmatic approach would be this
> > > > > > stop-gap.
> > > > > >
> > > > > 
> > > > > This is a good idea, and I vote +ULONG_MAX :)
> > > > > 
> > > > > and I also checked tda998x driver, it supports bridge. so swap the
> > > > > wholesale to brige is perfect. :)
> > > > > 
> > > > 
> > > > Well, as Mihail wrote, it's definitely not perfect.
> > > > 
> > > > Today, if you rmmod tda998x with the DPU driver still loaded,
> > > > everything will be unbound gracefully.
> > > > 
> > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge
> > > > driver the DPU is using) with the DPU driver still loaded will result
> > > > in a crash.
> > > 
> > > I haven't read the bridge code, but seems this is a bug of drm_bridge,
> > > since if the bridge is still in using by others, the rmmod should fail
> > > 
> > 
> > Correct, but there's no fix for that today. You can also take a look
> > at the thread linked from Mihail's cover letter.
> > 
> > > And personally opinion, if the bridge doesn't handle the dependence.
> > > for us:
> > > 
> > > - add such support to bridge
> > 
> > That would certainly be helpful. I don't know if there's consensus on
> > how to do that.
> > 
> > >   or
> > > - just do the insmod/rmmod in correct order.
> > > 
> > > > So, there really are proper benefits to sticking with the component
> > > > code for tda998x, which is why I'd like to understand why you're so
> > > > against this patch?
> > > >
> > > 
> > > This change handles two different connectors in komeda internally, compare
> > > with one interface, it increases the complexity, more risk of bug and more
> > > cost of maintainance.
> > > 
> > 
> > Well, it's only about how to bind the drivers - two different methods
> > of binding, not two different connectors. I would argue that carrying
> > our out-of-tree patches to support both platforms is a larger
> > maintenance burden.
> > 
> > Honestly this looks like a win-win to me. We get the superior approach
> > when its supported, and still get to support bridges which are more
> > common.
> >
> 
> My consideration is: if we support both link methods, we may suffering
> 
> - 1. bridge reference cnt problem
> - 2. maintance two link methods.
> 
> the 1) seems unavoidable, so swap all to bridage at least can avoid
> the pain of 2). that's why I thought your idea "swap all to bridage"
> is good.
> 
> Thanks
> James.
> 

Just to make sure my understanding is clear: If I respin the patch to
only use the drm_bridge i/f, you'd be happier with it and we can get it
merged?

> > As/when improvements are made to the bridge code we can remove the
> > component bits and not lose anything.
> > 
> > > So my suggestion is keeping on one single interface in komeda, no
> > > matter it is bridge or component, but I'd like it only one, but not
> > > them both in komeda.
> > 
> > If we can put the effort into fixing bridges then I guess that's the
> > best approach for everyone :-) Might not be easy though!
> > 
> > -Brian
> > 
> > > 
> > > Thanks
> > > James
> > > 
> > > > Thanks,
> > > > -Brian
> 


-- 
Mihail