[PATCH 0/4] Intel primary plane support (v7)

[PATCH 0/4] Intel primary plane support (v7)

[Matt Roper]

Re-posting the final versions of these patches now that they all have a r-b, as
requested by Daniel.  Corresponding i-g-t tests will be sent shortly.

Previous patch review thread and comments are here:
 http://lists.freedesktop.org/archives/intel-gfx/2014-April/044527.html

Matt Roper (4):
  drm: Check CRTC compatibility in setplane
  drm/plane-helper: Add drm_plane_helper_check_update() (v3)
  drm/i915: don't force full modeset if primary plane is disabled (v2)
  drm/i915: Intel-specific primary plane handling (v8)

 drivers/gpu/drm/drm_crtc.c           |   7 +
 drivers/gpu/drm/drm_plane_helper.c   | 130 +++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c | 255 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_plane_helper.h       |  22 +++
 4 files changed, 373 insertions(+), 41 deletions(-)

-- 
1.8.5.1

[PATCH 1/4] drm: Check CRTC compatibility in setplane

[Matt Roper]

The DRM core setplane code should check that the plane is usable on the
specified CRTC before calling into the driver.

Prior to this patch, a plane's possible_crtcs field was purely
informational for userspace and was never actually verified at the
kernel level (aside from the primary plane helper).

Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Chon Ming Lee <chon.ming.lee@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c         | 7 +++++++
 drivers/gpu/drm/drm_plane_helper.c | 6 ------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 37a3e07..1cfd1e3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2153,6 +2153,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	}
 	crtc = obj_to_crtc(obj);
 
+	/* Check whether this plane is usable on this CRTC */
+	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
+		DRM_DEBUG_KMS("Invalid crtc for plane\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
 	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
 	if (!fb) {
 		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index d966afa..b601233 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		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;
-	}
-
 	/* Disallow scaling */
 	src_w >>= 16;
 	src_h >>= 16;
-- 
1.8.5.1

[PATCH 2/4] drm/plane-helper: Add drm_plane_helper_check_update() (v3)

[Matt Roper]

Pull the parameter checking from drm_primary_helper_update() out into
its own function; drivers that provide their own setplane()
implementations rather than using the helper may still want to share
this parameter checking logic.

A few of the checks here were also updated based on suggestions by
Ville Syrjälä.

v3:
 - s/primary_helper/plane_helper/ --- this checking logic may be useful
   for other types of planes as well.
 - Fix visibility check (need to dereference visibility pointer)
v2:
 - Pass src/dest/clip rects and min/max scaling down to helper to avoid
   duplication of effort between helper and drivers (suggested by
   Ville).
 - Allow caller to specify whether the primary plane should be
   updatable while the crtc is disabled.

Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Chon Ming Lee <chon.ming.lee@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 124 ++++++++++++++++++++++++++++---------
 include/drm/drm_plane_helper.h     |  22 +++++++
 2 files changed, 116 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index b601233..fd401fc 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -66,6 +66,79 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc,
 }
 
 /**
+ * drm_plane_helper_check_update() - Check plane update for validity
+ * @plane: plane object to update
+ * @crtc: owning CRTC of owning plane
+ * @fb: framebuffer to flip onto plane
+ * @src: source coordinates in 16.16 fixed point
+ * @dest: integer destination coordinates
+ * @clip: integer clipping coordinates
+ * @min_scale: minimum @src:@dest scaling factor in 16.16 fixed point
+ * @max_scale: maximum @src:@dest scaling factor in 16.16 fixed point
+ * @can_position: is it legal to position the plane such that it
+ *                doesn't cover the entire crtc?  This will generally
+ *                only be false for primary planes.
+ * @can_update_disabled: can the plane be updated while the crtc
+ *                       is disabled?
+ * @visible: output parameter indicating whether plane is still visible after
+ *           clipping
+ *
+ * Checks that a desired plane update is valid.  Drivers that provide
+ * their own plane handling rather than helper-provided implementations may
+ * still wish to call this function to avoid duplication of error checking
+ * code.
+ *
+ * RETURNS:
+ * Zero if update appears valid, error code on failure
+ */
+int drm_plane_helper_check_update(struct drm_plane *plane,
+				    struct drm_crtc *crtc,
+				    struct drm_framebuffer *fb,
+				    struct drm_rect *src,
+				    struct drm_rect *dest,
+				    const struct drm_rect *clip,
+				    int min_scale,
+				    int max_scale,
+				    bool can_position,
+				    bool can_update_disabled,
+				    bool *visible)
+{
+	int hscale, vscale;
+
+	if (!crtc->enabled && !can_update_disabled) {
+		DRM_DEBUG_KMS("Cannot update plane of a disabled CRTC.\n");
+		return -EINVAL;
+	}
+
+	/* Check scaling */
+	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	if (hscale < 0 || vscale < 0) {
+		DRM_DEBUG_KMS("Invalid scaling of plane\n");
+		return -ERANGE;
+	}
+
+	*visible = drm_rect_clip_scaled(src, dest, clip, hscale, vscale);
+	if (!*visible)
+		/*
+		 * Plane isn't visible; some drivers can handle this
+		 * so we just return success here.  Drivers that can't
+		 * (including those that use the primary plane helper's
+		 * update function) will return an error from their
+		 * update_plane handler.
+		 */
+		return 0;
+
+	if (!can_position && !drm_rect_equals(dest, clip)) {
+		DRM_DEBUG_KMS("Plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_helper_check_update);
+
+/**
  * drm_primary_helper_update() - Helper for primary plane update
  * @plane: plane object to update
  * @crtc: owning CRTC of owning plane
@@ -113,51 +186,42 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x = src_x >> 16,
 		.y = src_y >> 16,
 	};
+	struct drm_rect src = {
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
 	struct drm_rect dest = {
 		.x1 = crtc_x,
 		.y1 = crtc_y,
 		.x2 = crtc_x + crtc_w,
 		.y2 = crtc_y + crtc_h,
 	};
-	struct drm_rect clip = {
+	const struct drm_rect clip = {
 		.x2 = crtc->mode.hdisplay,
 		.y2 = crtc->mode.vdisplay,
 	};
 	struct drm_connector **connector_list;
 	int num_connectors, ret;
+	bool visible;
 
-	if (!crtc->enabled) {
-		DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n");
-		return -EINVAL;
-	}
-
-	/* Disallow subpixel positioning */
-	if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) {
-		DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n");
-		return -EINVAL;
-	}
-
-	/* Disallow scaling */
-	src_w >>= 16;
-	src_h >>= 16;
-	if (crtc_w != src_w || crtc_h != src_h) {
-		DRM_DEBUG_KMS("Can't scale primary plane\n");
-		return -EINVAL;
-	}
-
-	/* Make sure primary plane covers entire CRTC */
-	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;
-	}
-
-	/* Framebuffer must be big enough to cover entire plane */
-	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, false, &visible);
 	if (ret)
 		return ret;
 
+	if (!visible)
+		/*
+		 * Primary plane isn't visible.  Note that unless a driver
+		 * provides their own disable function, this will just
+		 * wind up returning -EINVAL to userspace.
+		 */
+		return plane->funcs->disable_plane(plane);
+
 	/* Find current connectors for CRTC */
 	num_connectors = get_connectors_for_crtc(crtc, NULL, 0);
 	BUG_ON(num_connectors == 0);
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 09824be..a83a646 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -24,6 +24,17 @@
 #ifndef DRM_PLANE_HELPER_H
 #define DRM_PLANE_HELPER_H
 
