On Thu, Jun 15, 2017 at 02:58:15PM +0200, Thierry Reding wrote: > On Wed, May 24, 2017 at 04:51:45PM +0200, Daniel Vetter wrote: > [...] > > -Resources allocated by :c:func:`drm_vblank_init()` must be freed > > -with a call to :c:func:`drm_vblank_cleanup()` in the driver unload > > -operation handler. > > I think perhaps this is the reason why this was cargo-culted. It's > confusing to have the documentation say drivers have to call it, when in > fact the core already does. > > > @@ -396,6 +436,8 @@ EXPORT_SYMBOL(drm_vblank_cleanup); > > * @num_crtcs: number of CRTCs supported by @dev > > * > > * This function initializes vblank support for @num_crtcs display pipelines. > > + * Drivers do not need to call drm_vblank_cleanup(), cleanup is already handled > > + * by the DRM core. > > This is key, I think, in understanding why the patch series is correct. > Maybe this should go into the cover letter to provide more context. Yeah I put this all a bit backwards since I first just reviewed drivers, and only when removing the EXPORT_SYMBOL line did I digg out the real history behind all this - it was done to have a kms->ums fallback for radeon. > That said, there is one case where the core wouldn't be calling the > drm_vblank_cleanup() as part of teardown, and that is when the driver > implements a ->release() callback. From a very cursory look i915 is the > only driver that does so, and it ends up calling drm_dev_fini() and > therefore drm_vblank_cleanup(). Hm, good point, I'll augment the kernel-doc to make this a bit clearer. > Given all of the above, for the series: > > Acked-by: Thierry Reding <treding@xxxxxxxxxx> Thanks for taking a look, I'll vacuum up the core patches. Probably need to resend the remaining driver patches for another round of pings. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
