[PATCH] drm: Document caveats around atomic event handling

Fri Sep 30 09:41:04 UTC 2016

On Fri, Sep 30, 2016 at 9:56 AM, Andrzej Hajda <a.hajda at 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 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.

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