+#include <drm/drm_rect.h>
+
+/*
+ * Drivers that don't allow primary plane scaling may pass this macro in place
+ * of the min/max scale parameters of the update checker function.
+ *
+ * Due to src being in 16.16 fixed point and dest being in integer pixels,
+ * 1<<16 represents no scaling.
+ */
+#define DRM_PLANE_HELPER_NO_SCALING (1<<16)
+
 /**
  * DOC: plane helpers
  *
@@ -31,6 +42,17 @@
  * planes.
  */
 
+extern int drm_plane_helper_check_update(struct drm_plane *plane,
+					 struct drm_crtc *crtc,
+					 struct drm_framebuffer *fb,
+					 struct drm_rect *src,
+					 struct drm_rect *dest,
+					 const struct drm_rect *clip,
+					 int min_scale,
+					 int max_scale,
+					 bool can_position,
+					 bool can_update_disabled,
+					 bool *visible);
 extern int drm_primary_helper_update(struct drm_plane *plane,
 				     struct drm_crtc *crtc,
 				     struct drm_framebuffer *fb,
-- 
1.8.5.1

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

[PATCH 3/4] drm/i915: don't force full modeset if primary plane is disabled (v2)

[Matt Roper]

In a future patch, we'll allow the primary plane to be disabled by
userspace via the universal plane API.  If a modeset is requested while
the primary plane is disabled, crtc->primary->fb will be NULL which
generally triggers a full modeset (except in fastboot situations).  If
we detect that the crtc is active, but there's no primary plane fb,
we should still allow a simple plane update rather than a full modeset
if the mode isn't actually changing (after re-enabling the primary plane
of course).

v2:
 - Enable plane after set_base to avoid enabling the plane if set_base
   fails, and to make flip+enable atomic (suggested by Ville)
 - Drop BUG to WARN if we somehow enter the 'fb_changed' modeset case
   with the crtc disabled (suggested by Ville)

Reviewed-by: Chon Ming Lee <chon.ming.lee@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 731cd01..c2aedac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10544,12 +10544,17 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 	if (is_crtc_connector_off(set)) {
 		config->mode_changed = true;
 	} else if (set->crtc->primary->fb != set->fb) {
-		/* If we have no fb then treat it as a full mode set */
+		/*
+		 * If we have no fb, we can only flip as long as the crtc is
+		 * active, otherwise we need a full mode set.  The crtc may
+		 * be active if we've only disabled the primary plane, or
+		 * in fastboot situations.
+		 */
 		if (set->crtc->primary->fb == NULL) {
 			struct intel_crtc *intel_crtc =
 				to_intel_crtc(set->crtc);
 
-			if (intel_crtc->active && i915.fastboot) {
+			if (intel_crtc->active) {
 				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
 				config->fb_changed = true;
 			} else {
@@ -10787,10 +10792,24 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
+
+		/*
+		 * We need to make sure the primary plane is re-enabled if it
+		 * has previously been turned off.
+		 */
+		if (!intel_crtc->primary_enabled && ret == 0) {
+			WARN_ON(!intel_crtc->active);
+			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+						      intel_crtc->pipe);
+		}
+
 		/*
 		 * In the fastboot case this may be our only check of the
 		 * state after boot.  It would be better to only do it on
-- 
1.8.5.1

[PATCH 4/4] drm/i915: Intel-specific primary plane handling (v8)

[Matt Roper]

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

v8:
 - Pin/unpin properly when clipping causes the primary plane to be
   disabled when it has previously been enabled.
 - s/drm_primary_helper_check_update/drm_plane_helper_check_update/
v7:
 - Clip primary plane to invisible when crtc is disabled since
   intel_crtc->config.pipe_src_{w,h} may be garbage otherwise.
 - Unpin old fb before pinning new one in the "just pin and
   return" case that is used when the crtc is disabled.
 - Don't treat implicit disabling of the primary plane (caused by
   clipping) the same way as explicit disabling (caused by fb=0).
   For implicit disables, we should leave the fb set and pinned,
   whereas for explicit disables we need to unpin the fb before
   primary->fb is cleared.
v6:
 - Pass rectangles to primary helper check function and get plane
   visibility back.
 - Wait for pending pageflips on primary plane update/disable.
 - Allow primary plane to be updated while the crtc is disabled (changes
   will take effect when the crtc is re-enabled if modeset passes -1
   for the fb id).
 - Drop WARN() if we try to disable the primary plane when it's
   already been disabled.  This will happen if the crtc gets disabled
   after the primary plane has already been disabled independently.
v5:
 - Use new drm_primary_helper_check_update() helper function to check
   setplane parameter validity.
 - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä)
 - Cleanup primary plane properly on crtc init failure
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)

Reviewed-by: Chon Ming Lee <chon.ming.lee@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 232 ++++++++++++++++++++++++++++++++++-
 1 file changed, 229 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c2aedac..0ddffac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,10 +39,37 @@
 #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 */
+static const uint32_t intel_primary_formats_gen2[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+static const 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,
+};
+
 #define DIV_ROUND_CLOSEST_ULL(ll, d)	\
-	({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
+({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
 
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
@@ -10959,17 +10986,216 @@ 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_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;
+
+	BUG_ON(!plane->crtc);
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+
+	/*
+	 * Even though we checked plane->fb above, it's still possible that
+	 * the primary plane has been implicitly disabled because the crtc
+	 * coordinates given weren't visible, or because we detected
+	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
+	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
+	 * In either case, we need to unpin the FB and let the fb pointer get
+	 * updated, but otherwise we don't need to touch the hardware.
+	 */
+	if (!intel_crtc->primary_enabled)
+		goto disable_unpin;
+
+	intel_crtc_wait_for_pending_flips(plane->crtc);
+	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+				       intel_plane->pipe);
+
+disable_unpin:
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+	plane->fb = NULL;
+
+	return 0;
+}
+
+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 intel_plane *intel_plane = to_intel_plane(plane);
+	struct drm_rect dest = {
+		/* integer pixels */
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect src = {
+		/* 16.16 fixed point */
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
+	const struct drm_rect clip = {
+		/* integer pixels */
+		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
+	};
+	bool visible;
+	int ret;
+
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    false, true, &visible);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
+	 * updating the fb pointer, and returning without touching the
+	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
+	 * turn on the display with all planes setup as desired.
+	 */
+	if (!crtc->enabled) {
+		/*
+		 * If we already called setplane while the crtc was disabled,
+		 * we may have an fb pinned; unpin it.
+		 */
+		if (plane->fb)
+			intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+		/* Pin and return without programming hardware */
+		return intel_pin_and_fence_fb_obj(dev,
+						  to_intel_framebuffer(fb)->obj,
+						  NULL);
+	}
+
+	intel_crtc_wait_for_pending_flips(crtc);
+
+	/*
+	 * If clipping results in a non-visible primary plane, we'll disable
+	 * the primary plane.  Note that this is a bit different than what
+	 * happens if userspace explicitly disables the plane by passing fb=0
+	 * because plane->fb still gets set and pinned.
+	 */
+	if (!visible) {
+		/*
+		 * Try to pin the new fb first so that we can bail out if we
+		 * fail.
+		 */
+		if (plane->fb != fb) {
+			ret = intel_pin_and_fence_fb_obj(dev,
+							 to_intel_framebuffer(fb)->obj,
+							 NULL);
+			if (ret)
+				return ret;
+		}
+
+		if (intel_crtc->primary_enabled)
+			intel_disable_primary_hw_plane(dev_priv,
+						       intel_plane->plane,
+						       intel_plane->pipe);
+
+
+		if (plane->fb != fb)
+			if (plane->fb)
+				intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+		return 0;
+	}
+
+	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
+	if (ret)
+		return ret;
+
+	if (!intel_crtc->primary_enabled)
+		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+					      intel_crtc->pipe);
+
+	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->max_downscale = 1;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
+		primary->plane = !pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen2;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen2);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_universal_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_plane_cleanup(primary);
+		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

[PATCH i-g-t 1/5] kms: Add universal plane support

[Matt Roper]

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 d00250d..97e329b 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -500,27 +500,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]);
@@ -531,21 +528,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);
@@ -700,16 +743,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)
@@ -761,6 +817,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);
@@ -820,7 +878,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 */
@@ -838,8 +900,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;
@@ -889,7 +951,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;
@@ -940,6 +1002,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 8e80d4b..4955fc8 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -121,6 +121,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];
@@ -143,6 +144,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 */
@@ -150,6 +153,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

