[PATCH] drm: Document caveats around atomic event handling

[PATCH] drm: Document caveats around atomic event handling

[Daniel Vetter]

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.

Cc: Andrzej Hajda <a.hajda@samsung.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..8874b564ec76 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 even 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 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.
+ *
+ * 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.
+ *
  * 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..b381be639fd8 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 out scanning out the current
+	 *    framebuffers. It should contain the timestampe 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 timestampe 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

Re: [PATCH] drm: Document caveats around atomic event handling

[Alex Deucher]

On Thu, Sep 29, 2016 at 11:20 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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.

Thanks for filling in these details.  A few spelling typos noted below.

>
> Try to remedy this by documenting everything better.
>
> Cc: Andrzej Hajda <a.hajda@samsung.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..8874b564ec76 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 even in struct &drm_crtc_state

event

> + * 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 registes.

registers

> + * 2. A vblank happes, committing the hardware state. Also the corresponding

happens

> + *    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 savely is to prevent the vblank from firing

safely

> + * (and the hardware from committig anything else) until the entire atomic

committing

> + * 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..b381be639fd8 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

sent

> +        *    after the hardware has stopped out scanning out the current

stopped scanning out

> +        *    framebuffers. It should contain the timestampe and counter for the

timestamp

> +        *    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 timestampe and counter must

timestamp

> +        *    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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Document caveats around atomic event handling

[Daniel Stone]

Hi,

On 29 September 2016 at 16:20, Daniel Vetter <daniel.vetter@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Document caveats around atomic event handling

[Daniel Stone]

On 30 September 2016 at 11:55, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi,
>
> [...]

... and with that, Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel, off to find more coffee
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Document caveats around atomic event handling

[Daniel Vetter]

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

Re: [PATCH] drm: Document caveats around atomic event handling

[Daniel Vetter]

On Fri, Sep 30, 2016 at 9:56 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> 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@samsung.com>
>> Cc: Alex Deucher <alexdeucher@gmail.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..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.

Sending to late is better than sending too early, but you can't ensure
that through ordering of the cpu execution: The vblank irq handler
might be delayed too, which means even when you ordering the call to
drm_crtc_arm_vblank_event after committing hardware state you might
get things wrong:

1. vblank happens, irq gets fired but processing is delayed.
2. hardware commit happens. Because it just missed the vblank it'll
only show up on the next frame.
3. drm_crtc_arm_vblank_event runs.
4. Finally your irq handler is run, and immediately sends out the
vblank event, 1 frame too early.

You _really_ need to synchronize in some way with your hw commit logic
or it can get wrong. And there's really no generic code that's at
least safe (i.e. only sends out events late if it races).

>> + *
>> + * 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.

Yes, but wikipedia already has that so I don't think we need to define that.

> - vblank timestamp - timestamp of start of the vblank, or equivalently
> end of frame data transmission.

Timestampe needs to conform to OML_sync_control, which is start of
first line of the next frame. See the kerneldoc for
drm_calc_vbltimestamp_from_scanoutpos().
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@samsung.com>
> Cc: Alex Deucher <alexdeucher@gmail.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..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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Document caveats around atomic event handling

[Alex Deucher]

On Thu, Sep 29, 2016 at 11:50 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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.

Missed a few noted below.  With those fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Alex Deucher <alexdeucher@gmail.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..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.
> + *
> + * 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 to prevent the vblank from firing

safely

> + * (and the hardware from committig anything else) until the entire atomic


committing

> + * 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

[PATCH] drm: Document caveats around atomic event handling

[Daniel Vetter]

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@samsung.com>
Cc: Alex Deucher <alexdeucher@gmail.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..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.
+ *
+ * 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 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.
+	 *
+	 *  - 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