[PATCH] drm: Document caveats around atomic event handling

[Daniel Vetter <daniel.vetter@ffwll.ch>]

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.

v3: More typos Alex spotted.

Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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..ce8f90d07541 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.
+ *
+ * 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 safely is to prevent the vblank from firing
+ * (and the hardware from committing 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.
+	 *
+	 *  - Events for disabled CRTCs are not allowed, and drivers can ignore
+	 *    that case.
+	 *
+	 * This can be handled by the drm_crtc_send_vblank_event() function,
+	 * which the driver should call on the provided event upon completion of
+	 * the atomic commit. Note that if the driver supports vblank signalling
+	 * and timestamping the vblank counters and timestamps must agree with
+	 * the ones returned from page flip events. With the current vblank
+	 * helper infrastructure this can be achieved by holding a vblank
+	 * reference while the page flip is pending, acquired through
+	 * drm_crtc_vblank_get() and released with drm_crtc_vblank_put().
+	 * Drivers are free to implement their own vblank counter and timestamp
+	 * tracking though, e.g. if they have accurate timestamp registers in
+	 * hardware.
+	 *
+	 * For hardware which supports some means to synchronize vblank
+	 * interrupt delivery with committing display state there's also
+	 * drm_crtc_arm_vblank_event(). See the documentation of that function
+	 * for a detailed discussion of the constraints it needs to be used
+	 * safely.
+	 */
 	struct drm_pending_vblank_event *event;
 
 	struct drm_atomic_state *state;
@@ -835,17 +873,9 @@ struct drm_mode_config_funcs {
 	 * CRTC index supplied in &drm_event to userspace.
 	 *
 	 * The drm core will supply a struct &drm_event in the event
-	 * member of each CRTC's &drm_crtc_state structure. This can be handled by the
-	 * drm_crtc_send_vblank_event() function, which the driver should call on
-	 * the provided event upon completion of the atomic commit. Note that if
-	 * the driver supports vblank signalling and timestamping the vblank
-	 * counters and timestamps must agree with the ones returned from page
-	 * flip events. With the current vblank helper infrastructure this can
-	 * be achieved by holding a vblank reference while the page flip is
-	 * pending, acquired through drm_crtc_vblank_get() and released with
-	 * drm_crtc_vblank_put(). Drivers are free to implement their own vblank
-	 * counter and timestamp tracking though, e.g. if they have accurate
-	 * timestamp registers in hardware.
+	 * member of each CRTC's &drm_crtc_state structure. See the
+	 * documentation for &drm_crtc_state for more details about the precise
+	 * semantics of this event.
 	 *
 	 * NOTE:
 	 *
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel