Re: [PATCH 4/4] drm/imx: only send commit done event when all state has been applied
On Wed, Jan 23, 2019 at 12:35:02PM +0100, Philipp Zabel wrote:
> On Fri, 2018-10-05 at 17:11 +0200, Philipp Zabel wrote:
> > On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> > > Currently there is a small race window where we could manage to arm the
> > > vblank event from atomic flush, but programming the hardware was too close
> > > to the frame end, so the hardware will only apply the current state on the
> > > next vblank. In this case we will send out the commit done event too early
> > > causing userspace to reuse framebuffes that are still in use.
> > >
> > > Instead of using the event arming mechnism, just remember the pending
> > > event
> > > and send it from the vblank IRQ handler, once we are sure that all state
> > > has been applied sucessfully.
> > >
> > > Signed-off-by: Lucas Stach
> > > ---
> > > drivers/gpu/drm/imx/ipuv3-crtc.c | 32
> > > 1 file changed, 28 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > index 7d4b710b837a..b0c95565a28d 100644
> > > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > @@ -42,6 +42,7 @@ struct ipu_crtc {
> > > struct ipu_dc *dc;
> > > struct ipu_di *di;
> > > int irq;
> > > + struct drm_pending_vblank_event *event;
> > > };
> > >
> > > static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
> > > @@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
> > > static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
> > > {
> > > struct ipu_crtc *ipu_crtc = dev_id;
> > > + struct drm_crtc *crtc = _crtc->base;
> > > + unsigned long flags;
> > > + int i;
> > > +
> > > + drm_crtc_handle_vblank(crtc);
> > > +
> > > + if (ipu_crtc->event) {
> > > + for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
> > > + struct ipu_plane *plane = ipu_crtc->plane[i];
> > > +
> > > + if (!plane)
> > > + continue;
> > > +
> > > + if (!ipu_plane_atomic_update_done(>base))
> >
> > if (ipu_plane_atomic_update_pending(>base))
> >
> > > + break;
> > > + }
> > >
> > > - drm_crtc_handle_vblank(_crtc->base);
> > > + if (i == ARRAY_SIZE(ipu_crtc->plane)) {
> > > + spin_lock_irqsave(>dev->event_lock, flags);
> > > + drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
> > > + ipu_crtc->event = NULL;
> >
> > These two happen under the event spinlock, but where event is set in
> > ipu_crtc_atomic_flush, the locking is removed.
> >
> > > + drm_crtc_vblank_put(crtc);
> > > + spin_unlock_irqrestore(>dev->event_lock, flags);
> > > + }
> > > + }
> > >
> > > return IRQ_HANDLED;
> > > }
> > > @@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc
> > > *crtc,
> > > static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
> > > struct drm_crtc_state *old_crtc_state)
> > > {
> > > - spin_lock_irq(>dev->event_lock);
> > > + struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> > > +
> > > if (crtc->state->event) {
> > > WARN_ON(drm_crtc_vblank_get(crtc));
> > > - drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > > + ipu_crtc->event = crtc->state->event;
> >
> > We assume here that ipu_crtc->event is NULL and the irq handler is never
> > running at the same time, otherwise we would drop an event. This is non-
> > obvious to me, and I think it warrants a comment.
> >
> > My understanding is the following:
> >
> > - It is virtually impossible for atomic_flush to race against a delayed
> > previous ipu_irq_handler because the previous commit's commit_tail
> > would still be waiting for the vblank event to release it from
> > drm_atomic_helper_wait_for_flip_done.
> >
> > However, if the last commit's tail finishes after the irq_handler
> > calls drm_crtc_send_vblank_event(), and the new commit is issued, and
> > its tail work scheduled, all before the next line in the irq_handler,
> > ipu_crtc->event = NULL, then the new commit's tail could call
> > drm_atomic_helper_commit_planes and therefore ipu_crtc_atomic_flush
> > and ipu_crtc->event would be overwritten.
> >
> > - It is unproblematic for a delayed atomic_flush to race against the
> > next ipu_irq_handler because ipu_crtc->event will just not be set
> > when the irq handler checks it, and the vblank event will be deferred
> > to the next interrupt.
>
> How do we proceed with this? Keep the spin lock?
Yeah, standard practice is to protect these things with a spinlock,
usually the drm->event_lock. Then the flip_done wait should make sure
overall ordering is correct, too.
Might be good to improve the kerneldocs that this is a recommended
pattern.
-Daniel
--
Re: [PATCH 4/4] drm/imx: only send commit done event when all state has been applied
On Fri, 2018-10-05 at 17:11 +0200, Philipp Zabel wrote:
> On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> > Currently there is a small race window where we could manage to arm the
> > vblank event from atomic flush, but programming the hardware was too close
> > to the frame end, so the hardware will only apply the current state on the
> > next vblank. In this case we will send out the commit done event too early
> > causing userspace to reuse framebuffes that are still in use.
> >
> > Instead of using the event arming mechnism, just remember the pending event
> > and send it from the vblank IRQ handler, once we are sure that all state
> > has been applied sucessfully.
> >
> > Signed-off-by: Lucas Stach
> > ---
> > drivers/gpu/drm/imx/ipuv3-crtc.c | 32
> > 1 file changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > index 7d4b710b837a..b0c95565a28d 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -42,6 +42,7 @@ struct ipu_crtc {
> > struct ipu_dc *dc;
> > struct ipu_di *di;
> > int irq;
> > + struct drm_pending_vblank_event *event;
> > };
> >
> > static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
> > @@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
> > static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
> > {
> > struct ipu_crtc *ipu_crtc = dev_id;
> > + struct drm_crtc *crtc = _crtc->base;
> > + unsigned long flags;
> > + int i;
> > +
> > + drm_crtc_handle_vblank(crtc);
> > +
> > + if (ipu_crtc->event) {
> > + for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
> > + struct ipu_plane *plane = ipu_crtc->plane[i];
> > +
> > + if (!plane)
> > + continue;
> > +
> > + if (!ipu_plane_atomic_update_done(>base))
>
> if (ipu_plane_atomic_update_pending(>base))
>
> > + break;
> > + }
> >
> > - drm_crtc_handle_vblank(_crtc->base);
> > + if (i == ARRAY_SIZE(ipu_crtc->plane)) {
> > + spin_lock_irqsave(>dev->event_lock, flags);
> > + drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
> > + ipu_crtc->event = NULL;
>
> These two happen under the event spinlock, but where event is set in
> ipu_crtc_atomic_flush, the locking is removed.
>
> > + drm_crtc_vblank_put(crtc);
> > + spin_unlock_irqrestore(>dev->event_lock, flags);
> > + }
> > + }
> >
> > return IRQ_HANDLED;
> > }
> > @@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> > static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
> > struct drm_crtc_state *old_crtc_state)
> > {
> > - spin_lock_irq(>dev->event_lock);
> > + struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> > +
> > if (crtc->state->event) {
> > WARN_ON(drm_crtc_vblank_get(crtc));
> > - drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > + ipu_crtc->event = crtc->state->event;
>
> We assume here that ipu_crtc->event is NULL and the irq handler is never
> running at the same time, otherwise we would drop an event. This is non-
> obvious to me, and I think it warrants a comment.
>
> My understanding is the following:
>
> - It is virtually impossible for atomic_flush to race against a delayed
> previous ipu_irq_handler because the previous commit's commit_tail
> would still be waiting for the vblank event to release it from
> drm_atomic_helper_wait_for_flip_done.
>
> However, if the last commit's tail finishes after the irq_handler
> calls drm_crtc_send_vblank_event(), and the new commit is issued, and
> its tail work scheduled, all before the next line in the irq_handler,
> ipu_crtc->event = NULL, then the new commit's tail could call
> drm_atomic_helper_commit_planes and therefore ipu_crtc_atomic_flush
> and ipu_crtc->event would be overwritten.
>
> - It is unproblematic for a delayed atomic_flush to race against the
> next ipu_irq_handler because ipu_crtc->event will just not be set
> when the irq handler checks it, and the vblank event will be deferred
> to the next interrupt.
How do we proceed with this? Keep the spin lock?
regards
Philipp
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/imx: only send commit done event when all state has been applied
On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> Currently there is a small race window where we could manage to arm the
> vblank event from atomic flush, but programming the hardware was too close
> to the frame end, so the hardware will only apply the current state on the
> next vblank. In this case we will send out the commit done event too early
> causing userspace to reuse framebuffes that are still in use.
>
> Instead of using the event arming mechnism, just remember the pending event
> and send it from the vblank IRQ handler, once we are sure that all state
> has been applied sucessfully.
>
> Signed-off-by: Lucas Stach
> ---
> drivers/gpu/drm/imx/ipuv3-crtc.c | 32
> 1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c
> b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 7d4b710b837a..b0c95565a28d 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -42,6 +42,7 @@ struct ipu_crtc {
> struct ipu_dc *dc;
> struct ipu_di *di;
> int irq;
> + struct drm_pending_vblank_event *event;
> };
>
> static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
> @@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
> static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
> {
> struct ipu_crtc *ipu_crtc = dev_id;
> + struct drm_crtc *crtc = _crtc->base;
> + unsigned long flags;
> + int i;
> +
> + drm_crtc_handle_vblank(crtc);
> +
> + if (ipu_crtc->event) {
> + for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
> + struct ipu_plane *plane = ipu_crtc->plane[i];
> +
> + if (!plane)
> + continue;
> +
> + if (!ipu_plane_atomic_update_done(>base))
if (ipu_plane_atomic_update_pending(>base))
> + break;
> + }
>
> - drm_crtc_handle_vblank(_crtc->base);
> + if (i == ARRAY_SIZE(ipu_crtc->plane)) {
> + spin_lock_irqsave(>dev->event_lock, flags);
> + drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
> + ipu_crtc->event = NULL;
These two happen under the event spinlock, but where event is set in
ipu_crtc_atomic_flush, the locking is removed.
> + drm_crtc_vblank_put(crtc);
> + spin_unlock_irqrestore(>dev->event_lock, flags);
> + }
> + }
>
> return IRQ_HANDLED;
> }
> @@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc *crtc,
> static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
> - spin_lock_irq(>dev->event_lock);
> + struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> +
> if (crtc->state->event) {
> WARN_ON(drm_crtc_vblank_get(crtc));
> - drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> + ipu_crtc->event = crtc->state->event;
We assume here that ipu_crtc->event is NULL and the irq handler is never
running at the same time, otherwise we would drop an event. This is non-
obvious to me, and I think it warrants a comment.
My understanding is the following:
- It is virtually impossible for atomic_flush to race against a delayed
previous ipu_irq_handler because the previous commit's commit_tail
would still be waiting for the vblank event to release it from
drm_atomic_helper_wait_for_flip_done.
However, if the last commit's tail finishes after the irq_handler
calls drm_crtc_send_vblank_event(), and the new commit is issued, and
its tail work scheduled, all before the next line in the irq_handler,
ipu_crtc->event = NULL, then the new commit's tail could call
drm_atomic_helper_commit_planes and therefore ipu_crtc_atomic_flush
and ipu_crtc->event would be overwritten.
- It is unproblematic for a delayed atomic_flush to race against the
next ipu_irq_handler because ipu_crtc->event will just not be set
when the irq handler checks it, and the vblank event will be deferred
to the next interrupt.
regards
Philipp
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
