| Message ID | 1465388359-8070-6-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 08-06-16 om 14:18 schreef Daniel Vetter: > atomic_flush seems to be the right place, right after we commit the > plane updates. Again use the fullproof version, since the pipe might > be off. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c > index 4182a21f5923..f628b6d8f23f 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c > @@ -51,10 +51,22 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc, > { > struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); > struct sun4i_drv *drv = scrtc->drv; > + struct drm_pending_vblank_event *event = crtc->state->event; > > DRM_DEBUG_DRIVER("Committing plane changes\n"); > > sun4i_backend_commit(drv->backend); > + > + if (event) { > + crtc->state->event = NULL; > + > + spin_lock_irq(&crtc->dev->event_lock); > + if (drm_crtc_vblank_get(crtc) == 0) > + drm_crtc_arm_vblank_event(crtc, event); > + else > + drm_crtc_send_vblank_event(crtc, event); > + spin_unlock_irq(&crtc->dev->event_lock); > + } > } > > static void sun4i_crtc_disable(struct drm_crtc *crtc) Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Hi Daniel, On Wednesday 08 Jun 2016 14:18:58 Daniel Vetter wrote: > atomic_flush seems to be the right place, right after we commit the > plane updates. Again use the fullproof version, since the pipe might > be off. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c > b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 4182a21f5923..f628b6d8f23f > 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c > @@ -51,10 +51,22 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc > *crtc, { > struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); > struct sun4i_drv *drv = scrtc->drv; > + struct drm_pending_vblank_event *event = crtc->state->event; > > DRM_DEBUG_DRIVER("Committing plane changes\n"); > > sun4i_backend_commit(drv->backend); > + > + if (event) { > + crtc->state->event = NULL; > + > + spin_lock_irq(&crtc->dev->event_lock); > + if (drm_crtc_vblank_get(crtc) == 0) > + drm_crtc_arm_vblank_event(crtc, event); > + else > + drm_crtc_send_vblank_event(crtc, event); > + spin_unlock_irq(&crtc->dev->event_lock); > + } Is it me, or is it entirely useless given the code in sun4i_crtc_atomic_flush() that sets crtc->state->event to NULL ? AuthorDate: Wed Jun 8 14:18:58 2016 +0200 CommitDate: Fri Jun 10 16:55:48 2016 +0200 It would have been worth it waiting for the maintainers to review the patch... You've added similar code to a bunch of drivers. How do you protect against the race conditions documented in the drm_crtc_arm_vblank_event() function ? > } > > static void sun4i_crtc_disable(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 4182a21f5923..f628b6d8f23f 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c @@ -51,10 +51,22 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc, { struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); struct sun4i_drv *drv = scrtc->drv; + struct drm_pending_vblank_event *event = crtc->state->event; DRM_DEBUG_DRIVER("Committing plane changes\n"); sun4i_backend_commit(drv->backend); + + if (event) { + crtc->state->event = NULL; + + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); + } } static void sun4i_crtc_disable(struct drm_crtc *crtc)
atomic_flush seems to be the right place, right after we commit the plane updates. Again use the fullproof version, since the pipe might be off. Cc: Boris Brezillon <boris.brezillon@free-electrons.com> Cc: Maxime Ripard <maxime.ripard@free-electrons.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)