On 16.09.2016 15:27, Tobias Jakobi wrote: > Hello everyone, > > any update on this issue? I can see that the old/custom wait-for-vblank > code is still in place. > > Andrzej mentioned that this patch is quick/dirty, but isn't using DRM > core functionality actually the better approach?
Thanks for pinging the problem. As the patch changes exynos-drm core, it can influence all exynos crtc drivers. These drivers requires review and testing. I will look at them to verify if the patch does not introduce regression. Regards Andrzej > > With best wishes, > Tobias > > > Andrzej Hajda wrote: >> Exynos DRM devices update their registers at vblank time. Exynos-DRM uses >> custom mechanism to wait for vblank. This mechanism is error prone - >> variables are not updated atomically. As a result in certain circumstances >> user space can try to free buffers which are still in use by hardware, >> in such cases IOMMU can throw OOPS. >> The patch instead of fixing the mechanism replaces it with drm core helper. >> >> Signed-off-by: Andrzej Hajda <a.hajda at samsung.com> >> --- >> Hi Tobias, >> >> This is a quick/dirty patch just for checking if it solves your issue. >> Successfully tested on decon5433/dsi/i80 path. >> >> Please verify if it helps in your case. >> >> The patch is based on exynos-drm-next and >> "drm/exynos: fix cancel page flip code" [1]. >> >> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/53801 >> >> Regards >> Andrzej >> --- >> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 +----------- >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 44 >> +------------------------------- >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 2 -- >> 3 files changed, 2 insertions(+), 60 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> index 785ffa6..5b6845b 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >> @@ -134,8 +134,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct >> drm_device *drm_dev, >> exynos_crtc->ops = ops; >> exynos_crtc->ctx = ctx; >> >> - init_waitqueue_head(&exynos_crtc->wait_update); >> - >> crtc = &exynos_crtc->base; >> >> private->crtc[pipe] = crtc; >> @@ -175,13 +173,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device >> *dev, unsigned int pipe) >> exynos_crtc->ops->disable_vblank(exynos_crtc); >> } >> >> -void exynos_drm_crtc_wait_pending_update(struct exynos_drm_crtc >> *exynos_crtc) >> -{ >> - wait_event_timeout(exynos_crtc->wait_update, >> - (atomic_read(&exynos_crtc->pending_update) == 0), >> - msecs_to_jiffies(50)); >> -} >> - >> void exynos_drm_crtc_finish_update(struct exynos_drm_crtc *exynos_crtc, >> struct exynos_drm_plane *exynos_plane) >> { >> @@ -190,9 +181,6 @@ void exynos_drm_crtc_finish_update(struct >> exynos_drm_crtc *exynos_crtc, >> >> exynos_plane->pending_fb = NULL; >> >> - if (atomic_dec_and_test(&exynos_crtc->pending_update)) >> - wake_up(&exynos_crtc->wait_update); >> - >> spin_lock_irqsave(&crtc->dev->event_lock, flags); >> if (exynos_crtc->event) >> drm_crtc_send_vblank_event(crtc, exynos_crtc->event); >> @@ -235,10 +223,8 @@ void exynos_drm_crtc_cancel_page_flip(struct drm_crtc >> *crtc, >> spin_lock_irqsave(&crtc->dev->event_lock, flags); >> >> e = exynos_crtc->event; >> - if (e && e->base.file_priv == file) { >> + if (e && e->base.file_priv == file) >> exynos_crtc->event = NULL; >> - atomic_dec(&exynos_crtc->pending_update); >> - } >> >> spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> index 0281b30..cc96e85 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> @@ -45,37 +45,11 @@ struct exynos_atomic_commit { >> u32 crtcs; >> }; >> >> -static void exynos_atomic_wait_for_commit(struct drm_atomic_state *state) >> -{ >> - struct drm_crtc_state *crtc_state; >> - struct drm_crtc *crtc; >> - int i, ret; >> - >> - for_each_crtc_in_state(state, crtc, crtc_state, i) { >> - struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >> - >> - if (!crtc->state->enable) >> - continue; >> - >> - ret = drm_crtc_vblank_get(crtc); >> - if (ret) >> - continue; >> - >> - exynos_drm_crtc_wait_pending_update(exynos_crtc); >> - drm_crtc_vblank_put(crtc); >> - } >> -} >> - >> static void exynos_atomic_commit_complete(struct exynos_atomic_commit >> *commit) >> { >> struct drm_device *dev = commit->dev; >> struct exynos_drm_private *priv = dev->dev_private; >> struct drm_atomic_state *state = commit->state; >> - struct drm_plane *plane; >> - struct drm_crtc *crtc; >> - struct drm_plane_state *plane_state; >> - struct drm_crtc_state *crtc_state; >> - int i; >> >> drm_atomic_helper_commit_modeset_disables(dev, state); >> >> @@ -89,25 +63,9 @@ static void exynos_atomic_commit_complete(struct >> exynos_atomic_commit *commit) >> * have the relevant clocks enabled to perform the update. >> */ >> >> - for_each_crtc_in_state(state, crtc, crtc_state, i) { >> - struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >> - >> - atomic_set(&exynos_crtc->pending_update, 0); >> - } >> - >> - for_each_plane_in_state(state, plane, plane_state, i) { >> - struct exynos_drm_crtc *exynos_crtc = >> - to_exynos_crtc(plane->crtc); >> - >> - if (!plane->crtc) >> - continue; >> - >> - atomic_inc(&exynos_crtc->pending_update); >> - } >> - >> drm_atomic_helper_commit_planes(dev, state, false); >> >> - exynos_atomic_wait_for_commit(state); >> + drm_atomic_helper_wait_for_vblanks(dev, state); >> >> drm_atomic_helper_cleanup_planes(dev, state); >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> index 561925f..0bb3766 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> @@ -174,8 +174,6 @@ struct exynos_drm_crtc { >> enum exynos_drm_output_type type; >> unsigned int pipe; >> struct drm_pending_vblank_event *event; >> - wait_queue_head_t wait_update; >> - atomic_t pending_update; >> const struct exynos_drm_crtc_ops *ops; >> void *ctx; >> struct exynos_drm_clk *pipe_clk; >> > > >