[PATCH i-g-t 2/5] kms: Add igt_drm_plane_try_commit()

[Matt Roper]

For some of our tests, we want to make sure that bogus plane programming
attempts fail with the expected error code.  Add
igt_drm_plane_try_commit() that will just return the error code on
failure rather than failing the current subtest.  This lets us test the
return value against the expected error code.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c | 21 ++++++++++++++++++---
 lib/igt_kms.h |  1 +
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 97e329b..ce6ea87 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -810,7 +810,11 @@ static int igt_cursor_commit(igt_plane_t *plane, igt_output_t *output)
 	return 0;
 }
 
-static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
+/*
+ * Attempt to commit a plane; if the DRM call to program the plane fails,
+ * just return an error code (but don't fail the current subtest).
+ */
+int igt_drm_plane_try_commit(igt_plane_t *plane, igt_output_t *output)
 {
 	igt_display_t *display = output->display;
 	igt_pipe_t *pipe;
@@ -842,7 +846,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
 				      IGT_FIXED(0,0), /* src_w */
 				      IGT_FIXED(0,0) /* src_h */);
 
-		igt_assert(ret == 0);
+		if (ret)
+			return ret;
 
 		plane->fb_changed = false;
 	} else if (plane->fb_changed || plane->position_changed) {
@@ -866,7 +871,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
 				      IGT_FIXED(plane->fb->width,0), /* src_w */
 				      IGT_FIXED(plane->fb->height,0) /* src_h */);
 
-		igt_assert(ret == 0);
+		if (ret)
+			return ret;
 
 		plane->fb_changed = false;
 		plane->position_changed = false;
@@ -876,6 +882,15 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
 	return 0;
 }
 
+static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
+{
+	int ret = igt_drm_plane_try_commit(plane, output);
+
+	igt_assert(ret == 0);
+
+	return 0;
+}
+
 static int igt_plane_commit(igt_plane_t *plane, igt_output_t *output)
 {
 	struct igt_display *display = plane->pipe->display;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 4955fc8..2f72a1c 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -157,6 +157,7 @@ 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);
+int  igt_drm_plane_try_commit(igt_plane_t *plane, igt_output_t *output);
 
 const char *igt_output_name(igt_output_t *output);
 drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
-- 
1.8.5.1

Re: [PATCH i-g-t 2/5] kms: Add igt_drm_plane_try_commit()

[Damien Lespiau]

On Thu, May 29, 2014 at 08:09:14AM -0700, Matt Roper wrote:
> For some of our tests, we want to make sure that bogus plane programming
> attempts fail with the expected error code.  Add
> igt_drm_plane_try_commit() that will just return the error code on
> failure rather than failing the current subtest.  This lets us test the
> return value against the expected error code.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Huum, I was careful not to expose any of the sub-commit functions,
because of atomic modeset.

I don't think we want to expose the commit of a single plane here, so
it'd have to be igt_display_try_commit(). That should be equivalent for
your need but hopefully integrate with the atomic modeset ioctl() as
well.

-- 
Damien

> ---
>  lib/igt_kms.c | 21 ++++++++++++++++++---
>  lib/igt_kms.h |  1 +
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 97e329b..ce6ea87 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -810,7 +810,11 @@ static int igt_cursor_commit(igt_plane_t *plane, igt_output_t *output)
>  	return 0;
>  }
>  
> -static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
> +/*
> + * Attempt to commit a plane; if the DRM call to program the plane fails,
> + * just return an error code (but don't fail the current subtest).
> + */
> +int igt_drm_plane_try_commit(igt_plane_t *plane, igt_output_t *output)
>  {
>  	igt_display_t *display = output->display;
>  	igt_pipe_t *pipe;
> @@ -842,7 +846,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  				      IGT_FIXED(0,0), /* src_w */
>  				      IGT_FIXED(0,0) /* src_h */);
>  
> -		igt_assert(ret == 0);
> +		if (ret)
> +			return ret;
>  
>  		plane->fb_changed = false;
>  	} else if (plane->fb_changed || plane->position_changed) {
> @@ -866,7 +871,8 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  				      IGT_FIXED(plane->fb->width,0), /* src_w */
>  				      IGT_FIXED(plane->fb->height,0) /* src_h */);
>  
> -		igt_assert(ret == 0);
> +		if (ret)
> +			return ret;
>  
>  		plane->fb_changed = false;
>  		plane->position_changed = false;
> @@ -876,6 +882,15 @@ static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  	return 0;
>  }
>  
> +static int igt_drm_plane_commit(igt_plane_t *plane, igt_output_t *output)
> +{
> +	int ret = igt_drm_plane_try_commit(plane, output);
> +
> +	igt_assert(ret == 0);
> +
> +	return 0;
> +}
> +
>  static int igt_plane_commit(igt_plane_t *plane, igt_output_t *output)
>  {
>  	struct igt_display *display = plane->pipe->display;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 4955fc8..2f72a1c 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -157,6 +157,7 @@ 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);
> +int  igt_drm_plane_try_commit(igt_plane_t *plane, igt_output_t *output);
>  
>  const char *igt_output_name(igt_output_t *output);
>  drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 3/5] kms_plane: Update for universal plane changes

[Matt Roper]

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 5db0947..1d334ab 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

Re: [PATCH i-g-t 3/5] kms_plane: Update for universal plane changes

[Damien Lespiau]

On Thu, May 29, 2014 at 08:09:15AM -0700, Matt Roper wrote:
> 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>

I've hard time deciding if I'm happy with losing the ordering of planes.
I think I'm not :) Can we have the init function keep the plane
ordering?

-- 
Damien

> ---
>  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 5db0947..1d334ab 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 4/5] kms_universal_plane: Universal plane testing (v5)

[Matt Roper]

Add a simple test to exercise universal plane support.

v5:
 - Check that we don't have more than one primary or cursor.  This will
   catch accidental calls to drm_plane_init() in the kernel where
   drm_universal_plane_init() was intended (these don't cause a compile
   warning due to type compatibility between enum and bool).
v4:
 - Test disabling the primary plane explicitly when it has previously
   been implicitly disabled due to clipping.
 - Skip test if igt_pipe_crc_new() fails
v3:
 - For testing while crtc is off, switch between several different
   primary plane fb's before reenabling the crtc.  This will help
   catch pin/unpin mistakes.
v2:
 - Test that pageflips error out gracefully when the primary plane
   is disabled before the ioctl, or between ioctl and pageflip
   execution.
 - Test that nothing blows up if we try to disable the primary plane
   immediately after a pageflip (presumably before the pageflip actually
   completes).
 - Test that we can setup primary + sprite planes with the CRTC off and
   then have them show up properly when we enable the CRTC
   (drmModeSetCrtc with fb = -1).
 - Test that we can modeset properly after having disabled the primary
   plane
 - Test that proper error codes are returned for invalid plane
   programming attempts.

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

diff --git a/tests/.gitignore b/tests/.gitignore
index d7ad054..d1b01f7 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -124,6 +124,7 @@ kms_pipe_crc_basic
 kms_plane
 kms_render
 kms_setmode
+kms_universal_plane
 multi-tests.txt
 pm_lpsp
 pm_rpm
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index eca4af9..0132f8e 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -70,6 +70,7 @@ TESTS_progs_M = \
 	kms_plane \
 	kms_render \
 	kms_setmode \
+	kms_universal_plane \
 	pm_lpsp \
 	pm_rpm \
 	pm_rps \
diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
new file mode 100644
index 0000000..df48b50
--- /dev/null
+++ b/tests/kms_universal_plane.c
@@ -0,0 +1,598 @@
+/*
+ * 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;
+
+typedef struct {
+	data_t *data;
+	igt_pipe_crc_t *pipe_crc;
+	igt_crc_t crc_1, crc_2, crc_3, crc_4, crc_5, crc_6, crc_7, crc_8,
+		  crc_9, crc_10;
+	struct igt_fb red_fb, blue_fb, black_fb, yellow_fb;
+	drmModeModeInfo *mode;
+} functional_test_t;
+
+typedef struct {
+	data_t *data;
+	drmModeResPtr moderes;
+	struct igt_fb blue_fb, oversized_fb, undersized_fb;
+} sanity_test_t;
+
+typedef struct {
+	data_t *data;
+	struct igt_fb red_fb, blue_fb;
+} pageflip_test_t;
+
+static void
+functional_test_init(functional_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);
+	if (!test->pipe_crc)
+		igt_skip("auto crc not supported on this connector with pipe %i\n",
+		       pipe);
+
+
+	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);
+
+	test->mode = mode;
+}
+
+static void
+functional_test_fini(functional_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);
+}
+
+/*
+ * Universal plane functional 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)
+ *   - Yellow primary, no sprite
+ *   - Disable CRTC
+ *   - Program red sprite (while CRTC off)
+ *   - Program blue primary (while CRTC off)
+ *   - Enable CRTC, grab CRC:6 (should be same as CRC:2)
+ */
+static void
+functional_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	functional_test_t test = { .data = data };
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary, *sprite;
+	int ret;
+	int num_primary = 0, num_cursor = 0;
+	int i;
+
+	igt_skip_on(pipe >= display->n_pipes);
+
+	fprintf(stdout, "Testing connector %s using pipe %c\n",
+		igt_output_name(output), pipe_name(pipe));
+
+	functional_test_init(&test, output, pipe);
+
+	/*
+	 * Make sure we have no more than one primary or cursor plane per crtc.
+	 * If the kernel accidentally calls drm_plane_init() rather than
+	 * drm_universal_plane_init(), the type enum can get interpreted as a
+	 * boolean and show up in userspace as the wrong type.
+	 */
+	for (i = 0; i < display->pipes[pipe].n_planes; i++)
+		if (display->pipes[pipe].planes[i].is_primary)
+			num_primary++;
+		else if (display->pipes[pipe].planes[i].is_cursor)
+			num_cursor++;
+
+	igt_assert(num_primary == 1);
+	igt_assert(num_cursor <= 1);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+	sprite = igt_output_get_plane(output, IGT_PLANE_2);
+	if (!sprite) {
+		functional_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(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(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_2);
+
+	/* Step 3: Legacy API's, yellow primary (CRC 3) */
+	igt_plane_set_fb(primary, &test.yellow_fb);
+	igt_display_commit(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_3);
+
+	/* Step 4: Universal API's, blue primary, red sprite (CRC 4) */
+	igt_display_use_universal_commits(display, true);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	igt_display_commit(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_4);
+
+	/* Step 5: Universal API's, disable primary plane (CRC 5) */
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_5);
+
+	/* Step 6: Universal API's, re-enable primary with blue (CRC 6) */
+	igt_display_use_universal_commits(display, true);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_display_commit(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_6);
+
+	/* Step 7: Legacy API's, yellow primary, no sprite */
+	igt_display_use_universal_commits(display, false);
+	igt_plane_set_fb(primary, &test.yellow_fb);
+	igt_plane_set_fb(sprite, NULL);
+	igt_display_commit(display);
+
+	/* Step 8: Disable CRTC */
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(display);
+
+	/* Step 9: Universal API's with crtc off:
+	 *  - red sprite
+	 *  - multiple primary fb's, ending in blue
+	 */
+	igt_display_use_universal_commits(display, true);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	ret = igt_drm_plane_try_commit(sprite, output);
+	igt_assert(ret == 0);
+	igt_plane_set_fb(primary, &test.yellow_fb);
+	ret = igt_drm_plane_try_commit(primary, output);
+	igt_plane_set_fb(primary, &test.black_fb);
+	ret = igt_drm_plane_try_commit(primary, output);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	ret = igt_drm_plane_try_commit(primary, output);
+	igt_assert(ret == 0);
+	display->pipes[output->config.pipe].need_wait_for_vblank = false;
+
+	/* Step 10: Enable crtc (fb = -1), take CRC (CRC 7) */
+	ret = drmModeSetCrtc(data->drm_fd, output->config.crtc->crtc_id, -1,
+			     0, 0, &output->config.connector->connector_id,
+			     1, test.mode);
+	igt_assert(ret == 0);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_7);
+
+	/* Step 11: Disable primary plane */
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(display);
+
+	/* Step 12: Legacy modeset to yellow FB (CRC 8) */
+	igt_display_use_universal_commits(display, false);
+	igt_plane_set_fb(primary, &test.yellow_fb);
+	igt_display_commit(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_8);
+
+	/* Step 13: Legacy API', blue primary, red sprite */
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	igt_display_commit(display);
+
+	/* Step 14: Universal API, set primary completely offscreen (CRC 9) */
+	ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      test.blue_fb.fb_id, 0,
+			      9000, 9000,
+			      test.mode->hdisplay,
+			      test.mode->vdisplay,
+			      IGT_FIXED(0,0), IGT_FIXED(0,0),
+			      IGT_FIXED(test.mode->hdisplay,0),
+			      IGT_FIXED(test.mode->vdisplay,0));
+	igt_assert(ret == 0);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_9);
+
+	/*
+	 * Step 15: Explicitly disable primary after it's already been
+	 * implicitly disabled (CRC 10).
+	 */
+	igt_display_use_universal_commits(display, true);
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(display);
+	igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_10);
+
+	/* Step 16: Legacy API's, blue primary, red sprite */
+	igt_display_use_universal_commits(display, false);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_plane_set_fb(sprite, &test.red_fb);
+	igt_display_commit(display);
+
+	/* Blue bg + red sprite should be same under both types of API's */
+	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_4));
+
+	/* Disabling primary plane should be same as black primary */
+	igt_assert(igt_crc_equal(&test.crc_1, &test.crc_5));
+
+	/* Re-enabling primary should return to blue properly */
+	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_6));
+
+	/*
+	 * We should be able to setup plane FB's while CRTC is disabled and
+	 * then have them pop up correctly when the CRTC is re-enabled.
+	 */
+	igt_assert(igt_crc_equal(&test.crc_2, &test.crc_7));
+
+	/*
+	 * We should be able to modeset with the primary plane off
+	 * successfully
+	 */
+	igt_assert(igt_crc_equal(&test.crc_3, &test.crc_8));
+
+	/*
+	 * We should be able to move the primary plane completely offscreen
+	 * and have it disable successfully.
+	 */
+	igt_assert(igt_crc_equal(&test.crc_5, &test.crc_9));
+
+	/*
+	 * We should be able to explicitly disable an already
+	 * implicitly-disabled primary plane
+	 */
+	igt_assert(igt_crc_equal(&test.crc_5, &test.crc_10));
+
+	igt_plane_set_fb(primary, NULL);
+	igt_plane_set_fb(sprite, NULL);
+
+	functional_test_fini(&test, output);
+}
+
+static void
+sanity_test_init(sanity_test_t *test, igt_output_t *output, enum pipe pipe)
+{
+	data_t *data = test->data;
+	drmModeModeInfo *mode;
+
+	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, 1.0,
+			    &test->blue_fb);
+	igt_create_color_fb(data->drm_fd,
+			    mode->hdisplay + 100, mode->vdisplay + 100,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 1.0,
+			    &test->oversized_fb);
+	igt_create_color_fb(data->drm_fd,
+			    mode->hdisplay - 100, mode->vdisplay - 100,
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 1.0,
+			    &test->undersized_fb);
+
+	test->moderes = drmModeGetResources(data->drm_fd);
+}
+
+static void
+sanity_test_fini(sanity_test_t *test, igt_output_t *output)
+{
+	drmModeFreeResources(test->moderes);
+
+	igt_remove_fb(test->data->drm_fd, &test->oversized_fb);
+	igt_remove_fb(test->data->drm_fd, &test->undersized_fb);
+	igt_remove_fb(test->data->drm_fd, &test->blue_fb);
+
+	igt_display_use_universal_commits(&test->data->display, false);
+	igt_output_set_pipe(output, PIPE_ANY);
+	igt_display_commit(&test->data->display);
+}
+
+/*
+ * Universal plane sanity testing.
+ *   - Primary doesn't cover CRTC
+ *   - Primary plane tries to scale down
+ *   - Primary plane tries to scale up
+ */
+static void
+sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	sanity_test_t test = { .data = data };
+	igt_plane_t *primary;
+	drmModeModeInfo *mode;
+	int i, ret = 0;
+
+	igt_output_set_pipe(output, pipe);
+	mode = igt_output_get_mode(output);
+
+	sanity_test_init(&test, output, pipe);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+
+	/* Use legacy API to set a mode with a blue FB */
+	igt_display_use_universal_commits(&data->display, false);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_display_commit(&data->display);
+
+	/*
+	 * Try to use universal plane API to set primary plane that
+	 * doesn't cover CRTC (should fail).
+	 */
+	igt_plane_set_fb(primary, &test.undersized_fb);
+	ret = igt_drm_plane_try_commit(primary, output);
+	igt_assert(ret == -EINVAL);
+
+	/* Same as above, but different plane positioning. */
+	primary->crtc_x = 100;
+	primary->crtc_y = 100;
+	primary->position_changed = true;
+	ret = igt_drm_plane_try_commit(primary, output);
+	igt_assert(ret == -EINVAL);
+
+	primary->crtc_x = 0;
+	primary->crtc_y = 0;
+	primary->position_changed = false;
+
+	/* Try to use universal plane API to scale down (should fail) */
+	ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      test.oversized_fb.fb_id, 0,
+			      0, 0,
+			      mode->hdisplay + 100,
+			      mode->vdisplay + 100,
+			      IGT_FIXED(0,0), IGT_FIXED(0,0),
+			      IGT_FIXED(mode->hdisplay,0),
+			      IGT_FIXED(mode->vdisplay,0));
+	igt_assert(ret == -ERANGE);
+
+	/* Try to use universal plane API to scale up (should fail) */
+	ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
+			      output->config.crtc->crtc_id,
+			      test.oversized_fb.fb_id, 0,
+			      0, 0,
+			      mode->hdisplay,
+			      mode->vdisplay,
+			      IGT_FIXED(0,0), IGT_FIXED(0,0),
+			      IGT_FIXED(mode->hdisplay - 100,0),
+			      IGT_FIXED(mode->vdisplay - 100,0));
+	igt_assert(ret == -ERANGE);
+
+	/* Find other crtcs and try to program our primary plane on them */
+	for (i = 0; i < test.moderes->count_crtcs; i++)
+		if (test.moderes->crtcs[i] != output->config.crtc->crtc_id) {
+			ret = drmModeSetPlane(data->drm_fd,
+					      primary->drm_plane->plane_id,
+					      test.moderes->crtcs[i],
+					      test.blue_fb.fb_id, 0,
+					      0, 0,
+					      mode->hdisplay,
+					      mode->vdisplay,
+					      IGT_FIXED(0,0), IGT_FIXED(0,0),
+					      IGT_FIXED(mode->hdisplay,0),
+					      IGT_FIXED(mode->vdisplay,0));
+			igt_assert(ret == -EINVAL);
+		}
+
+	igt_plane_set_fb(primary, NULL);
+	sanity_test_fini(&test, output);
+}
+
+static void
+pageflip_test_init(pageflip_test_t *test, igt_output_t *output, enum pipe pipe)
+{
+	data_t *data = test->data;
+	drmModeModeInfo *mode;
+
+	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 */
+			    1.0, 0.0, 0.0,
+			    &test->red_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);
+}
+
+static void
+pageflip_test_fini(pageflip_test_t *test, igt_output_t *output)
+{
+	igt_remove_fb(test->data->drm_fd, &test->red_fb);
+	igt_remove_fb(test->data->drm_fd, &test->blue_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
+pageflip_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+	pageflip_test_t test = { .data = data };
+	igt_plane_t *primary;
+	struct timeval timeout = { .tv_sec = 0, .tv_usec = 500 };
+	drmEventContext evctx = { .version = DRM_EVENT_CONTEXT_VERSION };
+
+	fd_set fds;
+	int ret = 0;
+
+	igt_output_set_pipe(output, pipe);
+
+	pageflip_test_init(&test, output, pipe);
+
+	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+
+	/* Use legacy API to set a mode with a blue FB */
+	igt_display_use_universal_commits(&data->display, false);
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_display_commit(&data->display);
+
+	/* Disable the primary plane */
+	igt_display_use_universal_commits(&data->display, true);
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+
+	/*
+	 * Issue a pageflip to red FB
+	 *
+	 * Note that crtc->primary->fb = NULL causes flip to return EBUSY for
+	 * historical reasons...
+	 */
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      test.red_fb.fb_id, 0, NULL);
+	igt_assert(ret == -EBUSY);
+
+	/* Turn primary plane back on */
+	igt_plane_set_fb(primary, &test.blue_fb);
+	igt_display_commit(&data->display);
+
+	/*
+	 * Issue a pageflip to red, then immediately try to disable the primary
+	 * plane, hopefully before the pageflip has a chance to complete.  The
+	 * plane disable operation should wind up blocking while the pageflip
+	 * completes, which we don't have a good way to specifically test for,
+	 * but at least we can make sure that nothing blows up.
+	 */
+	ret = drmModePageFlip(data->drm_fd, output->config.crtc->crtc_id,
+			      test.red_fb.fb_id, DRM_MODE_PAGE_FLIP_EVENT,
+			      &test);
+	igt_assert(ret == 0);
+	igt_plane_set_fb(primary, NULL);
+	igt_display_commit(&data->display);
+
+	/* Wait for pageflip completion, then consume event on fd */
+	FD_ZERO(&fds);
+	FD_SET(data->drm_fd, &fds);
+	do {
+		ret = select(data->drm_fd + 1, &fds, NULL, NULL, &timeout);
+	} while (ret < 0 && errno == EINTR);
+	igt_assert(ret == 1);
+	ret = drmHandleEvent(data->drm_fd, &evctx);
+	igt_assert(ret == 0);
+
+	igt_display_use_universal_commits(&data->display, false);
+	igt_plane_set_fb(primary, NULL);
+	pageflip_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-functional", pipe_name(pipe))
+		for_each_connected_output(&data->display, output)
+			functional_test_pipe(data, pipe, output);
+
+	igt_subtest_f("universal-plane-pipe-%c-sanity", pipe_name(pipe))
+		for_each_connected_output(&data->display, output)
+			sanity_test_pipe(data, pipe, output);
+
+	igt_subtest_f("disable-primary-vs-flip-pipe-%c", pipe_name(pipe))
+		for_each_connected_output(&data->display, output)
+			pageflip_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

[PATCH i-g-t 5/5] kms_cursor_crc: Combine data_t and test_data_t

[Matt Roper]

If a subtest fails, cleanup_crtc() never gets called and then the
test_data_t structure for the test is lost, including the CRC file
descriptor that we never got a chance to release; this causes all
subsequent tests to fail with -EBUSY at igt_pipe_crc_new().

The split between permanent data_t and temporary test_data_t doesn't
seem to serve a purpose, so just combine the fields from both into
data_t.  This will prevent us from losing the CRC filedescriptor so that
we can properly close and reopen it after a failed test.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tests/kms_cursor_crc.c | 206 +++++++++++++++++++++++--------------------------
 1 file changed, 97 insertions(+), 109 deletions(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 06625ee..92d9a3c 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -44,10 +44,6 @@ typedef struct {
 	igt_display_t display;
 	struct igt_fb primary_fb;
 	struct igt_fb fb;
-} data_t;
-
-typedef struct {
-	data_t *data;
 	igt_output_t *output;
 	enum pipe pipe;
 	igt_crc_t ref_crc;
@@ -55,7 +51,7 @@ typedef struct {
 	int screenw, screenh;
 	int curw, curh; /* cursor size */
 	igt_pipe_crc_t *pipe_crc;
-} test_data_t;
+} data_t;
 
 static void draw_cursor(cairo_t *cr, int x, int y, int w)
 {
@@ -71,11 +67,10 @@ static void draw_cursor(cairo_t *cr, int x, int y, int w)
 	igt_paint_color_alpha(cr, x + w, y + w, w, w, 0.5, 0.5, 0.5, 1.0);
 }
 
-static void cursor_enable(test_data_t *test_data)
+static void cursor_enable(data_t *data)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
-	igt_output_t *output = test_data->output;
+	igt_output_t *output = data->output;
 	igt_plane_t *cursor;
 
 	cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR);
@@ -83,11 +78,10 @@ static void cursor_enable(test_data_t *test_data)
 	igt_display_commit(display);
 }
 
-static void cursor_disable(test_data_t *test_data)
+static void cursor_disable(data_t *data)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
-	igt_output_t *output = test_data->output;
+	igt_output_t *output = data->output;
 	igt_plane_t *cursor;
 
 	cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR);
@@ -96,11 +90,10 @@ static void cursor_disable(test_data_t *test_data)
 }
 
 
-static void do_single_test(test_data_t *test_data, int x, int y)
+static void do_single_test(data_t *data, int x, int y)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
-	igt_pipe_crc_t *pipe_crc = test_data->pipe_crc;
+	igt_pipe_crc_t *pipe_crc = data->pipe_crc;
 	igt_crc_t crc, ref_crc;
 	igt_plane_t *cursor;
 	cairo_t *cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb);
@@ -108,93 +101,93 @@ static void do_single_test(test_data_t *test_data, int x, int y)
 	igt_info("."); fflush(stdout);
 
 	/* Hardware test */
-	igt_paint_test_pattern(cr, test_data->screenw, test_data->screenh);
-	cursor_enable(test_data);
-	cursor = igt_output_get_plane(test_data->output, IGT_PLANE_CURSOR);
+	igt_paint_test_pattern(cr, data->screenw, data->screenh);
+	cursor_enable(data);
+	cursor = igt_output_get_plane(data->output, IGT_PLANE_CURSOR);
 	igt_plane_set_position(cursor, x, y);
 	igt_display_commit(display);
-	igt_wait_for_vblank(data->drm_fd, test_data->pipe);
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
 	igt_pipe_crc_collect_crc(pipe_crc, &crc);
-	cursor_disable(test_data);
+	cursor_disable(data);
 
 	/* Now render the same in software and collect crc */
-	draw_cursor(cr, x, y, test_data->curw);
+	draw_cursor(cr, x, y, data->curw);
 	igt_display_commit(display);
-	igt_wait_for_vblank(data->drm_fd, test_data->pipe);
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
 	igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
 	/* Clear screen afterwards */
-	igt_paint_color(cr, 0, 0, test_data->screenw, test_data->screenh,
-			    0.0, 0.0, 0.0);
+	igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
+			0.0, 0.0, 0.0);
 
 	igt_assert(igt_crc_equal(&crc, &ref_crc));
 }
 
-static void do_test(test_data_t *test_data,
+static void do_test(data_t *data,
 		    int left, int right, int top, int bottom)
 {
-	do_single_test(test_data, left, top);
-	do_single_test(test_data, right, top);
-	do_single_test(test_data, right, bottom);
-	do_single_test(test_data, left, bottom);
+	do_single_test(data, left, top);
+	do_single_test(data, right, top);
+	do_single_test(data, right, bottom);
+	do_single_test(data, left, bottom);
 }
 
-static void test_crc_onscreen(test_data_t *test_data)
+static void test_crc_onscreen(data_t *data)
 {
-	int left = test_data->left;
-	int right = test_data->right;
-	int top = test_data->top;
-	int bottom = test_data->bottom;
-	int cursor_w = test_data->curw;
-	int cursor_h = test_data->curh;
+	int left = data->left;
+	int right = data->right;
+	int top = data->top;
+	int bottom = data->bottom;
+	int cursor_w = data->curw;
+	int cursor_h = data->curh;
 
 	/* fully inside  */
-	do_test(test_data, left, right, top, bottom);
+	do_test(data, left, right, top, bottom);
 
 	/* 2 pixels inside */
-	do_test(test_data, left - (cursor_w-2), right + (cursor_w-2), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h-2), bottom + (cursor_h-2));
-	do_test(test_data, left - (cursor_w-2), right + (cursor_w-2), top - (cursor_h-2), bottom + (cursor_h-2));
+	do_test(data, left - (cursor_w-2), right + (cursor_w-2), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h-2), bottom + (cursor_h-2));
+	do_test(data, left - (cursor_w-2), right + (cursor_w-2), top - (cursor_h-2), bottom + (cursor_h-2));
 
 	/* 1 pixel inside */
-	do_test(test_data, left - (cursor_w-1), right + (cursor_w-1), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h-1), bottom + (cursor_h-1));
-	do_test(test_data, left - (cursor_w-1), right + (cursor_w-1), top - (cursor_h-1), bottom + (cursor_h-1));
+	do_test(data, left - (cursor_w-1), right + (cursor_w-1), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h-1), bottom + (cursor_h-1));
+	do_test(data, left - (cursor_w-1), right + (cursor_w-1), top - (cursor_h-1), bottom + (cursor_h-1));
 }
 
-static void test_crc_offscreen(test_data_t *test_data)
+static void test_crc_offscreen(data_t *data)
 {
-	int left = test_data->left;
-	int right = test_data->right;
-	int top = test_data->top;
-	int bottom = test_data->bottom;
-	int cursor_w = test_data->curw;
-	int cursor_h = test_data->curh;
+	int left = data->left;
+	int right = data->right;
+	int top = data->top;
+	int bottom = data->bottom;
+	int cursor_w = data->curw;
+	int cursor_h = data->curh;
 
 	/* fully outside */
-	do_test(test_data, left - (cursor_w), right + (cursor_w), top             , bottom             );
-	do_test(test_data, left             , right             , top - (cursor_h), bottom + (cursor_h));
-	do_test(test_data, left - (cursor_w), right + (cursor_w), top - (cursor_h), bottom + (cursor_h));
+	do_test(data, left - (cursor_w), right + (cursor_w), top             , bottom             );
+	do_test(data, left             , right             , top - (cursor_h), bottom + (cursor_h));
+	do_test(data, left - (cursor_w), right + (cursor_w), top - (cursor_h), bottom + (cursor_h));
 
 	/* fully outside by 1 extra pixels */
-	do_test(test_data, left - (cursor_w+1), right + (cursor_w+1), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h+1), bottom + (cursor_h+1));
-	do_test(test_data, left - (cursor_w+1), right + (cursor_w+1), top - (cursor_h+1), bottom + (cursor_h+1));
+	do_test(data, left - (cursor_w+1), right + (cursor_w+1), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h+1), bottom + (cursor_h+1));
+	do_test(data, left - (cursor_w+1), right + (cursor_w+1), top - (cursor_h+1), bottom + (cursor_h+1));
 
 	/* fully outside by 2 extra pixels */
-	do_test(test_data, left - (cursor_w+2), right + (cursor_w+2), top               , bottom               );
-	do_test(test_data, left               , right               , top - (cursor_h+2), bottom + (cursor_h+2));
-	do_test(test_data, left - (cursor_w+2), right + (cursor_w+2), top - (cursor_h+2), bottom + (cursor_h+2));
+	do_test(data, left - (cursor_w+2), right + (cursor_w+2), top               , bottom               );
+	do_test(data, left               , right               , top - (cursor_h+2), bottom + (cursor_h+2));
+	do_test(data, left - (cursor_w+2), right + (cursor_w+2), top - (cursor_h+2), bottom + (cursor_h+2));
 
 	/* fully outside by a lot of extra pixels */
-	do_test(test_data, left - (cursor_w+512), right + (cursor_w+512), top                 , bottom                 );
-	do_test(test_data, left                 , right                 , top - (cursor_h+512), bottom + (cursor_h+512));
-	do_test(test_data, left - (cursor_w+512), right + (cursor_w+512), top - (cursor_h+512), bottom + (cursor_h+512));
+	do_test(data, left - (cursor_w+512), right + (cursor_w+512), top                 , bottom                 );
+	do_test(data, left                 , right                 , top - (cursor_h+512), bottom + (cursor_h+512));
+	do_test(data, left - (cursor_w+512), right + (cursor_w+512), top - (cursor_h+512), bottom + (cursor_h+512));
 
 	/* go nuts */
-	do_test(test_data, INT_MIN, INT_MAX, INT_MIN, INT_MAX);
+	do_test(data, INT_MIN, INT_MAX, INT_MIN, INT_MAX);
 }
 
-static void test_crc_sliding(test_data_t *test_data)
+static void test_crc_sliding(data_t *data)
 {
 	int i;
 
@@ -202,34 +195,33 @@ static void test_crc_sliding(test_data_t *test_data)
 	 * no alignment issues. Horizontal, vertical and diagonal test.
 	 */
 	for (i = 0; i < 16; i++) {
-		do_single_test(test_data, i, 0);
-		do_single_test(test_data, 0, i);
-		do_single_test(test_data, i, i);
+		do_single_test(data, i, 0);
+		do_single_test(data, 0, i);
+		do_single_test(data, i, i);
 	}
 }
 
-static void test_crc_random(test_data_t *test_data)
+static void test_crc_random(data_t *data)
 {
 	int i;
 
 	/* Random cursor placement */
 	for (i = 0; i < 50; i++) {
-		int x = rand() % (test_data->screenw + test_data->curw * 2) - test_data->curw;
-		int y = rand() % (test_data->screenh + test_data->curh * 2) - test_data->curh;
-		do_single_test(test_data, x, y);
+		int x = rand() % (data->screenw + data->curw * 2) - data->curw;
+		int y = rand() % (data->screenh + data->curh * 2) - data->curh;
+		do_single_test(data, x, y);
 	}
 }
 
-static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
+static bool prepare_crtc(data_t *data, igt_output_t *output,
 			 int cursor_w, int cursor_h)
 {
 	drmModeModeInfo *mode;
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
 	igt_plane_t *primary;
 
 	/* select the pipe we want to use */
-	igt_output_set_pipe(output, test_data->pipe);
+	igt_output_set_pipe(output, data->pipe);
 	igt_display_commit(display);
 
 	if (!output->valid) {
@@ -241,10 +233,10 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
 	/* create and set the primary plane fb */
 	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,
-				&data->primary_fb);
+			    DRM_FORMAT_XRGB8888,
+			    false, /* tiled */
+			    0.0, 0.0, 0.0,
+			    &data->primary_fb);
 
 	primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
 	igt_plane_set_fb(primary, &data->primary_fb);
@@ -252,45 +244,44 @@ static bool prepare_crtc(test_data_t *test_data, igt_output_t *output,
 	igt_display_commit(display);
 
 	/* create the pipe_crc object for this pipe */
-	if (test_data->pipe_crc)
-		igt_pipe_crc_free(test_data->pipe_crc);
+	if (data->pipe_crc)
+		igt_pipe_crc_free(data->pipe_crc);
 
-	test_data->pipe_crc = igt_pipe_crc_new(test_data->pipe,
-					       INTEL_PIPE_CRC_SOURCE_AUTO);
-	if (!test_data->pipe_crc) {
+	data->pipe_crc = igt_pipe_crc_new(data->pipe,
+					  INTEL_PIPE_CRC_SOURCE_AUTO);
+	if (!data->pipe_crc) {
 		igt_info("auto crc not supported on this connector with pipe %i\n",
-			 test_data->pipe);
+			 data->pipe);
 		return false;
 	}
 
 	/* x/y position where the cursor is still fully visible */
-	test_data->left = 0;
-	test_data->right = mode->hdisplay - cursor_w;
-	test_data->top = 0;
-	test_data->bottom = mode->vdisplay - cursor_h;
-	test_data->screenw = mode->hdisplay;
-	test_data->screenh = mode->vdisplay;
-	test_data->curw = cursor_w;
-	test_data->curh = cursor_h;
+	data->left = 0;
+	data->right = mode->hdisplay - cursor_w;
+	data->top = 0;
+	data->bottom = mode->vdisplay - cursor_h;
+	data->screenw = mode->hdisplay;
+	data->screenh = mode->vdisplay;
+	data->curw = cursor_w;
+	data->curh = cursor_h;
 
 	/* make sure cursor is disabled */
-	cursor_disable(test_data);
-	igt_wait_for_vblank(data->drm_fd, test_data->pipe);
+	cursor_disable(data);
+	igt_wait_for_vblank(data->drm_fd, data->pipe);
 
 	/* get reference crc w/o cursor */
-	igt_pipe_crc_collect_crc(test_data->pipe_crc, &test_data->ref_crc);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc);
 
 	return true;
 }
 
