[PATCH] drm: Document caveats around atomic event handling

[Daniel Stone]

Hi,

On 29 September 2016 at 16:20, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> + * 1. Driver commits new hardware state into vblank-synchronized registes.
> + * 2. A vblank happes, committing the hardware state. Also the corresponding
> + *    vblank interrupt is fired off and fully processed by the interrupt
> + *    handler.
> + * 3. The atomic commit operation proceeds to call drm_crtc_arm_vblank_event().
> + * 4. The event is only send out for the next vblank, which is wrong.
> + *
> + * An equivalent race can happen when the driver calls
> + * drm_crtc_arm_vblank_event() before writing out the new hardware state.

Might be worth pointing out that the equivalent race is backwards,
i.e. the event will be delivered for the vblank before the commit took
effect.

> + * The only way to make this work savely is to prevent the vblank from firing
> + * (and the hardware from committig anything else) until the entire atomic
> + * commit sequence has run to completion. If the hardware does not have such a
> + * feature (e.g. using a "go" bit), then it is unsafe to use this functions.
> + * Instead drivers need to manually send out the event from their interrupt
> + * handler by calling drm_crtc_send_vblank_event() and make sure that there's no
> + * possible race with the hardware committing the atomic update.

I'd add an explicit 'e.g. by verifying that shadow register content
matches primary registers' or something, to make it really clear that
it's not just about queueing event delivery from the vblank IRQ
handler instead.

> + * The only way to make this work savely is to prevent the vblank from firing
> + * (and the hardware from committig anything else) until the entire atomic
> + * commit sequence has run to completion. If the hardware does not have such a
> + * feature (e.g. using a "go" bit), then it is unsafe to use this functions.
> + * Instead drivers need to manually send out the event from their interrupt
> + * handler by calling drm_crtc_send_vblank_event() and make sure that there's no
> + * possible race with the hardware committing the atomic update.

> +        *  - Events for disabled CRTCs are not allowed, and drivers can ignore
> +        *    that case.

I'd clarify that to 'CRTCs which are, and remain, disabled'.

Cheers,
Daniel