Re: [PATCH/RESEND 0/9] drm: tilcdc driver fixes

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jun 28, 2014 at 06:51:15AM -0400, Rob Clark wrote:
> On Fri, Jun 27, 2014 at 6:08 PM, Darren Etheridge <detheridge@ti.com> wrote:
> > Guido,
> >
> >
> > On 06/17/2014 09:17 AM, Guido Martínez wrote:
> >>
> >> The tilcdc driver could be compiled as a module, but was severely broken
> >> and could not be used as such. This patchset attempts to fix the issues
> >> preventing a proper load/unload of the module.
> >>
> >> Issues included dangling sysfs nodes, dangling devices, memory leaks and
> >> a double kfree.
> >>
> >> It now seems to be working ok. We have tested this by loading and
> >> unloading the driver repeteadly, with both panel and slave connectors
> >> and found no flaws.
> >>
> >> There is still one warning left on tilcdc_crtc_destroy, caused by
> >> destroying the connector while still in an ON status. We don't know why
> >> this happens or why it's an issue, so we did not fix it.
> >>
> >
> > Yes I see what you mean, it triggers the WARN_ON in tilcdc_crtc_destroy
> > because DRM_MODE_DPMS_ON is still set.  This WARN_ON does make some sense
> > because DPMS_OFF would have the effect of turning off clocks and putting the
> > monitor to sleep which seems logical considering we have torn down the
> > display.  Adding a tilcdc_crtc_dpms(DPMS_OFF) right before the WARN_ON
> > confirms this, but it seems strange that this hasn't happened automatically
> > (+ Russell doesn't need to do it in his Armada driver) - so I suspect there
> > is a better way.
> 
> tbh, I'm not entirely sure offhand why drm_mode_config_cleanup()
> doesn't remove the fb's first (which should have the effect of
> shutting down any lit crtc/encoder/connector)..  that would seem like
> the sensible way to shut down..

All userspace fbs should be cleared already before going into the driver
unload. Which only leaves you with driver internal fbs (usually just one
for fbdev emulation). It's the driver's job to clean that up explicitly.
Then you can call mode_config_cleanup and the WARN_ON in there is a really
nice space leak check.

If we'd unconditionally clean up all fbs we'd have trouble with
driver-private embedded fbs and their refcounting and would loose the
space leak check.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html