| Message ID | 20180330150904.30458-2-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Mar 2018 17:09:03 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Rework the atomic commit logic to handle async page flip requests in > the atomic update path. > > Async page flips are a bit more complicated than async cursor updates > (already handled in the vc4_atomic_commit() function) in that you need > to wait for fences on the new framebuffers to be signaled before doing > the flip. In order to ensure that, we schedule a commit_work work to do > the async update (note that the existing implementation already uses a > work to wait for fences to be signaled, so, the latency shouldn't > change that much). > > Of course, we have to modify a bit the atomic_complete_commit() > implementation to avoid waiting for things we don't care about when > doing an async page flip: > > * drm_atomic_helper_wait_for_dependencies() waits for flip_done which > we don't care about when doing async page flips. Instead we replace > that call by a loop that waits for hw_done only > * drm_atomic_helper_commit_modeset_disables() and > drm_atomic_helper_commit_modeset_enables() are not needed because > nothing except the planes' FBs are updated in async page flips > * drm_atomic_helper_commit_planes() is replaced by > vc4_plane_async_set_fb() which is doing only the FB update and is > thus much simpler > * drm_atomic_helper_wait_for_vblanks() is not needed because we don't > wait for the next VBLANK period to apply the new state, and flip > events are signaled manually just after the HW has been updated > > Thanks to this rework, we can get rid of the custom vc4_page_flip() > implementation and its dependencies and use > drm_atomic_helper_page_flip() instead. Another good reason for moving async page flip to the atomic commit path is that is fixes a bug we had: drm_atomic_helper_prepare/cleanup_planes() were not called in the async page flip path, which can lead to unbalanced vc4_inc/dec_usecnt() in some situations (when the plane is updated once with an async page flip and then with a regular update) or trigger a use after free if the BO passed to the plane is marked purgeable and the kernel decides to purge its cache. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 103 +---------------------------------------- > drivers/gpu/drm/vc4/vc4_kms.c | 101 +++++++++++++++++++++++++++++++++------- > 2 files changed, 86 insertions(+), 118 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c > index bf4667481935..3843c39dce61 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -757,107 +757,6 @@ static irqreturn_t vc4_crtc_irq_handler(int irq, void *data) > return ret; > } > > -struct vc4_async_flip_state { > - struct drm_crtc *crtc; > - struct drm_framebuffer *fb; > - struct drm_pending_vblank_event *event; > - > - struct vc4_seqno_cb cb; > -}; > - > -/* Called when the V3D execution for the BO being flipped to is done, so that > - * we can actually update the plane's address to point to it. > - */ > -static void > -vc4_async_page_flip_complete(struct vc4_seqno_cb *cb) > -{ > - struct vc4_async_flip_state *flip_state = > - container_of(cb, struct vc4_async_flip_state, cb); > - struct drm_crtc *crtc = flip_state->crtc; > - struct drm_device *dev = crtc->dev; > - struct vc4_dev *vc4 = to_vc4_dev(dev); > - struct drm_plane *plane = crtc->primary; > - > - vc4_plane_async_set_fb(plane, flip_state->fb); > - if (flip_state->event) { > - unsigned long flags; > - > - spin_lock_irqsave(&dev->event_lock, flags); > - drm_crtc_send_vblank_event(crtc, flip_state->event); > - spin_unlock_irqrestore(&dev->event_lock, flags); > - } > - > - drm_crtc_vblank_put(crtc); > - drm_framebuffer_put(flip_state->fb); > - kfree(flip_state); > - > - up(&vc4->async_modeset); > -} > - > -/* Implements async (non-vblank-synced) page flips. > - * > - * The page flip ioctl needs to return immediately, so we grab the > - * modeset semaphore on the pipe, and queue the address update for > - * when V3D is done with the BO being flipped to. > - */ > -static int vc4_async_page_flip(struct drm_crtc *crtc, > - struct drm_framebuffer *fb, > - struct drm_pending_vblank_event *event, > - uint32_t flags) > -{ > - struct drm_device *dev = crtc->dev; > - struct vc4_dev *vc4 = to_vc4_dev(dev); > - struct drm_plane *plane = crtc->primary; > - int ret = 0; > - struct vc4_async_flip_state *flip_state; > - struct drm_gem_cma_object *cma_bo = drm_fb_cma_get_gem_obj(fb, 0); > - struct vc4_bo *bo = to_vc4_bo(&cma_bo->base); > - > - flip_state = kzalloc(sizeof(*flip_state), GFP_KERNEL); > - if (!flip_state) > - return -ENOMEM; > - > - drm_framebuffer_get(fb); > - flip_state->fb = fb; > - flip_state->crtc = crtc; > - flip_state->event = event; > - > - /* Make sure all other async modesetes have landed. */ > - ret = down_interruptible(&vc4->async_modeset); > - if (ret) { > - drm_framebuffer_put(fb); > - kfree(flip_state); > - return ret; > - } > - > - WARN_ON(drm_crtc_vblank_get(crtc) != 0); > - > - /* Immediately update the plane's legacy fb pointer, so that later > - * modeset prep sees the state that will be present when the semaphore > - * is released. > - */ > - drm_atomic_set_fb_for_plane(plane->state, fb); > - plane->fb = fb; > - > - vc4_queue_seqno_cb(dev, &flip_state->cb, bo->seqno, > - vc4_async_page_flip_complete); > - > - /* Driver takes ownership of state on successful async commit. */ > - return 0; > -} > - > -static int vc4_page_flip(struct drm_crtc *crtc, > - struct drm_framebuffer *fb, > - struct drm_pending_vblank_event *event, > - uint32_t flags, > - struct drm_modeset_acquire_ctx *ctx) > -{ > - if (flags & DRM_MODE_PAGE_FLIP_ASYNC) > - return vc4_async_page_flip(crtc, fb, event, flags); > - else > - return drm_atomic_helper_page_flip(crtc, fb, event, flags, ctx); > -} > - > static struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc) > { > struct vc4_crtc_state *vc4_state; > @@ -902,7 +801,7 @@ vc4_crtc_reset(struct drm_crtc *crtc) > static const struct drm_crtc_funcs vc4_crtc_funcs = { > .set_config = drm_atomic_helper_set_config, > .destroy = vc4_crtc_destroy, > - .page_flip = vc4_page_flip, > + .page_flip = drm_atomic_helper_page_flip, > .set_property = NULL, > .cursor_set = NULL, /* handled by drm_mode_cursor_universal */ > .cursor_move = NULL, /* handled by drm_mode_cursor_universal */ > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index e791e498a3dd..faa2c26f1235 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -25,33 +25,89 @@ > #include "vc4_drv.h" > > static void > -vc4_atomic_complete_commit(struct drm_atomic_state *state) > +vc4_atomic_complete_commit(struct drm_atomic_state *state, bool async) > { > struct drm_device *dev = state->dev; > struct vc4_dev *vc4 = to_vc4_dev(dev); > > drm_atomic_helper_wait_for_fences(dev, state, false); > > - drm_atomic_helper_wait_for_dependencies(state); > + if (async) { > + struct drm_plane_state *plane_state; > + struct drm_crtc_state *new_crtc_state; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + int i; > + > + /* We need to wait for HW done before applying the new FBs > + * otherwise our update might be overwritten by the previous > + * commit. > + */ > + for_each_old_plane_in_state(state, plane, plane_state, i) { > + struct drm_crtc_commit *commit = plane_state->commit; > + int ret; > + > + if (!commit) > + continue; > + > + ret = wait_for_completion_timeout(&commit->hw_done, > + 10 * HZ); > + if (!ret) > + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n", > + plane->base.id, plane->name); > + } > > - drm_atomic_helper_commit_modeset_disables(dev, state); > + /* FIXME: > + * We could use drm_atomic_helper_async_commit() here, but > + * VC4's ->atomic_async_update() implementation expects > + * plane->state to point to the old_state and old/new states > + * have already been swapped. > + * Let's just call our custom function for now and see how we > + * can make that more generic afterwards. > + */ > + for_each_new_plane_in_state(state, plane, plane_state, i) > + vc4_plane_async_set_fb(plane, plane_state->fb); > + > + /* Now, send 'fake' VBLANK events to let the user now its > + * pageflip is done. By fake I mean they are really VBLANK > + * synchronized but it seems to be expected by the core. > + */ > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > + unsigned long flags; > + > + if (!new_crtc_state->event) > + continue; > + > + WARN_ON(drm_crtc_vblank_get(crtc)); > + spin_lock_irqsave(&dev->event_lock, flags); > + drm_crtc_send_vblank_event(crtc, new_crtc_state->event); > + spin_unlock_irqrestore(&dev->event_lock, flags); > + drm_crtc_vblank_put(crtc); > + } > + } else { > + drm_atomic_helper_wait_for_dependencies(state); > > - drm_atomic_helper_commit_planes(dev, state, 0); > + drm_atomic_helper_commit_modeset_disables(dev, state); > > - drm_atomic_helper_commit_modeset_enables(dev, state); > + drm_atomic_helper_commit_planes(dev, state, 0); > > - /* Make sure that drm_atomic_helper_wait_for_vblanks() > - * actually waits for vblank. If we're doing a full atomic > - * modeset (as opposed to a vc4_update_plane() short circuit), > - * then we need to wait for scanout to be done with our > - * display lists before we free it and potentially reallocate > - * and overwrite the dlist memory with a new modeset. > - */ > - state->legacy_cursor_update = false; > + drm_atomic_helper_commit_modeset_enables(dev, state); > + } > > drm_atomic_helper_commit_hw_done(state); > > - drm_atomic_helper_wait_for_vblanks(dev, state); > + if (!async) { > + /* Make sure that drm_atomic_helper_wait_for_vblanks() > + * actually waits for vblank. If we're doing a full atomic > + * modeset (as opposed to a vc4_update_plane() short circuit), > + * then we need to wait for scanout to be done with our > + * display lists before we free it and potentially reallocate > + * and overwrite the dlist memory with a new modeset. > + */ > + state->legacy_cursor_update = false; > + > + drm_atomic_helper_wait_for_vblanks(dev, state); > + } > > drm_atomic_helper_cleanup_planes(dev, state); > > @@ -67,7 +123,20 @@ static void commit_work(struct work_struct *work) > struct drm_atomic_state *state = container_of(work, > struct drm_atomic_state, > commit_work); > - vc4_atomic_complete_commit(state); > + struct drm_crtc_state *new_crtc_state; > + bool async_commit = true; > + struct drm_crtc *crtc; > + int i; > + > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > + if (new_crtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC) > + continue; > + > + async_commit = false; > + break; > + } > + > + vc4_atomic_complete_commit(state, async_commit); > } > > /** > @@ -163,7 +232,7 @@ static int vc4_atomic_commit(struct drm_device *dev, > if (nonblock) > queue_work(system_unbound_wq, &state->commit_work); > else > - vc4_atomic_complete_commit(state); > + vc4_atomic_complete_commit(state, false); > > return 0; > }
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index bf4667481935..3843c39dce61 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -757,107 +757,6 @@ static irqreturn_t vc4_crtc_irq_handler(int irq, void *data) return ret; } -struct vc4_async_flip_state { - struct drm_crtc *crtc; - struct drm_framebuffer *fb; - struct drm_pending_vblank_event *event; - - struct vc4_seqno_cb cb; -}; - -/* Called when the V3D execution for the BO being flipped to is done, so that - * we can actually update the plane's address to point to it. - */ -static void -vc4_async_page_flip_complete(struct vc4_seqno_cb *cb) -{ - struct vc4_async_flip_state *flip_state = - container_of(cb, struct vc4_async_flip_state, cb); - struct drm_crtc *crtc = flip_state->crtc; - struct drm_device *dev = crtc->dev; - struct vc4_dev *vc4 = to_vc4_dev(dev); - struct drm_plane *plane = crtc->primary; - - vc4_plane_async_set_fb(plane, flip_state->fb); - if (flip_state->event) { - unsigned long flags; - - spin_lock_irqsave(&dev->event_lock, flags); - drm_crtc_send_vblank_event(crtc, flip_state->event); - spin_unlock_irqrestore(&dev->event_lock, flags); - } - - drm_crtc_vblank_put(crtc); - drm_framebuffer_put(flip_state->fb); - kfree(flip_state); - - up(&vc4->async_modeset); -} - -/* Implements async (non-vblank-synced) page flips. - * - * The page flip ioctl needs to return immediately, so we grab the - * modeset semaphore on the pipe, and queue the address update for - * when V3D is done with the BO being flipped to. - */ -static int vc4_async_page_flip(struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event, - uint32_t flags) -{ - struct drm_device *dev = crtc->dev; - struct vc4_dev *vc4 = to_vc4_dev(dev); - struct drm_plane *plane = crtc->primary; - int ret = 0; - struct vc4_async_flip_state *flip_state; - struct drm_gem_cma_object *cma_bo = drm_fb_cma_get_gem_obj(fb, 0); - struct vc4_bo *bo = to_vc4_bo(&cma_bo->base); - - flip_state = kzalloc(sizeof(*flip_state), GFP_KERNEL); - if (!flip_state) - return -ENOMEM; - - drm_framebuffer_get(fb); - flip_state->fb = fb; - flip_state->crtc = crtc; - flip_state->event = event; - - /* Make sure all other async modesetes have landed. */ - ret = down_interruptible(&vc4->async_modeset); - if (ret) { - drm_framebuffer_put(fb); - kfree(flip_state); - return ret; - } - - WARN_ON(drm_crtc_vblank_get(crtc) != 0); - - /* Immediately update the plane's legacy fb pointer, so that later - * modeset prep sees the state that will be present when the semaphore - * is released. - */ - drm_atomic_set_fb_for_plane(plane->state, fb); - plane->fb = fb; - - vc4_queue_seqno_cb(dev, &flip_state->cb, bo->seqno, - vc4_async_page_flip_complete); - - /* Driver takes ownership of state on successful async commit. */ - return 0; -} - -static int vc4_page_flip(struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event, - uint32_t flags, - struct drm_modeset_acquire_ctx *ctx) -{ - if (flags & DRM_MODE_PAGE_FLIP_ASYNC) - return vc4_async_page_flip(crtc, fb, event, flags); - else - return drm_atomic_helper_page_flip(crtc, fb, event, flags, ctx); -} - static struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc) { struct vc4_crtc_state *vc4_state; @@ -902,7 +801,7 @@ vc4_crtc_reset(struct drm_crtc *crtc) static const struct drm_crtc_funcs vc4_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = vc4_crtc_destroy, - .page_flip = vc4_page_flip, + .page_flip = drm_atomic_helper_page_flip, .set_property = NULL, .cursor_set = NULL, /* handled by drm_mode_cursor_universal */ .cursor_move = NULL, /* handled by drm_mode_cursor_universal */ diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index e791e498a3dd..faa2c26f1235 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -25,33 +25,89 @@ #include "vc4_drv.h" static void -vc4_atomic_complete_commit(struct drm_atomic_state *state) +vc4_atomic_complete_commit(struct drm_atomic_state *state, bool async) { struct drm_device *dev = state->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); drm_atomic_helper_wait_for_fences(dev, state, false); - drm_atomic_helper_wait_for_dependencies(state); + if (async) { + struct drm_plane_state *plane_state; + struct drm_crtc_state *new_crtc_state; + struct drm_plane *plane; + struct drm_crtc *crtc; + int i; + + /* We need to wait for HW done before applying the new FBs + * otherwise our update might be overwritten by the previous + * commit. + */ + for_each_old_plane_in_state(state, plane, plane_state, i) { + struct drm_crtc_commit *commit = plane_state->commit; + int ret; + + if (!commit) + continue; + + ret = wait_for_completion_timeout(&commit->hw_done, + 10 * HZ); + if (!ret) + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n", + plane->base.id, plane->name); + } - drm_atomic_helper_commit_modeset_disables(dev, state); + /* FIXME: + * We could use drm_atomic_helper_async_commit() here, but + * VC4's ->atomic_async_update() implementation expects + * plane->state to point to the old_state and old/new states + * have already been swapped. + * Let's just call our custom function for now and see how we + * can make that more generic afterwards. + */ + for_each_new_plane_in_state(state, plane, plane_state, i) + vc4_plane_async_set_fb(plane, plane_state->fb); + + /* Now, send 'fake' VBLANK events to let the user now its + * pageflip is done. By fake I mean they are really VBLANK + * synchronized but it seems to be expected by the core. + */ + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + unsigned long flags; + + if (!new_crtc_state->event) + continue; + + WARN_ON(drm_crtc_vblank_get(crtc)); + spin_lock_irqsave(&dev->event_lock, flags); + drm_crtc_send_vblank_event(crtc, new_crtc_state->event); + spin_unlock_irqrestore(&dev->event_lock, flags); + drm_crtc_vblank_put(crtc); + } + } else { + drm_atomic_helper_wait_for_dependencies(state); - drm_atomic_helper_commit_planes(dev, state, 0); + drm_atomic_helper_commit_modeset_disables(dev, state); - drm_atomic_helper_commit_modeset_enables(dev, state); + drm_atomic_helper_commit_planes(dev, state, 0); - /* Make sure that drm_atomic_helper_wait_for_vblanks() - * actually waits for vblank. If we're doing a full atomic - * modeset (as opposed to a vc4_update_plane() short circuit), - * then we need to wait for scanout to be done with our - * display lists before we free it and potentially reallocate - * and overwrite the dlist memory with a new modeset. - */ - state->legacy_cursor_update = false; + drm_atomic_helper_commit_modeset_enables(dev, state); + } drm_atomic_helper_commit_hw_done(state); - drm_atomic_helper_wait_for_vblanks(dev, state); + if (!async) { + /* Make sure that drm_atomic_helper_wait_for_vblanks() + * actually waits for vblank. If we're doing a full atomic + * modeset (as opposed to a vc4_update_plane() short circuit), + * then we need to wait for scanout to be done with our + * display lists before we free it and potentially reallocate + * and overwrite the dlist memory with a new modeset. + */ + state->legacy_cursor_update = false; + + drm_atomic_helper_wait_for_vblanks(dev, state); + } drm_atomic_helper_cleanup_planes(dev, state); @@ -67,7 +123,20 @@ static void commit_work(struct work_struct *work) struct drm_atomic_state *state = container_of(work, struct drm_atomic_state, commit_work); - vc4_atomic_complete_commit(state); + struct drm_crtc_state *new_crtc_state; + bool async_commit = true; + struct drm_crtc *crtc; + int i; + + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC) + continue; + + async_commit = false; + break; + } + + vc4_atomic_complete_commit(state, async_commit); } /** @@ -163,7 +232,7 @@ static int vc4_atomic_commit(struct drm_device *dev, if (nonblock) queue_work(system_unbound_wq, &state->commit_work); else - vc4_atomic_complete_commit(state); + vc4_atomic_complete_commit(state, false); return 0; }
Rework the atomic commit logic to handle async page flip requests in the atomic update path. Async page flips are a bit more complicated than async cursor updates (already handled in the vc4_atomic_commit() function) in that you need to wait for fences on the new framebuffers to be signaled before doing the flip. In order to ensure that, we schedule a commit_work work to do the async update (note that the existing implementation already uses a work to wait for fences to be signaled, so, the latency shouldn't change that much). Of course, we have to modify a bit the atomic_complete_commit() implementation to avoid waiting for things we don't care about when doing an async page flip: * drm_atomic_helper_wait_for_dependencies() waits for flip_done which we don't care about when doing async page flips. Instead we replace that call by a loop that waits for hw_done only * drm_atomic_helper_commit_modeset_disables() and drm_atomic_helper_commit_modeset_enables() are not needed because nothing except the planes' FBs are updated in async page flips * drm_atomic_helper_commit_planes() is replaced by vc4_plane_async_set_fb() which is doing only the FB update and is thus much simpler * drm_atomic_helper_wait_for_vblanks() is not needed because we don't wait for the next VBLANK period to apply the new state, and flip events are signaled manually just after the HW has been updated Thanks to this rework, we can get rid of the custom vc4_page_flip() implementation and its dependencies and use drm_atomic_helper_page_flip() instead. Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/gpu/drm/vc4/vc4_crtc.c | 103 +---------------------------------------- drivers/gpu/drm/vc4/vc4_kms.c | 101 +++++++++++++++++++++++++++++++++------- 2 files changed, 86 insertions(+), 118 deletions(-)