| Message ID | 1475162433-24158-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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: *
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(-)