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
> > > <li...@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.atanas...@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+ker...@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+ker...@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...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
