[PATCH 0/2] Use drm-core helpers to wait for updates

[PATCH 0/2] Use drm-core helpers to wait for updates

[Andrzej Hajda]

Hi Inki,

These two patches replaces exynos-drm specific mechanism for waiting
for vblanks with the one provided by drm-core.
The patchset has nice diffstat and fixes some issues,
described in datail in patches.

As the patches touches exynos-drm core I have tested them in different
scenarios on different pipelines:
- TM2: vidi, tv, dsi,
- Odroid-u3: vidi, tv,
- Universal-c210: vidi, tv, rgb.

In all cases it worked.

Regards
Andrzej


Andrzej Hajda (2):
  drm/exynos/vidi: use timer for vblanks instead of sleeping worker
  drm/exynos: fix pending update handling

 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 -
 drivers/gpu/drm/exynos/exynos_drm_vidi.c | 66 ++++++++++----------------------
 4 files changed, 22 insertions(+), 106 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/exynos/vidi: use timer for vblanks instead of sleeping worker

[Andrzej Hajda]

VIDI driver uses fake vblank handler to generate vblank events.
It was implemented using worker which slept for vblank time, additionally
it did not work if there were no page flips. The patch replaces it with
timer, uses drm_crtc_vblank_(on|off) helpers to manage it and fixes
behavior for non-page-flip cases.
This change allows further improvements of vblank in exynos-drm framework.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c | 66 ++++++++++----------------------
 1 file changed, 20 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index e8f6c92..a91dad6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/component.h>
+#include <linux/timer.h>
 
 #include <drm/exynos_drm.h>
 
@@ -28,6 +29,9 @@
 #include "exynos_drm_plane.h"
 #include "exynos_drm_vidi.h"
 
+/* VIDI uses fixed refresh rate of 50Hz */
+#define VIDI_REFRESH_TIME (1000 / 50)
+
 /* vidi has totally three virtual windows. */
 #define WINDOWS_NR		3
 
@@ -43,12 +47,9 @@ struct vidi_context {
 	struct exynos_drm_plane		planes[WINDOWS_NR];
 	struct edid			*raw_edid;
 	unsigned int			clkdiv;
-	unsigned long			irq_flags;
 	unsigned int			connected;
-	bool				vblank_on;
 	bool				suspended;
-	bool				direct_vblank;
-	struct work_struct		work;
+	struct timer_list		timer;
 	struct mutex			lock;
 	int				pipe;
 };
@@ -102,30 +103,14 @@ static int vidi_enable_vblank(struct exynos_drm_crtc *crtc)
 	if (ctx->suspended)
 		return -EPERM;
 
