[Intel-gfx] [PATCH 10/37] drm/doc: vblank cleanup

[Daniel Vetter]

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 at nvidia.com>

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