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):