-static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
+static void cleanup_crtc(data_t *data, igt_output_t *output)
 {
-	data_t *data = test_data->data;
 	igt_display_t *display = &data->display;
 	igt_plane_t *primary;
 
-	igt_pipe_crc_free(test_data->pipe_crc);
-	test_data->pipe_crc = NULL;
+	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
 
 	igt_remove_fb(data->drm_fd, &data->primary_fb);
 
@@ -301,38 +292,35 @@ static void cleanup_crtc(test_data_t *test_data, igt_output_t *output)
 	igt_display_commit(display);
 }
 
-static void run_test(data_t *data, void (*testfunc)(test_data_t *), int cursor_w, int cursor_h)
+static void run_test(data_t *data, void (*testfunc)(data_t *), int cursor_w, int cursor_h)
 {
 	igt_display_t *display = &data->display;
 	igt_output_t *output;
 	enum pipe p;
-	test_data_t test_data = {
-		.data = data,
-	};
 	int valid_tests = 0;
 
 	for_each_connected_output(display, output) {
-		test_data.output = output;
+		data->output = output;
 		for (p = 0; p < igt_display_get_n_pipes(display); p++) {
-			test_data.pipe = p;
+			data->pipe = p;
 
-			if (!prepare_crtc(&test_data, output, cursor_w, cursor_h))
+			if (!prepare_crtc(data, output, cursor_w, cursor_h))
 				continue;
 
 			valid_tests++;
 
 			igt_info("Beginning %s on pipe %c, connector %s\n",
-				 igt_subtest_name(), pipe_name(test_data.pipe),
+				 igt_subtest_name(), pipe_name(data->pipe),
 				 igt_output_name(output));
 
-			testfunc(&test_data);
+			testfunc(data);
 
 			igt_info("\n%s on pipe %c, connector %s: PASSED\n\n",
-				 igt_subtest_name(), pipe_name(test_data.pipe),
+				 igt_subtest_name(), pipe_name(data->pipe),
 				 igt_output_name(output));
 
 			/* cleanup what prepare_crtc() has done */
-			cleanup_crtc(&test_data, output);
+			cleanup_crtc(data, output);
 		}
 	}
 
-- 
1.8.5.1

Re: [PATCH i-g-t 1/5] kms: Add universal plane support

[Damien Lespiau]

On Thu, May 29, 2014 at 08:09:13AM -0700, Matt Roper wrote:
> 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>

With the tiny change below, that patch looks reasonable. So:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

One thing we may want to clean up when we bring in even more ways to do
commits is how we store the states to flush. What I mean by that is:

- In the *_set_*() functions (eg. igt_plane_set_fb()), we store the
  states that change (eg. in set_fb() we only set fb_changed to true and
  don't touch need_set_crtc/need_set_cursor)

- In the commit, we resolve what we need to do depending on the states
  changed and the commit method.

> ---
>  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 d00250d..97e329b 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -500,27 +500,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);


Can we have:

#ifndef DRM_CLIENT_CAP_UNIVERSAL_PLANES
#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
#endif

As an easy way to make sure that's always defined, even if we don't have
the (unreleased) libdrm with this support?

>  	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]);
> @@ -531,21 +528,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);
> @@ -700,16 +743,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)
> @@ -761,6 +817,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);
> @@ -820,7 +878,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 */
> @@ -838,8 +900,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;
> @@ -889,7 +951,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;
> @@ -940,6 +1002,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 8e80d4b..4955fc8 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -121,6 +121,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];
> @@ -143,6 +144,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 */
> @@ -150,6 +153,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/5] kms: Add universal plane support

[Damien Lespiau]

As a small summary, I don't think we should be exposing the
plane_commit() function and always go through igt_display_commit() to
make hooking the atomic ioctl() easier. Loosing the plane ordering is a
bit meh but should be able to live with it. The rest looks good.

-- 
Damien

[PATCH 3/4] drm/i915: don't force full modeset if primary plane is disabled

[Matt Roper]

In a future patch, we'll allow the primary plane to be disabled by
userspace via the universal plane API.  If a modeset is requested while
the primary plane is disabled, crtc->primary->fb will be NULL which
generally triggers a full modeset (except in fastboot situations).  If
we detect that the crtc is active, but there's no primary plane fb,
we should still allow a simple plane update rather than a full modeset
if the mode isn't actually changing (after re-enabling the primary plane
of course).

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1e1239..04bd821 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10136,12 +10136,17 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 	if (is_crtc_connector_off(set)) {
 		config->mode_changed = true;
 	} else if (set->crtc->primary->fb != set->fb) {
-		/* If we have no fb then treat it as a full mode set */
+		/*
+		 * If we have no fb, we can only flip as long as the crtc is
+		 * active, otherwise we need a full mode set.  The crtc may
+		 * be active if we've only disabled the primary plane, or
+		 * in fastboot situations.
+		 */
 		if (set->crtc->primary->fb == NULL) {
 			struct intel_crtc *intel_crtc =
 				to_intel_crtc(set->crtc);
 
-			if (intel_crtc->active && i915.fastboot) {
+			if (intel_crtc->active) {
 				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
 				config->fb_changed = true;
 			} else {
@@ -10380,8 +10385,21 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
+		/*
+		 * We need to make sure the primary plane is re-enabled if it
+		 * has previously been turned off.
+		 */
+		if (!intel_crtc->primary_enabled) {
+			BUG_ON(!intel_crtc->active);
+			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+						      intel_crtc->pipe);
+		}
+
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
 		/*
-- 
1.8.5.1

[PATCH 3/4] drm/i915: don't force full modeset if primary plane is disabled (v2)

[Matt Roper]

In a future patch, we'll allow the primary plane to be disabled by
userspace via the universal plane API.  If a modeset is requested while
the primary plane is disabled, crtc->primary->fb will be NULL which
generally triggers a full modeset (except in fastboot situations).  If
we detect that the crtc is active, but there's no primary plane fb,
we should still allow a simple plane update rather than a full modeset
if the mode isn't actually changing (after re-enabling the primary plane
of course).

v2:
 - Enable plane after set_base to avoid enabling the plane if set_base
   fails, and to make flip+enable atomic (suggested by Ville)
 - Drop BUG to WARN if we somehow enter the 'fb_changed' modeset case
   with the crtc disabled (suggested by Ville)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f8f9bc..3aedc64 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10417,12 +10417,17 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 	if (is_crtc_connector_off(set)) {
 		config->mode_changed = true;
 	} else if (set->crtc->primary->fb != set->fb) {
-		/* If we have no fb then treat it as a full mode set */
+		/*
+		 * If we have no fb, we can only flip as long as the crtc is
+		 * active, otherwise we need a full mode set.  The crtc may
+		 * be active if we've only disabled the primary plane, or
+		 * in fastboot situations.
+		 */
 		if (set->crtc->primary->fb == NULL) {
 			struct intel_crtc *intel_crtc =
 				to_intel_crtc(set->crtc);
 
-			if (intel_crtc->active && i915.fastboot) {
+			if (intel_crtc->active) {
 				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
 				config->fb_changed = true;
 			} else {
@@ -10660,10 +10665,24 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
+
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
+
+		/*
+		 * We need to make sure the primary plane is re-enabled if it
+		 * has previously been turned off.
+		 */
+		if (!intel_crtc->primary_enabled && ret == 0) {
+			WARN_ON(!intel_crtc->active);
+			intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+						      intel_crtc->pipe);
+		}
+
 		/*
 		 * In the fastboot case this may be our only check of the
 		 * state after boot.  It would be better to only do it on
-- 
1.8.5.1