[PATCH RFC 0/3] drm/bridge: adjust bridge module's refcount and more

[PATCH RFC 0/3] drm/bridge: adjust bridge module's refcount and more

[Jyri Sarha]

Note! This series is not yet ready and should not be used as is, it is
just an RFC.

The function documentation is missing and so are the modifications to
all affected drivers. I have added the modification patches for tilcdc
and ti-tfp410 to serve as an example and I have put them into separate
patches for readability.

For the matter of adding devres version of of_drm_get_bridge() and
drm_bridge_alloc(), I think that is a good idea. With reference
counting, each user can hold on to their reference to the bridge
object as long as they need it. The bridge object exists as long as
any of the users still holds on to it.

Cheers,
Jyri

Jyri Sarha (3):
  drm/drm_bridge: adjust bridge module's refcount
  drm/bridge: ti-tfp410: Adapt to bridge object and module refcounting
  drm/tilcdc: Adapt to bridge object refcounting

 drivers/gpu/drm/bridge/ti-tfp410.c       | 16 ++++---
 drivers/gpu/drm/drm_bridge.c             | 78 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 15 ++++--
 include/drm/drm_bridge.h                 | 10 +++-
 4 files changed, 99 insertions(+), 20 deletions(-)

-- 
1.9.1

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

[PATCH RFC 1/3] drm/drm_bridge: adjust bridge module's refcount

[Jyri Sarha]

Store the module that provides the bridge and adjust its reference
count accordingly. The bridge module unload should not be allowed
while the bridge is attached. To do this safely we need to add
reference counting to drm_bridge objects. This has impact to both
bridge drivers and the drivers using the bridges.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_bridge.c | 78 +++++++++++++++++++++++++++++++++++++++-----
 include/drm/drm_bridge.h     | 10 +++++-
 2 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 0ee052b..4b0ff45 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/slab.h>
 
 #include <drm/drm_bridge.h>
 
@@ -60,6 +61,39 @@
 static DEFINE_MUTEX(bridge_lock);
 static LIST_HEAD(bridge_list);
 
