As per the docs, atomic_commit should return -EBUSY "if an asycnhronous updated is requested and there is an earlier updated pending".
v2: Use the status of the workqueue instead of vop->event, and don't add a superfluous wait on the workqueue.
v3: Drop work_busy, as there's a sizeable delay when the worker finishes, which introduces a race in which the client has already received the last flip event but the next page flip ioctl will still return -EBUSY because work_busy returns outdated information.
Signed-off-by: Tomeu Vizoso tomeu.vizoso@collabora.com --- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 13 ++++++++++++- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++++ 3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index 3529f692edb8..37952eefd33d 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -69,6 +69,7 @@ int rockchip_register_crtc_funcs(struct drm_crtc *crtc, void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc); int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type, int out_mode); +bool rockchip_drm_crtc_has_pending_event(struct drm_crtc *crtc); int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, struct device *dev); void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 3b8f652698f8..8305bbd2a4d7 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev, { struct rockchip_drm_private *private = dev->dev_private; struct rockchip_atomic_commit *commit = &private->commit; - int ret; + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + int i, ret; + + if (async) { + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (crtc->state->event || + rockchip_drm_crtc_has_pending_event(crtc)) { + return -EBUSY; + } + } + }
ret = drm_atomic_helper_prepare_planes(dev, state); if (ret) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index e2118e62345b..5bcdd8dc7499 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -848,6 +848,11 @@ int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, } EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
+bool rockchip_drm_crtc_has_pending_event(struct drm_crtc *crtc) +{ + return to_vop(crtc)->event; +} + static int vop_crtc_enable_vblank(struct drm_crtc *crtc) { struct vop *vop = to_vop(crtc);
Hi Tomeu,
On 4 April 2016 at 14:55, Tomeu Vizoso tomeu.vizoso@collabora.com wrote:
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 3b8f652698f8..8305bbd2a4d7 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev, { struct rockchip_drm_private *private = dev->dev_private; struct rockchip_atomic_commit *commit = &private->commit;
int ret;
struct drm_crtc_state *crtc_state;
struct drm_crtc *crtc;
int i, ret;
if (async) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (crtc->state->event ||
rockchip_drm_crtc_has_pending_event(crtc)) {
return -EBUSY;
}
}
}
Hmmm ...
Doesn't this trigger before the VOP atomic_begin() helper, meaning that anything with an event set will trigger the check? Seems like it should be && rather than ||.
Also, I know rockchip_drm_vop.c isn't holding dev->event_lock when it checks vop->event, but it really should be ... and so should this check. Otherwise you can race with the IRQ handler, which isn't required to hold the CRTC lock.
Cheers, Daniel
On 4 April 2016 at 17:44, Daniel Stone daniel@fooishbar.org wrote:
Hi Tomeu,
On 4 April 2016 at 14:55, Tomeu Vizoso tomeu.vizoso@collabora.com wrote:
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index 3b8f652698f8..8305bbd2a4d7 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev, { struct rockchip_drm_private *private = dev->dev_private; struct rockchip_atomic_commit *commit = &private->commit;
int ret;
struct drm_crtc_state *crtc_state;
struct drm_crtc *crtc;
int i, ret;
if (async) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (crtc->state->event ||
rockchip_drm_crtc_has_pending_event(crtc)) {
return -EBUSY;
}
}
}
Hmmm ...
Doesn't this trigger before the VOP atomic_begin() helper, meaning that anything with an event set will trigger the check? Seems like it should be && rather than ||.
So, these are the two cases that this code aims to handle:
1. A previous request with an event set hasn't progressed to atomic_begin yet, so the event field in drm_crtc_state (at this point, the old state) is still populated but vop->event still isn't.
2. A previous request has already progressed to atomic_begin but we haven't gotten yet the interrupt that signals its completion, so vop->event is populated but crtc->state->event isn't anymore.
My understanding is that in both cases there's a pending flip event and we have to return -EBUSY if it's as async request.
Also, I know rockchip_drm_vop.c isn't holding dev->event_lock when it checks vop->event, but it really should be ... and so should this check. Otherwise you can race with the IRQ handler, which isn't required to hold the CRTC lock.
Cool, thanks.
Tomeu
Cheers, Daniel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Tomeu,
On 5 April 2016 at 16:07, Tomeu Vizoso tomeu.vizoso@collabora.com wrote:
On 4 April 2016 at 17:44, Daniel Stone daniel@fooishbar.org wrote:
On 4 April 2016 at 14:55, Tomeu Vizoso tomeu.vizoso@collabora.com wrote:
if (async) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (crtc->state->event ||
rockchip_drm_crtc_has_pending_event(crtc)) {
return -EBUSY;
}
}
}
Hmmm ...
Doesn't this trigger before the VOP atomic_begin() helper, meaning that anything with an event set will trigger the check? Seems like it should be && rather than ||.
So, these are the two cases that this code aims to handle:
- A previous request with an event set hasn't progressed to
atomic_begin yet, so the event field in drm_crtc_state (at this point, the old state) is still populated but vop->event still isn't.
Ah right, this was what I was missing: the async (non-blocking) implementation. Sounds good to me then.
Cheers, Daniel
dri-devel@lists.freedesktop.org