-	if (!test_and_set_bit(0, &ctx->irq_flags))
-		ctx->vblank_on = true;
-
-	ctx->direct_vblank = true;
-
-	/*
-	 * in case of page flip request, vidi_finish_pageflip function
-	 * will not be called because direct_vblank is true and then
-	 * that function will be called by crtc_ops->update_plane callback
-	 */
-	schedule_work(&ctx->work);
+	mod_timer(&ctx->timer,
+		jiffies + msecs_to_jiffies(VIDI_REFRESH_TIME) - 1);
 
 	return 0;
 }
 
 static void vidi_disable_vblank(struct exynos_drm_crtc *crtc)
 {
-	struct vidi_context *ctx = crtc->ctx;
-
-	if (ctx->suspended)
-		return;
-
-	if (test_and_clear_bit(0, &ctx->irq_flags))
-		ctx->vblank_on = false;
 }
 
 static void vidi_update_plane(struct exynos_drm_crtc *crtc,
@@ -140,9 +125,6 @@ static void vidi_update_plane(struct exynos_drm_crtc *crtc,
 
 	addr = exynos_drm_fb_dma_addr(state->fb, 0);
 	DRM_DEBUG_KMS("dma_addr = %pad\n", &addr);
-
-	if (ctx->vblank_on)
-		schedule_work(&ctx->work);
 }
 
 static void vidi_enable(struct exynos_drm_crtc *crtc)
@@ -153,17 +135,17 @@ static void vidi_enable(struct exynos_drm_crtc *crtc)
 
 	ctx->suspended = false;
 
-	/* if vblank was enabled status, enable it again. */
-	if (test_and_clear_bit(0, &ctx->irq_flags))
-		vidi_enable_vblank(ctx->crtc);
-
 	mutex_unlock(&ctx->lock);
+
+	drm_crtc_vblank_on(&crtc->base);
 }
 
 static void vidi_disable(struct exynos_drm_crtc *crtc)
 {
 	struct vidi_context *ctx = crtc->ctx;
 
+	drm_crtc_vblank_off(&crtc->base);
+
 	mutex_lock(&ctx->lock);
 
 	ctx->suspended = true;
@@ -190,28 +172,17 @@ static const struct exynos_drm_crtc_ops vidi_crtc_ops = {
 	.update_plane = vidi_update_plane,
 };
 
-static void vidi_fake_vblank_handler(struct work_struct *work)
+static void vidi_fake_vblank_timer(unsigned long arg)
 {
-	struct vidi_context *ctx = container_of(work, struct vidi_context,
-					work);
+	struct vidi_context *ctx = (void *)arg;
 	int win;
 
 	if (ctx->pipe < 0)
 		return;
 
-	/* refresh rate is about 50Hz. */
-	usleep_range(16000, 20000);
-
-	mutex_lock(&ctx->lock);
-
-	if (ctx->direct_vblank) {
-		drm_crtc_handle_vblank(&ctx->crtc->base);
-		ctx->direct_vblank = false;
-		mutex_unlock(&ctx->lock);
-		return;
-	}
-
-	mutex_unlock(&ctx->lock);
+	if (drm_crtc_handle_vblank(&ctx->crtc->base))
+		mod_timer(&ctx->timer,
+			jiffies + msecs_to_jiffies(VIDI_REFRESH_TIME) - 1);
 
 	for (win = 0 ; win < WINDOWS_NR ; win++) {
 		struct exynos_drm_plane *plane = &ctx->planes[win];
@@ -489,6 +460,9 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
 
 static void vidi_unbind(struct device *dev, struct device *master, void *data)
 {
+	struct vidi_context *ctx = dev_get_drvdata(dev);
+
+	del_timer_sync(&ctx->timer);
 }
 
 static const struct component_ops vidi_component_ops = {
@@ -507,7 +481,7 @@ static int vidi_probe(struct platform_device *pdev)
 
 	ctx->pdev = pdev;
 
-	INIT_WORK(&ctx->work, vidi_fake_vblank_handler);
+	setup_timer(&ctx->timer, vidi_fake_vblank_timer, (unsigned long)ctx);
 
 	mutex_init(&ctx->lock);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/exynos/vidi: use timer for vblanks instead of sleeping worker

[Gustavo Padovan]

2016-09-23 Andrzej Hajda <a.hajda@samsung.com>:

> VIDI driver uses fake vblank handler to generate vblank events.
> It was implemented using worker which slept for vblank time, additionally
> it did not work if there were no page flips. The patch replaces it with
> timer, uses drm_crtc_vblank_(on|off) helpers to manage it and fixes
> behavior for non-page-flip cases.
> This change allows further improvements of vblank in exynos-drm framework.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c | 66 ++++++++++----------------------
>  1 file changed, 20 insertions(+), 46 deletions(-)

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Gustavo

[PATCH 2/2] drm/exynos: fix pending update handling

[Andrzej Hajda]

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@samsung.com>
---
 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 877d2efa..a1bc13f 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 7f1a49d..cee3a4c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -173,8 +173,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;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/exynos: fix pending update handling

[Gustavo Padovan]

2016-09-23 Andrzej Hajda <a.hajda@samsung.com>:

> 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@samsung.com>
> ---
>  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(-)

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel