All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Intel-specific primary plane handling (v2)
@ 2014-04-11  0:24 Matt Roper
  2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Matt Roper @ 2014-04-11  0:24 UTC (permalink / raw)
  To: intel-gfx

Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---

This patch was previously part of my universal plane series on dri-devel
(http://lists.freedesktop.org/archives/dri-devel/2014-March/055852.html) but I
dropped it in v4 and v5 of that series to focus on the DRM core changes.  Now
that universal planes have been merged, we can start building on that framework
to provide i915-specific functionality.

Some i-g-t changes to test this will be sent shortly.


Matt


 drivers/gpu/drm/i915/intel_display.c | 165 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 2 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1af1d14..a0bc251 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,8 +39,24 @@
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+const static uint32_t intel_primary_formats[] = {
+       DRM_FORMAT_C8,
+       DRM_FORMAT_XRGB1555,
+       DRM_FORMAT_ARGB1555,
+       DRM_FORMAT_RGB565,
+       DRM_FORMAT_XRGB8888,
+       DRM_FORMAT_ARGB8888,
+       DRM_FORMAT_XBGR8888,
+       DRM_FORMAT_ABGR8888,
+       DRM_FORMAT_XRGB2101010,
+       DRM_FORMAT_ARGB2101010,
+       DRM_FORMAT_XBGR2101010,
+       DRM_FORMAT_ABGR2101010,
+};
+
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
@@ -10553,17 +10569,162 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_setplane(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 drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_framebuffer *tmpfb;
+	struct drm_rect dest = {
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
+	int ret;
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	/*
+	 * Current hardware can't reposition the primary plane or scale it
+	 * (although this could change in the future).
+	 */
+	drm_rect_intersect(&dest, &clip);
+	if (dest.x1 != 0 || dest.y1 != 0 ||
+	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
+		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	if (crtc_w != src_w || crtc_h != src_h) {
+		DRM_DEBUG_KMS("Can't scale primary plane\n");
+		return -EINVAL;
+	}
+
+	/* Primary planes are locked to their owning CRTC */
+	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
+		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
+		return -EINVAL;
+	}
+
+	/* Framebuffer must be big enough to cover entire plane */
+	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	if (ret)
+		return ret;
+
+	/*
+	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
+	 * code that called us expects to handle the framebuffer update and
+	 * reference counting; save and restore the current fb before
+	 * calling it.
+	 */
+	tmpfb = plane->fb;
+	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
+	if (ret)
+		return ret;
+	plane->fb = tmpfb;
+
+	if (!intel_crtc->primary_enabled)
+		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+					      intel_crtc->pipe);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc))
+		return -EINVAL;
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+	if (intel_crtc->primary_enabled)
+		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+					       intel_plane->pipe);
+
+	/*
+	 * N.B. The DRM setplane code will update the plane->fb pointer after
+	 * we finish here.
+	 */
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	intel_primary_plane_disable(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+
+	drm_plane_init(dev, &primary->base, 0,
+		       &intel_primary_plane_funcs, intel_primary_formats,
+		       ARRAY_SIZE(intel_primary_formats),
+		       DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_crtc_cleanup(&intel_crtc->base);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c551472..2ce9cc3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -363,6 +363,7 @@ struct intel_crtc {
 	bool active;
 	unsigned long enabled_power_domains;
 	bool eld_vld;
+	struct intel_plane *primary_plane;
 	bool primary_enabled; /* is the primary plane (partially) visible? */
 	bool lowfreq_avail;
 	struct intel_overlay *overlay;
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH i-g-t 0/3] Universal plane testing
  2014-04-11  0:24 [PATCH] drm/i915: Intel-specific primary plane handling (v2) Matt Roper
@ 2014-04-11  0:26 ` Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 1/3] kms: Add universal plane support Matt Roper
                     ` (2 more replies)
  2014-04-11  9:34 ` [PATCH] drm/i915: Intel-specific primary plane handling (v2) Daniel Vetter
  2014-04-11 20:44 ` [PATCH] drm/i915: Intel-specific primary plane handling (v3) Matt Roper
  2 siblings, 3 replies; 18+ messages in thread
From: Matt Roper @ 2014-04-11  0:26 UTC (permalink / raw)
  To: intel-gfx

Now that the DRM core supports universal planes, we should update
intel-gpu-tools to exercise this new functionality.

The first patch here reworks some of the igt_kms library internals to handle
primary and cursor planes coming from the DRM plane list.  This breaks a few
invariants that i-g-t previously relied upon (primary plane must be first in
the plane list, cursor plane must be last), so the second patch here updates
the existing kms_plane test to allow it to continue working with these changes
(that's the only test I noticed that made assumptions about plane order).
Finally, the third patch adds a new test for Intel-specific primary plane
handling via the universal plane API.  Note that this new test depends on i915
patch "drm/i915: Intel-specific primary plane handling" to allow the primary
plane to be disabled independently of the CRTC.  This test will fail if the
DRM's primary plane helper is used rather than the Intel-specific support from
that patch.

Matt Roper (3):
  kms: Add universal plane support
  kms_plane: Update for universal plane changes
  kms_universal_plane: Universal plane testing

 lib/igt_kms.c               | 132 ++++++++++++++++++++-------
 lib/igt_kms.h               |   5 ++
 tests/Makefile.sources      |   1 +
 tests/kms_plane.c           |  11 ++-
 tests/kms_universal_plane.c | 211 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 327 insertions(+), 33 deletions(-)
 create mode 100644 tests/kms_universal_plane.c

-- 
1.8.5.1

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH i-g-t 1/3] kms: Add universal plane support
  2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
@ 2014-04-11  0:26   ` Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 2/3] kms_plane: Update for universal plane changes Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing Matt Roper
  2 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2014-04-11  0:26 UTC (permalink / raw)
  To: intel-gfx

Add support for universal planes.  This involves revamping the existing
plane handling a bit to allow primary & cursor planes to come from the
DRM plane list, rather than always being manually added.  Also,
eliminate the hard-coded position of primary & cursor in the plane list
since the DRM could return them in any order.

To minimize impact for existing tests, we add a new
igt_display_use_universal_commits() call to toggle between universal and
legacy API's for programming the primary and cursor planes.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c | 132 +++++++++++++++++++++++++++++++++++++++++++++-------------
 lib/igt_kms.h |   5 +++
 2 files changed, 107 insertions(+), 30 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 9dee855..4ec7c97 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -473,27 +473,24 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 	 */
 	display->n_pipes = resources->count_crtcs;
 
+	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
 	plane_resources = drmModeGetPlaneResources(display->drm_fd);
 	igt_assert(plane_resources);
 
 	for (i = 0; i < display->n_pipes; i++) {
 		igt_pipe_t *pipe = &display->pipes[i];
-		igt_plane_t *plane;
-		int p, j;
+		igt_plane_t *plane = NULL;
+		int p = 0, j;
 
 		pipe->display = display;
 		pipe->pipe = i;
 
-		/* primary plane */
-		p = IGT_PLANE_PRIMARY;
-		plane = &pipe->planes[p];
-		plane->pipe = pipe;
-		plane->index = p;
-		plane->is_primary = true;
-
 		/* add the planes that can be used with that pipe */
 		for (j = 0; j < plane_resources->count_planes; j++) {
 			drmModePlane *drm_plane;
+			drmModeObjectPropertiesPtr proplist;
+			drmModePropertyPtr prop;
+			int k;
 
 			drm_plane = drmModeGetPlane(display->drm_fd,
 						    plane_resources->planes[j]);
@@ -504,21 +501,67 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 				continue;
 			}
 
-			p++;
 			plane = &pipe->planes[p];
 			plane->pipe = pipe;
-			plane->index = p;
+			plane->index = p++;
 			plane->drm_plane = drm_plane;
+
+			prop = NULL;
+			proplist = drmModeObjectGetProperties(display->drm_fd,
+							      plane_resources->planes[j],
+							      DRM_MODE_OBJECT_PLANE);
+			for (k = 0; k < proplist->count_props; k++) {
+				prop = drmModeGetProperty(display->drm_fd, proplist->props[k]);
+				if (!prop)
+					continue;
+
+				if (strcmp(prop->name, "type") != 0) {
+					drmModeFreeProperty(prop);
+					continue;
+				}
+
+				switch (proplist->prop_values[k]) {
+				case DRM_PLANE_TYPE_PRIMARY:
+					plane->is_primary = 1;
+					display->has_universal_planes = 1;
+					break;
+				case DRM_PLANE_TYPE_CURSOR:
+					plane->is_cursor = 1;
+					pipe->has_cursor = 1;
+					display->has_universal_planes = 1;
+					break;
+				}
+
+				drmModeFreeProperty(prop);
+				break;
+			}
+
 		}
 
-		/* cursor plane */
-		p++;
-		plane = &pipe->planes[p];
-		plane->pipe = pipe;
-		plane->index = p;
-		plane->is_cursor = true;
+		/*
+		 * Add a drm_plane-less primary plane for kernels without
+		 * universal plane support.
+		 */
+		if (!display->has_universal_planes) {
+			plane = &pipe->planes[p];
+			plane->pipe = pipe;
+			plane->index = p++;
+			plane->is_primary = true;
+		}
 
-		pipe->n_planes = ++p;
+		/*
+		 * Add drm_plane-less cursor plane for kernels that don't
+		 * expose a universal cursor plane.
+		 */
+		if (!pipe->has_cursor) {
+			/* cursor plane */
+			plane = &pipe->planes[p];
+			plane->pipe = pipe;
+			plane->index = p++;
+			plane->is_cursor = true;
+		}
+
+		pipe->n_planes = p;
 
 		/* make sure we don't overflow the plane array */
 		igt_assert(pipe->n_planes <= IGT_MAX_PLANES);
@@ -673,16 +716,29 @@ static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, enum igt_plane plane)
 {
 	int idx;
 
-	/* Cursor plane is always the upper plane */
-	if (plane == IGT_PLANE_CURSOR)
-		idx = pipe->n_planes - 1;
-	else {
-		igt_assert_f(plane >= 0 && plane < (pipe->n_planes),
-			     "plane=%d\n", plane);
-		idx = plane;
+	for (idx = 0; idx < pipe->n_planes; idx++) {
+		switch (plane) {
+		case IGT_PLANE_PRIMARY:
+			if (pipe->planes[idx].is_primary)
+				return &pipe->planes[idx];
+			break;
+		case IGT_PLANE_CURSOR:
+			if (pipe->planes[idx].is_cursor)
+				return &pipe->planes[idx];
+			break;
+		case IGT_PLANE_2:
+			if (!pipe->planes[idx].is_primary &&
+			    !pipe->planes[idx].is_cursor)
+				return &pipe->planes[idx];
+		default:
+			if (!pipe->planes[idx].is_primary &&
+			    !pipe->planes[idx].is_cursor)
+				/* Consume this overlay and keep searching for another */
+				plane--;
+		}
 	}
 
-	return &pipe->planes[idx];
+	return NULL;
 }
 
 static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
@@ -734,6 +790,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
 	uint32_t fb_id, crtc_id;
 	int ret;
 
+	igt_assert(plane->drm_plane);
+
 	fb_id = igt_plane_get_fb_id(plane);
 	crtc_id = output->config.crtc->crtc_id;
 	pipe = igt_output_get_driving_pipe(output);
@@ -793,7 +851,11 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
 
 static int igt_plane_commit(igt_plane_t *plane, igt_output_t *output)
 {
-	if (plane->is_cursor) {
+	struct igt_display *display = plane->pipe->display;
+
+	if (display->commit_universal && plane->drm_plane) {
+		igt_drm_plane_commit(plane, output);
+	} else if (plane->is_cursor) {
 		igt_cursor_commit(plane, output);
 	} else if (plane->is_primary) {
 		/* state updated by SetCrtc */
@@ -811,8 +873,8 @@ static int igt_output_commit(igt_output_t *output)
 	int i;
 
 	pipe = igt_output_get_driving_pipe(output);
-	if (pipe->need_set_crtc) {
-		igt_plane_t *primary = &pipe->planes[0];
+	if (pipe->need_set_crtc && !display->commit_universal) {
+		igt_plane_t *primary = igt_pipe_get_plane(pipe, IGT_PLANE_PRIMARY);
 		drmModeModeInfo *mode;
 		uint32_t fb_id, crtc_id;
 		int ret;
@@ -862,7 +924,7 @@ static int igt_output_commit(igt_output_t *output)
 		primary->fb_changed = false;
 	}
 
-	if (pipe->need_set_cursor) {
+	if (pipe->need_set_cursor && !display->commit_universal) {
 		igt_plane_t *cursor;
 		uint32_t gem_handle, crtc_id;
 		int ret;
@@ -913,6 +975,16 @@ static int igt_output_commit(igt_output_t *output)
 	return 0;
 }
 
+/*
+ * Indicate whether future display commits should use universal plane API's
+ * or legacy API's.
+ */
+void igt_display_use_universal_commits(igt_display_t *display,
+				       bool use_universal)
+{
+	display->commit_universal = use_universal;
+}
+
 int igt_display_commit(igt_display_t *display)
 {
 	int i;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 439a634..24d5d6a 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -120,6 +120,7 @@ struct igt_pipe {
 	unsigned int need_set_crtc        : 1;
 	unsigned int need_set_cursor      : 1;
 	unsigned int need_wait_for_vblank : 1;
+	unsigned int has_cursor           : 1;
 #define IGT_MAX_PLANES	4
 	int n_planes;
 	igt_plane_t planes[IGT_MAX_PLANES];
@@ -142,6 +143,8 @@ struct igt_display {
 	unsigned long pipes_in_use;
 	igt_output_t *outputs;
 	igt_pipe_t pipes[I915_MAX_PIPES];
+	bool has_universal_planes;
+	bool commit_universal;
 };
 
 /* set vt into graphics mode, required to prevent fbcon from interfering */
@@ -149,6 +152,8 @@ void igt_set_vt_graphics_mode(void);
 
 void igt_display_init(igt_display_t *display, int drm_fd);
 void igt_display_fini(igt_display_t *display);
+void igt_display_use_universal_commits(igt_display_t *display,
+				       bool use_universal);
 int  igt_display_commit(igt_display_t *display);
 int  igt_display_get_n_pipes(igt_display_t *display);
 
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH i-g-t 2/3] kms_plane: Update for universal plane changes
  2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 1/3] kms: Add universal plane support Matt Roper
@ 2014-04-11  0:26   ` Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing Matt Roper
  2 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2014-04-11  0:26 UTC (permalink / raw)
  To: intel-gfx

Recent changes in igt_kms to support universal planes have removed the
plane list order guarantees that kms_plane depended upon.  Ensure that
we loop over the entire plane list properly and then selectively skip
non-overlay planes.

While we're at it, update a couple igt_output_get_plane() calls to use
plane enums rather than integer values to make it clear what we're
actually doing.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/kms_plane.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/kms_plane.c b/tests/kms_plane.c
index 6bdbca3..bbeab0a 100644
--- a/tests/kms_plane.c
+++ b/tests/kms_plane.c
@@ -94,7 +94,7 @@ test_position_init(test_position_t *test, igt_output_t *output, enum pipe pipe)
 	test->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
 
 	igt_output_set_pipe(output, pipe);
-	primary = igt_output_get_plane(output, 0);
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
 
 	mode = igt_output_get_mode(output);
 	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
@@ -146,9 +146,14 @@ test_plane_position_with_output(data_t *data,
 	test_position_init(&test, output, pipe);
 
 	mode = igt_output_get_mode(output);
-	primary = igt_output_get_plane(output, 0);
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
 	sprite = igt_output_get_plane(output, plane);
 
+	if (sprite->is_primary) {
+		test_position_fini(&test, output);
+		igt_skip("Skipping primary plane\n");
+	}
+
 	create_fb_for_mode__position(data, mode, 100, 100, 64, 64,
 				     &primary_fb);
 	igt_plane_set_fb(primary, &primary_fb);
@@ -213,7 +218,7 @@ run_tests_for_pipe(data_t *data, enum pipe pipe)
 {
 	int plane;
 
-	for (plane = 1; plane < IGT_MAX_PLANES; plane++)
+	for (plane = 0; plane < IGT_MAX_PLANES; plane++)
 		run_tests_for_pipe_plane(data, pipe, plane);
 }
 
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing
  2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 1/3] kms: Add universal plane support Matt Roper
  2014-04-11  0:26   ` [PATCH i-g-t 2/3] kms_plane: Update for universal plane changes Matt Roper
@ 2014-04-11  0:26   ` Matt Roper
  2014-04-11  9:22     ` Daniel Vetter
  2 siblings, 1 reply; 18+ messages in thread
From: Matt Roper @ 2014-04-11  0:26 UTC (permalink / raw)
  To: intel-gfx

Add a simple test to exercise universal plane support.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/Makefile.sources      |   1 +
 tests/kms_universal_plane.c | 211 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+)
 create mode 100644 tests/kms_universal_plane.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index bf02a48..4911914 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -63,6 +63,7 @@ TESTS_progs_M = \
 	kms_plane \
 	kms_render \
 	kms_setmode \
+	kms_universal_plane \
 	pm_lpsp \
 	pm_pc8 \
 	pm_rps \
diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
new file mode 100644
index 0000000..f1ec6fb
--- /dev/null
+++ b/tests/kms_universal_plane.c
@@ -0,0 +1,211 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+} data_t;
+
+/*
+ * Universal plane testing.
+ *   - Black primary plane via traditional interfaces, red sprite, grab CRC:1.
+ *   - Blue primary plane via traditional interfaces, red sprite, grab CRC:2.
+ *   - Yellow primary via traditional interfaces
+ *   - Blue primary plane, red sprite via universal planes, grab CRC:3 and compare
+ *     with CRC:2 (should be the same)
+ *   - Disable primary plane, grab CRC:4 (should be same as CRC:1)
+ *   - Reenable primary, grab CRC:5 (should be same as CRC:2 and CRC:3)
+ */
+
+typedef struct {
+	data_t *data;
+	igt_pipe_crc_t *pipe_crc;
+	igt_crc_t crc_1, crc_2, crc_3, crc_4, crc_5;
+	struct igt_fb red_fb, blue_fb, black_fb, yellow_fb;
+} test_t;
+
+static void
+test_init(test_t *test, igt_output_t *output, enum pipe pipe)
+{
+	data_t *data = test->data;
+	drmModeModeInfo *mode;
+
+	test->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+
+	igt_output_set_pipe(output, pipe);
+
+	mode = igt_output_get_mode(output);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+				DRM_FORMAT_XRGB8888,
+				false, /* tiled */
+				0.0, 0.0, 0.0,
+				&test->black_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+				DRM_FORMAT_XRGB8888,
+				false, /* tiled */
+				0.0, 0.0, 1.0,
+				&test->blue_fb);
+	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+				DRM_FORMAT_XRGB8888,
+				false, /* tiled */
+				1.0, 1.0, 0.0,
+				&test->yellow_fb);
+	igt_create_color_fb(data->drm_fd, 100, 100,
+				DRM_FORMAT_XRGB8888,
+				false, /* tiled */
+				1.0, 0.0, 0.0,
+				&test->red_fb);
+}
+
+static void
+test_fini(test_t *test, igt_output_t *output)
+{
+	igt_pipe_crc_free(test->pipe_crc);
+
+	igt_remove_fb(test->data->drm_fd, &test->black_fb);
+	igt_remove_fb(test->data->drm_fd, &test->blue_fb);
+	igt_remove_fb(test->data->drm_fd, &test->red_fb);
+	igt_remove_fb(test->data->drm_fd, &test->yellow_fb);
+
+	igt_display_use_universal_commits(&test->data->display, false);
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(&test->data->display);
+}
+
+static void
+test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	test_t test = { .data = data };
+	igt_plane_t *primary, *sprite;
+
+	igt_skip_on(pipe >= data->display.n_pipes);
+
+	fprintf(stdout, "Testing connector %s using pipe %c\n",
+		igt_output_name(output), pipe_name(pipe));
+
+	test_init(&test, output, pipe);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+	sprite = igt_output_get_plane(output, IGT_PLANE_2);
+	if (!sprite) {
+		test_fini(&test, output);
+		igt_skip("No sprite plane available\n");
+	}
+
+	igt_plane_set_position(sprite, 100, 100);
+
+	/* Step 1: Legacy API's, black primary, red sprite (CRC 1) */
+	igt_plane_set_fb(primary, &test.black_fb);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_1);
+
+	/* Step 2: Legacy API', blue primary, red sprite (CRC 2) */
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_2);
+
+	/* Step 3: Legacy API's, yellow primary */
+	igt_plane_set_fb(primary, &test.yellow_fb);
+	igt_display_commit(&data->display);
+
+	/* Step 4: Universal API's, blue primary, red sprite (CRC 3) */
+	igt_display_use_universal_commits(&test.data->display, true);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_3);
+
+	/* Step 5: Universal API's, disable primary plane (CRC 4) */
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_4);
+
+	/* Step 6: Universal API's, re-enable primary with blue (CRC 5) */
+	igt_display_use_universal_commits(&test.data->display, true);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_display_commit(&data->display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_5);
+
+	/* Blue bg + red sprite should be same under both types of API's */
+	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_3));
+
+	/* Disabling primary plane should be same as black primary */
+	igt_assert(igt_crc_equal(&test.crc_1, &test.crc_4));
+
+	/* Re-enabling primary should return to blue properly */
+	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_5));
+
+	igt_plane_set_fb(primary, NULL);
+	igt_plane_set_fb(sprite, NULL);
+
+	test_fini(&test, output);
+}
+
+static void
+run_tests_for_pipe(data_t *data, enum pipe pipe)
+{
+	igt_output_t *output;
+
+	igt_assert(data->display.has_universal_planes);
+
+	igt_subtest_f("universal-plane-pipe-%c", pipe_name(pipe))
+		for_each_connected_output(&data->display, output)
+			test_pipe(data, pipe, output);
+}
+
+static data_t data;
+
+igt_main
+{
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.drm_fd = drm_open_any();
+
+		igt_set_vt_graphics_mode();
+
+		igt_require_pipe_crc();
+		igt_display_init(&data.display, data.drm_fd);
+
+		igt_require(data.display.has_universal_planes);
+	}
+
+	for (int pipe = 0; pipe < 3; pipe++)
+		run_tests_for_pipe(&data, pipe);
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing
  2014-04-11  0:26   ` [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing Matt Roper
@ 2014-04-11  9:22     ` Daniel Vetter
  2014-04-11 11:57       ` Daniel Vetter
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2014-04-11  9:22 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Thu, Apr 10, 2014 at 05:26:25PM -0700, Matt Roper wrote:
> Add a simple test to exercise universal plane support.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Looks like a good functional test, but I think we need to add a bit more
api nastiness still. So no functional tests with CRCs, but just tests to
make sure the kernel doesn't fall over.

- primary plane set_plane calls vs. legacy setcrtc primary plane updates.
  We'll very likely have mixed userspace (e.g. boot splash vs. display
  manager). E.g. disable primary plane (but keep everything working), then
  setCrtc a new plane framebuffer.

- primary plane vs. other ioctls. Might be easier to extend existing tests
  for this. E.g. doing a pageflip ioctl if the primary plane is off
  (might need to decide what we really want to do and if we decide that it
  should enable the primary plane then we need a CRC based test to make
  sure that the transition is perfect).

  Or primary plane changes vs. dpms and suspend/resume. For those
  functional checks based on CRC would be good to make sure we properly
  restore everything.

- Maybe exercise some of the checks in the primary plane helper to make
  sure they work. In the future we'll probably lift those limitations (not
  on current hw afaik though), but then we can adjust those tests to skip
  on these platforms.

- Anything else that was pointed out in review or was tricky while
  developing this stuff.

Cheers, Daniel
> ---
>  tests/Makefile.sources      |   1 +
>  tests/kms_universal_plane.c | 211 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 212 insertions(+)
>  create mode 100644 tests/kms_universal_plane.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index bf02a48..4911914 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -63,6 +63,7 @@ TESTS_progs_M = \
>  	kms_plane \
>  	kms_render \
>  	kms_setmode \
> +	kms_universal_plane \
>  	pm_lpsp \
>  	pm_pc8 \
>  	pm_rps \
> diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> new file mode 100644
> index 0000000..f1ec6fb
> --- /dev/null
> +++ b/tests/kms_universal_plane.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "drmtest.h"
> +#include "igt_debugfs.h"
> +#include "igt_kms.h"
> +
> +typedef struct {
> +	int drm_fd;
> +	igt_display_t display;
> +} data_t;
> +
> +/*
> + * Universal plane testing.
> + *   - Black primary plane via traditional interfaces, red sprite, grab CRC:1.
> + *   - Blue primary plane via traditional interfaces, red sprite, grab CRC:2.
> + *   - Yellow primary via traditional interfaces
> + *   - Blue primary plane, red sprite via universal planes, grab CRC:3 and compare
> + *     with CRC:2 (should be the same)
> + *   - Disable primary plane, grab CRC:4 (should be same as CRC:1)
> + *   - Reenable primary, grab CRC:5 (should be same as CRC:2 and CRC:3)
> + */
> +
> +typedef struct {
> +	data_t *data;
> +	igt_pipe_crc_t *pipe_crc;
> +	igt_crc_t crc_1, crc_2, crc_3, crc_4, crc_5;
> +	struct igt_fb red_fb, blue_fb, black_fb, yellow_fb;
> +} test_t;
> +
> +static void
> +test_init(test_t *test, igt_output_t *output, enum pipe pipe)
> +{
> +	data_t *data = test->data;
> +	drmModeModeInfo *mode;
> +
> +	test->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
> +
> +	igt_output_set_pipe(output, pipe);
> +
> +	mode = igt_output_get_mode(output);
> +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +				DRM_FORMAT_XRGB8888,
> +				false, /* tiled */
> +				0.0, 0.0, 0.0,
> +				&test->black_fb);
> +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +				DRM_FORMAT_XRGB8888,
> +				false, /* tiled */
> +				0.0, 0.0, 1.0,
> +				&test->blue_fb);
> +	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +				DRM_FORMAT_XRGB8888,
> +				false, /* tiled */
> +				1.0, 1.0, 0.0,
> +				&test->yellow_fb);
> +	igt_create_color_fb(data->drm_fd, 100, 100,
> +				DRM_FORMAT_XRGB8888,
> +				false, /* tiled */
> +				1.0, 0.0, 0.0,
> +				&test->red_fb);
> +}
> +
> +static void
> +test_fini(test_t *test, igt_output_t *output)
> +{
> +	igt_pipe_crc_free(test->pipe_crc);
> +
> +	igt_remove_fb(test->data->drm_fd, &test->black_fb);
> +	igt_remove_fb(test->data->drm_fd, &test->blue_fb);
> +	igt_remove_fb(test->data->drm_fd, &test->red_fb);
> +	igt_remove_fb(test->data->drm_fd, &test->yellow_fb);
> +
> +	igt_display_use_universal_commits(&test->data->display, false);
> +	igt_output_set_pipe(output, PIPE_ANY);
> +	igt_display_commit(&test->data->display);
> +}
> +
> +static void
> +test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> +{
> +	test_t test = { .data = data };
> +	igt_plane_t *primary, *sprite;
> +
> +	igt_skip_on(pipe >= data->display.n_pipes);
> +
> +	fprintf(stdout, "Testing connector %s using pipe %c\n",
> +		igt_output_name(output), pipe_name(pipe));
> +
> +	test_init(&test, output, pipe);
> +
> +	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
> +	sprite = igt_output_get_plane(output, IGT_PLANE_2);
> +	if (!sprite) {
> +		test_fini(&test, output);
> +		igt_skip("No sprite plane available\n");
> +	}
> +
> +	igt_plane_set_position(sprite, 100, 100);
> +
> +	/* Step 1: Legacy API's, black primary, red sprite (CRC 1) */
> +	igt_plane_set_fb(primary, &test.black_fb);
> +	igt_plane_set_fb(sprite, &test.red_fb);
> +	igt_display_commit(&data->display);
> +	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_1);
> +
> +	/* Step 2: Legacy API', blue primary, red sprite (CRC 2) */
> +	igt_plane_set_fb(primary, &test.blue_fb);
> +	igt_plane_set_fb(sprite, &test.red_fb);
> +	igt_display_commit(&data->display);
> +	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_2);
> +
> +	/* Step 3: Legacy API's, yellow primary */
> +	igt_plane_set_fb(primary, &test.yellow_fb);
> +	igt_display_commit(&data->display);
> +
> +	/* Step 4: Universal API's, blue primary, red sprite (CRC 3) */
> +	igt_display_use_universal_commits(&test.data->display, true);
> +	igt_plane_set_fb(primary, &test.blue_fb);
> +	igt_plane_set_fb(sprite, &test.red_fb);
> +	igt_display_commit(&data->display);
> +	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_3);
> +
> +	/* Step 5: Universal API's, disable primary plane (CRC 4) */
> +	igt_plane_set_fb(primary, NULL);
> +	igt_display_commit(&data->display);
> +	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_4);
> +
> +	/* Step 6: Universal API's, re-enable primary with blue (CRC 5) */
> +	igt_display_use_universal_commits(&test.data->display, true);
> +	igt_plane_set_fb(primary, &test.blue_fb);
> +	igt_display_commit(&data->display);
> +	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_5);
> +
> +	/* Blue bg + red sprite should be same under both types of API's */
> +	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_3));
> +
> +	/* Disabling primary plane should be same as black primary */
> +	igt_assert(igt_crc_equal(&test.crc_1, &test.crc_4));
> +
> +	/* Re-enabling primary should return to blue properly */
> +	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_5));
> +
> +	igt_plane_set_fb(primary, NULL);
> +	igt_plane_set_fb(sprite, NULL);
> +
> +	test_fini(&test, output);
> +}
> +
> +static void
> +run_tests_for_pipe(data_t *data, enum pipe pipe)
> +{
> +	igt_output_t *output;
> +
> +	igt_assert(data->display.has_universal_planes);
> +
> +	igt_subtest_f("universal-plane-pipe-%c", pipe_name(pipe))
> +		for_each_connected_output(&data->display, output)
> +			test_pipe(data, pipe, output);
> +}
> +
> +static data_t data;
> +
> +igt_main
> +{
> +
> +	igt_skip_on_simulation();
> +
> +	igt_fixture {
> +		data.drm_fd = drm_open_any();
> +
> +		igt_set_vt_graphics_mode();
> +
> +		igt_require_pipe_crc();
> +		igt_display_init(&data.display, data.drm_fd);
> +
> +		igt_require(data.display.has_universal_planes);
> +	}
> +
> +	for (int pipe = 0; pipe < 3; pipe++)
> +		run_tests_for_pipe(&data, pipe);
> +
> +	igt_fixture {
> +		igt_display_fini(&data.display);
> +	}
> +}
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v2)
  2014-04-11  0:24 [PATCH] drm/i915: Intel-specific primary plane handling (v2) Matt Roper
  2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
@ 2014-04-11  9:34 ` Daniel Vetter
  2014-04-11 14:17   ` Matt Roper
  2014-04-11 17:41   ` Matt Roper
  2014-04-11 20:44 ` [PATCH] drm/i915: Intel-specific primary plane handling (v3) Matt Roper
  2 siblings, 2 replies; 18+ messages in thread
From: Daniel Vetter @ 2014-04-11  9:34 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote:
> Intel hardware allows the primary plane to be disabled independently of
> the CRTC.  Provide custom primary plane handling to allow this.
> 
> v2:
>  - Unpin fb properly on primary plane disable
>  - Provide an Intel-specific set of primary plane formats
>  - Additional sanity checks on setplane (in line with the checks
>    currently being done by the DRM core primary plane helper)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> 
> This patch was previously part of my universal plane series on dri-devel
> (http://lists.freedesktop.org/archives/dri-devel/2014-March/055852.html) but I
> dropped it in v4 and v5 of that series to focus on the DRM core changes.  Now
> that universal planes have been merged, we can start building on that framework
> to provide i915-specific functionality.
> 
> Some i-g-t changes to test this will be sent shortly.
> 
> 
> Matt
> 
> 
>  drivers/gpu/drm/i915/intel_display.c | 165 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
>  2 files changed, 164 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1af1d14..a0bc251 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -39,8 +39,24 @@
>  #include "i915_trace.h"
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  
> +const static uint32_t intel_primary_formats[] = {
> +       DRM_FORMAT_C8,
> +       DRM_FORMAT_XRGB1555,
> +       DRM_FORMAT_ARGB1555,
> +       DRM_FORMAT_RGB565,
> +       DRM_FORMAT_XRGB8888,
> +       DRM_FORMAT_ARGB8888,
> +       DRM_FORMAT_XBGR8888,
> +       DRM_FORMAT_ABGR8888,
> +       DRM_FORMAT_XRGB2101010,
> +       DRM_FORMAT_ARGB2101010,
> +       DRM_FORMAT_XBGR2101010,
> +       DRM_FORMAT_ABGR2101010,
> +};

I think going the extra step and having correct per-gen format lists would
be good. See intel_framebuffer_init for which platfrom can do what.

Unfortunately we can't yet change those checks to WARNs since the core
doesn't yet check the primary plane format against this list ... And we
can't do that yet since not all drivers are converted yet to have at least
an explicit format list :(

> +
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> @@ -10553,17 +10569,162 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_setplane(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 drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_framebuffer *tmpfb;
> +	struct drm_rect dest = {
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect clip = {
> +		.x2 = crtc->mode.hdisplay,
> +		.y2 = crtc->mode.vdisplay,
> +	};
> +	int ret;
> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	/*
> +	 * Current hardware can't reposition the primary plane or scale it
> +	 * (although this could change in the future).
> +	 */
> +	drm_rect_intersect(&dest, &clip);
> +	if (dest.x1 != 0 || dest.y1 != 0 ||
> +	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
> +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	if (crtc_w != src_w || crtc_h != src_h) {
> +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> +		return -EINVAL;
> +	}

Subpixel check seems to be missing. And can't we extract all these checks
both here and from the primary plane helper? I guess there'll be other hw
which doesn't have scaling primary planes, but which wants to allow
primary plane enable/disable.

> +
> +	/* Primary planes are locked to their owning CRTC */
> +	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
> +		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Framebuffer must be big enough to cover entire plane */
> +	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
> +	 * code that called us expects to handle the framebuffer update and
> +	 * reference counting; save and restore the current fb before
> +	 * calling it.
> +	 */
> +	tmpfb = plane->fb;
> +	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
> +	if (ret)
> +		return ret;
> +	plane->fb = tmpfb;
> +
> +	if (!intel_crtc->primary_enabled)
> +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +					      intel_crtc->pipe);

Hm, I've thought we could do a simple

	if (intel_crtc->primary_enabled)
		call_primary_plane_helper
	else
		enable_the_hw_plane

But we need to do all the arg checking for the !primary_enabled case :(
Anyway more code sharing make me happier.

Cheers, Daniel

> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	if (WARN_ON(!plane->crtc))
> +		return -EINVAL;
> +
> +	intel_crtc = to_intel_crtc(plane->crtc);
> +	if (intel_crtc->primary_enabled)
> +		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +					       intel_plane->pipe);
> +
> +	/*
> +	 * N.B. The DRM setplane code will update the plane->fb pointer after
> +	 * we finish here.
> +	 */
> +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	intel_primary_plane_disable(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> +						    int pipe)
> +{
> +	struct intel_plane *primary;
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		return NULL;
> +
> +	primary->can_scale = false;
> +	primary->pipe = pipe;
> +	primary->plane = pipe;
> +
> +	drm_plane_init(dev, &primary->base, 0,
> +		       &intel_primary_plane_funcs, intel_primary_formats,
> +		       ARRAY_SIZE(intel_primary_formats),
> +		       DRM_PLANE_TYPE_PRIMARY);
> +	return &primary->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	primary = intel_primary_plane_create(dev, pipe);
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> +					NULL, &intel_crtc_funcs);
> +	if (ret) {
> +		drm_crtc_cleanup(&intel_crtc->base);
> +		kfree(intel_crtc);
> +		return;
> +	}
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c551472..2ce9cc3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -363,6 +363,7 @@ struct intel_crtc {
>  	bool active;
>  	unsigned long enabled_power_domains;
>  	bool eld_vld;
> +	struct intel_plane *primary_plane;
>  	bool primary_enabled; /* is the primary plane (partially) visible? */
>  	bool lowfreq_avail;
>  	struct intel_overlay *overlay;
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing
  2014-04-11  9:22     ` Daniel Vetter
@ 2014-04-11 11:57       ` Daniel Vetter
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2014-04-11 11:57 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Apr 11, 2014 at 11:22:39AM +0200, Daniel Vetter wrote:
> On Thu, Apr 10, 2014 at 05:26:25PM -0700, Matt Roper wrote:
> > Add a simple test to exercise universal plane support.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Looks like a good functional test, but I think we need to add a bit more
> api nastiness still. So no functional tests with CRCs, but just tests to
> make sure the kernel doesn't fall over.
> 
> - primary plane set_plane calls vs. legacy setcrtc primary plane updates.
>   We'll very likely have mixed userspace (e.g. boot splash vs. display
>   manager). E.g. disable primary plane (but keep everything working), then
>   setCrtc a new plane framebuffer.
> 
> - primary plane vs. other ioctls. Might be easier to extend existing tests
>   for this. E.g. doing a pageflip ioctl if the primary plane is off
>   (might need to decide what we really want to do and if we decide that it
>   should enable the primary plane then we need a CRC based test to make
>   sure that the transition is perfect).
> 
>   Or primary plane changes vs. dpms and suspend/resume. For those
>   functional checks based on CRC would be good to make sure we properly
>   restore everything.
> 
> - Maybe exercise some of the checks in the primary plane helper to make
>   sure they work. In the future we'll probably lift those limitations (not
>   on current hw afaik though), but then we can adjust those tests to skip
>   on these platforms.
> 
> - Anything else that was pointed out in review or was tricky while
>   developing this stuff.

More ideas! My main concern is interactions with existing features when
the primary plane is disabled and no other plane/cursor enabled, i.e. when
we scan out a solid black.

- solid black vs. dpms for all connector/pipe pairings. The modeset
  sequence is different for different connectors and my gut says not
  enabling the primary plane might cause trouble.

- solid black vs. vblank events. When transition through a "no planes"
  situation we need to make sure that vblank events keep on working. A
  testcase (maybe in combination of some of the dpms and susped/resume
  case above too) would be good.

I'll leave it to you to somewhat sensibly group all these ideas into real
testcases ;-) For further inspiration maybe look at the corner cases
existing kms_* tests exercise a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v2)
  2014-04-11  9:34 ` [PATCH] drm/i915: Intel-specific primary plane handling (v2) Daniel Vetter
@ 2014-04-11 14:17   ` Matt Roper
  2014-04-11 14:23     ` Daniel Vetter
  2014-04-11 17:41   ` Matt Roper
  1 sibling, 1 reply; 18+ messages in thread
From: Matt Roper @ 2014-04-11 14:17 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote:
> On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote:
...
> > +	/* setplane API takes shifted source rectangle values; unshift them */
> > +	src_x >>= 16;
> > +	src_y >>= 16;
> > +	src_w >>= 16;
> > +	src_h >>= 16;
> > +
> > +	/*
> > +	 * Current hardware can't reposition the primary plane or scale it
> > +	 * (although this could change in the future).
> > +	 */
> > +	drm_rect_intersect(&dest, &clip);
> > +	if (dest.x1 != 0 || dest.y1 != 0 ||
> > +	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
> > +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (crtc_w != src_w || crtc_h != src_h) {
> > +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> > +		return -EINVAL;
> > +	}
> 
> Subpixel check seems to be missing. And can't we extract all these checks
> both here and from the primary plane helper? I guess there'll be other hw
> which doesn't have scaling primary planes, but which wants to allow
> primary plane enable/disable.

I was a bit unsure about this.  At first I thought I needed to check the
subpixel part, but the DocBook reference indicates

        Devices that don't support subpixel plane coordinates can ignore
        the fractional part.

which sounds to me like we're supposed to just silently ignore the
subpixel bits on i915 and other devices that don't support it.  Which
would probably also mean that I should remove the (subpixel bits == 0)
test from the primary helper...


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v2)
  2014-04-11 14:17   ` Matt Roper
@ 2014-04-11 14:23     ` Daniel Vetter
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2014-04-11 14:23 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Apr 11, 2014 at 07:17:41AM -0700, Matt Roper wrote:
> On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote:
> > On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote:
> ...
> > > +	/* setplane API takes shifted source rectangle values; unshift them */
> > > +	src_x >>= 16;
> > > +	src_y >>= 16;
> > > +	src_w >>= 16;
> > > +	src_h >>= 16;
> > > +
> > > +	/*
> > > +	 * Current hardware can't reposition the primary plane or scale it
> > > +	 * (although this could change in the future).
> > > +	 */
> > > +	drm_rect_intersect(&dest, &clip);
> > > +	if (dest.x1 != 0 || dest.y1 != 0 ||
> > > +	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
> > > +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (crtc_w != src_w || crtc_h != src_h) {
> > > +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> > > +		return -EINVAL;
> > > +	}
> > 
> > Subpixel check seems to be missing. And can't we extract all these checks
> > both here and from the primary plane helper? I guess there'll be other hw
> > which doesn't have scaling primary planes, but which wants to allow
> > primary plane enable/disable.
> 
> I was a bit unsure about this.  At first I thought I needed to check the
> subpixel part, but the DocBook reference indicates
> 
>         Devices that don't support subpixel plane coordinates can ignore
>         the fractional part.
> 
> which sounds to me like we're supposed to just silently ignore the
> subpixel bits on i915 and other devices that don't support it.  Which
> would probably also mean that I should remove the (subpixel bits == 0)
> test from the primary helper...

Hm ... yeah I guess you're right. For now it probably won't matter too
much.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v2)
  2014-04-11  9:34 ` [PATCH] drm/i915: Intel-specific primary plane handling (v2) Daniel Vetter
  2014-04-11 14:17   ` Matt Roper
@ 2014-04-11 17:41   ` Matt Roper
  2014-04-11 18:27     ` Daniel Vetter
  1 sibling, 1 reply; 18+ messages in thread
From: Matt Roper @ 2014-04-11 17:41 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote:
> On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote:
...
> 
> Hm, I've thought we could do a simple
> 
> 	if (intel_crtc->primary_enabled)
> 		call_primary_plane_helper
> 	else
> 		enable_the_hw_plane
> 
> But we need to do all the arg checking for the !primary_enabled case :(
> Anyway more code sharing make me happier.
> 
> Cheers, Daniel

I think the problem here is that the helper has a bunch of tests
targetted at the lowest common denominator hardware.  Some of the things
it rejects are things that our hardware may begin to allow at some point
in the future (e.g., primary plane scaling, partial CRTC coverage of
primary plane, etc.).  We can probably call into the helper today and
get the behavior we want, but I'd expect that some of those restrictions
will need to be relaxed in the future and then we'll have to switch the
code back at that point.  Given that we still need to do all this
checking in the 'if (!enabled)' case, I don't think it's worth trying to
call through the helper for the 'if (enabled)' case (especially since
the actual "work" here after we're done testing is just a couple lines
of code)?


Matt


-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v2)
  2014-04-11 17:41   ` Matt Roper
@ 2014-04-11 18:27     ` Daniel Vetter
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2014-04-11 18:27 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Apr 11, 2014 at 10:41:56AM -0700, Matt Roper wrote:
> On Fri, Apr 11, 2014 at 11:34:36AM +0200, Daniel Vetter wrote:
> > On Thu, Apr 10, 2014 at 05:24:36PM -0700, Matt Roper wrote:
> ...
> > 
> > Hm, I've thought we could do a simple
> > 
> > 	if (intel_crtc->primary_enabled)
> > 		call_primary_plane_helper
> > 	else
> > 		enable_the_hw_plane
> > 
> > But we need to do all the arg checking for the !primary_enabled case :(
> > Anyway more code sharing make me happier.
> > 
> > Cheers, Daniel
> 
> I think the problem here is that the helper has a bunch of tests
> targetted at the lowest common denominator hardware.  Some of the things
> it rejects are things that our hardware may begin to allow at some point
> in the future (e.g., primary plane scaling, partial CRTC coverage of
> primary plane, etc.).  We can probably call into the helper today and
> get the behavior we want, but I'd expect that some of those restrictions
> will need to be relaxed in the future and then we'll have to switch the
> code back at that point.  Given that we still need to do all this
> checking in the 'if (!enabled)' case, I don't think it's worth trying to
> call through the helper for the 'if (enabled)' case (especially since
> the actual "work" here after we're done testing is just a couple lines
> of code)?

Well for that future I simply expect that we'll get a completely new
update_plane function. I agree that reusing the helper completely doesn't
work really, but sharing the tests would be nice imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] drm/i915: Intel-specific primary plane handling (v3)
  2014-04-11  0:24 [PATCH] drm/i915: Intel-specific primary plane handling (v2) Matt Roper
  2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
  2014-04-11  9:34 ` [PATCH] drm/i915: Intel-specific primary plane handling (v2) Daniel Vetter
@ 2014-04-11 20:44 ` Matt Roper
  2014-04-11 21:13   ` [PATCH] drm/i915: Intel-specific primary plane handling (v4) Matt Roper
  2 siblings, 1 reply; 18+ messages in thread
From: Matt Roper @ 2014-04-11 20:44 UTC (permalink / raw)
  To: intel-gfx

Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v3:
 - Provide gen-specific primary plane format lists (suggested by Daniel
   Vetter).
 - If the primary plane is already enabled, go ahead and just call the
   primary plane helper to do the update (suggested by Daniel Vetter).
 - Don't try to disable the primary plane on destruction; the DRM layer
   should have already taken care of this for us.
v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 197 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 2 files changed, 196 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..3e4d36a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,8 +39,35 @@
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+/* Primary plane formats supported by all gen */
+#define COMMON_PRIMARY_FORMATS \
+	DRM_FORMAT_C8, \
+	DRM_FORMAT_RGB565, \
+	DRM_FORMAT_XRGB8888, \
+	DRM_FORMAT_ARGB8888
+
+/* Primary plane formats for gen <= 3 */
+const static uint32_t intel_primary_formats_gen3[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+const static uint32_t intel_primary_formats_gen4[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+};
+
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
@@ -10556,17 +10583,183 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_setplane(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 drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_framebuffer *tmpfb;
+	struct drm_rect dest = {
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
+	int ret;
+
+	/*
+	 * At the moment we use the same set of setplane restrictions as the
+	 * DRM primary plane helper, so go ahead and just call the helper if
+	 * the primary plane is already enabled.  We only need to take special
+	 * action if the primary plane is disabled (something i915 can do but
+	 * the generic helper can't).
+	 */
+	if (intel_crtc->primary_enabled)
+		return drm_primary_helper_update(plane, crtc, fb,
+						 crtc_x, crtc_y,
+						 crtc_w, crtc_h,
+						 src_x, src_y,
+						 src_w, src_h);
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	/*
+	 * Current hardware can't reposition the primary plane or scale it
+	 * (although this could change in the future).
+	 */
+	drm_rect_intersect(&dest, &clip);
+	if (dest.x1 != 0 || dest.y1 != 0 ||
+	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
+		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	if (crtc_w != src_w || crtc_h != src_h) {
+		DRM_DEBUG_KMS("Can't scale primary plane\n");
+		return -EINVAL;
+	}
+
+	/* Primary planes are locked to their owning CRTC */
+	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
+		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
+		return -EINVAL;
+	}
+
+	/* Framebuffer must be big enough to cover entire plane */
+	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	if (ret)
+		return ret;
+
+	/*
+	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
+	 * code that called us expects to handle the framebuffer update and
+	 * reference counting; save and restore the current fb before
+	 * calling it.
+	 */
+	tmpfb = plane->fb;
+	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
+	if (ret)
+		return ret;
+	plane->fb = tmpfb;
+
+	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+				      intel_crtc->pipe);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc))
+		return -EINVAL;
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+	if (intel_crtc->primary_enabled)
+		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+					       intel_plane->pipe);
+
+	/*
+	 * N.B. The DRM setplane code will update the plane->fb pointer after
+	 * we finish here.
+	 */
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+	const uint32_t *intel_primary_formats;
+	int num_formats;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen3;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_plane_init(dev, &primary->base, 0,
+		       &intel_primary_plane_funcs, intel_primary_formats,
+		       num_formats, DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_crtc_cleanup(&intel_crtc->base);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c551472..2ce9cc3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -363,6 +363,7 @@ struct intel_crtc {
 	bool active;
 	unsigned long enabled_power_domains;
 	bool eld_vld;
+	struct intel_plane *primary_plane;
 	bool primary_enabled; /* is the primary plane (partially) visible? */
 	bool lowfreq_avail;
 	struct intel_overlay *overlay;
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH] drm/i915: Intel-specific primary plane handling (v4)
  2014-04-11 20:44 ` [PATCH] drm/i915: Intel-specific primary plane handling (v3) Matt Roper
@ 2014-04-11 21:13   ` Matt Roper
  2014-04-22 12:47     ` Ville Syrjälä
  0 siblings, 1 reply; 18+ messages in thread
From: Matt Roper @ 2014-04-11 21:13 UTC (permalink / raw)
  To: intel-gfx

Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v4:
 - Don't add a primary_plane field to intel_crtc; that was left over
   from a much earlier iteration of this patch series, but is no longer
   needed/used now that the DRM core primary plane support has been
   merged.
v3:
 - Provide gen-specific primary plane format lists (suggested by Daniel
   Vetter).
 - If the primary plane is already enabled, go ahead and just call the
   primary plane helper to do the update (suggested by Daniel Vetter).
 - Don't try to disable the primary plane on destruction; the DRM layer
   should have already taken care of this for us.
v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 197 ++++++++++++++++++++++++++++++++++-
 1 file changed, 195 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..3e4d36a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,8 +39,35 @@
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+/* Primary plane formats supported by all gen */
+#define COMMON_PRIMARY_FORMATS \
+	DRM_FORMAT_C8, \
+	DRM_FORMAT_RGB565, \
+	DRM_FORMAT_XRGB8888, \
+	DRM_FORMAT_ARGB8888
+
+/* Primary plane formats for gen <= 3 */
+const static uint32_t intel_primary_formats_gen3[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+const static uint32_t intel_primary_formats_gen4[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+};
+
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
@@ -10556,17 +10583,183 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_setplane(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 drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_framebuffer *tmpfb;
+	struct drm_rect dest = {
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
+	int ret;
+
+	/*
+	 * At the moment we use the same set of setplane restrictions as the
+	 * DRM primary plane helper, so go ahead and just call the helper if
+	 * the primary plane is already enabled.  We only need to take special
+	 * action if the primary plane is disabled (something i915 can do but
+	 * the generic helper can't).
+	 */
+	if (intel_crtc->primary_enabled)
+		return drm_primary_helper_update(plane, crtc, fb,
+						 crtc_x, crtc_y,
+						 crtc_w, crtc_h,
+						 src_x, src_y,
+						 src_w, src_h);
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	/*
+	 * Current hardware can't reposition the primary plane or scale it
+	 * (although this could change in the future).
+	 */
+	drm_rect_intersect(&dest, &clip);
+	if (dest.x1 != 0 || dest.y1 != 0 ||
+	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
+		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	if (crtc_w != src_w || crtc_h != src_h) {
+		DRM_DEBUG_KMS("Can't scale primary plane\n");
+		return -EINVAL;
+	}
+
+	/* Primary planes are locked to their owning CRTC */
+	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
+		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
+		return -EINVAL;
+	}
+
+	/* Framebuffer must be big enough to cover entire plane */
+	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	if (ret)
+		return ret;
+
+	/*
+	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
+	 * code that called us expects to handle the framebuffer update and
+	 * reference counting; save and restore the current fb before
+	 * calling it.
+	 */
+	tmpfb = plane->fb;
+	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
+	if (ret)
+		return ret;
+	plane->fb = tmpfb;
+
+	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+				      intel_crtc->pipe);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc))
+		return -EINVAL;
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+	if (intel_crtc->primary_enabled)
+		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+					       intel_plane->pipe);
+
+	/*
+	 * N.B. The DRM setplane code will update the plane->fb pointer after
+	 * we finish here.
+	 */
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+	const uint32_t *intel_primary_formats;
+	int num_formats;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen3;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_plane_init(dev, &primary->base, 0,
+		       &intel_primary_plane_funcs, intel_primary_formats,
+		       num_formats, DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_crtc_cleanup(&intel_crtc->base);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v4)
  2014-04-11 21:13   ` [PATCH] drm/i915: Intel-specific primary plane handling (v4) Matt Roper
@ 2014-04-22 12:47     ` Ville Syrjälä
  2014-04-22 15:18       ` Matt Roper
  0 siblings, 1 reply; 18+ messages in thread
From: Ville Syrjälä @ 2014-04-22 12:47 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
> Intel hardware allows the primary plane to be disabled independently of
> the CRTC.  Provide custom primary plane handling to allow this.
> 
> v4:
>  - Don't add a primary_plane field to intel_crtc; that was left over
>    from a much earlier iteration of this patch series, but is no longer
>    needed/used now that the DRM core primary plane support has been
>    merged.
> v3:
>  - Provide gen-specific primary plane format lists (suggested by Daniel
>    Vetter).
>  - If the primary plane is already enabled, go ahead and just call the
>    primary plane helper to do the update (suggested by Daniel Vetter).
>  - Don't try to disable the primary plane on destruction; the DRM layer
>    should have already taken care of this for us.
> v2:
>  - Unpin fb properly on primary plane disable
>  - Provide an Intel-specific set of primary plane formats
>  - Additional sanity checks on setplane (in line with the checks
>    currently being done by the DRM core primary plane helper)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 197 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 195 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..3e4d36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -39,8 +39,35 @@
>  #include "i915_trace.h"
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  
> +/* Primary plane formats supported by all gen */
> +#define COMMON_PRIMARY_FORMATS \
> +	DRM_FORMAT_C8, \
> +	DRM_FORMAT_RGB565, \
> +	DRM_FORMAT_XRGB8888, \
> +	DRM_FORMAT_ARGB8888
> +
> +/* Primary plane formats for gen <= 3 */
> +const static uint32_t intel_primary_formats_gen3[] = {q

'static const' is the more typical order

> +	COMMON_PRIMARY_FORMATS,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ARGB1555,
> +};
> +
> +/* Primary plane formats for gen >= 4 */
> +const static uint32_t intel_primary_formats_gen4[] = {

ditto

> +	COMMON_PRIMARY_FORMATS, \
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_ABGR2101010,
> +};
> +
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> @@ -10556,17 +10583,183 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_setplane(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 drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_framebuffer *tmpfb;
> +	struct drm_rect dest = {
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect clip = {
> +		.x2 = crtc->mode.hdisplay,
> +		.y2 = crtc->mode.vdisplay,
> +	};

'clip' can be const, and these should be pipe_src_{w,h} just like
we have in the sprite code.

> +	int ret;
> +
> +	/*
> +	 * At the moment we use the same set of setplane restrictions as the
> +	 * DRM primary plane helper, so go ahead and just call the helper if
> +	 * the primary plane is already enabled.  We only need to take special
> +	 * action if the primary plane is disabled (something i915 can do but
> +	 * the generic helper can't).
> +	 */
> +	if (intel_crtc->primary_enabled)
> +		return drm_primary_helper_update(plane, crtc, fb,
> +						 crtc_x, crtc_y,
> +						 crtc_w, crtc_h,
> +						 src_x, src_y,
> +						 src_w, src_h);

Why would we want to call that if we have a custom implementation
anyway?

> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	/*
> +	 * Current hardware can't reposition the primary plane or scale it
> +	 * (although this could change in the future).
> +	 */
> +	drm_rect_intersect(&dest, &clip);

src needs to be clipped too. I guess we should get a sufficiently good
result if we just call 'drm_rect_clip_scaled(..., 1, 1)' here. Ie.
clip after throwing away the sub-pixel parts from the src coordinates.

To match the sprite behaviour a bit better we should also allow cases
where the primary plane becomes fully clipped. In such cases we should
just disable the plane.

> +	if (dest.x1 != 0 || dest.y1 != 0 ||
> +	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {

!drm_rect_equals(&dest, &clip)

> +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	if (crtc_w != src_w || crtc_h != src_h) {
> +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> +		return -EINVAL;
> +	}

This check could be moved to happen before we clip.

> +
> +	/* Primary planes are locked to their owning CRTC */
> +	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
> +		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
> +		return -EINVAL;
> +	}

Looks like possible_crtcs check could be in drm_mode_setplane(). No need
to special case primary planes here.

> +
> +	/* Framebuffer must be big enough to cover entire plane */
> +	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
> +	if (ret)
> +		return ret;

drm_mode_setplane() should already have checked that the viewport
doesn't exceed the fb.

> +
> +	/*
> +	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
> +	 * code that called us expects to handle the framebuffer update and
> +	 * reference counting; save and restore the current fb before
> +	 * calling it.
> +	 */
> +	tmpfb = plane->fb;
> +	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
> +	if (ret)
> +		return ret;
> +	plane->fb = tmpfb;
> +
> +	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +				      intel_crtc->pipe);

Needs a primary_enabled check (+ a visibility check for the fully
clipped case).

> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	if (WARN_ON(!plane->crtc))
> +		return -EINVAL;
> +
> +	intel_crtc = to_intel_crtc(plane->crtc);
> +	if (intel_crtc->primary_enabled)
> +		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +					       intel_plane->pipe);
> +
> +	/*
> +	 * N.B. The DRM setplane code will update the plane->fb pointer after
> +	 * we finish here.
> +	 */
> +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> +						    int pipe)
> +{
> +	struct intel_plane *primary;
> +	const uint32_t *intel_primary_formats;
> +	int num_formats;
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		return NULL;
> +
> +	primary->can_scale = false;
> +	primary->pipe = pipe;
> +	primary->plane = pipe;

We need to handle the pre-gen4 fbc primary plane swapping somewhere. I
guess we still have intel_crtc->plane as well. That should probably get
converted to use crtc->primary->plane instead.

> +
> +	if (INTEL_INFO(dev)->gen <= 3) {
> +		intel_primary_formats = intel_primary_formats_gen3;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> +	} else {
> +		intel_primary_formats = intel_primary_formats_gen4;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> +	}
> +
> +	drm_plane_init(dev, &primary->base, 0,
> +		       &intel_primary_plane_funcs, intel_primary_formats,
> +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> +	return &primary->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	primary = intel_primary_plane_create(dev, pipe);
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> +					NULL, &intel_crtc_funcs);
> +	if (ret) {
> +		drm_crtc_cleanup(&intel_crtc->base);

That should be drm_plane_cleanup() no?

> +		kfree(intel_crtc);
> +		return;
> +	}
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v4)
  2014-04-22 12:47     ` Ville Syrjälä
@ 2014-04-22 15:18       ` Matt Roper
  2014-04-22 17:14         ` Daniel Vetter
  0 siblings, 1 reply; 18+ messages in thread
From: Matt Roper @ 2014-04-22 15:18 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
...
> > +	int ret;
> > +
> > +	/*
> > +	 * At the moment we use the same set of setplane restrictions as the
> > +	 * DRM primary plane helper, so go ahead and just call the helper if
> > +	 * the primary plane is already enabled.  We only need to take special
> > +	 * action if the primary plane is disabled (something i915 can do but
> > +	 * the generic helper can't).
> > +	 */
> > +	if (intel_crtc->primary_enabled)
> > +		return drm_primary_helper_update(plane, crtc, fb,
> > +						 crtc_x, crtc_y,
> > +						 crtc_w, crtc_h,
> > +						 src_x, src_y,
> > +						 src_w, src_h);
> 
> Why would we want to call that if we have a custom implementation
> anyway?

This was something Daniel requested on a previous patch iteration; even
though we're stuck duplicating most of the checks here for the !enabled
case, he still wanted to see us call into the helper for the enabled
case (although this will have to change in the future if/when we want to
start relaxing some of the tests that the helper does, such as plane
scaling).

...
> > +	/*
> > +	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
> > +	 * code that called us expects to handle the framebuffer update and
> > +	 * reference counting; save and restore the current fb before
> > +	 * calling it.
> > +	 */
> > +	tmpfb = plane->fb;
> > +	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
> > +	if (ret)
> > +		return ret;
> > +	plane->fb = tmpfb;
> > +
> > +	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > +				      intel_crtc->pipe);
> 
> Needs a primary_enabled check (+ a visibility check for the fully
> clipped case).

If primary_enabled, we already called directly into the helper and
returned up at the top of the function.

I'll go ahead and add the visibility test farther up the function
though.

...
> > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > +						    int pipe)
> > +{
> > +	struct intel_plane *primary;
> > +	const uint32_t *intel_primary_formats;
> > +	int num_formats;
> > +
> > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > +	if (primary == NULL)
> > +		return NULL;
> > +
> > +	primary->can_scale = false;
> > +	primary->pipe = pipe;
> > +	primary->plane = pipe;
> 
> We need to handle the pre-gen4 fbc primary plane swapping somewhere. I
> guess we still have intel_crtc->plane as well. That should probably get
> converted to use crtc->primary->plane instead.

Hmm, this is something I'm not terribly familiar with.  I'm guessing I
just need to copy the logic from intel_crtc_init()?

I can write a Coccinelle patch to replace the intel_crtc->plane with
crtc->primary->plane as well; thanks for pointing that out.


Thanks for the review; I'll send along a new patch that incorporates
your other feedback shortly (and I think you've pointed out a few things
here that apply to the primary plane helper code too).  I'm still
reworking my i-g-t patches for this functionality as well, but I've been
traveling lately and haven't had too much time to work on it.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v4)
  2014-04-22 15:18       ` Matt Roper
@ 2014-04-22 17:14         ` Daniel Vetter
  2014-04-22 17:32           ` Matt Roper
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2014-04-22 17:14 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

On Tue, Apr 22, 2014 at 08:18:30AM -0700, Matt Roper wrote:
> On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
> ...
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * At the moment we use the same set of setplane restrictions as the
> > > +	 * DRM primary plane helper, so go ahead and just call the helper if
> > > +	 * the primary plane is already enabled.  We only need to take special
> > > +	 * action if the primary plane is disabled (something i915 can do but
> > > +	 * the generic helper can't).
> > > +	 */
> > > +	if (intel_crtc->primary_enabled)
> > > +		return drm_primary_helper_update(plane, crtc, fb,
> > > +						 crtc_x, crtc_y,
> > > +						 crtc_w, crtc_h,
> > > +						 src_x, src_y,
> > > +						 src_w, src_h);
> > 
> > Why would we want to call that if we have a custom implementation
> > anyway?
> 
> This was something Daniel requested on a previous patch iteration; even
> though we're stuck duplicating most of the checks here for the !enabled
> case, he still wanted to see us call into the helper for the enabled
> case (although this will have to change in the future if/when we want to
> start relaxing some of the tests that the helper does, such as plane
> scaling).

To clarify my request: I was unhappy with all the duplicated tests we have
and would like some way to share them with the plane helper code. If
there's no sane way to do that, then I'm ok with duplication.

I'm not sure any more what was the issue with extracting the tests from
the plane helper into a new function and reusing them with i915 though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] drm/i915: Intel-specific primary plane handling (v4)
  2014-04-22 17:14         ` Daniel Vetter
@ 2014-04-22 17:32           ` Matt Roper
  0 siblings, 0 replies; 18+ messages in thread
From: Matt Roper @ 2014-04-22 17:32 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, Apr 22, 2014 at 07:14:22PM +0200, Daniel Vetter wrote:
> On Tue, Apr 22, 2014 at 08:18:30AM -0700, Matt Roper wrote:
> > On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
> > ...
> > > > +	int ret;
> > > > +
> > > > +	/*
> > > > +	 * At the moment we use the same set of setplane restrictions as the
> > > > +	 * DRM primary plane helper, so go ahead and just call the helper if
> > > > +	 * the primary plane is already enabled.  We only need to take special
> > > > +	 * action if the primary plane is disabled (something i915 can do but
> > > > +	 * the generic helper can't).
> > > > +	 */
> > > > +	if (intel_crtc->primary_enabled)
> > > > +		return drm_primary_helper_update(plane, crtc, fb,
> > > > +						 crtc_x, crtc_y,
> > > > +						 crtc_w, crtc_h,
> > > > +						 src_x, src_y,
> > > > +						 src_w, src_h);
> > > 
> > > Why would we want to call that if we have a custom implementation
> > > anyway?
> > 
> > This was something Daniel requested on a previous patch iteration; even
> > though we're stuck duplicating most of the checks here for the !enabled
> > case, he still wanted to see us call into the helper for the enabled
> > case (although this will have to change in the future if/when we want to
> > start relaxing some of the tests that the helper does, such as plane
> > scaling).
> 
> To clarify my request: I was unhappy with all the duplicated tests we have
> and would like some way to share them with the plane helper code. If
> there's no sane way to do that, then I'm ok with duplication.
> 
> I'm not sure any more what was the issue with extracting the tests from
> the plane helper into a new function and reusing them with i915 though.
> -Daniel

Ah, okay.  I think I may have misunderstood what you were asking for the
previous time around.  Extracting the tests from the helper into a new
function should be doable; I'll include that in my next iteration.
Sorry for the confusion.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2014-04-22 17:33 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-11  0:24 [PATCH] drm/i915: Intel-specific primary plane handling (v2) Matt Roper
2014-04-11  0:26 ` [PATCH i-g-t 0/3] Universal plane testing Matt Roper
2014-04-11  0:26   ` [PATCH i-g-t 1/3] kms: Add universal plane support Matt Roper
2014-04-11  0:26   ` [PATCH i-g-t 2/3] kms_plane: Update for universal plane changes Matt Roper
2014-04-11  0:26   ` [PATCH i-g-t 3/3] kms_universal_plane: Universal plane testing Matt Roper
2014-04-11  9:22     ` Daniel Vetter
2014-04-11 11:57       ` Daniel Vetter
2014-04-11  9:34 ` [PATCH] drm/i915: Intel-specific primary plane handling (v2) Daniel Vetter
2014-04-11 14:17   ` Matt Roper
2014-04-11 14:23     ` Daniel Vetter
2014-04-11 17:41   ` Matt Roper
2014-04-11 18:27     ` Daniel Vetter
2014-04-11 20:44 ` [PATCH] drm/i915: Intel-specific primary plane handling (v3) Matt Roper
2014-04-11 21:13   ` [PATCH] drm/i915: Intel-specific primary plane handling (v4) Matt Roper
2014-04-22 12:47     ` Ville Syrjälä
2014-04-22 15:18       ` Matt Roper
2014-04-22 17:14         ` Daniel Vetter
2014-04-22 17:32           ` Matt Roper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.