| Message ID | 20170811134912.6429-8-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laurent, Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 11/08/17 16:49, Laurent Pinchart wrote: > The omapdrm platform device is a virtual device created for the sole > purpose of handling the omapdss/omapdrm driver split. It should > eventually be removed. As a first step to ease refactoring move its > registration from platform code to driver code. > > The omapdrm driver name must be changed internally to avoid probing both > the device registered in platform code and the device registered in the > omapdss driver, as that would otherwise break bisection. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > Changes since v1: > > - Drop the CONFIG_DRM_OMAP conditional compilation > - Unregister the platform device at exit time > --- > drivers/gpu/drm/omapdrm/dss/core.c | 15 +++++++++++++++ > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c > index 4dabe32c7098..8678d8b4efce 100644 > --- a/drivers/gpu/drm/omapdrm/dss/core.c > +++ b/drivers/gpu/drm/omapdrm/dss/core.c > @@ -22,6 +22,7 @@ > > #define DSS_SUBSYS_NAME "CORE" > > +#include <linux/dma-mapping.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/clk.h> > @@ -103,6 +104,14 @@ static void (*dss_output_drv_unreg_funcs[])(void) = { > dss_uninit_platform_driver, > }; > > +static struct platform_device omap_drm_device = { > + .dev = { > + .coherent_dma_mask = DMA_BIT_MASK(32), > + }, > + .name = "omapdrm_", > + .id = 0, > +}; > + > static int __init omap_dss_init(void) > { > int r; > @@ -118,6 +127,10 @@ static int __init omap_dss_init(void) > goto err_reg; > } > > + r = platform_device_register(&omap_drm_device); > + if (r) > + goto err_reg; > + > return 0; > > err_reg: > @@ -135,6 +148,8 @@ static void __exit omap_dss_exit(void) > { > int i; > > + platform_device_unregister(&omap_drm_device); > + > for (i = 0; i < ARRAY_SIZE(dss_output_drv_unreg_funcs); ++i) > dss_output_drv_unreg_funcs[i](); > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c > index 022029ea6972..9ab22e0c0b84 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -734,7 +734,7 @@ static SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume); > > static struct platform_driver pdev = { > .driver = { > - .name = DRIVER_NAME, > + .name = "omapdrm_", > .pm = &omapdrm_pm_ops, > }, > .probe = pdev_probe, When unloading: [ 40.099847] WARNING: CPU: 1 PID: 247 at drivers/base/core.c:818 device_release+0x8c/0x9c [ 40.107992] Device 'omapdrm_.0' does not have a release() function, it is broken and must be fixed. [ 40.119120] Modules linked in: cfbfillrect cfbimgblt cfbcopyarea omapdss(-) omapdss_base [last unloaded: snd_soc_omap_hdmi_audio] [ 40.131311] CPU: 1 PID: 247 Comm: rmmod Not tainted 4.13.0-rc3-00056-g2e3e69f7fd0d #26 [ 40.139279] Hardware name: Generic DRA74X (Flattened Device Tree) [ 40.145412] Backtrace: [ 40.147903] [<c010dc10>] (dump_backtrace) from [<c010debc>] (show_stack+0x18/0x1c) [ 40.155521] r7:c0ed4910 r6:60080013 r5:00000000 r4:c0ed4910 [ 40.161241] [<c010dea4>] (show_stack) from [<c09288c4>] (dump_stack+0xa8/0xdc) r[ 40.168517] [<c092881c>] (dump_stack) from [<c013bd80>] (__warn+0xdc/0x108) [ 40.175599] r9:c05caef0 r8:00000332 r7:00000009 r6:c0c2eb78 r5:00000000 r4:ed0ebe98 [ 40.183396] [<c013bca4>] (__warn) from [<c013bde8>] (warn_slowpath_fmt+0x3c/0x44) [ 40.190926] r9:ed0ea000 r8:c0108c04 r7:ee2415c0 r6:ed10da80 r5:bf041010 r4:c0c2ebbc [ 40.198722] [<c013bdb0>] (warn_slowpath_fmt) from [<c05caef0>] (device_release+0x8c/0x9c) [ 40.206947] r3:ee2415c0 r2:c0c2ebbc [ 40.210550] r4:bf041018 [ 40.213115] [<c05cae64>] (device_release) from [<c092ce94>] (kobject_put+0xf8/0x228) [ 40.220905] r7:ee2415c0 r6:c0eec2b0 r5:c0ea1870 r4:bf041018 [ 40.226616] [<c092cd9c>] (kobject_put) from [<c05cb3d8>] (put_device+0x1c/0x20) [ 40.234050] r7:00000081 r6:0003cd4c r5:00000800 r4:bf041000 [ 40.239758] [<c05cb3bc>] (put_device) from [<c05d2dd8>] (platform_device_unregister+0x24/0x28) [ 40.248493] [<c05d2db4>] (platform_device_unregister) from [<bf032f1c>] (omap_dss_exit+0x14/0x3c [omapdss]) [ 40.258291] r5:00000800 r4:bf042880 [ 40.261961] [<bf032f08>] (omap_dss_exit [omapdss]) from [<c01ec8e4>] (SyS_delete_module+0x178/0x250) [ 40.271147] r5:00000800 r4:bf042880 [ 40.274759] [<c01ec76c>] (SyS_delete_module) from [<c0108a60>] (ret_fast_syscall+0x0/0x1c) [ 40.283074] r6:beac9c10 r5:0003cd10 r4:beac9dfc [ 40.289781] ---[ end trace 197bbc25cd50eb0f ]--- I think we didn't encounter this earlier, as the device was never freed. Tomi
Hi Laurent, Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 11/08/17 16:49, Laurent Pinchart wrote: > The omapdrm platform device is a virtual device created for the sole > purpose of handling the omapdss/omapdrm driver split. It should > eventually be removed. As a first step to ease refactoring move its > registration from platform code to driver code. > > The omapdrm driver name must be changed internally to avoid probing both > the device registered in platform code and the device registered in the > omapdss driver, as that would otherwise break bisection. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > Changes since v1: > > - Drop the CONFIG_DRM_OMAP conditional compilation > - Unregister the platform device at exit time > --- > drivers/gpu/drm/omapdrm/dss/core.c | 15 +++++++++++++++ > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c > index 4dabe32c7098..8678d8b4efce 100644 > --- a/drivers/gpu/drm/omapdrm/dss/core.c > +++ b/drivers/gpu/drm/omapdrm/dss/core.c > @@ -22,6 +22,7 @@ > > #define DSS_SUBSYS_NAME "CORE" > > +#include <linux/dma-mapping.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/clk.h> > @@ -103,6 +104,14 @@ static void (*dss_output_drv_unreg_funcs[])(void) = { > dss_uninit_platform_driver, > }; > > +static struct platform_device omap_drm_device = { > + .dev = { > + .coherent_dma_mask = DMA_BIT_MASK(32), > + }, > + .name = "omapdrm_", > + .id = 0, > +}; > + > static int __init omap_dss_init(void) > { > int r; > @@ -118,6 +127,10 @@ static int __init omap_dss_init(void) > goto err_reg; > } > > + r = platform_device_register(&omap_drm_device); > + if (r) > + goto err_reg; > + Was there a reason to create the omapdrm device in omap_dss_init (i.e. module_init), instead of omapdss's probe? Now the omapdrm device is always created even if there's no DSS device on the SoC. Tomi
Hi Tomi, On Thursday, 28 September 2017 11:03:32 EEST Tomi Valkeinen wrote: > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > On 11/08/17 16:49, Laurent Pinchart wrote: > > The omapdrm platform device is a virtual device created for the sole > > purpose of handling the omapdss/omapdrm driver split. It should > > eventually be removed. As a first step to ease refactoring move its > > registration from platform code to driver code. > > > > The omapdrm driver name must be changed internally to avoid probing both > > the device registered in platform code and the device registered in the > > omapdss driver, as that would otherwise break bisection. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > --- > > Changes since v1: > > > > - Drop the CONFIG_DRM_OMAP conditional compilation > > - Unregister the platform device at exit time > > --- > > > > drivers/gpu/drm/omapdrm/dss/core.c | 15 +++++++++++++++ > > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/omapdrm/dss/core.c > > b/drivers/gpu/drm/omapdrm/dss/core.c index 4dabe32c7098..8678d8b4efce > > 100644 > > --- a/drivers/gpu/drm/omapdrm/dss/core.c > > +++ b/drivers/gpu/drm/omapdrm/dss/core.c > > @@ -22,6 +22,7 @@ > > > > #define DSS_SUBSYS_NAME "CORE" > > > > +#include <linux/dma-mapping.h> > > > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/clk.h> > > > > @@ -103,6 +104,14 @@ static void (*dss_output_drv_unreg_funcs[])(void) = { > > > > dss_uninit_platform_driver, > > > > }; > > > > +static struct platform_device omap_drm_device = { > > + .dev = { > > + .coherent_dma_mask = DMA_BIT_MASK(32), > > + }, > > + .name = "omapdrm_", > > + .id = 0, > > +}; > > + > > > > static int __init omap_dss_init(void) > > { > > > > int r; > > > > @@ -118,6 +127,10 @@ static int __init omap_dss_init(void) > > > > goto err_reg; > > > > } > > > > + r = platform_device_register(&omap_drm_device); > > + if (r) > > + goto err_reg; > > + > > Was there a reason to create the omapdrm device in omap_dss_init (i.e. > module_init), instead of omapdss's probe? Now the omapdrm device is > always created even if there's no DSS device on the SoC. Not really, no. Do you think this requires a quick -stable fix ? I'm working on a patch series that will remove the omapdrm device completely.
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki On 28/09/17 11:56, Laurent Pinchart wrote: >> Was there a reason to create the omapdrm device in omap_dss_init (i.e. >> module_init), instead of omapdss's probe? Now the omapdrm device is >> always created even if there's no DSS device on the SoC. > > Not really, no. > > Do you think this requires a quick -stable fix ? I'm working on a patch series > that will remove the omapdrm device completely. With -stable, you mean 4.14? Afaics, the only downside is a small amount of memory that's not used. It does behave differently than before, as the now-removed platform code did only add the omapdrm device if there's a dss device in the DT data. I don't think that really matters, though. But still, maybe it's nice to fix for 4.14, as we're still only at -rc2. We encountered this with TI kernel, which has DSS6 driver. The omapdrm side is shared, but the dss side is different than with the current dss. So, the dss6 driver needs to create omapdrm device, but old dss driver already creates it... We'll hack around it, as it's only an issue in a development kernel. Tomi
diff --git a/drivers/gpu/drm/omapdrm/dss/core.c b/drivers/gpu/drm/omapdrm/dss/core.c index 4dabe32c7098..8678d8b4efce 100644 --- a/drivers/gpu/drm/omapdrm/dss/core.c +++ b/drivers/gpu/drm/omapdrm/dss/core.c @@ -22,6 +22,7 @@ #define DSS_SUBSYS_NAME "CORE" +#include <linux/dma-mapping.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/clk.h> @@ -103,6 +104,14 @@ static void (*dss_output_drv_unreg_funcs[])(void) = { dss_uninit_platform_driver, }; +static struct platform_device omap_drm_device = { + .dev = { + .coherent_dma_mask = DMA_BIT_MASK(32), + }, + .name = "omapdrm_", + .id = 0, +}; + static int __init omap_dss_init(void) { int r; @@ -118,6 +127,10 @@ static int __init omap_dss_init(void) goto err_reg; } + r = platform_device_register(&omap_drm_device); + if (r) + goto err_reg; + return 0; err_reg: @@ -135,6 +148,8 @@ static void __exit omap_dss_exit(void) { int i; + platform_device_unregister(&omap_drm_device); + for (i = 0; i < ARRAY_SIZE(dss_output_drv_unreg_funcs); ++i) dss_output_drv_unreg_funcs[i](); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 022029ea6972..9ab22e0c0b84 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -734,7 +734,7 @@ static SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume); static struct platform_driver pdev = { .driver = { - .name = DRIVER_NAME, + .name = "omapdrm_", .pm = &omapdrm_pm_ops, }, .probe = pdev_probe,