Re: [PATCH] drm/i2c: tda998x: don't register the connector

[Brian Starkey <brian.starkey@arm.com>]

On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
>On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
>> On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
>> > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
>> > > enabled, but we never got the call to turn off the CRTC. Brian is still
>> > > tracking through the fbdev emulation to figure out the cause for that.
>> >
>> > fbdev emulation doesn't do that for you. If you need/want to shut down
>> > all the crtcs on driver unload, you need to do that yourself. There's
>> > atomic helpers to do that for you that for you.
>>
>> The problem is a sort-of circular dependency between ->lastclose (at
>> least the common implementation of it), unregister and disabling
>> fbdev.
>>
>> I want to move drm_dev_unregister() to be the first thing we do at
>> rmmod-time. However we need to disable fbdev first, otherwise
>> ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
>> on for drm_vsync_cleanup() to then WARN on.
>>
>> There's a slightly different (perceived) problem - the one that Liviu
>> mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
>> You say it's not the fbdev helpers' responsibility to teardown their
>> modeset, but regardless I have nowhere to disable the CRTC if I want
>> to do drm_dev_unregister() first; and if the CRTC isn't disabled
>> there's always a chance of hitting the same vsync WARN even without
>> fbdev.
>
>Just disable all crtc in a suitable place (after drm_dev_unregister,
>before you tear down fbdev).

I think this is predicated on first removing the drm_vblank_cleanup
call.

>>
>> We *could* add an ->unload and disable everything there, but as that's
>> deprecated I'm guessing there should be another way.
>> Perhaps we should track ->firstopen/->lastclose pairs so we can detect
>> that ->lastclose is being called from unregister and use it to
>> disable everything in that case.
>
>Hm, maybe we should simply not call ->lastclose for kms drivers. That is
>kinda only a hack for ums/dri1 drivers.
>

To be clear (and in response to Russell's question) - you mean
only the call to ->lastclose in drm_dev_unregister, not in general?

>But even with that gone you might still unload while fbdev is 
>enabled, so
>this won't fix it all.
>

Yeah it will be tidier, but I don't think it actually fixes anything.

>> drm_vblank_cleanup() seems to have been carried over to unregister
>> from drm_put_dev(), but drm_dev_register() doesn't call
>> drm_vblank_init() so it seems a little strange to have it there.
>> I can see other drivers I'd expect to hit the same WARN but I don't
>> have HW to test it on.
>
>Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
>should push it into all callers instead I think.

OK so two things to do - remove drm_vblank_cleanup() from
drm_dev_unregister(), and then do the teardown like so:

	drm_dev_unregister();
	drm_crtc_force_disable_all(); // or atomic equivalent
	fbdev_teardown();
	...

Seems good to me. Are there any ordering constraints you're aware of
for drm_vblank_cleanup()? Or you think just putting it after
drm_dev_unregister() should be OK?

Thanks,
-Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>