Re: [PATCH 07/12] drm/irq: kerneldoc polish

["Michel Dänzer" <michel@daenzer.net>]

On 15.05.2014 03:51, Daniel Vetter wrote:
> @@ -964,8 +1005,13 @@ EXPORT_SYMBOL(drm_vblank_off);
>  
>  /**
>   * drm_vblank_on - enable vblank events on a CRTC
> - * @dev: DRM device
> + * @dev: drm device
>   * @crtc: CRTC in question
> + *
> + * This functions restores the vblank interrupt state captured with
> + * drm_vblank_off() again. Note that calls to drm_vblank_on() and
> + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called
> + * in driver load code to reflect the current hardware state of the crtc.

Given this description, maybe something like drm_vblank_save/restore
would describe better what these functions do than drm_vblank_off/on?


> @@ -981,11 +1027,26 @@ EXPORT_SYMBOL(drm_vblank_on);
>  
>  /**
>   * drm_vblank_pre_modeset - account for vblanks across mode sets
> - * @dev: DRM device
> + * @dev: drm device
>   * @crtc: CRTC in question
>   *
>   * Account for vblank events across mode setting events, which will likely
>   * reset the hardware frame counter.
> + *
> + * This is done by grabbing a temporary vblank reference to ensure that the
> + * vblank interrupt keeps running across the modeset sequence. With this the
> + * software-side vblank frame counting will ensure that there are no jumps or
> + * discontinuities.
> + *
> + * Unfortunately this approach is racy and also doesn't work when the vblank
> + * interrupt stops running, e.g. across system suspend resume. It is therefore
> + * highly recommended that drivers use the newer drm_vblank_off() and
> + * drm_vblank_on() instead. drm_vblank_pre_modeset() only works correctly when
> + * using "cooked" software vblank frame counters and not relying on any hardware
> + * counters.

That last statement is not true IME with radeon[0].

Basically, it sounds to me like drm_vblank_off/on do more or less what
drm_vblank_pre/post_modeset are intended to do (e.g. the latter can also
be nested arbitrarily). Still not really sure why we need two sets of
these instead of fixing any problems in one set.


[0] Though we certainly don't have as rigorous testing for that as you
guys do in intel-gpu-tools. Any chance some of that could be moved to
somewhere more generic?

-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer