commit b5dc0d108cd3c0b50ddcb6f6c54be1bea4c39e01 breaks imx-drm support

[Lucas Stach]

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 at 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 at ffwll.ch>
>  
>  Missing features (not necessarily for moving out of staging):
>  

-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |