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(-)
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);
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; }
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; }
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@lists.freedesktop.org
-
Jyri Sarha