[PATCH] drm: Document caveats around atomic event handling

[Andrzej Hajda]

Hi Daniel,

Thanks for very descriptive comment.
Few comments/questions below.

On 29.09.2016 17:50, Daniel Vetter wrote:
> It's not that obvious how a driver can all race the atomic commit with
> handling the completion event. And there's unfortunately a pile of
> drivers with rather bad event handling which misdirect people into the
> wrong direction.
>
> Try to remedy this by documenting everything better.
>
> v2: Type fixes Alex spotted.
>
> Cc: Andrzej Hajda <a.hajda at samsung.com>
> Cc: Alex Deucher <alexdeucher at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 32 +++++++++++++++++++++++++--
>  include/drm/drm_crtc.h    | 56 ++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 73 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 10611a936059..dd59c0d8b652 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1008,6 +1008,31 @@ static void send_vblank_event(struct drm_device *dev,
>   * period. This helper function implements exactly the required vblank arming
>   * behaviour.
>   *
> + * NOTE: Drivers using this to send out the event in struct &drm_crtc_state
> + * as part of an atomic commit must ensure that the next vblank happens at
> + * exactly the same time as the atomic commit is committed to the hardware. This
> + * function itself does **not** protect again the next vblank interrupt racing
> + * with either this function call or the atomic commit operation. A possible
> + * sequence could be:
> + *
> + * 1. Driver commits new hardware state into vblank-synchronized registers.
> + * 2. A vblank happens, 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.

How disastrous is this delayed event?
I would like to say that in some cases hardware does not provide
reliable ways
to make it always right. In such case sending event for next vblank is
safer -
we are sure that hw will not touch old buffers. Otherwise there is
danger of page
faults.

> + *
> + * An equivalent race can happen when the driver calls
> + * drm_crtc_arm_vblank_event() before writing out the new hardware state.
> + *
> + * The only way to make this work savely 

Is it typo? or old English on purpose, something between safely and savvy :)

> 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.
> + *
>   * Caller must hold event lock. Caller must also hold a vblank reference for
>   * the event @e, which will be dropped when the next vblank arrives.
>   */
> @@ -1030,8 +1055,11 @@ EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
>   * @crtc: the source CRTC of the vblank event
>   * @e: the event to send
>   *
> - * Updates sequence # and timestamp on event, and sends it to userspace.
> - * Caller must hold event lock.
> + * Updates sequence # and timestamp on event for the most recently processed
> + * vblank, and sends it to userspace.  Caller must hold event lock.
> + *
> + * See drm_crtc_arm_vblank_event() for a helper which can be used in certain
> + * situation, especially to send out events for atomic commit operations.
>   */
>  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  				struct drm_pending_vblank_event *e)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f88f9a2d05c1..eac3e4067fe5 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -109,8 +109,6 @@ struct drm_plane_helper_funcs;
>   * @ctm: Transformation matrix
>   * @gamma_lut: Lookup table for converting pixel data after the
>   *	conversion matrix
> - * @event: optional pointer to a DRM event to signal upon completion of the
> - * 	state update
>   * @state: backpointer to global drm_atomic_state
>   *
>   * Note that the distinction between @enable and @active is rather subtile:
> @@ -159,6 +157,46 @@ struct drm_crtc_state {
>  	struct drm_property_blob *ctm;
>  	struct drm_property_blob *gamma_lut;
>  
> +	/**
> +	 * @event:
> +	 *
> +	 * Optional pointer to a DRM event to signal upon completion of the
> +	 * state update. The driver must send out the event when the atomic
> +	 * commit operation completes. There are two cases:
> +	 *
> +	 *  - The event is for a CRTC which is being disabled through this
> +	 *    atomic commit. In that case the event can be send out any time
> +	 *    after the hardware has stopped scanning out the current
> +	 *    framebuffers. It should contain the timestamp and counter for the
> +	 *    last vblank before the display pipeline was shut off.
> +	 *
> +	 *  - For a CRTC which is enabled at the end of the commit (even when it
> +	 *    undergoes an full modeset) the vblank timestamp and counter must
> +	 *    be for the vblank right before the first frame that scans out the
> +	 *    new set of buffers. Again the event can only be sent out after the
> +	 *    hardware has stopped scanning out the old buffers.

For both cases. What if state update is performed during vblank period.
I guess in such case event timestamp should/could be set to end of state
update, am I right?

One more thing, I have not found precise definition of vblank, so to be
sure we
think about the same thing my adhoc 'definitions':
- vblank - period of time when active crtc does not transmit image,
  in case of video mode it is since beginning of front porch to the end
of back porch,
  command mode case is clear.
- vblank timestamp - timestamp of start of the vblank, or equivalently
end of frame data transmission.

Are these definitions correct?

Regards
Andrzej