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:
- Driver commits new hardware state into vblank-synchronized registers.
- A vblank happens, committing the hardware state. Also the corresponding
- vblank interrupt is fired off and fully processed by the interrupt
- handler.
- The atomic commit operation proceeds to call drm_crtc_arm_vblank_event().
- 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