+static void drm_bridge_release(struct kref *ref)
+{
+	struct drm_bridge *bridge = container_of(ref, struct drm_bridge, kref);
+
+	kfree(bridge);
+}
+
+struct drm_bridge *drm_bridge_alloc(struct module *module)
+{
+	struct drm_bridge *bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+
+	if (!bridge)
+		return NULL;
+
+	bridge->module = module;
+	kref_init(&bridge->kref);
+
+	return bridge;
+}
+EXPORT_SYMBOL(drm_bridge_alloc);
+
+void drm_bridge_get(struct drm_bridge *bridge)
+{
+	kref_get(&bridge->kref);
+}
+EXPORT_SYMBOL(drm_bridge_get);
+
+void drm_bridge_put(struct drm_bridge *bridge)
+{
+	kref_put(&bridge->kref, drm_bridge_release);
+}
+EXPORT_SYMBOL(drm_bridge_put);
+
 /**
  * drm_bridge_add - add the given bridge to the global bridge list
  *
@@ -70,6 +104,8 @@
  */
 int drm_bridge_add(struct drm_bridge *bridge)
 {
+	drm_bridge_get(bridge);
+
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
@@ -88,6 +124,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 	mutex_lock(&bridge_lock);
 	list_del_init(&bridge->list);
 	mutex_unlock(&bridge_lock);
+
+	drm_bridge_put(bridge);
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 
@@ -108,18 +146,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
  */
 int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
 {
+	int ret = 0;
+
 	if (!dev || !bridge)
 		return -EINVAL;
 
+	mutex_lock(&bridge_lock);
+
 	if (bridge->dev)
-		return -EBUSY;
+		ret = -EBUSY;
+	/* Check that the bridge is still part of the list */
+	else if (list_empty(&bridge->list))
+		ret = -ENOENT;
+	else if (!try_module_get(bridge->module))
+		ret = -ENOENT;
+	else
+		bridge->dev = dev;
 
-	bridge->dev = dev;
+	mutex_unlock(&bridge_lock);
 
-	if (bridge->funcs->attach)
-		return bridge->funcs->attach(bridge);
+	if (ret)
+		return ret;
 
-	return 0;
+	if (bridge->funcs->attach) {
+		ret = bridge->funcs->attach(bridge);
+		if (ret)
+			module_put(bridge->module);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
 
@@ -144,6 +199,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	module_put(bridge->module);
+
 	bridge->dev = NULL;
 }
 EXPORT_SYMBOL(drm_bridge_detach);
@@ -312,15 +369,17 @@ void drm_bridge_enable(struct drm_bridge *bridge)
 
 #ifdef CONFIG_OF
 /**
- * of_drm_find_bridge - find the bridge corresponding to the device node in
- *			the global bridge list
+ * of_drm_get_bridge - find the bridge corresponding to the device node in
+ *		       the global bridge list and increase its refcount.
+ *                     It is caller responsibility to call drm_bridge_put()
+ *                     when it no longer needs the object.
  *
  * @np: device node
  *
  * RETURNS:
  * drm_bridge control struct on success, NULL on failure
  */
-struct drm_bridge *of_drm_find_bridge(struct device_node *np)
+struct drm_bridge *of_drm_get_bridge(struct device_node *np)
 {
 	struct drm_bridge *bridge;
 
@@ -328,6 +387,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 
 	list_for_each_entry(bridge, &bridge_list, list) {
 		if (bridge->of_node == np) {
+			drm_bridge_get(bridge);
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
@@ -336,7 +396,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	mutex_unlock(&bridge_lock);
 	return NULL;
 }
-EXPORT_SYMBOL(of_drm_find_bridge);
+EXPORT_SYMBOL(of_drm_get_bridge);
 #endif
 
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 530a1d6..cd39b7c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -25,6 +25,7 @@
 
 #include <linux/list.h>
 #include <linux/ctype.h>
+#include <linux/module.h>
 #include <drm/drm_mode_object.h>
 #include <drm/drm_modes.h>
 
@@ -192,15 +193,22 @@ struct drm_bridge {
 #ifdef CONFIG_OF
 	struct device_node *of_node;
 #endif
+	struct kref kref;
+	struct module *module;
+
 	struct list_head list;
 
 	const struct drm_bridge_funcs *funcs;
 	void *driver_private;
 };
 
+struct drm_bridge *drm_bridge_alloc(struct module *module);
+void drm_bridge_get(struct drm_bridge *bridge);
+void drm_bridge_put(struct drm_bridge *bridge);
+
 int 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_get_bridge(struct device_node *np);
 int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
 void drm_bridge_detach(struct drm_bridge *bridge);
 
-- 
1.9.1

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

[PATCH RFC 2/3] drm/bridge: ti-tfp410: Adapt to bridge object and module refcounting

[Jyri Sarha]

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index b054ea3..f0c81dd 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -19,7 +19,7 @@
 #include <drm/drm_crtc_helper.h>
 
 struct tfp410 {
-	struct drm_bridge	bridge;
+	struct drm_bridge	*bridge;
 	struct drm_connector	connector;
 
 	struct i2c_adapter	*ddc;
@@ -30,7 +30,7 @@ struct tfp410 {
 static inline struct tfp410 *
 drm_bridge_to_tfp410(struct drm_bridge *bridge)
 {
-	return container_of(bridge, struct tfp410, bridge);
+	return bridge->driver_private;
 }
 
 static inline struct tfp410 *
@@ -171,16 +171,18 @@ static int tfp410_init(struct device *dev)
 	if (!dvi)
 		return -ENOMEM;
 	dev_set_drvdata(dev, dvi);
+	dvi->bridge = drm_bridge_alloc(THIS_MODULE);
+	dvi->bridge->driver_private = dvi;
 
-	dvi->bridge.funcs = &tfp410_bridge_funcs;
-	dvi->bridge.of_node = dev->of_node;
+	dvi->bridge->funcs = &tfp410_bridge_funcs;
+	dvi->bridge->of_node = dev->of_node;
 	dvi->dev = dev;
 
 	ret = tfp410_get_connector_ddc(dvi);
 	if (ret)
 		goto fail;
 
-	ret = drm_bridge_add(&dvi->bridge);
+	ret = drm_bridge_add(dvi->bridge);
 	if (ret) {
 		dev_err(dev, "drm_bridge_add() failed: %d\n", ret);
 		goto fail;
@@ -196,11 +198,13 @@ static int tfp410_fini(struct device *dev)
 {
 	struct tfp410 *dvi = dev_get_drvdata(dev);
 
-	drm_bridge_remove(&dvi->bridge);
+	drm_bridge_remove(dvi->bridge);
 
 	if (dvi->ddc)
 		i2c_put_adapter(dvi->ddc);
 
+	drm_bridge_put(dvi->bridge);
+
 	return 0;
 }
 
-- 
1.9.1

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

[PATCH RFC 3/3] drm/tilcdc: Adapt to bridge object refcounting

[Jyri Sarha]

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index 80184f0..31c6092 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -154,8 +154,10 @@ void tilcdc_remove_external_device(struct drm_device *dev)
 		drm_connector_helper_add(priv->external_connector,
 					 priv->connector_funcs);
 
-	if (priv->external_encoder && priv->external_encoder->bridge)
+	if (priv->external_encoder && priv->external_encoder->bridge) {
 		drm_bridge_detach(priv->external_encoder->bridge);
+		drm_bridge_put(priv->external_encoder->bridge);
+	}
 }
 
 static const struct drm_encoder_funcs tilcdc_external_encoder_funcs = {
@@ -234,7 +236,7 @@ int tilcdc_attach_external_device(struct drm_device *ddev)
 	if (!remote_node)
 		return 0;
 
-	bridge = of_drm_find_bridge(remote_node);
+	bridge = of_drm_get_bridge(remote_node);
 	of_node_put(remote_node);
 	if (!bridge)
 		return -EPROBE_DEFER;
@@ -242,20 +244,25 @@ int tilcdc_attach_external_device(struct drm_device *ddev)
 	priv->external_encoder = devm_kzalloc(ddev->dev,
 					      sizeof(*priv->external_encoder),
 					      GFP_KERNEL);
-	if (!priv->external_encoder)
+	if (!priv->external_encoder) {
+		drm_bridge_put(bridge);
 		return -ENOMEM;
+	}
 
 	ret = drm_encoder_init(ddev, priv->external_encoder,
 			       &tilcdc_external_encoder_funcs,
 			       DRM_MODE_ENCODER_NONE, NULL);
 	if (ret) {
 		dev_err(ddev->dev, "drm_encoder_init() failed %d\n", ret);
+		drm_bridge_put(bridge);
 		return ret;
 	}
 
 	ret = tilcdc_attach_bridge(ddev, bridge);
-	if (ret)
+	if (ret) {
 		drm_encoder_cleanup(priv->external_encoder);
+		drm_bridge_put(bridge);
+	}
 
 	return ret;
 }
-- 
1.9.1

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

Re: [PATCH RFC 0/3] drm/bridge: adjust bridge module's refcount and more

[Jyri Sarha]

There has been no comments to these patches, so I guess there are no
major objections. I can go forward with this, and I think a similar job
should be done for drm_panel too. But I wonder if this should be done
for 4.10 any more?

Best regards,
Jyri

On 12/01/16 17:52, Jyri Sarha wrote:
> Note! This series is not yet ready and should not be used as is, it is
> just an RFC.
> 
> The function documentation is missing and so are the modifications to
> all affected drivers. I have added the modification patches for tilcdc
> and ti-tfp410 to serve as an example and I have put them into separate
> patches for readability.
> 
> For the matter of adding devres version of of_drm_get_bridge() and
> drm_bridge_alloc(), I think that is a good idea. With reference
> counting, each user can hold on to their reference to the bridge
> object as long as they need it. The bridge object exists as long as
> any of the users still holds on to it.
> 
> Cheers,
> Jyri
> 
> Jyri Sarha (3):
>   drm/drm_bridge: adjust bridge module's refcount
>   drm/bridge: ti-tfp410: Adapt to bridge object and module refcounting
>   drm/tilcdc: Adapt to bridge object refcounting
> 
>  drivers/gpu/drm/bridge/ti-tfp410.c       | 16 ++++---
>  drivers/gpu/drm/drm_bridge.c             | 78 ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/tilcdc/tilcdc_external.c | 15 ++++--
>  include/drm/drm_bridge.h                 | 10 +++-
>  4 files changed, 99 insertions(+), 20 deletions(-)
> 

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