On Tue, Aug 27, 2013 at 9:46 AM, Lothar Waßmann LW@karo-electronics.de wrote:
the function imx_drm_driver_load() must have been called before calling imx_drm_add_crtc(), but the following hunk in the commit: @@ -446,6 +434,9 @@ static int imx_drm_driver_load(struct drm_device *drm, unsigned long flags) */ imxdrm->drm->vblank_disable_allowed = 1;
if (!imx_drm_device_get())ret = -EINVAL;ret = 0;err_init:
makes imx_drm_add_crtc() bail out at:
if (imxdrm->references) { ret = -EBUSY; goto err_busy; }Thus it is not possible to register any CRTCs.
Ok I've now read around a bit in the imx core and I think my call to rip this out was right ;-)
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
So as a stopgap measuret I sugges you rip out the imxdrm->references trickery since it won't actually protect you from anything truly nasty happening. And I wouldn't worry about module unloading and refcounting for now since core drm is terminally broken in that area already anyway.
Then we can move ahead and how to really fix this init ordering. So I think we have another TODO here:
Cheers, Daniel
--- diff --git a/drivers/staging/imx-drm/TODO b/drivers/staging/imx-drm/TODO index f806415..f8ef245 100644 --- a/drivers/staging/imx-drm/TODO +++ b/drivers/staging/imx-drm/TODO @@ -6,6 +6,11 @@ TODO: - Factor out more code to common helper functions - decide where to put the base driver. It is not specific to a subsystem and would be used by DRM/KMS and media/V4L2 +- Fix up the driver load sequence to make sure all submodules for encoders/crtcs + are fully loaded before the drm driver is registered. Might require a core drm + rework to break away the drm core init sequence from its midlayer drug and + assorted control inversion issues. Or we restructure imx to just built in + everything, dunno. Requested by Daniel Vetter daniel.vetter@ffwll.ch
Missing features (not necessarily for moving out of staging):
Hi Daniel,
Am Dienstag, den 27.08.2013, 10:08 +0200 schrieb Daniel Vetter:
On Tue, Aug 27, 2013 at 9:46 AM, Lothar Waßmann LW@karo-electronics.de wrote:
the function imx_drm_driver_load() must have been called before calling imx_drm_add_crtc(), but the following hunk in the commit: @@ -446,6 +434,9 @@ static int imx_drm_driver_load(struct drm_device *drm, unsigned long flags) */ imxdrm->drm->vblank_disable_allowed = 1;
if (!imx_drm_device_get())ret = -EINVAL;ret = 0;err_init:
makes imx_drm_add_crtc() bail out at:
if (imxdrm->references) { ret = -EBUSY; goto err_busy; }Thus it is not possible to register any CRTCs.
Ok I've now read around a bit in the imx core and I think my call to rip this out was right ;-)
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
You know the logic you broke here was just a moderately sane approach to get around the monolithic DRM driver nonsense, while not having to use real hotplug in DRM.
The thing is we don't know if we will ever have all submodules loaded, as this driver is mostly used on embedded devices where people randomly decide to exclude things from their kernel config, because they don't use the feature on their board. The current logic is under the premise that there are no early DRM clients in something like Plymouth, which is a bit flaky, but worked very well for the targeted use-cases.
Regards, Lucas
So as a stopgap measuret I sugges you rip out the imxdrm->references trickery since it won't actually protect you from anything truly nasty happening. And I wouldn't worry about module unloading and refcounting for now since core drm is terminally broken in that area already anyway.
Then we can move ahead and how to really fix this init ordering. So I think we have another TODO here:
Cheers, Daniel
diff --git a/drivers/staging/imx-drm/TODO b/drivers/staging/imx-drm/TODO index f806415..f8ef245 100644 --- a/drivers/staging/imx-drm/TODO +++ b/drivers/staging/imx-drm/TODO @@ -6,6 +6,11 @@ TODO:
- Factor out more code to common helper functions
- decide where to put the base driver. It is not specific to a subsystem and would be used by DRM/KMS and media/V4L2
+- Fix up the driver load sequence to make sure all submodules for encoders/crtcs
- are fully loaded before the drm driver is registered. Might require a core drm
- rework to break away the drm core init sequence from its midlayer drug and
- assorted control inversion issues. Or we restructure imx to just built in
- everything, dunno. Requested by Daniel Vetter daniel.vetter@ffwll.ch
Missing features (not necessarily for moving out of staging):
On Tue, Aug 27, 2013 at 10:26 AM, Lucas Stach l.stach@pengutronix.de wrote:
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
You know the logic you broke here was just a moderately sane approach to get around the monolithic DRM driver nonsense, while not having to use real hotplug in DRM.
The thing is we don't know if we will ever have all submodules loaded, as this driver is mostly used on embedded devices where people randomly decide to exclude things from their kernel config, because they don't use the feature on their board. The current logic is under the premise that there are no early DRM clients in something like Plymouth, which is a bit flaky, but worked very well for the targeted use-cases.
Imo the imxdrm->references logic is broken already and simply removing those checks would be the right course of action - pretending to have fixed races but not actually having fixed much is imo much worse than openly admitting that the code is racy and needs work.
Wrt embedded guys shaving off a few kb by not loading a bunch of modules I think you should simply make that compile-time options instead of modules. More hassle but at least it should work.
And if we ever see the need to hotplug crtcs I think the right way is to hotplug an entire drm driver. Connector/encoder hotplugging might eventually be required for real to support stuff like multi-stream DP, but until that happens I really don't see a need for funny games, especially on SoC boards where everything is soldered on and can't possibly be hotplugged. -Daniel
On Tue, Aug 27, 2013 at 10:41:20AM +0200, Daniel Vetter wrote:
On Tue, Aug 27, 2013 at 10:26 AM, Lucas Stach l.stach@pengutronix.de wrote:
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
You know the logic you broke here was just a moderately sane approach to get around the monolithic DRM driver nonsense, while not having to use real hotplug in DRM.
The thing is we don't know if we will ever have all submodules loaded, as this driver is mostly used on embedded devices where people randomly decide to exclude things from their kernel config, because they don't use the feature on their board. The current logic is under the premise that there are no early DRM clients in something like Plymouth, which is a bit flaky, but worked very well for the targeted use-cases.
Imo the imxdrm->references logic is broken already and simply removing those checks would be the right course of action - pretending to have fixed races but not actually having fixed much is imo much worse than openly admitting that the code is racy and needs work.
Wrt embedded guys shaving off a few kb by not loading a bunch of modules I think you should simply make that compile-time options instead of modules. More hassle but at least it should work.
It's not about the few kb. The problem is that our devices are not monolithic, but instead we have multiple devices in and around the SoC which form a DRM device.
Sascha
On Tue, Aug 27, 2013 at 4:56 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Tue, Aug 27, 2013 at 10:41:20AM +0200, Daniel Vetter wrote:
On Tue, Aug 27, 2013 at 10:26 AM, Lucas Stach l.stach@pengutronix.de wrote:
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
You know the logic you broke here was just a moderately sane approach to get around the monolithic DRM driver nonsense, while not having to use real hotplug in DRM.
The thing is we don't know if we will ever have all submodules loaded, as this driver is mostly used on embedded devices where people randomly decide to exclude things from their kernel config, because they don't use the feature on their board. The current logic is under the premise that there are no early DRM clients in something like Plymouth, which is a bit flaky, but worked very well for the targeted use-cases.
Imo the imxdrm->references logic is broken already and simply removing those checks would be the right course of action - pretending to have fixed races but not actually having fixed much is imo much worse than openly admitting that the code is racy and needs work.
Wrt embedded guys shaving off a few kb by not loading a bunch of modules I think you should simply make that compile-time options instead of modules. More hassle but at least it should work.
It's not about the few kb. The problem is that our devices are not monolithic, but instead we have multiple devices in and around the SoC which form a DRM device.
right, but the cores on the SoC, and even any external encoder chips, are not actually hot-pluggable. I have a similar scenario w/ msm drm, where there is the core display controller (for ex, 'mdp4'), plus hdmi/dsi/etc blocks around that (with their own irq, io/register region, etc). It would be nice if the kernel provided a better mechanism for composite drivers, but what I do is just register the platform drivers for those other blocks first (in the init code, before calling 'platform_driver_register(&msm_platform_driver)'. This way, if the device is actually present, I know before drm dev_load. I do not attempt to build hdmi/dsi/etc as separate modules. I could if I wanted to include/exclude them at compile time, but separate modules seems like a bad idea.
BR, -R
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 27, 2013 at 7:19 AM, Rob Clark robdclark@gmail.com wrote:
On Tue, Aug 27, 2013 at 4:56 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Tue, Aug 27, 2013 at 10:41:20AM +0200, Daniel Vetter wrote:
On Tue, Aug 27, 2013 at 10:26 AM, Lucas Stach l.stach@pengutronix.de wrote:
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
You know the logic you broke here was just a moderately sane approach to get around the monolithic DRM driver nonsense, while not having to use real hotplug in DRM.
The thing is we don't know if we will ever have all submodules loaded, as this driver is mostly used on embedded devices where people randomly decide to exclude things from their kernel config, because they don't use the feature on their board. The current logic is under the premise that there are no early DRM clients in something like Plymouth, which is a bit flaky, but worked very well for the targeted use-cases.
Imo the imxdrm->references logic is broken already and simply removing those checks would be the right course of action - pretending to have fixed races but not actually having fixed much is imo much worse than openly admitting that the code is racy and needs work.
Wrt embedded guys shaving off a few kb by not loading a bunch of modules I think you should simply make that compile-time options instead of modules. More hassle but at least it should work.
It's not about the few kb. The problem is that our devices are not monolithic, but instead we have multiple devices in and around the SoC which form a DRM device.
right, but the cores on the SoC, and even any external encoder chips, are not actually hot-pluggable. I have a similar scenario w/ msm drm,
oh, and come to think of it, same approach it tilcdc. And I'm sure there are other drivers with the same scenario.
BR, -R
where there is the core display controller (for ex, 'mdp4'), plus hdmi/dsi/etc blocks around that (with their own irq, io/register region, etc). It would be nice if the kernel provided a better mechanism for composite drivers, but what I do is just register the platform drivers for those other blocks first (in the init code, before calling 'platform_driver_register(&msm_platform_driver)'. This way, if the device is actually present, I know before drm dev_load. I do not attempt to build hdmi/dsi/etc as separate modules. I could if I wanted to include/exclude them at compile time, but separate modules seems like a bad idea.
BR, -R
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Aug 27, 2013 at 1:22 PM, Rob Clark robdclark@gmail.com wrote:
right, but the cores on the SoC, and even any external encoder chips, are not actually hot-pluggable. I have a similar scenario w/ msm drm,
oh, and come to think of it, same approach it tilcdc. And I'm sure there are other drivers with the same scenario.
i915 and nouveau also support encoder slaves, and they also have them all built-in. And I think requesting the module at driver init time (where I mean any point in time between when the master module loads and userspace first starts using it) seems fraught with with deadlock issues if we have to wait for it.
So imo in the current drm state there is no way you can built slave drivers as modules. It's simply broken. I agree that it'd be nice to have a better solution, but atm I don't even have an idea what it is. But applying duct-tape in ->firstopen is certainly not the right way. -Daniel
On Tue, Aug 27, 2013 at 01:26:32PM +0200, Daniel Vetter wrote:
On Tue, Aug 27, 2013 at 1:22 PM, Rob Clark robdclark@gmail.com wrote:
right, but the cores on the SoC, and even any external encoder chips, are not actually hot-pluggable. I have a similar scenario w/ msm drm,
oh, and come to think of it, same approach it tilcdc. And I'm sure there are other drivers with the same scenario.
i915 and nouveau also support encoder slaves, and they also have them all built-in. And I think requesting the module at driver init time (where I mean any point in time between when the master module loads and userspace first starts using it) seems fraught with with deadlock issues if we have to wait for it.
So imo in the current drm state there is no way you can built slave drivers as modules. It's simply broken. I agree that it'd be nice to have a better solution, but atm I don't even have an idea what it is. But applying duct-tape in ->firstopen is certainly not the right way.
The imx-drm does not request modules itself. It also does not do hotplug. It also does not change the drm device structure while being opened. The drm device structure is only ever changed when the device is closed. Once it's opened it's completely static and will not be changed. All this 'core' stuff in the drm driver is only done to be able to separate the different encoders into linux platform_devices, module support is only a side effect of this.
What the imx-drm driver does is:
- collect subcomponents via imx_drm_add_connector, imx_drm_add_crtc and imx_drm_add_encoder - During drm device open time the try_module_get will make sure the modules providing encoders/crtcs do not get unloaded. once the device is opened imx_drm_add_* will return -EBUSY. - When the drm device is closed module_put is called and components are allowed to be registered/unregistered again.
Indeed hotplug support for components would be nice, but as said this is not supported by drm and by the imx-drm driver.
Sascha
Am Dienstag, den 27.08.2013, 10:41 +0200 schrieb Daniel Vetter:
On Tue, Aug 27, 2013 at 10:26 AM, Lucas Stach l.stach@pengutronix.de wrote:
If I understand stuff correctly you have a main driver plus a bunch of encoder/crtc modules and you expect that everything gets loaded and then only when the kms driver is first opened. The current drm core just doesn't support hotplugging of encoder/crtc objects after driver init has completed. If you try to do that it'll go down in flames due to all the races involved.
You know the logic you broke here was just a moderately sane approach to get around the monolithic DRM driver nonsense, while not having to use real hotplug in DRM.
The thing is we don't know if we will ever have all submodules loaded, as this driver is mostly used on embedded devices where people randomly decide to exclude things from their kernel config, because they don't use the feature on their board. The current logic is under the premise that there are no early DRM clients in something like Plymouth, which is a bit flaky, but worked very well for the targeted use-cases.
Imo the imxdrm->references logic is broken already and simply removing those checks would be the right course of action - pretending to have fixed races but not actually having fixed much is imo much worse than openly admitting that the code is racy and needs work.
I don't see how this is overly racy. We are doing delayed DRM device setup not delayed HW setup. We simply look around which modules are there and what DRM we can build up from them.
With some small work we would even be able to make the Plymouth + modules in initrd case work. The only case that would not really be solvable without full hotplug is the Plymouth in initrd + modules on the late rootfs.
Wrt embedded guys shaving off a few kb by not loading a bunch of modules I think you should simply make that compile-time options instead of modules. More hassle but at least it should work.
This would be really moving in the wrong direction. We want to get more modular, not less. We have a lot different on- and off-chip encoders and other stuff where we want distinct drivers and not some cludged together thing.
And if we ever see the need to hotplug crtcs I think the right way is to hotplug an entire drm driver. Connector/encoder hotplugging might eventually be required for real to support stuff like multi-stream DP, but until that happens I really don't see a need for funny games, especially on SoC boards where everything is soldered on and can't possibly be hotplugged.
Regards, Lucas
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Lucas Stach -
Rob Clark -
Sascha Hauer