[PATCH v4 0/9] drm/tegra: Miscellaneous enhancements

[PATCH v4 0/9] drm/tegra: Miscellaneous enhancements

[Thierry Reding]

This patch series introduces a number of useful features for the Tegra
DRM driver. Patch 1 is a documentation update and adds a warning to the
page-flip IOCTL to catch buggy drivers that return success but failed to
update the crtc->fb field, which would cause the reference counting to
become unbalanced.

Patch 2 removes usage of the tegra_framebuffer structure which was used
before the driver moved to the CMA FB helpers. It is wrong to use it but
it happens to work because the drm_fb_cma and tegra_framebuffers were
sufficiently similar.

Patch 3 adds support for the additional two planes available on Tegra
hardware, while patch 4 implement .mode_set_base() to allow for some
nice speed up when changing the framebuffer without actually changing
the resolution. Patch 5 adds VBLANK support and patch 6 builds on the
previous two to provide the page-flipping IOCTL. Patch 7 splits the
DC_CMD_STATE_CONTROL register writes into two consecutive writes as
required by the TRM.

Finally patch 8 fixes color expansions for pixel formats with less than
24 bits per pixel and patch 9 adds a new file, named "framebuffers" to
debugfs which can be used to dump a list of framebuffers attached to the
DRM device. This is most useful to inspect the reference count but could
also be helpful in diagnosing out-of-memory conditions and such.

This series is based on Dave's drm-next branch.

Thierry

Thierry Reding (9):
  drm: Add consistency check for page-flipping
  drm/tegra: Remove bogus tegra_framebuffer structure
  drm/tegra: Add plane support
  drm/tegra: Implement .mode_set_base()
  drm/tegra: Implement VBLANK support
  drm/tegra: Implement page-flipping support
  drm/tegra: Split DC_CMD_STATE_CONTROL register write
  drm/tegra: Fix color expansion
  drm/tegra: Add list of framebuffers to debugfs

 Documentation/DocBook/drm.tmpl |   6 +
 drivers/gpu/drm/drm_crtc.c     |   7 +
 drivers/gpu/drm/tegra/dc.c     | 585 +++++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/tegra/dc.h     |  14 +-
 drivers/gpu/drm/tegra/drm.c    | 103 ++++++++
 drivers/gpu/drm/tegra/drm.h    |  43 ++-
 6 files changed, 635 insertions(+), 123 deletions(-)

-- 
1.8.1.2

[PATCH v4 1/9] drm: Add consistency check for page-flipping

[Thierry Reding]

Driver implementations of the drm_crtc's .page_flip() function are
required to update the crtc->fb field on success to reflect that the new
framebuffer is now in use. This is important to keep reference counting
on the framebuffers balanced.

While at it, document this requirement to keep others from falling into
the same trap.

Suggested-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
---
 Documentation/DocBook/drm.tmpl | 6 ++++++
 drivers/gpu/drm/drm_crtc.c     | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 51e1904..a6428dd 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -1161,6 +1161,12 @@ int max_width, max_height;</synopsis>
             any new rendering to the frame buffer until the page flip completes.
           </para>
           <para>
+            If a page flip can be successfully scheduled the driver must set the
+            <code>drm_crtc-&lt;fb</code> field to the new framebuffer pointed to
+            by <code>fb</code>. This is important so that the reference counting
+            on framebuffers stays balanced.
+          </para>
+          <para>
             If a page flip is already pending, the
             <methodname>page_flip</methodname> operation must return
             -<errorname>EBUSY</errorname>.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 781aef5..3bdf2a6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3792,6 +3792,13 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		/* Keep the old fb, don't unref it. */
 		old_fb = NULL;
 	} else {
+		/*
+		 * Warn if the driver hasn't properly updated the crtc->fb
+		 * field to reflect that the new framebuffer is now used.
+		 * Failing to do so will screw with the reference counting
+		 * on framebuffers.
+		 */
+		WARN_ON(crtc->fb != fb);
 		/* Unref only the old framebuffer. */
 		fb = NULL;
 	}
-- 
1.8.1.2

[PATCH v4 2/9] drm/tegra: Remove bogus tegra_framebuffer structure

[Thierry Reding]

Tegra uses the CMA FB helpers so framebuffers passed to the driver need
to use the corresponding functions to access the underlying GEM objects.

This used to work because struct tegra_framebuffer was sufficiently
similar to struct drm_fb_cma but that isn't guaranteed to stay that way.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c  |  2 +-
 drivers/gpu/drm/tegra/drm.h | 11 -----------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 656b2e3..d35ff8b 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -158,7 +158,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 			       struct drm_display_mode *adjusted,
 			       int x, int y, struct drm_framebuffer *old_fb)
 {
-	struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb);
+	struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
 	struct tegra_dc *dc = to_tegra_dc(crtc);
 	unsigned int h_dda, v_dda, bpp;
 	struct tegra_dc_window win;
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 741b5dc..3c61aab 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -18,16 +18,6 @@
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fixed.h>
 
-struct tegra_framebuffer {
-	struct drm_framebuffer base;
-	struct drm_gem_cma_object *obj;
-};
-
-static inline struct tegra_framebuffer *to_tegra_fb(struct drm_framebuffer *fb)
-{
-	return container_of(fb, struct tegra_framebuffer, base);
-}
-
 struct host1x {
 	struct drm_device *drm;
 	struct device *dev;
@@ -44,7 +34,6 @@ struct host1x {
 	struct list_head clients;
 
 	struct drm_fbdev_cma *fbdev;
-	struct tegra_framebuffer fb;
 };
 
 struct host1x_client;
-- 
1.8.1.2

[PATCH v4 3/9] 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. Currently 32-bit
XRGB as well as UYVY, YUV420 and YUV422 pixel formats are advertised.
Other formats should be easy to add but these are the most common ones
and should cover the majority of use-cases.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes
- adjust plane index to match window index
- drop support for YUV422 format for now
- disable active planes when CRTC is disabled
- disable plane on module unload

Changes in v4:
- add proper YUV420, YUV422 and UYVY support

 drivers/gpu/drm/tegra/dc.c  | 431 ++++++++++++++++++++++++++++++++++----------
 drivers/gpu/drm/tegra/dc.h  |  12 +-
 drivers/gpu/drm/tegra/drm.h |  24 +++
 3 files changed, 372 insertions(+), 95 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index d35ff8b..52bf63f 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -18,26 +18,143 @@
 #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)
+{
+	struct tegra_plane *p = to_tegra_plane(plane);
+	struct tegra_dc *dc = to_tegra_dc(crtc);
+	struct tegra_dc_window window;
+	unsigned int i;
+
+	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;
+
+	for (i = 0; i < drm_format_num_planes(fb->pixel_format); i++) {
+		struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, i);
+
+		window.base[i] = gem->paddr + fb->offsets[i];
+
+		/*
+		 * Tegra doesn't support different strides for U and V planes
+		 * so we display a warning if the user tries to display a
+		 * framebuffer with such a configuration.
+		 */
+		if (i >= 2) {
+			if (fb->pitches[i] != window.stride[1])
+				DRM_ERROR("unsupported UV-plane configuration\n");
+		} else {
+			window.stride[i] = fb->pitches[i];
+		}
+	}
+
+	return tegra_dc_setup_window(dc, p->index, &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 long value;
+
+	value = WINDOW_A_SELECT << p->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);
+
+	tegra_dc_writel(dc, WIN_A_UPDATE << p->index, DC_CMD_STATE_CONTROL);
+	tegra_dc_writel(dc, WIN_A_ACT_REQ << p->index, DC_CMD_STATE_CONTROL);
+
+	return 0;
+}
+
+static void tegra_plane_destroy(struct drm_plane *plane)
+{
+	tegra_plane_disable(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_UYVY,
+	DRM_FORMAT_YUV420,
+	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 = 1 + 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,
 };
 
-static void tegra_crtc_dpms(struct drm_crtc *crtc, int mode)
+static void tegra_crtc_disable(struct drm_crtc *crtc)
 {
+	struct drm_device *drm = crtc->dev;
+	struct drm_plane *plane;
+
+	list_for_each_entry(plane, &drm->mode_config.plane_list, head) {
+		if (plane->crtc == crtc) {
+			tegra_plane_disable(plane);
+			plane->crtc = NULL;
+
+			if (plane->fb) {
+				drm_framebuffer_unreference(plane->fb);
+				plane->fb = NULL;
+			}
+		}
+	}
 }
 
 static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -47,10 +164,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 +198,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 +272,185 @@ static int tegra_crtc_setup_clk(struct drm_crtc *crtc,
 	return 0;
 }
 
+static bool tegra_dc_format_is_yuv(unsigned int format, bool *planar)
+{
+	switch (format) {
+	case WIN_COLOR_DEPTH_YCbCr422:
+	case WIN_COLOR_DEPTH_YUV422:
+		if (planar)
+			*planar = false;
+
+		return true;
+
+	case WIN_COLOR_DEPTH_YCbCr420P:
+	case WIN_COLOR_DEPTH_YUV420P:
+	case WIN_COLOR_DEPTH_YCbCr422P:
+	case WIN_COLOR_DEPTH_YUV422P:
+	case WIN_COLOR_DEPTH_YCbCr422R:
+	case WIN_COLOR_DEPTH_YUV422R:
+	case WIN_COLOR_DEPTH_YCbCr422RA:
+	case WIN_COLOR_DEPTH_YUV422RA:
+		if (planar)
+			*planar = true;
+
+		return true;
+	}
+
+	return false;
+}
+
+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;
+	bool yuv, planar;
+
+	/*
+	 * For YUV planar modes, the number of bytes per pixel takes into
+	 * account only the luma component and therefore is 1.
+	 */
+	yuv = tegra_dc_format_is_yuv(window->format, &planar);
+	if (!yuv)
+		bpp = window->bits_per_pixel / 8;
+	else
+		bpp = planar ? 1 : 2;
+
+	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);
+
+	/*
+	 * For DDA computations the number of bytes per pixel for YUV planar
+	 * modes needs to take into account all Y, U and V components.
+	 */
+	if (yuv && planar)
+		bpp = 2;
+
+	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[0], DC_WINBUF_START_ADDR);
+
+	if (yuv && planar) {
+		tegra_dc_writel(dc, window->base[1], DC_WINBUF_START_ADDR_U);
+		tegra_dc_writel(dc, window->base[2], DC_WINBUF_START_ADDR_V);
+		value = window->stride[1] << 16 | window->stride[0];
+		tegra_dc_writel(dc, value, DC_WIN_LINE_STRIDE);
+	} else {
+		tegra_dc_writel(dc, window->stride[0], 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 (yuv) {
+		/* setup default colorspace conversion coefficients */
+		tegra_dc_writel(dc, 0x00f0, DC_WIN_CSC_YOF);
+		tegra_dc_writel(dc, 0x012a, DC_WIN_CSC_KYRGB);
+		tegra_dc_writel(dc, 0x0000, DC_WIN_CSC_KUR);
+		tegra_dc_writel(dc, 0x0198, DC_WIN_CSC_KVR);
+		tegra_dc_writel(dc, 0x039b, DC_WIN_CSC_KUG);
+		tegra_dc_writel(dc, 0x032f, DC_WIN_CSC_KVG);
+		tegra_dc_writel(dc, 0x0204, DC_WIN_CSC_KUB);
+		tegra_dc_writel(dc, 0x0000, DC_WIN_CSC_KVB);
+
+		value |= CSC_ENABLE;
+	} else 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;
+	}
+
+	tegra_dc_writel(dc, WIN_A_UPDATE << index, DC_CMD_STATE_CONTROL);
+	tegra_dc_writel(dc, WIN_A_ACT_REQ << index, 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;
+
+	case DRM_FORMAT_UYVY:
+		return WIN_COLOR_DEPTH_YCbCr422;
+
+	case DRM_FORMAT_YUV420:
+		return WIN_COLOR_DEPTH_YCbCr420P;
+
+	case DRM_FORMAT_YUV422:
+		return WIN_COLOR_DEPTH_YCbCr422P;
+
+	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 +458,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 {
 	struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
 	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 +489,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[0] = crtc->fb->pitches[0];
+	window.base[0] = gem->paddr;
+
+	err = tegra_dc_setup_window(dc, 0, &window);
+	if (err < 0)
+		dev_err(dc->dev, "failed to enable root plane\n");
 
 	return 0;
 }
@@ -347,7 +586,7 @@ static void tegra_crtc_load_lut(struct drm_crtc *crtc)
 }
 
 static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = {
-	.dpms = tegra_crtc_dpms,
+	.disable = tegra_crtc_disable,
 	.mode_fixup = tegra_crtc_mode_fixup,
 	.mode_set = tegra_crtc_mode_set,
 	.prepare = tegra_crtc_prepare,
@@ -588,7 +827,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 +929,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..e2fa328 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -290,8 +290,18 @@
 #define DC_DISP_SD_HW_K_VALUES			0x4dd
 #define DC_DISP_SD_MAN_K_VALUES			0x4de
 
+#define DC_WIN_CSC_YOF				0x611
+#define DC_WIN_CSC_KYRGB			0x612
+#define DC_WIN_CSC_KUR				0x613
+#define DC_WIN_CSC_KVR				0x614
+#define DC_WIN_CSC_KUG				0x615
+#define DC_WIN_CSC_KVG				0x616
+#define DC_WIN_CSC_KUB				0x617
+#define DC_WIN_CSC_KVB				0x618
+
 #define DC_WIN_WIN_OPTIONS			0x700
 #define COLOR_EXPAND (1 <<  6)
+#define CSC_ENABLE   (1 << 18)
 #define WIN_ENABLE   (1 << 30)
 
 #define DC_WIN_BYTE_SWAP			0x701
@@ -359,7 +369,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 3c61aab..896ff43 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -107,6 +107,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[2];
+	unsigned long base[3];
+};
+
+/* 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 tegra_output_ops {
 	int (*enable)(struct tegra_output *output);
 	int (*disable)(struct tegra_output *output);
-- 
1.8.1.2

[PATCH v4 4/9] 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>
---
Changes in v3:
- split DC_CMD_STATE_CONTROL writes

Changes in v4:
- reset line stride to fix cases where the framebuffer size doesn't
  match the display mode (suggested by Mark Zhang)

 drivers/gpu/drm/tegra/dc.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 52bf63f..ceb2f52 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -134,6 +134,29 @@ 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 drm_framebuffer *fb)
+{
+	struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, 0);
+	unsigned long value;
+
+	tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER);
+
+	value = fb->offsets[0] + y * fb->pitches[0] +
+		x * fb->bits_per_pixel / 8;
+
+	tegra_dc_writel(dc, gem->paddr + value, DC_WINBUF_START_ADDR);
+	tegra_dc_writel(dc, fb->pitches[0], DC_WIN_LINE_STRIDE);
+
+	value = GENERAL_UPDATE | WIN_A_UPDATE;
+	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+
+	value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
+	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,
@@ -510,6 +533,14 @@ 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_dc *dc = to_tegra_dc(crtc);
+
+	return tegra_dc_set_base(dc, x, y, crtc->fb);
+}
+
 static void tegra_crtc_prepare(struct drm_crtc *crtc)
 {
 	struct tegra_dc *dc = to_tegra_dc(crtc);
@@ -589,6 +620,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = {
 	.disable = tegra_crtc_disable,
 	.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.2

[PATCH v4 5/9] 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>
---
Changes in v3:
- implement dummy .get_vblank_counter()

 drivers/gpu/drm/tegra/dc.c  | 55 +++++++++++++++++++++++++++++++--------------
 drivers/gpu/drm/tegra/drm.c | 50 +++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tegra/drm.h |  3 +++
 3 files changed, 91 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index ceb2f52..5f55a25 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -157,6 +157,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,
@@ -485,6 +511,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);
@@ -585,31 +613,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)
@@ -626,7 +646,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;
@@ -971,7 +991,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,
@@ -1020,6 +1040,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..4e31dac 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,48 @@ 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 u32 tegra_drm_get_vblank_counter(struct drm_device *dev, int crtc)
+{
+	/* TODO: implement real hardware counter using syncpoints */
+	return drm_vblank_count(dev, crtc);
+}
+
+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 +142,10 @@ struct drm_driver tegra_drm_driver = {
 	.open = tegra_drm_open,
 	.lastclose = tegra_drm_lastclose,
 
+	.get_vblank_counter = tegra_drm_get_vblank_counter,
+	.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 896ff43..01f0aee 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -64,6 +64,7 @@ struct tegra_output;
 
 struct tegra_dc {
 	struct host1x_client client;
+	spinlock_t lock;
 
 	struct host1x *host1x;
 	struct device *dev;
@@ -130,6 +131,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 tegra_output_ops {
 	int (*enable)(struct tegra_output *output);
-- 
1.8.1.2

[PATCH v4 6/9] 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>
---
Changes in v3:
- use drm_send_vblank_event()
- set crtc->fb field

Changes in v4:
- fix a potential race by checking that a framebuffer base has been
  latched when completing a page-flip

 drivers/gpu/drm/tegra/dc.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tegra/dc.h  |  2 ++
 drivers/gpu/drm/tegra/drm.c |  9 +++++++
 drivers/gpu/drm/tegra/drm.h |  5 ++++
 4 files changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 5f55a25..c5d825f 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
+	struct drm_crtc *crtc = &dc->base;
+	struct drm_gem_cma_object *gem;
+	unsigned long flags, base;
+
+	if (!dc->event)
+		return;
+
+	gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
+
+	/* check if new start address has been latched */
+	tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
+	base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
+	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
+
+	if (base == gem->paddr + crtc->fb->offsets[0]) {
+		spin_lock_irqsave(&drm->event_lock, flags);
+		drm_send_vblank_event(drm, dc->pipe, dc->event);
+		drm_vblank_put(drm, dc->pipe);
+		dc->event = NULL;
+		spin_unlock_irqrestore(&drm->event_lock, flags);
+	}
+}
+
+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_dc *dc = to_tegra_dc(crtc);
+	struct drm_device *drm = crtc->dev;
+
+	if (dc->event)
+		return -EBUSY;
+
+	if (event) {
+		event->pipe = dc->pipe;
+		dc->event = event;
+		drm_vblank_get(drm, dc->pipe);
+	}
+
+	tegra_dc_set_base(dc, 0, 0, fb);
+	crtc->fb = fb;
+
+	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,
 };
@@ -665,6 +730,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/dc.h b/drivers/gpu/drm/tegra/dc.h
index e2fa328..79eaec9 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -58,6 +58,8 @@
 #define DC_CMD_SIGNAL_RAISE3			0x03e
 
 #define DC_CMD_STATE_ACCESS			0x040
+#define READ_MUX  (1 << 0)
+#define WRITE_MUX (1 << 2)
 
 #define DC_CMD_STATE_CONTROL			0x041
 #define GENERAL_ACT_REQ (1 <<  0)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 4e31dac..97485af 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -135,11 +135,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 = tegra_drm_get_vblank_counter,
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 01f0aee..6dd75a2 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -84,6 +84,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)
@@ -133,6 +136,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 tegra_output_ops {
 	int (*enable)(struct tegra_output *output);
-- 
1.8.1.2

[PATCH v4 7/9] drm/tegra: Split DC_CMD_STATE_CONTROL register write

[Thierry Reding]

The Tegra TRM says that the ACT_REQ and UPDATE fields cannot be
programmed at the same time so they are updated in two consecutive
writes instead.

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

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index c5d825f..2086400 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -689,9 +689,10 @@ static void tegra_crtc_commit(struct drm_crtc *crtc)
 	struct tegra_dc *dc = to_tegra_dc(crtc);
 	unsigned long value;
 
-	value = GENERAL_ACT_REQ | WIN_A_ACT_REQ |
-		GENERAL_UPDATE | WIN_A_UPDATE;
+	value = GENERAL_UPDATE | WIN_A_UPDATE;
+	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
 
+	value = GENERAL_ACT_REQ | WIN_A_ACT_REQ;
 	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
 
 	drm_vblank_post_modeset(crtc->dev, dc->pipe);
-- 
1.8.1.2

[PATCH v4 8/9] drm/tegra: Fix color expansion

[Thierry Reding]

bpp stores the number of bytes per pixel, but color expansion needs to
be enabled for less than 24 bits per pixel.

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

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 2086400..bf8095c 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -500,7 +500,7 @@ int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
 		tegra_dc_writel(dc, 0x0000, DC_WIN_CSC_KVB);
 
 		value |= CSC_ENABLE;
-	} else if (bpp < 24) {
+	} else if (window->bits_per_pixel < 24) {
 		value |= COLOR_EXPAND;
 	}
 
-- 
1.8.1.2

[PATCH v4 9/9] drm/tegra: Add list of framebuffers to debugfs

[Thierry Reding]

This list is most useful to inspect whether framebuffer reference
counting works as expected. The code is loosely based on the i915
implementation.

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

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 97485af..181a370 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -143,6 +143,45 @@ static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file)
 		tegra_dc_cancel_page_flip(crtc, file);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static int tegra_debugfs_framebuffers(struct seq_file *s, void *data)
+{
+	struct drm_info_node *node = (struct drm_info_node *)s->private;
+	struct drm_device *drm = node->minor->dev;
+	struct drm_framebuffer *fb;
+
+	mutex_lock(&drm->mode_config.fb_lock);
+
+	list_for_each_entry(fb, &drm->mode_config.fb_list, head) {
+		seq_printf(s, "%3d: user size: %d x %d, depth %d, %d bpp, refcount %d\n",
+			   fb->base.id, fb->width, fb->height, fb->depth,
+			   fb->bits_per_pixel,
+			   atomic_read(&fb->refcount.refcount));
+	}
+
+	mutex_unlock(&drm->mode_config.fb_lock);
+
+	return 0;
+}
+
+static struct drm_info_list tegra_debugfs_list[] = {
+	{ "framebuffers", tegra_debugfs_framebuffers, 0 },
+};
+
+static int tegra_debugfs_init(struct drm_minor *minor)
+{
+	return drm_debugfs_create_files(tegra_debugfs_list,
+					ARRAY_SIZE(tegra_debugfs_list),
+					minor->debugfs_root, minor);
+}
+
+static void tegra_debugfs_cleanup(struct drm_minor *minor)
+{
+	drm_debugfs_remove_files(tegra_debugfs_list,
+				 ARRAY_SIZE(tegra_debugfs_list), minor);
+}
+#endif
+
 struct drm_driver tegra_drm_driver = {
 	.driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM,
 	.load = tegra_drm_load,
@@ -155,6 +194,11 @@ struct drm_driver tegra_drm_driver = {
 	.enable_vblank = tegra_drm_enable_vblank,
 	.disable_vblank = tegra_drm_disable_vblank,
 
+#if defined(CONFIG_DEBUG_FS)
+	.debugfs_init = tegra_debugfs_init,
+	.debugfs_cleanup = tegra_debugfs_cleanup,
+#endif
+
 	.gem_free_object = drm_gem_cma_free_object,
 	.gem_vm_ops = &drm_gem_cma_vm_ops,
 	.dumb_create = drm_gem_cma_dumb_create,
-- 
1.8.1.2

Re: [PATCH v4 6/9] drm/tegra: Implement page-flipping support

[Mario Kleiner]

On 02/21/2013 03:35 PM, Thierry Reding wrote:
> 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@avionic-design.de>
> ---
> Changes in v3:
> - use drm_send_vblank_event()
> - set crtc->fb field
>
> Changes in v4:
> - fix a potential race by checking that a framebuffer base has been
>    latched when completing a page-flip
>
>   drivers/gpu/drm/tegra/dc.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/tegra/dc.h  |  2 ++
>   drivers/gpu/drm/tegra/drm.c |  9 +++++++
>   drivers/gpu/drm/tegra/drm.h |  5 ++++
>   4 files changed, 82 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 5f55a25..c5d825f 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
> +	struct drm_crtc *crtc = &dc->base;
> +	struct drm_gem_cma_object *gem;
> +	unsigned long flags, base;
> +

The checks for properly latched scanout look good to me and should fix 
the race between software and hardware. But shouldn't you already 
spin_lock_irqsave() here, before checking dc->event and performing write 
access on the DC_CMD_STATE_ACCESS registers? And lock around most of the 
tegra_dc_page_flip() code below, like in your previous iteration of the 
patch, to prevent a race between pageflip ioctl and the vblank irq handler?

> +	if (!dc->event)

-> !dc->event may exit prematurely because the irq handler on one core 
doesn't notice the new setup of dc->event by tegra_dc_page_flip() on a 
different core? Don't know if that can happen, don't know the memory 
ordering of arm, but could imagine that this or the compiler reordering 
instructions could cause trouble without lock protection.

> +		return;
> +
> +	gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
-> crtc->fb gets updated in tegra_dc_page_flip() after 
tegra_dc_set_base() without lock -> possibly stale fb here?

> +
> +	/* check if new start address has been latched */
> +	tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
> +	base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
> +	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> +

-> Can other code in the driver write to DC_CMD_STATE_ACCESS, 
simultaneously to tegra_dc_page_flip writing->reading->writing and cause 
trouble?

> +	if (base == gem->paddr + crtc->fb->offsets[0]) {
> +		spin_lock_irqsave(&drm->event_lock, flags);
> +		drm_send_vblank_event(drm, dc->pipe, dc->event);
> +		drm_vblank_put(drm, dc->pipe);
> +		dc->event = NULL;
> +		spin_unlock_irqrestore(&drm->event_lock, flags);
> +	}
> +}
> +
> +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_dc *dc = to_tegra_dc(crtc);
> +	struct drm_device *drm = crtc->dev;
> +
> +	if (dc->event)
> +		return -EBUSY;
> +

-> Lock already here?

> +	if (event) {
> +		event->pipe = dc->pipe;
> +		dc->event = event;
> +		drm_vblank_get(drm, dc->pipe);
> +	}
> +
> +	tegra_dc_set_base(dc, 0, 0, fb);
> +	crtc->fb = fb;
> +

-> Unlock here?

> +	return 0;
> +}
> +

thanks,
-mario


>   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,
>   };
> @@ -665,6 +730,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/dc.h b/drivers/gpu/drm/tegra/dc.h
> index e2fa328..79eaec9 100644
> --- a/drivers/gpu/drm/tegra/dc.h
> +++ b/drivers/gpu/drm/tegra/dc.h
> @@ -58,6 +58,8 @@
>   #define DC_CMD_SIGNAL_RAISE3			0x03e
>   
>   #define DC_CMD_STATE_ACCESS			0x040
> +#define READ_MUX  (1 << 0)
> +#define WRITE_MUX (1 << 2)
>   
>   #define DC_CMD_STATE_CONTROL			0x041
>   #define GENERAL_ACT_REQ (1 <<  0)
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 4e31dac..97485af 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -135,11 +135,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 = tegra_drm_get_vblank_counter,
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 01f0aee..6dd75a2 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -84,6 +84,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)
> @@ -133,6 +136,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 tegra_output_ops {
>   	int (*enable)(struct tegra_output *output);

Re: [PATCH v4 6/9] drm/tegra: Implement page-flipping support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4605 bytes --]

On Fri, Feb 22, 2013 at 05:13:47PM +0100, Mario Kleiner wrote:
> On 02/21/2013 03:35 PM, Thierry Reding wrote:
> >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@avionic-design.de>
> >---
> >Changes in v3:
> >- use drm_send_vblank_event()
> >- set crtc->fb field
> >
> >Changes in v4:
> >- fix a potential race by checking that a framebuffer base has been
> >   latched when completing a page-flip
> >
> >  drivers/gpu/drm/tegra/dc.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/tegra/dc.h  |  2 ++
> >  drivers/gpu/drm/tegra/drm.c |  9 +++++++
> >  drivers/gpu/drm/tegra/drm.h |  5 ++++
> >  4 files changed, 82 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >index 5f55a25..c5d825f 100644
> >--- a/drivers/gpu/drm/tegra/dc.c
> >+++ b/drivers/gpu/drm/tegra/dc.c
> >@@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
> >+	struct drm_crtc *crtc = &dc->base;
> >+	struct drm_gem_cma_object *gem;
> >+	unsigned long flags, base;
> >+
> 
> The checks for properly latched scanout look good to me and should
> fix the race between software and hardware. But shouldn't you
> already spin_lock_irqsave() here, before checking dc->event and
> performing write access on the DC_CMD_STATE_ACCESS registers? And
> lock around most of the tegra_dc_page_flip() code below, like in
> your previous iteration of the patch, to prevent a race between
> pageflip ioctl and the vblank irq handler?

Currently there's nothing else that touches the DC_CMD_STATE_ACCESS
register, so that part should be safe.

As to the locking that I removed since the previous patch, I looked at
it more closely and didn't think it was necessary. Also nothing in my
tests seemed to point to any race here, though I probably didn't cover
everything.

> >+	if (!dc->event)
> 
> -> !dc->event may exit prematurely because the irq handler on one
> core doesn't notice the new setup of dc->event by
> tegra_dc_page_flip() on a different core? Don't know if that can
> happen, don't know the memory ordering of arm, but could imagine
> that this or the compiler reordering instructions could cause
> trouble without lock protection.

I'm not sure I follow here. If the handler sees a NULL here, why would
it want to continue? It can safely assume that the page flip wasn't
setup for this interval, can't it?

If indeed the VBLANK interrupt happens exactly around the assignment to
dc->event, then we'll just schedule the page-flip for the next VBLANK.

> >+		return;
> >+
> >+	gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
> -> crtc->fb gets updated in tegra_dc_page_flip() after
> tegra_dc_set_base() without lock -> possibly stale fb here?

Good point. That may indeed require a lock. I'll need to look more
closely.

> >+	/* check if new start address has been latched */
> >+	tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
> >+	base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
> >+	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> >+
> 
> -> Can other code in the driver write to DC_CMD_STATE_ACCESS,
> simultaneously to tegra_dc_page_flip writing->reading->writing and
> cause trouble?

This is the only location where the DC_CMD_STATE_ACCESS register is
actually used in the driver so we should be safe for now.

> >+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >+			      struct drm_pending_vblank_event *event)
> >+{
> >+	struct tegra_dc *dc = to_tegra_dc(crtc);
> >+	struct drm_device *drm = crtc->dev;
> >+
> >+	if (dc->event)
> >+		return -EBUSY;
> >+
> 
> -> Lock already here?
> 
> >+	if (event) {
> >+		event->pipe = dc->pipe;
> >+		dc->event = event;
> >+		drm_vblank_get(drm, dc->pipe);
> >+	}
> >+
> >+	tegra_dc_set_base(dc, 0, 0, fb);
> >+	crtc->fb = fb;
> >+
> 
> -> Unlock here?

I tried to expand the lock after we discussed this previously but it
caused the code to deadlock. While in the process of tracking it down I
decided that the lock wasn't actually required at all.

But I hadn't thought about the stale FB issue, so I'll check again. I
notice that Dave has already merged this series, so if nobody objects
I'll fix it up in a follow-up patch.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v4 6/9] drm/tegra: Implement page-flipping support

[Mario Kleiner]

On 02/25/2013 08:06 AM, Thierry Reding wrote:
> On Fri, Feb 22, 2013 at 05:13:47PM +0100, Mario Kleiner wrote:
>> On 02/21/2013 03:35 PM, Thierry Reding wrote:
>>> 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@avionic-design.de>
>>> ---
>>> Changes in v3:
>>> - use drm_send_vblank_event()
>>> - set crtc->fb field
>>>
>>> Changes in v4:
>>> - fix a potential race by checking that a framebuffer base has been
>>>    latched when completing a page-flip
>>>
>>>   drivers/gpu/drm/tegra/dc.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/tegra/dc.h  |  2 ++
>>>   drivers/gpu/drm/tegra/drm.c |  9 +++++++
>>>   drivers/gpu/drm/tegra/drm.h |  5 ++++
>>>   4 files changed, 82 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>> index 5f55a25..c5d825f 100644
>>> --- a/drivers/gpu/drm/tegra/dc.c
>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>> @@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
>>> +	struct drm_crtc *crtc = &dc->base;
>>> +	struct drm_gem_cma_object *gem;
>>> +	unsigned long flags, base;
>>> +
>>
>> The checks for properly latched scanout look good to me and should
>> fix the race between software and hardware. But shouldn't you
>> already spin_lock_irqsave() here, before checking dc->event and
>> performing write access on the DC_CMD_STATE_ACCESS registers? And
>> lock around most of the tegra_dc_page_flip() code below, like in
>> your previous iteration of the patch, to prevent a race between
>> pageflip ioctl and the vblank irq handler?
>
> Currently there's nothing else that touches the DC_CMD_STATE_ACCESS
> register, so that part should be safe.
>

ok.

> As to the locking that I removed since the previous patch, I looked at
> it more closely and didn't think it was necessary. Also nothing in my
> tests seemed to point to any race here, though I probably didn't cover
> everything.
>
>>> +	if (!dc->event)
>>
>> -> !dc->event may exit prematurely because the irq handler on one
>> core doesn't notice the new setup of dc->event by
>> tegra_dc_page_flip() on a different core? Don't know if that can
>> happen, don't know the memory ordering of arm, but could imagine
>> that this or the compiler reordering instructions could cause
>> trouble without lock protection.
>
> I'm not sure I follow here. If the handler sees a NULL here, why would
> it want to continue? It can safely assume that the page flip wasn't
> setup for this interval, can't it?
>
> If indeed the VBLANK interrupt happens exactly around the assignment to
> dc->event, then we'll just schedule the page-flip for the next VBLANK.
>

I meant the other way round. dc->event is initially NULL. Then pageflip 
ioctl is called, close to vblank, the code e.g., executing on core 1. if 
(event) branch in tegra_dc_page_flip() executes, assigns dc->event = 
event etc. calls tegra_dc_set_base() before onset of vblank, pageflip 
completes in vblank, but the irq handler and thereby 
tegra_dc_finish_page_flip() running on a different core doesn't notice 
dc->event is non-NULL, because the changes haven't propagated to the 
different core without some memory write barrier etc., thereby doesn't 
run the flip completion in the vblank of actual flip completion. But i 
now think my point is probably pointless, because the successive calls 
to drm_vblank_get() and tegra_dc_set_base() contain enough locking and 
implicit memory barriers that such a stale value there won't happen.


>>> +		return;
>>> +
>>> +	gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
>> -> crtc->fb gets updated in tegra_dc_page_flip() after
>> tegra_dc_set_base() without lock -> possibly stale fb here?
>
> Good point. That may indeed require a lock. I'll need to look more
> closely.
>

Yep, i think here my argument from above may hold for crtc->fb, nothing 
here to prevent a race afaics.

>>> +	/* check if new start address has been latched */
>>> +	tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
>>> +	base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
>>> +	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
>>> +
>>
>> -> Can other code in the driver write to DC_CMD_STATE_ACCESS,
>> simultaneously to tegra_dc_page_flip writing->reading->writing and
>> cause trouble?
>
> This is the only location where the DC_CMD_STATE_ACCESS register is
> actually used in the driver so we should be safe for now.
>

Ok. I'm just paranoid about forgetting such things on later changes.


>>> +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>>> +			      struct drm_pending_vblank_event *event)
>>> +{
>>> +	struct tegra_dc *dc = to_tegra_dc(crtc);
>>> +	struct drm_device *drm = crtc->dev;
>>> +
>>> +	if (dc->event)
>>> +		return -EBUSY;
>>> +
>>
>> -> Lock already here?
>>
>>> +	if (event) {
>>> +		event->pipe = dc->pipe;
>>> +		dc->event = event;
>>> +		drm_vblank_get(drm, dc->pipe);
>>> +	}
>>> +
>>> +	tegra_dc_set_base(dc, 0, 0, fb);
>>> +	crtc->fb = fb;
>>> +
>>
>> -> Unlock here?
>
> I tried to expand the lock after we discussed this previously but it
> caused the code to deadlock. While in the process of tracking it down I
> decided that the lock wasn't actually required at all.
>
> But I hadn't thought about the stale FB issue, so I'll check again. I
> notice that Dave has already merged this series, so if nobody objects
> I'll fix it up in a follow-up patch.
>

Fine with me.

thanks,
-mario