[PATCH 0/5] drm/tegra: Miscellaneous enhancements

[PATCH 0/5] drm/tegra: Miscellaneous enhancements

[Thierry Reding]

Hi,

This patch series introduces a number of useful features for the Tegra
DRM driver. The first patch allows VBLANK support to be used in drivers
that don't or can't use DRIVER_HAVE_IRQ for interrupt support.

Patch 2 adds support for the additional two planes available on Tegra
hardware, while patch 3 implement .mode_set_base() to allow for some
nice speed up when changing the framebuffer without actually changing
the resolution. Patch 4 adds VBLANK support and finally patch 5 builds
on it to provide the page-flipping IOCTL.

Thierry

Thierry Reding (5):
  drm: Allow vblank support without DRIVER_HAVE_IRQ
  drm/tegra: Add plane support
  drm/tegra: Implement .mode_set_base()
  drm/tegra: Implement VBLANK support
  drm/tegra: Implement page-flipping support

 drivers/gpu/drm/drm_irq.c   |   5 +-
 drivers/gpu/drm/tegra/dc.c  | 475 ++++++++++++++++++++++++++++++++++----------
 drivers/gpu/drm/tegra/dc.h  |   2 +-
 drivers/gpu/drm/tegra/drm.c |  53 +++++
 drivers/gpu/drm/tegra/drm.h |  37 ++++
 5 files changed, 461 insertions(+), 111 deletions(-)

-- 
1.8.1

[PATCH 1/5] drm: Allow vblank support without DRIVER_HAVE_IRQ

[Thierry Reding]

Drivers that register interrupt handlers without the DRM core helpers
don't initialize the .irq_enabled field and drm_dev_to_irq() may fail
when called on them. This shouldn't preclude them from implementing
the vblank IOCTL.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/gpu/drm/drm_irq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 19c01ca..71f8205 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1218,8 +1218,9 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	int ret;
 	unsigned int flags, seq, crtc, high_crtc;
 
-	if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled))
-		return -EINVAL;
+	if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
+		if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled))
+			return -EINVAL;
 
 	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
 		return -EINVAL;
-- 
1.8.1

[PATCH 2/5] drm/tegra: Add plane support

[Thierry Reding]

Add support for the B and C planes which support RGB and YUV pixel
formats and can be used as overlays or hardware cursor.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c  | 321 +++++++++++++++++++++++++++++++-------------
 drivers/gpu/drm/tegra/dc.h  |   2 +-
 drivers/gpu/drm/tegra/drm.h |  29 ++++
 3 files changed, 259 insertions(+), 93 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 656b2e3..157e962 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -18,19 +18,104 @@
 #include "drm.h"
 #include "dc.h"
 
-struct tegra_dc_window {
-	fixed20_12 x;
-	fixed20_12 y;
-	fixed20_12 w;
-	fixed20_12 h;
-	unsigned int outx;
-	unsigned int outy;
-	unsigned int outw;
-	unsigned int outh;
-	unsigned int stride;
-	unsigned int fmt;
+struct tegra_plane {
+	struct drm_plane base;
+	unsigned int index;
 };
 
+static inline struct tegra_plane *to_tegra_plane(struct drm_plane *plane)
+{
+	return container_of(plane, struct tegra_plane, base);
+}
+
+static int tegra_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+			      struct drm_framebuffer *fb, int crtc_x,
+			      int crtc_y, unsigned int crtc_w,
+			      unsigned int crtc_h, uint32_t src_x,
+			      uint32_t src_y, uint32_t src_w, uint32_t src_h)
+{
+	unsigned long base = tegra_framebuffer_base(fb);
+	struct tegra_plane *p = to_tegra_plane(plane);
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+	struct tegra_dc_window window;
+
+	memset(&window, 0, sizeof(window));
+	window.src.x = src_x >> 16;
+	window.src.y = src_y >> 16;
+	window.src.w = src_w >> 16;
+	window.src.h = src_h >> 16;
+	window.dst.x = crtc_x;
+	window.dst.y = crtc_y;
+	window.dst.w = crtc_w;
+	window.dst.h = crtc_h;
+	window.format = tegra_dc_format(fb->pixel_format);
+	window.bits_per_pixel = fb->bits_per_pixel;
+	window.stride = fb->pitches[0];
+	window.base = base;
+
+	return tegra_dc_setup_window(dc, p->index + 1, &window);
+}
+
+static int tegra_plane_disable(struct drm_plane *plane)
+{
+	struct tegra_dc *dc = to_tegra_dc(plane->crtc);
+	struct tegra_plane *p = to_tegra_plane(plane);
+	unsigned int index = p->index + 1;
+	unsigned long value;
+
+	value = WINDOW_A_SELECT << index;
+	tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+	value = tegra_dc_readl(dc, DC_WIN_WIN_OPTIONS);
+	value &= ~WIN_ENABLE;
+	tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS);
+
+	value = (WIN_A_ACT_REQ << index) | (WIN_A_UPDATE << index);
+	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+	return 0;
+}
+
+static void tegra_plane_destroy(struct drm_plane *plane)
+{
+	drm_plane_cleanup(plane);
+}
+
+static const struct drm_plane_funcs tegra_plane_funcs = {
+	.update_plane = tegra_plane_update,
+	.disable_plane = tegra_plane_disable,
+	.destroy = tegra_plane_destroy,
+};
+
+static const uint32_t plane_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_YUV422,
+};
+
+static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
+{
+	unsigned int i;
+	int err = 0;
+
+	for (i = 0; i < 2; i++) {
+		struct tegra_plane *plane;
+
+		plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
+		if (!plane)
+			return -ENOMEM;
+
+		plane->index = i;
+
+		err = drm_plane_init(drm, &plane->base, 1 << dc->pipe,
+				     &tegra_plane_funcs, plane_formats,
+				     ARRAY_SIZE(plane_formats), false);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 static const struct drm_crtc_funcs tegra_crtc_funcs = {
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = drm_crtc_cleanup,
@@ -47,10 +132,11 @@ static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static inline u32 compute_dda_inc(fixed20_12 inf, unsigned int out, bool v,
+static inline u32 compute_dda_inc(unsigned int in, unsigned int out, bool v,
 				  unsigned int bpp)
 {
 	fixed20_12 outf = dfixed_init(out);
+	fixed20_12 inf = dfixed_init(in);
 	u32 dda_inc;
 	int max;
 
@@ -80,9 +166,10 @@ static inline u32 compute_dda_inc(fixed20_12 inf, unsigned int out, bool v,
 	return dda_inc;
 }
 
-static inline u32 compute_initial_dda(fixed20_12 in)
+static inline u32 compute_initial_dda(unsigned int in)
 {
-	return dfixed_frac(in);
+	fixed20_12 inf = dfixed_init(in);
+	return dfixed_frac(inf);
 }
 
 static int tegra_dc_set_timings(struct tegra_dc *dc,
@@ -153,6 +240,111 @@ static int tegra_crtc_setup_clk(struct drm_crtc *crtc,
 	return 0;
 }
 
+int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
+			  const struct tegra_dc_window *window)
+{
+	unsigned h_offset, v_offset, h_size, v_size, h_dda, v_dda, bpp;
+	unsigned long value;
+
+	bpp = window->bits_per_pixel / 8;
+
+	value = WINDOW_A_SELECT << index;
+	tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+	tegra_dc_writel(dc, window->format, DC_WIN_COLOR_DEPTH);
+	tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP);
+
+	value = V_POSITION(window->dst.y) | H_POSITION(window->dst.x);
+	tegra_dc_writel(dc, value, DC_WIN_POSITION);
+
+	value = V_SIZE(window->dst.h) | H_SIZE(window->dst.w);
+	tegra_dc_writel(dc, value, DC_WIN_SIZE);
+
+	h_offset = window->src.x * bpp;
+	v_offset = window->src.y;
+	h_size = window->src.w * bpp;
+	v_size = window->src.h;
+
+	value = V_PRESCALED_SIZE(v_size) | H_PRESCALED_SIZE(h_size);
+	tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE);
+
+	h_dda = compute_dda_inc(window->src.w, window->dst.w, false, bpp);
+	v_dda = compute_dda_inc(window->src.h, window->dst.h, true, bpp);
+
+	value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda);
+	tegra_dc_writel(dc, value, DC_WIN_DDA_INC);
+
+	h_dda = compute_initial_dda(window->src.x);
+	v_dda = compute_initial_dda(window->src.y);
+
+	tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA);
+	tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA);
+
+	tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE);
+	tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE);
+
+	tegra_dc_writel(dc, window->base, DC_WINBUF_START_ADDR);
+	tegra_dc_writel(dc, window->stride, DC_WIN_LINE_STRIDE);
+	tegra_dc_writel(dc, h_offset, DC_WINBUF_ADDR_H_OFFSET);
+	tegra_dc_writel(dc, v_offset, DC_WINBUF_ADDR_V_OFFSET);
+
+	value = WIN_ENABLE;
+
+	if (bpp < 24)
+		value |= COLOR_EXPAND;
+
+	tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS);
+
+	/*
+	 * Disable blending and assume Window A is the bottom-most window,
+	 * Window C is the top-most window and Window B is in the middle.
+	 */
+	tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_NOKEY);
+	tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_1WIN);
+
+	switch (index) {
+	case 0:
+		tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_X);
+		tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_Y);
+		tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_3WIN_XY);
+		break;
+
+	case 1:
+		tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_X);
+		tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_Y);
+		tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_3WIN_XY);
+		break;
+
+	case 2:
+		tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_X);
+		tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_Y);
+		tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_3WIN_XY);
+		break;
+	}
+
+	value = (WIN_A_ACT_REQ << index) | (WIN_A_UPDATE << index);
+	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+	return 0;
+}
+
+unsigned int tegra_dc_format(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_XRGB8888:
+		return WIN_COLOR_DEPTH_B8G8R8A8;
+
+	case DRM_FORMAT_RGB565:
+		return WIN_COLOR_DEPTH_B5G6R5;
+
+	default:
+		break;
+	}
+
+	WARN(1, "unsupported pixel format %u, using default\n", format);
+	return WIN_COLOR_DEPTH_B8G8R8A8;
+}
+
 static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 			       struct drm_display_mode *mode,
 			       struct drm_display_mode *adjusted,
@@ -160,8 +352,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 {
 	struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb);
 	struct tegra_dc *dc = to_tegra_dc(crtc);
-	unsigned int h_dda, v_dda, bpp;
-	struct tegra_dc_window win;
+	struct tegra_dc_window window;
 	unsigned long div, value;
 	int err;
 
@@ -192,81 +383,23 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 	tegra_dc_writel(dc, value, DC_DISP_DISP_CLOCK_CONTROL);
 
 	/* setup window parameters */
-	memset(&win, 0, sizeof(win));
-	win.x.full = dfixed_const(0);
-	win.y.full = dfixed_const(0);
-	win.w.full = dfixed_const(mode->hdisplay);
-	win.h.full = dfixed_const(mode->vdisplay);
-	win.outx = 0;
-	win.outy = 0;
-	win.outw = mode->hdisplay;
-	win.outh = mode->vdisplay;
-
-	switch (crtc->fb->pixel_format) {
-	case DRM_FORMAT_XRGB8888:
-		win.fmt = WIN_COLOR_DEPTH_B8G8R8A8;
-		break;
-
-	case DRM_FORMAT_RGB565:
-		win.fmt = WIN_COLOR_DEPTH_B5G6R5;
-		break;
-
-	default:
-		win.fmt = WIN_COLOR_DEPTH_B8G8R8A8;
-		WARN_ON(1);
-		break;
-	}
-
-	bpp = crtc->fb->bits_per_pixel / 8;
-	win.stride = crtc->fb->pitches[0];
-
-	/* program window registers */
-	value = WINDOW_A_SELECT;
-	tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER);
-
-	tegra_dc_writel(dc, win.fmt, DC_WIN_COLOR_DEPTH);
-	tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP);
-
-	value = V_POSITION(win.outy) | H_POSITION(win.outx);
-	tegra_dc_writel(dc, value, DC_WIN_POSITION);
-
-	value = V_SIZE(win.outh) | H_SIZE(win.outw);
-	tegra_dc_writel(dc, value, DC_WIN_SIZE);
-
-	value = V_PRESCALED_SIZE(dfixed_trunc(win.h)) |
-		H_PRESCALED_SIZE(dfixed_trunc(win.w) * bpp);
-	tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE);
-
-	h_dda = compute_dda_inc(win.w, win.outw, false, bpp);
-	v_dda = compute_dda_inc(win.h, win.outh, true, bpp);
-
-	value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda);
-	tegra_dc_writel(dc, value, DC_WIN_DDA_INC);
-
-	h_dda = compute_initial_dda(win.x);
-	v_dda = compute_initial_dda(win.y);
-
-	tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA);
-	tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA);
-
-	tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE);
-	tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE);
-
-	tegra_dc_writel(dc, fb->obj->paddr, DC_WINBUF_START_ADDR);
-	tegra_dc_writel(dc, win.stride, DC_WIN_LINE_STRIDE);
-	tegra_dc_writel(dc, dfixed_trunc(win.x) * bpp,
-			DC_WINBUF_ADDR_H_OFFSET);
-	tegra_dc_writel(dc, dfixed_trunc(win.y), DC_WINBUF_ADDR_V_OFFSET);
-
-	value = WIN_ENABLE;
-
-	if (bpp < 24)
-		value |= COLOR_EXPAND;
-
-	tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS);
-
-	tegra_dc_writel(dc, 0xff00, DC_WIN_BLEND_NOKEY);
-	tegra_dc_writel(dc, 0xff00, DC_WIN_BLEND_1WIN);
+	memset(&window, 0, sizeof(window));
+	window.src.x = 0;
+	window.src.y = 0;
+	window.src.w = mode->hdisplay;
+	window.src.h = mode->vdisplay;
+	window.dst.x = 0;
+	window.dst.y = 0;
+	window.dst.w = mode->hdisplay;
+	window.dst.h = mode->vdisplay;
+	window.format = tegra_dc_format(crtc->fb->pixel_format);
+	window.bits_per_pixel = crtc->fb->bits_per_pixel;
+	window.stride = crtc->fb->pitches[0];
+	window.base = fb->obj->paddr;
+
+	err = tegra_dc_setup_window(dc, 0, &window);
+	if (err < 0)
+		dev_err(dc->dev, "failed to enable root plane\n");
 
 	return 0;
 }
@@ -588,7 +721,7 @@ static int tegra_dc_show_regs(struct seq_file *s, void *data)
 	DUMP_REG(DC_WIN_BLEND_1WIN);
 	DUMP_REG(DC_WIN_BLEND_2WIN_X);
 	DUMP_REG(DC_WIN_BLEND_2WIN_Y);
-	DUMP_REG(DC_WIN_BLEND32WIN_XY);
+	DUMP_REG(DC_WIN_BLEND_3WIN_XY);
 	DUMP_REG(DC_WIN_HP_FETCH_CONTROL);
 	DUMP_REG(DC_WINBUF_START_ADDR);
 	DUMP_REG(DC_WINBUF_START_ADDR_NS);
@@ -690,6 +823,10 @@ static int tegra_dc_drm_init(struct host1x_client *client,
 		return err;
 	}
 
+	err = tegra_dc_add_planes(drm, dc);
+	if (err < 0)
+		return err;
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_dc_debugfs_init(dc, drm->primary);
 		if (err < 0)
diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
index 99977b5..ccfc220 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -359,7 +359,7 @@
 #define DC_WIN_BLEND_1WIN			0x710
 #define DC_WIN_BLEND_2WIN_X			0x711
 #define DC_WIN_BLEND_2WIN_Y			0x712
-#define DC_WIN_BLEND32WIN_XY			0x713
+#define DC_WIN_BLEND_3WIN_XY			0x713
 
 #define DC_WIN_HP_FETCH_CONTROL			0x714
 
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 056a6d9..d7ab6bc 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -31,6 +31,11 @@ static inline struct tegra_framebuffer *to_tegra_fb(struct drm_framebuffer *fb)
 	return container_of(fb, struct tegra_framebuffer, base);
 }
 
+static inline unsigned long tegra_framebuffer_base(struct drm_framebuffer *fb)
+{
+	return to_tegra_fb(fb)->obj->paddr;
+}
+
 struct host1x {
 	struct drm_device *drm;
 	struct device *dev;
@@ -121,6 +126,30 @@ static inline unsigned long tegra_dc_readl(struct tegra_dc *dc,
 	return readl(dc->regs + (reg << 2));
 }
 
+struct tegra_dc_window {
+	struct {
+		unsigned int x;
+		unsigned int y;
+		unsigned int w;
+		unsigned int h;
+	} src;
+	struct {
+		unsigned int x;
+		unsigned int y;
+		unsigned int w;
+		unsigned int h;
+	} dst;
+	unsigned int bits_per_pixel;
+	unsigned int format;
+	unsigned int stride;
+	unsigned long base;
+};
+
+/* from dc.c */
+extern unsigned int tegra_dc_format(uint32_t format);
+extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
+				 const struct tegra_dc_window *window);
+
 struct display {
 	unsigned int width;
 	unsigned int height;
-- 
1.8.1

[PATCH 3/5] drm/tegra: Implement .mode_set_base()

[Thierry Reding]

The sequence for replacing the scanout buffer is much shorter than a
full mode change operation so implementing this callback considerably
speeds up cases where only a new framebuffer is to be scanned out.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 157e962..8dd7d8a 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -116,6 +116,25 @@ static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
 	return 0;
 }
 
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
+			     struct tegra_framebuffer *fb)
+{
+	unsigned long value;
+
+	tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+	value = fb->base.offsets[0] + y * fb->base.pitches[0] +
+		x * fb->base.bits_per_pixel / 8;
+
+	tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR);
+
+	value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+	value |= GENERAL_UPDATE | WIN_A_UPDATE;
+	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+	return 0;
+}
+
 static const struct drm_crtc_funcs tegra_crtc_funcs = {
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = drm_crtc_cleanup,
@@ -404,6 +423,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 	return 0;
 }
 
+static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
+				    struct drm_framebuffer *old_fb)
+{
+	struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb);
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+
+	return tegra_dc_set_base(dc, x, y, fb);
+}
+
 static void tegra_crtc_prepare(struct drm_crtc *crtc)
 {
 	struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -483,6 +511,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = {
 	.dpms = tegra_crtc_dpms,
 	.mode_fixup = tegra_crtc_mode_fixup,
 	.mode_set = tegra_crtc_mode_set,
+	.mode_set_base = tegra_crtc_mode_set_base,
 	.prepare = tegra_crtc_prepare,
 	.commit = tegra_crtc_commit,
 	.load_lut = tegra_crtc_load_lut,
-- 
1.8.1

[PATCH 4/5] drm/tegra: Implement VBLANK support

[Thierry Reding]

Implement support for the VBLANK IOCTL. Note that Tegra is somewhat
special in this case because it doesn't use the generic IRQ support
provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one
interrupt handler for each display controller.

While at it, clean up the way that interrupts are enabled to ensure
that the VBLANK interrupt only gets enabled when required.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c  | 55 +++++++++++++++++++++++++++++++--------------
 drivers/gpu/drm/tegra/drm.c | 44 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tegra/drm.h |  3 +++
 3 files changed, 85 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8dd7d8a..3aa7ccc 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -135,6 +135,32 @@ static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y,
 	return 0;
 }
 
+void tegra_dc_enable_vblank(struct tegra_dc *dc)
+{
+	unsigned long value, flags;
+
+	spin_lock_irqsave(&dc->lock, flags);
+
+	value = tegra_dc_readl(dc, DC_CMD_INT_MASK);
+	value |= VBLANK_INT;
+	tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
+
+	spin_unlock_irqrestore(&dc->lock, flags);
+}
+
+void tegra_dc_disable_vblank(struct tegra_dc *dc)
+{
+	unsigned long value, flags;
+
+	spin_lock_irqsave(&dc->lock, flags);
+
+	value = tegra_dc_readl(dc, DC_CMD_INT_MASK);
+	value &= ~VBLANK_INT;
+	tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
+
+	spin_unlock_irqrestore(&dc->lock, flags);
+}
+
 static const struct drm_crtc_funcs tegra_crtc_funcs = {
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = drm_crtc_cleanup,
@@ -375,6 +401,8 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 	unsigned long div, value;
 	int err;
 
+	drm_vblank_pre_modeset(crtc->dev, dc->pipe);
+
 	err = tegra_crtc_setup_clk(crtc, mode, &div);
 	if (err) {
 		dev_err(dc->dev, "failed to setup clock for CRTC: %d\n", err);
@@ -476,31 +504,23 @@ static void tegra_crtc_prepare(struct drm_crtc *crtc)
 	tegra_dc_writel(dc, value, DC_DISP_DISP_MEM_HIGH_PRIORITY_TIMER);
 
 	value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
-	tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
-
-	value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
 	tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE);
+
+	value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT;
+	tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
 }
 
 static void tegra_crtc_commit(struct drm_crtc *crtc)
 {
 	struct tegra_dc *dc = to_tegra_dc(crtc);
-	unsigned long update_mask;
 	unsigned long value;
 
-	update_mask = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
-
-	tegra_dc_writel(dc, update_mask << 8, DC_CMD_STATE_CONTROL);
-
-	value = tegra_dc_readl(dc, DC_CMD_INT_ENABLE);
-	value |= FRAME_END_INT;
-	tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE);
+	value = GENERAL_ACT_REQ | WIN_A_ACT_REQ |
+		GENERAL_UPDATE | WIN_A_UPDATE;
 
-	value = tegra_dc_readl(dc, DC_CMD_INT_MASK);
-	value |= FRAME_END_INT;
-	tegra_dc_writel(dc, value, DC_CMD_INT_MASK);
+	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
 
-	tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL);
+	drm_vblank_post_modeset(crtc->dev, dc->pipe);
 }
 
 static void tegra_crtc_load_lut(struct drm_crtc *crtc)
@@ -517,7 +537,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = {
 	.load_lut = tegra_crtc_load_lut,
 };
 
-static irqreturn_t tegra_drm_irq(int irq, void *data)
+static irqreturn_t tegra_dc_irq(int irq, void *data)
 {
 	struct tegra_dc *dc = data;
 	unsigned long status;
@@ -862,7 +882,7 @@ static int tegra_dc_drm_init(struct host1x_client *client,
 			dev_err(dc->dev, "debugfs setup failed: %d\n", err);
 	}
 
-	err = devm_request_irq(dc->dev, dc->irq, tegra_drm_irq, 0,
+	err = devm_request_irq(dc->dev, dc->irq, tegra_dc_irq, 0,
 			       dev_name(dc->dev), dc);
 	if (err < 0) {
 		dev_err(dc->dev, "failed to request IRQ#%u: %d\n", dc->irq,
@@ -911,6 +931,7 @@ static int tegra_dc_probe(struct platform_device *pdev)
 	if (!dc)
 		return -ENOMEM;
 
+	spin_lock_init(&dc->lock);
 	INIT_LIST_HEAD(&dc->list);
 	dc->dev = &pdev->dev;
 
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 3a503c9..62f8121 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -40,6 +40,10 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
 	if (err < 0)
 		return err;
 
+	err = drm_vblank_init(drm, drm->mode_config.num_crtc);
+	if (err < 0)
+		return err;
+
 	err = tegra_drm_fb_init(drm);
 	if (err < 0)
 		return err;
@@ -89,6 +93,42 @@ static const struct file_operations tegra_drm_fops = {
 	.llseek = noop_llseek,
 };
 
+static struct drm_crtc *tegra_crtc_from_pipe(struct drm_device *drm, int pipe)
+{
+	struct drm_crtc *crtc;
+
+	list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) {
+		struct tegra_dc *dc = to_tegra_dc(crtc);
+
+		if (dc->pipe == pipe)
+			return crtc;
+	}
+
+	return NULL;
+}
+
+static int tegra_drm_enable_vblank(struct drm_device *drm, int pipe)
+{
+	struct drm_crtc *crtc = tegra_crtc_from_pipe(drm, pipe);
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+
+	if (!crtc)
+		return -ENODEV;
+
+	tegra_dc_enable_vblank(dc);
+
+	return 0;
+}
+
+static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe)
+{
+	struct drm_crtc *crtc = tegra_crtc_from_pipe(drm, pipe);
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+
+	if (crtc)
+		tegra_dc_disable_vblank(dc);
+}
+
 struct drm_driver tegra_drm_driver = {
 	.driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM,
 	.load = tegra_drm_load,
@@ -96,6 +136,10 @@ struct drm_driver tegra_drm_driver = {
 	.open = tegra_drm_open,
 	.lastclose = tegra_drm_lastclose,
 
+	.get_vblank_counter = drm_vblank_count,
+	.enable_vblank = tegra_drm_enable_vblank,
+	.disable_vblank = tegra_drm_disable_vblank,
+
 	.gem_free_object = drm_gem_cma_free_object,
 	.gem_vm_ops = &drm_gem_cma_vm_ops,
 	.dumb_create = drm_gem_cma_dumb_create,
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index d7ab6bc..bb5e9d3 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -83,6 +83,7 @@ struct tegra_output;
 
 struct tegra_dc {
 	struct host1x_client client;
+	spinlock_t lock;
 
 	struct host1x *host1x;
 	struct device *dev;
@@ -149,6 +150,8 @@ struct tegra_dc_window {
 extern unsigned int tegra_dc_format(uint32_t format);
 extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
 				 const struct tegra_dc_window *window);
+extern void tegra_dc_enable_vblank(struct tegra_dc *dc);
+extern void tegra_dc_disable_vblank(struct tegra_dc *dc);
 
 struct display {
 	unsigned int width;
-- 
1.8.1

Re: [PATCH 4/5] drm/tegra: Implement VBLANK support

[Jon Mayo]

On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> Implement support for the VBLANK IOCTL. Note that Tegra is somewhat
> special in this case because it doesn't use the generic IRQ support
> provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one
> interrupt handler for each display controller.
>
> While at it, clean up the way that interrupts are enabled to ensure
> that the VBLANK interrupt only gets enabled when required.
>
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---

Sorry Thierry, but is this a useful feature? Are applications really
making use of it? Because it means that that an IRQ will have to
trigger every 16.67ms when it is taken, which means the device can't
sleep. (probably OK because it should go back to idle when the app
lets go of the vblank). But worse, for one-shot panels there is no
continuous vblank. I've been doing weird hacks to run a timer when the
smart panel is idle to trick applications into thinking they have a
vblank pulse. But really I think the whole feature of a vblank pulse
is pretty lame and I wish it would die. Were you going to use it for
something specific, or just adding it for completeness?

- Jon

Re: [PATCH 4/5] drm/tegra: Implement VBLANK support

[Mark Zhang]

On 01/23/2013 02:21 AM, Jon Mayo wrote:
> On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> Implement support for the VBLANK IOCTL. Note that Tegra is somewhat
>> special in this case because it doesn't use the generic IRQ support
>> provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one
>> interrupt handler for each display controller.
>>
>> While at it, clean up the way that interrupts are enabled to ensure
>> that the VBLANK interrupt only gets enabled when required.
>>
>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>> ---
> 
> Sorry Thierry, but is this a useful feature? Are applications really
> making use of it? Because it means that that an IRQ will have to
> trigger every 16.67ms when it is taken, which means the device can't
> sleep. (probably OK because it should go back to idle when the app
> lets go of the vblank). But worse, for one-shot panels there is no
> continuous vblank. I've been doing weird hacks to run a timer when the
> smart panel is idle to trick applications into thinking they have a
> vblank pulse. But really I think the whole feature of a vblank pulse
> is pretty lame and I wish it would die. Were you going to use it for
> something specific, or just adding it for completeness?
> 

I guess people don't use this to really wait vblanks because that can be
done by polling drm fd. Normally they use this ioctl to get the vblank
counts which may be useful for their applications.

Mark
> - Jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/5] drm/tegra: Implement VBLANK support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]

On Tue, Jan 22, 2013 at 10:21:40AM -0800, Jon Mayo wrote:
> On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > Implement support for the VBLANK IOCTL. Note that Tegra is somewhat
> > special in this case because it doesn't use the generic IRQ support
> > provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one
> > interrupt handler for each display controller.
> >
> > While at it, clean up the way that interrupts are enabled to ensure
> > that the VBLANK interrupt only gets enabled when required.
> >
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> > ---
> 
> Sorry Thierry, but is this a useful feature? Are applications really
> making use of it? Because it means that that an IRQ will have to
> trigger every 16.67ms when it is taken, which means the device can't
> sleep. (probably OK because it should go back to idle when the app
> lets go of the vblank). But worse, for one-shot panels there is no
> continuous vblank. I've been doing weird hacks to run a timer when the
> smart panel is idle to trick applications into thinking they have a
> vblank pulse. But really I think the whole feature of a vblank pulse
> is pretty lame and I wish it would die. Were you going to use it for
> something specific, or just adding it for completeness?

This is mainly added for completeness. I know that on Tegra we can do a
lot better by using syncpoints, but applications such as Weston (in
general applications that use the generic DRM API) rely on this to sync
rendering with VBLANK.

I don't think there's another way to achieve the same thing. And as you
already mentioned, this is only enabled when an application actively
uses it, in which case the device won't sleep anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5] drm/tegra: Implement VBLANK support

[Daniel Vetter]

On Wed, Jan 30, 2013 at 9:34 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Tue, Jan 22, 2013 at 10:21:40AM -0800, Jon Mayo wrote:
>> On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding
>> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> > Implement support for the VBLANK IOCTL. Note that Tegra is somewhat
>> > special in this case because it doesn't use the generic IRQ support
>> > provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one
>> > interrupt handler for each display controller.
>> >
>> > While at it, clean up the way that interrupts are enabled to ensure
>> > that the VBLANK interrupt only gets enabled when required.
>> >
>> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>> > ---
>>
>> Sorry Thierry, but is this a useful feature? Are applications really
>> making use of it? Because it means that that an IRQ will have to
>> trigger every 16.67ms when it is taken, which means the device can't
>> sleep. (probably OK because it should go back to idle when the app
>> lets go of the vblank). But worse, for one-shot panels there is no
>> continuous vblank. I've been doing weird hacks to run a timer when the
>> smart panel is idle to trick applications into thinking they have a
>> vblank pulse. But really I think the whole feature of a vblank pulse
>> is pretty lame and I wish it would die. Were you going to use it for
>> something specific, or just adding it for completeness?
>
> This is mainly added for completeness. I know that on Tegra we can do a
> lot better by using syncpoints, but applications such as Weston (in
> general applications that use the generic DRM API) rely on this to sync
> rendering with VBLANK.
>
> I don't think there's another way to achieve the same thing. And as you
> already mentioned, this is only enabled when an application actively
> uses it, in which case the device won't sleep anyway.

I think driving animations with the vblank signal is nice, but we
kinda only need that with the pageflip support. Unfortunately the
current drm code is a bit a mess in that area, so pageflips force you
to enable the vblank interrupt for otherwise the timestamp'ed pageflip
completion events won't work.

Recent intel hw has pageflip timestamp registers, so we don't really
need that. And I guess other hw is similar. So we probably should
clean up and untangle the infrastructure a bit around the drm vblank
support code.

Another issue is that the vblank ioctl itself doesn't deal with
modeset crtc ids, so adding a new interface for that would be good.
Otoh most kms clients don't really use it, but only care about
pageflips.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/5] drm/tegra: Implement VBLANK support

[Mario Kleiner]

On 30.01.13 13:36, Daniel Vetter wrote:
> On Wed, Jan 30, 2013 at 9:34 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> On Tue, Jan 22, 2013 at 10:21:40AM -0800, Jon Mayo wrote:
>>> On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding
>>> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>>>> Implement support for the VBLANK IOCTL. Note that Tegra is somewhat
>>>> special in this case because it doesn't use the generic IRQ support
>>>> provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one
>>>> interrupt handler for each display controller.
>>>>
>>>> While at it, clean up the way that interrupts are enabled to ensure
>>>> that the VBLANK interrupt only gets enabled when required.
>>>>
>>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>>> ---
>>>
>>> Sorry Thierry, but is this a useful feature? Are applications really
>>> making use of it? Because it means that that an IRQ will have to
>>> trigger every 16.67ms when it is taken, which means the device can't
>>> sleep. (probably OK because it should go back to idle when the app
>>> lets go of the vblank). But worse, for one-shot panels there is no
>>> continuous vblank. I've been doing weird hacks to run a timer when the
>>> smart panel is idle to trick applications into thinking they have a
>>> vblank pulse. But really I think the whole feature of a vblank pulse
>>> is pretty lame and I wish it would die. Were you going to use it for
>>> something specific, or just adding it for completeness?
>>
>> This is mainly added for completeness. I know that on Tegra we can do a
>> lot better by using syncpoints, but applications such as Weston (in
>> general applications that use the generic DRM API) rely on this to sync
>> rendering with VBLANK.
>>
>> I don't think there's another way to achieve the same thing. And as you
>> already mentioned, this is only enabled when an application actively
>> uses it, in which case the device won't sleep anyway.
>
> I think driving animations with the vblank signal is nice, but we
> kinda only need that with the pageflip support. Unfortunately the
> current drm code is a bit a mess in that area, so pageflips force you
> to enable the vblank interrupt for otherwise the timestamp'ed pageflip
> completion events won't work.
>

Extensions like SGI_video_sync, OML_sync_control, INTEL_swap_events 
expose vblank counts and related functionality for synchronizing to the 
vblank and some applications really use and need them in addition to the 
pageflip timestamps, so you need a running reliable vblank counter to 
support these. Some scienctific/medical applications need to also 
synchronize things like audio playback or capture, or triggering of 
special research equipment (fMRI/MEG/EEG brain scanners and recordign 
equipment, eye trackers, TMS brain stimulators etc.) to the vblank of a 
completed or future bufferswap, with sometimes better than 1 msec 
precision, sometimes ahead of the event.

That we can't timestamp non-pageflipped swaps precisely/reliably is a 
limitation, not a feature for such uses.

> Recent intel hw has pageflip timestamp registers, so we don't really
> need that. And I guess other hw is similar. So we probably should
> clean up and untangle the infrastructure a bit around the drm vblank
> support code.

At least AMD hw as of a while ago didn't, older intel hw didn't (afaik), 
and NVidia i don't know. You'd also need hw vblank timestamps and 
counters independent of page-flip completion to not regress existing 
important functionality if you want to get rid of the irq based method.

-mario

>
> Another issue is that the vblank ioctl itself doesn't deal with
> modeset crtc ids, so adding a new interface for that would be good.
> Otoh most kms clients don't really use it, but only care about
> pageflips.
> -Daniel
>

Re: [PATCH 4/5] drm/tegra: Implement VBLANK support

[Daniel Vetter]

On Thu, Jan 31, 2013 at 3:06 AM, Mario Kleiner
<mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org> wrote:
> At least AMD hw as of a while ago didn't, older intel hw didn't (afaik), and
> NVidia i don't know. You'd also need hw vblank timestamps and counters
> independent of page-flip completion to not regress existing important
> functionality if you want to get rid of the irq based method.

Intel hw has counters+timestamps for pageflips and vblanks, so you get
the cake and eat it, too. I'm also aware of your stringent
requirements wrt timestamps, so the recently pimped kms_flip test we
have for drm/i915.ko checks vblank timestamps, pageflip timestamps and
whether those don't get out of sync somehow in a rather anal way.
Results thus far is that the current vblank stuff is (at least for
intel) pretty horribly broken - QA reported random and sometimes
not-so-random failures across all generations.

So we still have plenty of fixing to do until we can get around to
implement pageflip timestamping without the irq vblank running all the
time.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

All the necessary support bits like .mode_set_base() and VBLANK are now
available, so page-flipping case easily be implemented on top.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c  | 72 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tegra/drm.c |  9 ++++++
 drivers/gpu/drm/tegra/drm.h |  5 ++++
 3 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 3aa7ccc..ce4e14a 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -161,7 +161,78 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc)
 	spin_unlock_irqrestore(&dc->lock, flags);
 }
 
+static void tegra_dc_finish_page_flip(struct tegra_dc *dc)
+{
+	struct drm_pending_vblank_event *event;
+	struct drm_device *drm = dc->base.dev;
+	unsigned long flags;
+	struct timeval now;
+
+	spin_lock_irqsave(&drm->event_lock, flags);
+	event = dc->event;
+	dc->event = NULL;
+	spin_unlock_irqrestore(&drm->event_lock, flags);
+
+	if (!event)
+		return;
+
+	event->event.sequence = drm_vblank_count_and_time(drm, dc->pipe, &now);
+	event->event.tv_sec = now.tv_sec;
+	event->event.tv_usec = now.tv_usec;
+
+	spin_lock_irqsave(&drm->event_lock, flags);
+	list_add_tail(&event->base.link, &event->base.file_priv->event_list);
+	wake_up_interruptible(&event->base.file_priv->event_wait);
+	spin_unlock_irqrestore(&drm->event_lock, flags);
+
+	drm_vblank_put(drm, dc->pipe);
+}
+
+void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file)
+{
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+	struct drm_device *drm = crtc->dev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drm->event_lock, flags);
+
+	if (dc->event && dc->event->base.file_priv == file) {
+		dc->event->base.destroy(&dc->event->base);
+		drm_vblank_put(drm, dc->pipe);
+		dc->event = NULL;
+	}
+
+	spin_unlock_irqrestore(&drm->event_lock, flags);
+}
+
+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
+			      struct drm_pending_vblank_event *event)
+{
+	struct tegra_framebuffer *newfb = to_tegra_fb(fb);
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+	struct drm_device *drm = crtc->dev;
+	unsigned long flags;
+
+	if (dc->event)
+		return -EBUSY;
+
+	tegra_dc_set_base(dc, 0, 0, newfb);
+
+	if (event) {
+		event->pipe = dc->pipe;
+
+		spin_lock_irqsave(&drm->event_lock, flags);
+		dc->event = event;
+		spin_unlock_irqrestore(&drm->event_lock, flags);
+
+		drm_vblank_get(drm, dc->pipe);
+	}
+
+	return 0;
+}
+
 static const struct drm_crtc_funcs tegra_crtc_funcs = {
+	.page_flip = tegra_dc_page_flip,
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = drm_crtc_cleanup,
 };
@@ -556,6 +627,7 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
 		dev_dbg(dc->dev, "%s(): vertical blank\n", __func__);
 		*/
 		drm_handle_vblank(dc->base.dev, dc->pipe);
+		tegra_dc_finish_page_flip(dc);
 	}
 
 	if (status & (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) {
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 62f8121..7bab784 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -129,11 +129,20 @@ static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe)
 		tegra_dc_disable_vblank(dc);
 }
 
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
+{
+	struct drm_crtc *crtc;
+
+	list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
+		tegra_dc_cancel_page_flip(crtc, file);
+}
+
 struct drm_driver tegra_drm_driver = {
 	.driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM,
 	.load = tegra_drm_load,
 	.unload = tegra_drm_unload,
 	.open = tegra_drm_open,
+	.preclose = tegra_drm_preclose,
 	.lastclose = tegra_drm_lastclose,
 
 	.get_vblank_counter = drm_vblank_count,
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index bb5e9d3..3aa21b1 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -103,6 +103,9 @@ struct tegra_dc {
 	struct drm_info_list *debugfs_files;
 	struct drm_minor *minor;
 	struct dentry *debugfs;
+
+	/* page-flip handling */
+	struct drm_pending_vblank_event *event;
 };
 
 static inline struct tegra_dc *host1x_client_to_dc(struct host1x_client *client)
@@ -152,6 +155,8 @@ extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
 				 const struct tegra_dc_window *window);
 extern void tegra_dc_enable_vblank(struct tegra_dc *dc);
 extern void tegra_dc_disable_vblank(struct tegra_dc *dc);
+extern void tegra_dc_cancel_page_flip(struct drm_crtc *crtc,
+				      struct drm_file *file);
 
 struct display {
 	unsigned int width;
-- 
1.8.1

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Daniel Vetter]

On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
> +{
> +       struct drm_crtc *crtc;
> +
> +       list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
> +               tegra_dc_cancel_page_flip(crtc, file);
> +}
> +

Why that? If userspace dies while a flip is outstanding, it's imo ok
to execute it - otherwise there might be an accounting mismatch if the
hw still scans out the old fb, but drm believes the new one is used.
Or do I miss something?

The reason I've skimmed through the patches is to check for
implications with my modeset locking rework. Review on that would be
highly appreciated ...


Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
> On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
> > +{
> > +       struct drm_crtc *crtc;
> > +
> > +       list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
> > +               tegra_dc_cancel_page_flip(crtc, file);
> > +}
> > +
> 
> Why that? If userspace dies while a flip is outstanding, it's imo ok
> to execute it - otherwise there might be an accounting mismatch if the
> hw still scans out the old fb, but drm believes the new one is used.
> Or do I miss something?

I looked at the shmobile driver for inspiration and they do this as
well. Doing so seemed reasonable since there'd be no file to deliver the
event to.

> The reason I've skimmed through the patches is to check for
> implications with my modeset locking rework. Review on that would be
> highly appreciated ...

I'm not sure how suited I am for review given my limited experience, but
I'll see if I can make some time to take a look.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Daniel Vetter]

On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
>> On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding
>> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
>> > +{
>> > +       struct drm_crtc *crtc;
>> > +
>> > +       list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
>> > +               tegra_dc_cancel_page_flip(crtc, file);
>> > +}
>> > +
>>
>> Why that? If userspace dies while a flip is outstanding, it's imo ok
>> to execute it - otherwise there might be an accounting mismatch if the
>> hw still scans out the old fb, but drm believes the new one is used.
>> Or do I miss something?
>
> I looked at the shmobile driver for inspiration and they do this as
> well. Doing so seemed reasonable since there'd be no file to deliver the
> event to.

Hm, is the code in drm_events_release not good enough? And if it's
buggy, we need to fix it. Also adding Laurent to figure out why he
added that code in shmob ...

>> The reason I've skimmed through the patches is to check for
>> implications with my modeset locking rework. Review on that would be
>> highly appreciated ...
>
> I'm not sure how suited I am for review given my limited experience, but
> I'll see if I can make some time to take a look.

The commit message should nicely explain why I've picked the design
and the various implications for drivers. So just checking whether
anything collides with your upcoming stuff would be good ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

On Wed, Jan 16, 2013 at 10:43:15AM +0100, Daniel Vetter wrote:
> On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
> >> On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding
> >> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> >> > +static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
> >> > +{
> >> > +       struct drm_crtc *crtc;
> >> > +
> >> > +       list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
> >> > +               tegra_dc_cancel_page_flip(crtc, file);
> >> > +}
> >> > +
> >>
> >> Why that? If userspace dies while a flip is outstanding, it's imo ok
> >> to execute it - otherwise there might be an accounting mismatch if the
> >> hw still scans out the old fb, but drm believes the new one is used.
> >> Or do I miss something?
> >
> > I looked at the shmobile driver for inspiration and they do this as
> > well. Doing so seemed reasonable since there'd be no file to deliver the
> > event to.
> 
> Hm, is the code in drm_events_release not good enough? And if it's
> buggy, we need to fix it. Also adding Laurent to figure out why he
> added that code in shmob ...

drm_events_release() should be enough to clean up the events, but I
suspect the reason why Laurent put that code in was that the drm_crtc
private data still has a reference to the event and needs to clear it.
Otherwise the next page flip won't be scheduled because .page_flip()
would return -EBUSY.

However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
could both be simplified a lot and just set their event to NULL. Then
again, maybe keeping a separate reference isn't all that useful. Maybe
the better thing to do here is iterate over the list of pending VBLANK
events in *_finish_page_flip() and process each of them? That would
allow more than one user-space process to queue page flips.

> >> The reason I've skimmed through the patches is to check for
> >> implications with my modeset locking rework. Review on that would be
> >> highly appreciated ...
> >
> > I'm not sure how suited I am for review given my limited experience, but
> > I'll see if I can make some time to take a look.
> 
> The commit message should nicely explain why I've picked the design
> and the various implications for drivers. So just checking whether
> anything collides with your upcoming stuff would be good ...

Okay, I'll take a closer look.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Daniel Vetter]

On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> drm_events_release() should be enough to clean up the events, but I
> suspect the reason why Laurent put that code in was that the drm_crtc
> private data still has a reference to the event and needs to clear it.
> Otherwise the next page flip won't be scheduled because .page_flip()
> would return -EBUSY.

Hm, indeed we seem to have a nice bug in most drivers there :(

> However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
> could both be simplified a lot and just set their event to NULL. Then
> again, maybe keeping a separate reference isn't all that useful. Maybe
> the better thing to do here is iterate over the list of pending VBLANK
> events in *_finish_page_flip() and process each of them? That would
> allow more than one user-space process to queue page flips.

I think we need a slightly more generally useful solution, since most
drivers are currently broken. I've read a bit through the code, but
short of refcounting drm events and adding event->file_priv checks at
relevent places I don't see a sane solution ... And even that one is
rather invasive. Do you have an idea? Imo doing the cleanup in each
driver will be rather error-prone, and since usually kms clients wait
for flips to complete, also guaranteed to be little tested.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Rob Clark]

On Wed, Jan 16, 2013 at 6:36 AM, Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org> wrote:
> On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> drm_events_release() should be enough to clean up the events, but I
>> suspect the reason why Laurent put that code in was that the drm_crtc
>> private data still has a reference to the event and needs to clear it.
>> Otherwise the next page flip won't be scheduled because .page_flip()
>> would return -EBUSY.
>
> Hm, indeed we seem to have a nice bug in most drivers there :(
>
>> However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
>> could both be simplified a lot and just set their event to NULL. Then
>> again, maybe keeping a separate reference isn't all that useful. Maybe
>> the better thing to do here is iterate over the list of pending VBLANK
>> events in *_finish_page_flip() and process each of them? That would
>> allow more than one user-space process to queue page flips.
>
> I think we need a slightly more generally useful solution, since most
> drivers are currently broken. I've read a bit through the code, but
> short of refcounting drm events and adding event->file_priv checks at
> relevent places I don't see a sane solution ... And even that one is
> rather invasive. Do you have an idea? Imo doing the cleanup in each
> driver will be rather error-prone, and since usually kms clients wait
> for flips to complete, also guaranteed to be little tested.

I suppose we could move the *pending_vblank_event to 'struct
drm_crtc'.. I think probably all/most drivers need the same thing
anyway.  If a driver needs to do something special, it could just
never set crtc->pending_vblank_event and instead do it's own cleanup.

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
> On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > drm_events_release() should be enough to clean up the events, but I
> > suspect the reason why Laurent put that code in was that the drm_crtc
> > private data still has a reference to the event and needs to clear it.
> > Otherwise the next page flip won't be scheduled because .page_flip()
> > would return -EBUSY.
> 
> Hm, indeed we seem to have a nice bug in most drivers there :(
> 
> > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
> > could both be simplified a lot and just set their event to NULL. Then
> > again, maybe keeping a separate reference isn't all that useful. Maybe
> > the better thing to do here is iterate over the list of pending VBLANK
> > events in *_finish_page_flip() and process each of them? That would
> > allow more than one user-space process to queue page flips.
> 
> I think we need a slightly more generally useful solution, since most
> drivers are currently broken. I've read a bit through the code, but
> short of refcounting drm events and adding event->file_priv checks at
> relevent places I don't see a sane solution ... And even that one is
> rather invasive. Do you have an idea? Imo doing the cleanup in each
> driver will be rather error-prone, and since usually kms clients wait
> for flips to complete, also guaranteed to be little tested.

While this probably doesn't improve the situation much, maybe adding
more extensive tests to libdrm or so would help. I wrote a couple of
small programs to test vblank and plane support.

The vblank one basically generates two framebuffers with different
patterns and uses page-flipping to alternate between them. The plane
test does something similar, sets up a plane and associates a buffer
with it. It includes scaling the plane to test that functionality as
well.

Perhaps these tests could even be added to the existing libdrm tests,
but maybe having separate binaries wouldn't hurt.

Back to the original topic: should it not work to queue a page flip and
immediately exit(0) after that to test the cleanup code? I suppose if
that can be tested on all drivers we should have pretty good coverage.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

Hi Thierry,

On Wednesday 16 January 2013 19:56:06 Thierry Reding wrote:
> On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding wrote:
> > > drm_events_release() should be enough to clean up the events, but I
> > > suspect the reason why Laurent put that code in was that the drm_crtc
> > > private data still has a reference to the event and needs to clear it.
> > > Otherwise the next page flip won't be scheduled because .page_flip()
> > > would return -EBUSY.
> > 
> > Hm, indeed we seem to have a nice bug in most drivers there :(
> > 
> > > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
> > > could both be simplified a lot and just set their event to NULL. Then
> > > again, maybe keeping a separate reference isn't all that useful. Maybe
> > > the better thing to do here is iterate over the list of pending VBLANK
> > > events in *_finish_page_flip() and process each of them? That would
> > > allow more than one user-space process to queue page flips.
> > 
> > I think we need a slightly more generally useful solution, since most
> > drivers are currently broken. I've read a bit through the code, but
> > short of refcounting drm events and adding event->file_priv checks at
> > relevent places I don't see a sane solution ... And even that one is
> > rather invasive. Do you have an idea? Imo doing the cleanup in each
> > driver will be rather error-prone, and since usually kms clients wait
> > for flips to complete, also guaranteed to be little tested.
> 
> While this probably doesn't improve the situation much, maybe adding
> more extensive tests to libdrm or so would help. I wrote a couple of
> small programs to test vblank and plane support.
> 
> The vblank one basically generates two framebuffers with different
> patterns and uses page-flipping to alternate between them. The plane
> test does something similar, sets up a plane and associates a buffer
> with it. It includes scaling the plane to test that functionality as
> well.
> 
> Perhaps these tests could even be added to the existing libdrm tests,
> but maybe having separate binaries wouldn't hurt.

Further cleanup of the modetest application is somewhere on my to-do list (but 
probably so low that I'll never get to it unless there's a real incentive 
;-)). It's a good candidate for more page flip testing (there's basic page 
flip support there already).

> Back to the original topic: should it not work to queue a page flip and
> immediately exit(0) after that to test the cleanup code? I suppose if
> that can be tested on all drivers we should have pretty good coverage.

Maybe we need some kind of compliance test suite tool ?

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Daniel Vetter]

On Sat, Feb 2, 2013 at 12:05 AM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
>> Back to the original topic: should it not work to queue a page flip and
>> immediately exit(0) after that to test the cleanup code? I suppose if
>> that can be tested on all drivers we should have pretty good coverage.
>
> Maybe we need some kind of compliance test suite tool ?

kms_flip from our intel-specific testsuite is probably the most
paranoid thing for testing page flips out there:

http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/kms_flip.c

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
> On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > drm_events_release() should be enough to clean up the events, but I
> > suspect the reason why Laurent put that code in was that the drm_crtc
> > private data still has a reference to the event and needs to clear it.
> > Otherwise the next page flip won't be scheduled because .page_flip()
> > would return -EBUSY.
> 
> Hm, indeed we seem to have a nice bug in most drivers there :(

I think I may just recently have run into this bug on Intel hardware.
Although perhaps I just used this wrongly.

Just for the fun of it I wanted to implement Conway's Game of Life on
top of DRM/KMS. So I use two dumb buffer objects to alternately render
to. Then I wanted to use page-flipping to synchronize with VBLANK.

So the sequence is basically:

	while (!done) {
		grid_tick(grid);
		grid_draw(grid, screen);
		screen_flip(screen);
		grid_swap(grid);
	}

Where screen_flip() chooses the framebuffer and passes it to
drmModePageFlip() like so:

	int fb = screen->fb[screen->current];

	drmModePageFlip(screen->fd, screen->crtc, fb,
			DRM_MODE_PAGE_FLIP_EVENT, screen);

This runs for about 3 seconds and then hangs, so the display is no
longer updated. I've also verified that the same happens on Radeon.
But maybe I am mistaken and this isn't the proper programming sequence?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Ville Syrjälä]

On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote:
> On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
> > <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > > drm_events_release() should be enough to clean up the events, but I
> > > suspect the reason why Laurent put that code in was that the drm_crtc
> > > private data still has a reference to the event and needs to clear it.
> > > Otherwise the next page flip won't be scheduled because .page_flip()
> > > would return -EBUSY.
> > 
> > Hm, indeed we seem to have a nice bug in most drivers there :(
> 
> I think I may just recently have run into this bug on Intel hardware.
> Although perhaps I just used this wrongly.
> 
> Just for the fun of it I wanted to implement Conway's Game of Life on
> top of DRM/KMS. So I use two dumb buffer objects to alternately render
> to. Then I wanted to use page-flipping to synchronize with VBLANK.
> 
> So the sequence is basically:
> 
> 	while (!done) {
> 		grid_tick(grid);
> 		grid_draw(grid, screen);
> 		screen_flip(screen);
> 		grid_swap(grid);
> 	}
> 
> Where screen_flip() chooses the framebuffer and passes it to
> drmModePageFlip() like so:
> 
> 	int fb = screen->fb[screen->current];
> 
> 	drmModePageFlip(screen->fd, screen->crtc, fb,
> 			DRM_MODE_PAGE_FLIP_EVENT, screen);
> 
> This runs for about 3 seconds and then hangs, so the display is no
> longer updated. I've also verified that the same happens on Radeon.
> But maybe I am mistaken and this isn't the proper programming sequence?

You asked for page flip events. Do you actually handle them in your code?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Wed, Jan 30, 2013 at 11:42:40AM +0200, Ville Syrjälä wrote:
> On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote:
> > On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
> > > <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > > > drm_events_release() should be enough to clean up the events, but I
> > > > suspect the reason why Laurent put that code in was that the drm_crtc
> > > > private data still has a reference to the event and needs to clear it.
> > > > Otherwise the next page flip won't be scheduled because .page_flip()
> > > > would return -EBUSY.
> > > 
> > > Hm, indeed we seem to have a nice bug in most drivers there :(
> > 
> > I think I may just recently have run into this bug on Intel hardware.
> > Although perhaps I just used this wrongly.
> > 
> > Just for the fun of it I wanted to implement Conway's Game of Life on
> > top of DRM/KMS. So I use two dumb buffer objects to alternately render
> > to. Then I wanted to use page-flipping to synchronize with VBLANK.
> > 
> > So the sequence is basically:
> > 
> > 	while (!done) {
> > 		grid_tick(grid);
> > 		grid_draw(grid, screen);
> > 		screen_flip(screen);
> > 		grid_swap(grid);
> > 	}
> > 
> > Where screen_flip() chooses the framebuffer and passes it to
> > drmModePageFlip() like so:
> > 
> > 	int fb = screen->fb[screen->current];
> > 
> > 	drmModePageFlip(screen->fd, screen->crtc, fb,
> > 			DRM_MODE_PAGE_FLIP_EVENT, screen);
> > 
> > This runs for about 3 seconds and then hangs, so the display is no
> > longer updated. I've also verified that the same happens on Radeon.
> > But maybe I am mistaken and this isn't the proper programming sequence?
> 
> You asked for page flip events. Do you actually handle them in your code?

Duh. No I wasn't =) I suppose some queue must be running full if the
event isn't handled by calling drmHandleEvent(). Okay, this now works
properly with page-flipping.

Thanks.
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On Wed, Jan 30, 2013 at 12:14:36PM +0100, Thierry Reding wrote:
> On Wed, Jan 30, 2013 at 11:42:40AM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote:
> > > On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
> > > > On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding
> > > > <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > > > > drm_events_release() should be enough to clean up the events, but I
> > > > > suspect the reason why Laurent put that code in was that the drm_crtc
> > > > > private data still has a reference to the event and needs to clear it.
> > > > > Otherwise the next page flip won't be scheduled because .page_flip()
> > > > > would return -EBUSY.
> > > > 
> > > > Hm, indeed we seem to have a nice bug in most drivers there :(
> > > 
> > > I think I may just recently have run into this bug on Intel hardware.
> > > Although perhaps I just used this wrongly.
> > > 
> > > Just for the fun of it I wanted to implement Conway's Game of Life on
> > > top of DRM/KMS. So I use two dumb buffer objects to alternately render
> > > to. Then I wanted to use page-flipping to synchronize with VBLANK.
> > > 
> > > So the sequence is basically:
> > > 
> > > 	while (!done) {
> > > 		grid_tick(grid);
> > > 		grid_draw(grid, screen);
> > > 		screen_flip(screen);
> > > 		grid_swap(grid);
> > > 	}
> > > 
> > > Where screen_flip() chooses the framebuffer and passes it to
> > > drmModePageFlip() like so:
> > > 
> > > 	int fb = screen->fb[screen->current];
> > > 
> > > 	drmModePageFlip(screen->fd, screen->crtc, fb,
> > > 			DRM_MODE_PAGE_FLIP_EVENT, screen);
> > > 
> > > This runs for about 3 seconds and then hangs, so the display is no
> > > longer updated. I've also verified that the same happens on Radeon.
> > > But maybe I am mistaken and this isn't the proper programming sequence?
> > 
> > You asked for page flip events. Do you actually handle them in your code?
> 
> Duh. No I wasn't =) I suppose some queue must be running full if the
> event isn't handled by calling drmHandleEvent(). Okay, this now works
> properly with page-flipping.

Just in case anybody's interested, the code is here:

	https://gitorious.org/thierryreding/kmslife

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] drm/tegra: Implement page-flipping support

[Laurent Pinchart]

On Wednesday 16 January 2013 13:36:17 Daniel Vetter wrote:
> On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding wrote:
> > drm_events_release() should be enough to clean up the events, but I
> > suspect the reason why Laurent put that code in was that the drm_crtc
> > private data still has a reference to the event and needs to clear it.
> > Otherwise the next page flip won't be scheduled because .page_flip()
> > would return -EBUSY.
> 
> Hm, indeed we seem to have a nice bug in most drivers there :(

That's not the only reason.

drm_events_release() handles vblank events added to the vblank_event_list. 
That list only contains vblank events added by drm_queue_vblank_event(), only 
called by drm_wait_vblank(). Page flip events never get there, so they need to 
be cleaned up manually.

I wrote in the DRM documentation, in the page flipping section,

"FIXME: Could drivers that don't need to wait for rendering to complete just 
add the event to dev->vblank_event_list and let the DRM core handle 
everything, as for "normal" vertical blanking events?"

We should investigate that.

> > However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip()
> > could both be simplified a lot and just set their event to NULL. Then
> > again, maybe keeping a separate reference isn't all that useful. Maybe
> > the better thing to do here is iterate over the list of pending VBLANK
> > events in *_finish_page_flip() and process each of them? That would
> > allow more than one user-space process to queue page flips.
> 
> I think we need a slightly more generally useful solution, since most
> drivers are currently broken. I've read a bit through the code, but
> short of refcounting drm events and adding event->file_priv checks at
> relevent places I don't see a sane solution ... And even that one is
> rather invasive. Do you have an idea? Imo doing the cleanup in each
> driver will be rather error-prone, and since usually kms clients wait
> for flips to complete, also guaranteed to be little tested.

-- 
Regards,

Laurent Pinchart