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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 29, 2016 at 11:20 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> 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@xxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux