[PATCH v3 0/7] drm/tegra: Miscellaneous enhancements

[PATCH v3 0/7] 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 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 patch 5 builds on the
previous two to provide the page-flipping IOCTL. Patch 6 splits the
DC_CMD_STATE_CONTROL register writes into two consecutive writes as
required by the TRM.

Finally patch 7 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.

Thierry

Thierry Reding (7):
  drm: Add consistency check for page-flipping
  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: Add list of framebuffers to debugfs

 Documentation/DocBook/drm.tmpl |   6 +
 drivers/gpu/drm/drm_crtc.c     |   7 +
 drivers/gpu/drm/tegra/dc.c     | 489 ++++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/tegra/dc.h     |   2 +-
 drivers/gpu/drm/tegra/drm.c    | 103 +++++++++
 drivers/gpu/drm/tegra/drm.h    |  37 ++++
 6 files changed, 533 insertions(+), 111 deletions(-)

-- 
1.8.1.2

[PATCH v3 1/7] 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>
---
 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 fae8018..8dfaeb0 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 838c9b6..d86edc1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3766,6 +3766,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

Re: [PATCH v3 1/7] drm: Add consistency check for page-flipping

[Daniel Vetter]

On Wed, Feb 13, 2013 at 05:05:00PM +0100, Thierry Reding wrote:
> 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 fae8018..8dfaeb0 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 838c9b6..d86edc1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3766,6 +3766,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH v3 2/7] 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
only 32-bit RGBA pixel formats are advertised.

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

 drivers/gpu/drm/tegra/dc.c  | 338 ++++++++++++++++++++++++++++++++------------
 drivers/gpu/drm/tegra/dc.h  |   2 +-
 drivers/gpu/drm/tegra/drm.h |  29 ++++
 3 files changed, 274 insertions(+), 95 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b6679b3..8f97b1c 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -17,26 +17,124 @@
 #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, &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,
+};
+
+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,
@@ -46,10 +144,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;
 
@@ -79,9 +178,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,
@@ -152,6 +252,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;
+	}
+
+	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;
+
+	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,
@@ -159,8 +364,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;
 
@@ -191,81 +395,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;
 }
@@ -346,7 +492,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,
@@ -587,7 +733,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);
@@ -689,6 +835,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 c7a0507..8202e38 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.2

Re: [PATCH v3 2/7] drm/tegra: Add plane support

[Mark Zhang]

On 02/14/2013 12:05 AM, Thierry Reding wrote:
> 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
> only 32-bit RGBA pixel formats are advertised.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
[...]
> +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);

Using "devm_kzalloc" here seems like not a good idea. Everytime plane
disable or crtc disable, we should free "struct tegra_plane" which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.

> +		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;
> +			}
> +		}
> +	}

If what I mentioned above(about using devm_kzalloc to allocate "struct
tegra_plane") is correct, we need to free "struct tegra_plane" here.

>  }
>  
>  static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> @@ -46,10 +144,11 @@ static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
>  	return true;
>  }
>
[...]
> 

Re: [PATCH v3 2/7] drm/tegra: Add plane support

[Thierry Reding]

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

On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
> On 02/14/2013 12:05 AM, Thierry Reding wrote:
> > 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
> > only 32-bit RGBA pixel formats are advertised.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> [...]
> > +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);
> 
> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
> disable or crtc disable, we should free "struct tegra_plane" which is
> allocated here. But the memory which devm_kzalloc allocates is only
> freed when the driver detach. This makes lots of memory can't be
> recycled when the plane enable/disable frequently.

Why would we want to do that? I don't think doing so would even work,
since the planes are resources that need to be registered with the DRM
core and therefore can't be allocated on-demand.

Thierry

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

Re: [PATCH v3 2/7] drm/tegra: Add plane support

[Mark Zhang]

On 02/18/2013 02:40 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>> 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
>>> only 32-bit RGBA pixel formats are advertised.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>> [...]
>>> +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);
>>
>> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
>> disable or crtc disable, we should free "struct tegra_plane" which is
>> allocated here. But the memory which devm_kzalloc allocates is only
>> freed when the driver detach. This makes lots of memory can't be
>> recycled when the plane enable/disable frequently.
> 
> Why would we want to do that? I don't think doing so would even work,
> since the planes are resources that need to be registered with the DRM
> core and therefore can't be allocated on-demand.
> 

I'm wondering if we add power management codes in the future, the
crtc/plane will be disabled/enabled frequently, e.g: system going to
suspend state then resume.

> Thierry
> 

Re: [PATCH v3 2/7] drm/tegra: Add plane support

[Thierry Reding]

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

On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
> On 02/18/2013 02:40 PM, Thierry Reding wrote:
> > On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
> >> On 02/14/2013 12:05 AM, Thierry Reding wrote:
> >>> 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
> >>> only 32-bit RGBA pixel formats are advertised.
> >>>
> >>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> >> [...]
> >>> +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);
> >>
> >> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
> >> disable or crtc disable, we should free "struct tegra_plane" which is
> >> allocated here. But the memory which devm_kzalloc allocates is only
> >> freed when the driver detach. This makes lots of memory can't be
> >> recycled when the plane enable/disable frequently.
> > 
> > Why would we want to do that? I don't think doing so would even work,
> > since the planes are resources that need to be registered with the DRM
> > core and therefore can't be allocated on-demand.
> > 
> 
> I'm wondering if we add power management codes in the future, the
> crtc/plane will be disabled/enabled frequently, e.g: system going to
> suspend state then resume.

In that case it makes even less sense to allocate and deallocate the
plane every time around. However, any optimizations aside, the allocated
memory is passed into the core via drm_plane_init(), which registers
that plane with the given CRTC. So if we deallocate the memory when the
plane when it is disabled, we'd have to make sure to remove it from the
core as well. However that would also mean that it disappears from the
list of planes supported by the CRTC and therefore we wouldn't be able
to use it again.

Thierry

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

Re: [PATCH v3 2/7] drm/tegra: Add plane support

[Mark Zhang]

On 02/18/2013 03:01 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
>> On 02/18/2013 02:40 PM, Thierry Reding wrote:
>>> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
>>>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>>>> 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
>>>>> only 32-bit RGBA pixel formats are advertised.
>>>>>
>>>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>>> [...]
>>>>> +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);
>>>>
>>>> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
>>>> disable or crtc disable, we should free "struct tegra_plane" which is
>>>> allocated here. But the memory which devm_kzalloc allocates is only
>>>> freed when the driver detach. This makes lots of memory can't be
>>>> recycled when the plane enable/disable frequently.
>>>
>>> Why would we want to do that? I don't think doing so would even work,
>>> since the planes are resources that need to be registered with the DRM
>>> core and therefore can't be allocated on-demand.
>>>
>>
>> I'm wondering if we add power management codes in the future, the
>> crtc/plane will be disabled/enabled frequently, e.g: system going to
>> suspend state then resume.
> 
> In that case it makes even less sense to allocate and deallocate the
> plane every time around. However, any optimizations aside, the allocated
> memory is passed into the core via drm_plane_init(), which registers
> that plane with the given CRTC. So if we deallocate the memory when the
> plane when it is disabled, we'd have to make sure to remove it from the
> core as well. However that would also mean that it disappears from the
> list of planes supported by the CRTC and therefore we wouldn't be able
> to use it again.
> 

Alright, I mixed up the "disable" & "remove" of planes and crtcs. You're
right. Just forget what I said in this thread.

Mark
> Thierry
> 

[PATCH v3 3/7] 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

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

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 8f97b1c..cc4c85e 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -114,6 +114,27 @@ 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_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,
@@ -416,6 +437,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);
@@ -495,6 +525,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

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Mark Zhang]

On 02/14/2013 12:05 AM, Thierry Reding wrote:
> 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
> 
>  drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 8f97b1c..cc4c85e 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -114,6 +114,27 @@ 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_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;
> +}

Again, what do you think about the "line stride" problem I mentioned:

http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html

Don't get me wrong that I also don't want to add a line stride update
here because that doesn't make sense. It's just a workaround. But we
need to find a way to make multi-head page flip working.

Mark
> +
>  static const struct drm_crtc_funcs tegra_crtc_funcs = {
>  	.set_config = drm_crtc_helper_set_config,
>  	.destroy = drm_crtc_cleanup,
> @@ -416,6 +437,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);
> @@ -495,6 +525,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,
> 

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Mark Zhang]

On 02/18/2013 02:17 PM, Mark Zhang wrote:
> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>> 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
>>
>>  drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>> index 8f97b1c..cc4c85e 100644
>> --- a/drivers/gpu/drm/tegra/dc.c
>> +++ b/drivers/gpu/drm/tegra/dc.c
>> @@ -114,6 +114,27 @@ 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_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;
>> +}
> 
> Again, what do you think about the "line stride" problem I mentioned:
> 
> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
> 
> Don't get me wrong that I also don't want to add a line stride update
> here because that doesn't make sense. It's just a workaround. But we
> need to find a way to make multi-head page flip working.

Sorry, it's not "multi-head page flip", it should be "multi-head fb
change". For example, if LVDS & HDMI are connected, I can create 4 fbs
for them(every is double-buffered) and call drmModeSetCrtc to switch 2
fbs of LVDS & HDMI to show something.

Mark
> 
> Mark
>> +
>>  static const struct drm_crtc_funcs tegra_crtc_funcs = {
>>  	.set_config = drm_crtc_helper_set_config,
>>  	.destroy = drm_crtc_cleanup,
>> @@ -416,6 +437,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);
>> @@ -495,6 +525,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,
>>

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Thierry Reding]

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

On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
> On 02/14/2013 12:05 AM, Thierry Reding wrote:
> > 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
> > 
> >  drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index 8f97b1c..cc4c85e 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -114,6 +114,27 @@ 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_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;
> > +}
> 
> Again, what do you think about the "line stride" problem I mentioned:
> 
> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
> 
> Don't get me wrong that I also don't want to add a line stride update
> here because that doesn't make sense. It's just a workaround. But we
> need to find a way to make multi-head page flip working.

I'm not sure that it's something we need to support. .mode_set_base() is
explicitly used only for cases where the framebuffer configuration
doesn't change. That is, only in cases where the only thing that changes
is the physical address of the framebuffer to be displayed.

The current case where one framebuffer is used as scanout for both
outputs isn't something that page-flipping can support. Page-flipping is
always per-CRTC because typically each CRTC would run at a different
frequency (or even if both ran at the same frequency the VBLANK is very
unlikely to coincide anyway).

So an application that wants to support page-flipping on two outputs
also needs to take care to set them up with different framebuffers, in
which case what you're describing here can't really happen.

Thierry

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

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Mark Zhang]

On 02/18/2013 02:48 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>> 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
>>>
>>>  drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
>>>  1 file changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>> index 8f97b1c..cc4c85e 100644
>>> --- a/drivers/gpu/drm/tegra/dc.c
>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>> @@ -114,6 +114,27 @@ 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_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;
>>> +}
>>
>> Again, what do you think about the "line stride" problem I mentioned:
>>
>> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
>>
>> Don't get me wrong that I also don't want to add a line stride update
>> here because that doesn't make sense. It's just a workaround. But we
>> need to find a way to make multi-head page flip working.
> 
> I'm not sure that it's something we need to support. .mode_set_base() is
> explicitly used only for cases where the framebuffer configuration
> doesn't change. That is, only in cases where the only thing that changes
> is the physical address of the framebuffer to be displayed.
> 
> The current case where one framebuffer is used as scanout for both
> outputs isn't something that page-flipping can support. Page-flipping is
> always per-CRTC because typically each CRTC would run at a different
> frequency (or even if both ran at the same frequency the VBLANK is very
> unlikely to coincide anyway).
> 
> So an application that wants to support page-flipping on two outputs
> also needs to take care to set them up with different framebuffers, in
> which case what you're describing here can't really happen.

Yes, the userspace application needs to setup different framebuffers for
different crtcs. So if LVDS & HDMI are connected, here is what the
application does:

1. drm reports that 2 connectors: LVDS & HDMI are present in the system
2. The resolution of them are: 1366x768 & 1080p
3. Userspace application allocates 2 fbs according to the resolution got
above.
4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
created.
5. At this time, remember the line stride setting in dc which connects
with LVDS is calculated according to the 1080p resolution. So finally we
got a corrupt display because we're showing a 1366x768 fb but with
1080p's line stride.

Hope I explained this clear. Try this test application if you still have
problems:

https://github.com/dvdhrm/docs/blob/master/drm-howto/modeset-vsync.c

Mark
> 
> Thierry
> 

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Thierry Reding]

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

On Mon, Feb 18, 2013 at 03:06:10PM +0800, Mark Zhang wrote:
> On 02/18/2013 02:48 PM, Thierry Reding wrote:
> > On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
> >> On 02/14/2013 12:05 AM, Thierry Reding wrote:
> >>> 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
> >>>
> >>>  drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
> >>>  1 file changed, 31 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >>> index 8f97b1c..cc4c85e 100644
> >>> --- a/drivers/gpu/drm/tegra/dc.c
> >>> +++ b/drivers/gpu/drm/tegra/dc.c
> >>> @@ -114,6 +114,27 @@ 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_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;
> >>> +}
> >>
> >> Again, what do you think about the "line stride" problem I mentioned:
> >>
> >> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
> >>
> >> Don't get me wrong that I also don't want to add a line stride update
> >> here because that doesn't make sense. It's just a workaround. But we
> >> need to find a way to make multi-head page flip working.
> > 
> > I'm not sure that it's something we need to support. .mode_set_base() is
> > explicitly used only for cases where the framebuffer configuration
> > doesn't change. That is, only in cases where the only thing that changes
> > is the physical address of the framebuffer to be displayed.
> > 
> > The current case where one framebuffer is used as scanout for both
> > outputs isn't something that page-flipping can support. Page-flipping is
> > always per-CRTC because typically each CRTC would run at a different
> > frequency (or even if both ran at the same frequency the VBLANK is very
> > unlikely to coincide anyway).
> > 
> > So an application that wants to support page-flipping on two outputs
> > also needs to take care to set them up with different framebuffers, in
> > which case what you're describing here can't really happen.
> 
> Yes, the userspace application needs to setup different framebuffers for
> different crtcs. So if LVDS & HDMI are connected, here is what the
> application does:
> 
> 1. drm reports that 2 connectors: LVDS & HDMI are present in the system
> 2. The resolution of them are: 1366x768 & 1080p
> 3. Userspace application allocates 2 fbs according to the resolution got
> above.
> 4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
> created.
> 5. At this time, remember the line stride setting in dc which connects
> with LVDS is calculated according to the 1080p resolution. So finally we
> got a corrupt display because we're showing a 1366x768 fb but with
> 1080p's line stride.

I don't see how the 1080p stride would still be relevant here. Setting
the LVDS to the new 1366x768 framebuffer should trigger a full modeset
and therefore set the stride to the value required by the new 1366x768
framebuffer.

Thierry

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

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Mark Zhang]

On 02/18/2013 03:20 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 03:06:10PM +0800, Mark Zhang wrote:
>> On 02/18/2013 02:48 PM, Thierry Reding wrote:
>>> On Mon, Feb 18, 2013 at 02:17:53PM +0800, Mark Zhang wrote:
>>>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>>>> 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
>>>>>
>>>>>  drivers/gpu/drm/tegra/dc.c | 31 +++++++++++++++++++++++++++++++
>>>>>  1 file changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>> index 8f97b1c..cc4c85e 100644
>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>> @@ -114,6 +114,27 @@ 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_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;
>>>>> +}
>>>>
>>>> Again, what do you think about the "line stride" problem I mentioned:
>>>>
>>>> http://lists.freedesktop.org/archives/dri-devel/2013-January/033561.html
>>>>
>>>> Don't get me wrong that I also don't want to add a line stride update
>>>> here because that doesn't make sense. It's just a workaround. But we
>>>> need to find a way to make multi-head page flip working.
>>>
>>> I'm not sure that it's something we need to support. .mode_set_base() is
>>> explicitly used only for cases where the framebuffer configuration
>>> doesn't change. That is, only in cases where the only thing that changes
>>> is the physical address of the framebuffer to be displayed.
>>>
>>> The current case where one framebuffer is used as scanout for both
>>> outputs isn't something that page-flipping can support. Page-flipping is
>>> always per-CRTC because typically each CRTC would run at a different
>>> frequency (or even if both ran at the same frequency the VBLANK is very
>>> unlikely to coincide anyway).
>>>
>>> So an application that wants to support page-flipping on two outputs
>>> also needs to take care to set them up with different framebuffers, in
>>> which case what you're describing here can't really happen.
>>
>> Yes, the userspace application needs to setup different framebuffers for
>> different crtcs. So if LVDS & HDMI are connected, here is what the
>> application does:
>>
>> 1. drm reports that 2 connectors: LVDS & HDMI are present in the system
>> 2. The resolution of them are: 1366x768 & 1080p
>> 3. Userspace application allocates 2 fbs according to the resolution got
>> above.
>> 4. call drmModeSetCrtc to switch the fb shown in LVDS to the new fb we
>> created.
>> 5. At this time, remember the line stride setting in dc which connects
>> with LVDS is calculated according to the 1080p resolution. So finally we
>> got a corrupt display because we're showing a 1366x768 fb but with
>> 1080p's line stride.
> 
> I don't see how the 1080p stride would still be relevant here. Setting
> the LVDS to the new 1366x768 framebuffer should trigger a full modeset
> and therefore set the stride to the value required by the new 1366x768
> framebuffer.
> 

Actually the dc which connects with LVDS doesn't know about 1080p
stuffs. All the video mode infos stored in this dc is still 1366x768(I
just checked the register values on my Tegra 30 cardhu). The dc just
scans 1366 pixels then jump to next line(based on the 1080p line stride
we set) and keep scanning.

So the drm crtc helper(I believe it's function:
drm_crtc_helper_set_config) finds out that video mode is not changed so
a fb_change while not full modeset is triggered.

I started to test this new series patch set just now. I'll let you know
whether the issue I described still exists.

Mark
> Thierry
> 

Re: [PATCH v3 3/7] drm/tegra: Implement .mode_set_base()

[Mark Zhang]

On 02/18/2013 04:40 PM, Mark Zhang wrote:
> On 02/18/2013 03:20 PM, Thierry Reding wrote:
[...]
>>
> 
> Actually the dc which connects with LVDS doesn't know about 1080p
> stuffs. All the video mode infos stored in this dc is still 1366x768(I
> just checked the register values on my Tegra 30 cardhu). The dc just
> scans 1366 pixels then jump to next line(based on the 1080p line stride
> we set) and keep scanning.
> 
> So the drm crtc helper(I believe it's function:
> drm_crtc_helper_set_config) finds out that video mode is not changed so
> a fb_change while not full modeset is triggered.
> 
> I started to test this new series patch set just now. I'll let you know
> whether the issue I described still exists.
> 

Tested. This issue still exists.

Mark
> Mark
>> Thierry
>>

[PATCH v3 4/7] 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 cc4c85e..9e21c84 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,
@@ -389,6 +415,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);
@@ -490,31 +518,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)
@@ -531,7 +551,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;
@@ -876,7 +896,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,
@@ -925,6 +945,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 d980dc7..8a31f44 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -39,6 +39,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;
@@ -88,6 +92,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,
@@ -95,6 +141,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 8202e38..51bc7b8 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.2

[PATCH v3 5/7] 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 v2:
- use drm_send_vblank_event()
- set crtc->fb field

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

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 9e21c84..0328963 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -161,7 +161,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_pending_vblank_event *event;
+	struct drm_device *drm = dc->base.dev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drm->event_lock, flags);
+
+	event = dc->event;
+	dc->event = NULL;
+
+	if (!event) {
+		spin_unlock_irqrestore(&drm->event_lock, flags);
+		return;
+	}
+
+	drm_send_vblank_event(drm, dc->pipe, event);
+	drm_vblank_put(drm, dc->pipe);
+
+	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_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);
+	crtc->fb = fb;
+
+	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,
 };
@@ -570,6 +635,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 8a31f44..edaf9c0 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -134,11 +134,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 51bc7b8..f7d93e4 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.2

[PATCH v3 6/7] 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 0328963..9b2a551 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -594,9 +594,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 v3 7/7] 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 edaf9c0..9d452df 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -142,6 +142,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,
@@ -154,6 +193,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 v3 0/7] drm/tegra: Miscellaneous enhancements

[Mark Zhang]

Hi Thierry,

Which version is this patch set based on? I tried to apply it on
linux-next 0206 & tot 0215 but failed:

Applying: drm: Add consistency check for page-flipping
Applying: drm/tegra: Add plane support
error: patch failed: drivers/gpu/drm/tegra/drm.h:121
error: drivers/gpu/drm/tegra/drm.h: patch does not apply
Patch failed at 0002 drm/tegra: Add plane support
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Mark
On 02/14/2013 12:04 AM, Thierry Reding wrote:
> 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 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 patch 5 builds on the
> previous two to provide the page-flipping IOCTL. Patch 6 splits the
> DC_CMD_STATE_CONTROL register writes into two consecutive writes as
> required by the TRM.
> 
> Finally patch 7 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.
> 
> Thierry
> 
> Thierry Reding (7):
>   drm: Add consistency check for page-flipping
>   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: Add list of framebuffers to debugfs
> 
>  Documentation/DocBook/drm.tmpl |   6 +
>  drivers/gpu/drm/drm_crtc.c     |   7 +
>  drivers/gpu/drm/tegra/dc.c     | 489 ++++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/tegra/dc.h     |   2 +-
>  drivers/gpu/drm/tegra/drm.c    | 103 +++++++++
>  drivers/gpu/drm/tegra/drm.h    |  37 ++++
>  6 files changed, 533 insertions(+), 111 deletions(-)
> 

Re: [PATCH v3 0/7] drm/tegra: Miscellaneous enhancements

[Thierry Reding]

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

On Mon, Feb 18, 2013 at 03:42:39PM +0800, Mark Zhang wrote:
> Hi Thierry,
> 
> Which version is this patch set based on? I tried to apply it on
> linux-next 0206 & tot 0215 but failed:

This was based on next-20130213.

Thierry

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