[RFC 0/4] Wire gen9 cursor interface to universal plane registers

[RFC 0/4] Wire gen9 cursor interface to universal plane registers

[Matt Roper]

Gen9 has a traditional cursor plane that is mutually exclusive with the
system's top-most "universal" plane; it seems likely that two planes are really
a single shared hardware unit with two different register interfaces.  Thus far
i915 has exposed a cursor plane to userspace that's hooked up to the old-style
cursor registers; we just pretended that the top-most universal plane didn't
exist and reported one fewer "sprite/overlay" planes for each pipe than the
platform technically has.  Let's switch this around so that the cursor exposed
to userspace is actually wired up to the previously-unused top-most universal
plane registers.  With this change we'll still present the same cursor ABI to
userspace that we always have, but internally we'll just be programming a
different set of registers and doing so in a way that's more consistent with
how all the other gen9 planes work --- less cursor-specific special cases
throughout the code.

Aside from making the code a bit simpler (fewer cursor-specific special
cases), this will also pave the way to eventually exposing the top-most
plane in a more full-featured manner to userspace clients that don't
need a traditional cursor and wish to opt into having access to an
additional sprite/overlay-style plane instead.

It's worth noting that a lot of the special-cased cursor-specific code
was in the gen9 watermark programming.  It's good to simplify that code,
but we should keep an eye out for any unwanted side effects of this
patch; since sprites/overlays tend to get used less than cursors, it's
possible that this could help us uncover additional underruns that
nobody had run across yet.  Or it could have the opposite effect and
eliminate some of the underruns that we haven't been able to get rid of
yet.

This is just an RFC at this point; we should probably land all of the in-flight
watermark work before this so that those series don't need additional rebasing.
There's also still one known problem I need to track down --- it seems the
color correction of the plane is slightly different when programming the
universal registers vs the legacy cursor registers, so IGT tests like
kms_cursor_crc report CRC mismatches, even though the results look correct
visually.

Note that patches #1 and 2 are trivial renaming patches and could probably be
merged immediately if nobody has concerns with them.


Matt Roper (4):
  drm/i915: Rename for_each_plane -> for_each_universal_plane
  drm/i915: Use macro in place of open-coded for_each_universal_plane
    loop
  drm/i915/gen9: Expose top-most universal plane as cursor
  drm/i915/gen9: Skip debugfs cursor output for universal plane
    platforms

 drivers/gpu/drm/i915/i915_debugfs.c      | 32 ++++++-----
 drivers/gpu/drm/i915/i915_drv.h          | 13 ++++-
 drivers/gpu/drm/i915/intel_device_info.c | 38 ++++++++----
 drivers/gpu/drm/i915/intel_display.c     | 99 ++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h         | 16 +++---
 drivers/gpu/drm/i915/intel_pm.c          | 89 ++++------------------------
 drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
 7 files changed, 116 insertions(+), 177 deletions(-)

-- 
2.1.4

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

[RFC 1/4] drm/i915: Rename for_each_plane -> for_each_universal_plane

[Matt Roper]

This macro's name is a bit misleading; it doesn't actually iterate over
all planes since it omits the cursor plane.  Its only uses are in gen9
code which is using it to iterate over the universal planes (which we
treat as primary+sprites); in these cases the legacy cursor registers
are programmed independently if necessary.  The macro's iterator value
(0 for primary plane, spritenum+1 for each secondary plane) also isn't
meaningful outside the gen9 context where the hardware considers them to
all be "universal" planes that follow this numbering.

This is just a renaming/clarification patch with no functional change.
However it will make the subsequent patches more clear.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index be92efe..9f5a392 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3463,7 +3463,7 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 	for_each_pipe(dev_priv, pipe) {
 		seq_printf(m, "Pipe %c\n", pipe_name(pipe));
 
-		for_each_plane(dev_priv, pipe, plane) {
+		for_each_universal_plane(dev_priv, pipe, plane) {
 			entry = &ddb->plane[pipe][plane];
 			seq_printf(m, "  Plane%-8d%8u%8u%8u\n", plane + 1,
 				   entry->start, entry->end,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 55afb66..4714051 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -312,7 +312,7 @@ struct i915_hotplug {
 #define for_each_pipe_masked(__dev_priv, __p, __mask) \
 	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) \
 		for_each_if ((__mask) & (1 << (__p)))
-#define for_each_plane(__dev_priv, __pipe, __p)				\
+#define for_each_universal_plane(__dev_priv, __pipe, __p)		\
 	for ((__p) = 0;							\
 	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1;	\
 	     (__p)++)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 895b3dc..cb7dd11 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13485,7 +13485,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
 	sw_ddb = &dev_priv->wm.skl_hw.ddb;
 
 	/* planes */
-	for_each_plane(dev_priv, pipe, plane) {
+	for_each_universal_plane(dev_priv, pipe, plane) {
 		hw_plane_wm = &hw_wm.planes[plane];
 		sw_plane_wm = &sw_wm->planes[plane];
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9e0e874..58d3ba0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3160,7 +3160,7 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 		if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 			continue;
 
-		for_each_plane(dev_priv, pipe, plane) {
+		for_each_universal_plane(dev_priv, pipe, plane) {
 			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
 			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane],
 						   val);
-- 
2.1.4

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

[RFC 2/4] drm/i915: Use macro in place of open-coded for_each_universal_plane loop

[Matt Roper]

This was the only use of (misleadingly-named) intel_num_planes()
function, so we can remove it as well.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c2f3863..c31fddd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1108,15 +1108,6 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
 	return container_of(intel_hdmi, struct intel_digital_port, hdmi);
 }
 
-/*
- * Returns the number of planes for this pipe, ie the number of sprites + 1
- * (primary plane). This doesn't count the cursor plane then.
- */
-static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
-{
-	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] + 1;
-}
-
 /* intel_fifo_underrun.c */
 bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
 					   enum pipe pipe, bool enable);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 58d3ba0..6f19e60 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4232,7 +4232,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	if (crtc->state->active_changed) {
 		int plane;
 
-		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
+		for_each_universal_plane(dev_priv, pipe, plane)
 			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
 					   &results->ddb, plane);
 
-- 
2.1.4

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

[RFC 3/4] drm/i915/gen9: Expose top-most universal plane as cursor

[Matt Roper]

Gen9 has a traditional cursor plane that is mutually exclusive with the
system's top-most "universal" plane; it seems likely that two planes are
really a single shared hardware unit with two different register
interfaces.  Thus far i915 has exposed a cursor plane to userspace
that's hooked up to the old-style cursor registers; we just pretended
that the top-most universal plane didn't exist and reported one fewer
"sprite/overlay" planes for each pipe than the platform technically has.
Let's switch this around so that the cursor exposed to userspace is
actually wired up to top-most universal plane registers.  We'll continue
to present the same cursor ABI to userspace that we always have, but
internally we'll just be programming a different set of registers and
doing so in a way that's more consistent with how all the other gen9
planes work --- less cursor-specific special cases throughout the code.

Aside from making the code a bit simpler (fewer cursor-specific special
cases), this will also pave the way to eventually exposing the top-most
plane in a more full-featured manner to userspace clients that don't
need a traditional cursor and wish to opt into having access to an
additional sprite/overlay-style plane instead.

It's worth noting that a lot of the special-cased cursor-specific code
was in the gen9 watermark programming.  It's good to simplify that code,
but we should keep an eye out for any unwanted side effects of this
patch; since sprites/overlays tend to get used less than cursors, it's
possible that this could help us uncover additional underruns that
nobody had run across yet.  Or it could have the opposite effect and
eliminate some of the underruns that we haven't been able to get rid of
yet.

Cc: Bob Paauwe <bob.j.paauwe@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  4 --
 drivers/gpu/drm/i915/i915_drv.h          | 11 +++-
 drivers/gpu/drm/i915/intel_device_info.c | 38 +++++++++----
 drivers/gpu/drm/i915/intel_display.c     | 97 ++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h         |  7 +++
 drivers/gpu/drm/i915/intel_pm.c          | 85 ++++------------------------
 drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
 7 files changed, 93 insertions(+), 155 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9f5a392..0bba472 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3469,10 +3469,6 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 				   entry->start, entry->end,
 				   skl_ddb_entry_size(entry));
 		}
-
-		entry = &ddb->plane[pipe][PLANE_CURSOR];
-		seq_printf(m, "  %-13s%8u%8u%8u\n", "Cursor", entry->start,
-			   entry->end, skl_ddb_entry_size(entry));
 	}
 
 	drm_modeset_unlock_all(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4714051..83aaed2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -178,7 +178,7 @@ enum plane {
 	PLANE_A = 0,
 	PLANE_B,
 	PLANE_C,
-	PLANE_CURSOR,
+	PLANE_D,
 	I915_MAX_PLANES,
 };
 #define plane_name(p) ((p) + 'A')
@@ -316,9 +316,15 @@ struct i915_hotplug {
 	for ((__p) = 0;							\
 	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1;	\
 	     (__p)++)
+
+/*
+ * Only iterate over sprites exposed as sprites; omit sprites that
+ * are being repurposed to simulate a cursor.
+ */
 #define for_each_sprite(__dev_priv, __p, __s)				\
 	for ((__s) = 0;							\
-	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)];	\
+	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)] -	\
+	             (INTEL_INFO(__dev_priv)->has_real_cursor ? 0 : 1);	\
 	     (__s)++)
 
 #define for_each_port_masked(__port, __ports_mask) \
@@ -687,6 +693,7 @@ struct intel_csr {
 	func(has_psr); \
 	func(has_rc6); \
 	func(has_rc6p); \
+	func(has_real_cursor); \
 	func(has_resource_streamer); \
 	func(has_runtime_pm); \
 	func(has_snoop); \
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index d6a8f11..a464e0e 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -271,23 +271,39 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
 	enum pipe pipe;
 
 	/*
-	 * Skylake and Broxton currently don't expose the topmost plane as its
-	 * use is exclusive with the legacy cursor and we only want to expose
-	 * one of those, not both. Until we can safely expose the topmost plane
-	 * as a DRM_PLANE_TYPE_CURSOR with all the features exposed/supported,
-	 * we don't expose the topmost plane at all to prevent ABI breakage
-	 * down the line.
+	 * Gen9 platforms have a top-most universal (i.e., sprite) plane and a
+	 * cursor plane that are mutually exclusive.  If we use the cursor
+	 * plane we permanently lose the ability to make use of the more
+	 * full-featured universal plane.  So instead let's use all of the
+	 * universal planes, ignore the cursor plane, but hook the top-most
+	 * universal plane up to the legacy cursor ioctl's and expose it to
+	 * userspace as DRM_PLANE_TYPE_CURSOR.  This won't result in any
+	 * visible behavior change to userspace; we're just internally
+	 * programming a different set of registers.
+	 *
+	 * For the purposes of device_info, we're only concerned with the
+	 * number of universal planes we're programming, regardless of how they
+	 * get mapped to userspace interfaces, so we'll report the true number of
+	 * universal planes for gen9.
 	 */
 	if (IS_BROXTON(dev_priv)) {
-		info->num_sprites[PIPE_A] = 2;
-		info->num_sprites[PIPE_B] = 2;
-		info->num_sprites[PIPE_C] = 1;
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		info->has_real_cursor = 0;
+		info->num_sprites[PIPE_A] = 3;
+		info->num_sprites[PIPE_B] = 3;
+		info->num_sprites[PIPE_C] = 2;
+	} else if (IS_GEN9(dev_priv)) {
+		info->has_real_cursor = 0;
 		for_each_pipe(dev_priv, pipe)
 			info->num_sprites[pipe] = 2;
-	else
+	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		info->has_real_cursor = 1;
+		for_each_pipe(dev_priv, pipe)
+			info->num_sprites[pipe] = 2;
+	} else {
+		info->has_real_cursor = 1;
 		for_each_pipe(dev_priv, pipe)
 			info->num_sprites[pipe] = 1;
+	}
 
 	if (i915.disable_display) {
 		DRM_INFO("Display disabled (module parameter)\n");
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cb7dd11..9a8c2b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1232,6 +1232,23 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
 	     pipe_name(pipe));
 }
 
+/*
+ * Cursor state for platforms that use a universal plane as a cursor.
+ * Primary is universal #0, others are universal #1-numsprites.  Cursor
+ * will be the final universal plane for the pipe.
+ */
+static bool
+universal_cursor_state(struct drm_i915_private *dev_priv,
+		       enum pipe pipe)
+{
+	unsigned int planenum = INTEL_INFO(dev_priv)->num_sprites[pipe];
+	u32 val;
+
+	val = I915_READ(PLANE_CTL(pipe, planenum));
+
+	return val & PLANE_CTL_ENABLE;
+}
+
 static void assert_cursor(struct drm_i915_private *dev_priv,
 			  enum pipe pipe, bool state)
 {
@@ -1239,6 +1256,8 @@ static void assert_cursor(struct drm_i915_private *dev_priv,
 
 	if (IS_845G(dev_priv) || IS_I865G(dev_priv))
 		cur_state = I915_READ(CURCNTR(PIPE_A)) & CURSOR_ENABLE;
+	else if (!INTEL_INFO(dev_priv)->has_real_cursor)
+		cur_state = universal_cursor_state(dev_priv, pipe);
 	else
 		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
 
@@ -10841,15 +10860,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
-	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
+	/*
+	 * Although gen9 has legacy cursor registers, their use is mutually
+	 * exclusive with the top-most universal plane.  We'll just use the
+	 * universal plane to simulate the legacy cursor interface instead,
+	 * which means we'll never enter here on gen9 platforms.
+	 */
+	WARN_ON(IS_GEN9(dev_priv));
 
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
@@ -13528,56 +13548,6 @@ static void verify_wm_state(struct drm_crtc *crtc,
 				  hw_ddb_entry->start, hw_ddb_entry->end);
 		}
 	}
-
-	/*
-	 * cursor
-	 * If the cursor plane isn't active, we may not have updated it's ddb
-	 * allocation. In that case since the ddb allocation will be updated
-	 * once the plane becomes visible, we can skip this check
-	 */
-	if (intel_crtc->cursor_addr) {
-		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
-		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
-
-		/* Watermarks */
-		for (level = 0; level <= max_level; level++) {
-			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
-						&sw_plane_wm->wm[level]))
-				continue;
-
-			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
-				  pipe_name(pipe), level,
-				  sw_plane_wm->wm[level].plane_en,
-				  sw_plane_wm->wm[level].plane_res_b,
-				  sw_plane_wm->wm[level].plane_res_l,
-				  hw_plane_wm->wm[level].plane_en,
-				  hw_plane_wm->wm[level].plane_res_b,
-				  hw_plane_wm->wm[level].plane_res_l);
-		}
-
-		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
-					 &sw_plane_wm->trans_wm)) {
-			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
-				  pipe_name(pipe),
-				  sw_plane_wm->trans_wm.plane_en,
-				  sw_plane_wm->trans_wm.plane_res_b,
-				  sw_plane_wm->trans_wm.plane_res_l,
-				  hw_plane_wm->trans_wm.plane_en,
-				  hw_plane_wm->trans_wm.plane_res_b,
-				  hw_plane_wm->trans_wm.plane_res_l);
-		}
-
-		/* DDB */
-		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
-
-		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
-			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
-				  pipe_name(pipe),
-				  sw_ddb_entry->start, sw_ddb_entry->end,
-				  hw_ddb_entry->start, hw_ddb_entry->end);
-		}
-	}
 }
 
 static void
@@ -15215,11 +15185,18 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
-	cursor->plane = pipe;
 	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
-	cursor->check_plane = intel_check_cursor_plane;
-	cursor->update_plane = intel_update_cursor_plane;
-	cursor->disable_plane = intel_disable_cursor_plane;
+	if (INTEL_INFO(dev)->has_real_cursor) {
+		cursor->plane = pipe;  /* unused? */
+		cursor->check_plane = intel_check_cursor_plane;
+		cursor->update_plane = intel_update_cursor_plane;
+		cursor->disable_plane = intel_disable_cursor_plane;
+	} else {
+		cursor->plane = INTEL_INFO(dev)->num_sprites[pipe] - 1;
+		cursor->check_plane = intel_check_sprite_plane;
+		cursor->update_plane = skl_update_plane;
+		cursor->disable_plane = skl_disable_plane;
+	}
 
 	ret = drm_universal_plane_init(dev, &cursor->base, 0,
 				       &intel_plane_funcs,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c31fddd..50874e2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1790,6 +1790,13 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 void intel_pipe_update_start(struct intel_crtc *crtc);
 void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work);
+int intel_check_sprite_plane(struct drm_plane *plane,
+			     struct intel_crtc_state *crtc_state,
+			     struct intel_plane_state *state);
+void skl_update_plane(struct drm_plane *drm_plane,
+		      const struct intel_crtc_state *crtc_state,
+		      const struct intel_plane_state *plane_state);
+void skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6f19e60..e75f6d8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2863,9 +2863,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
 
 /*
  * Return the index of a plane in the SKL DDB and wm result arrays.  Primary
- * plane is always in slot 0, cursor is always in slot I915_MAX_PLANES-1, and
- * other universal planes are in indices 1..n.  Note that this may leave unused
- * indices between the top "sprite" plane and the cursor.
+ * plane is always in slot 0 and other universal planes are in indices 1..n.
  */
 static int
 skl_wm_plane_id(const struct intel_plane *plane)
@@ -2874,7 +2872,6 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	case DRM_PLANE_TYPE_PRIMARY:
 		return 0;
 	case DRM_PLANE_TYPE_CURSOR:
-		return PLANE_CURSOR;
 	case DRM_PLANE_TYPE_OVERLAY:
 		return plane->plane + 1;
 	default:
@@ -3128,14 +3125,6 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	alloc->end = alloc->start + pipe_size;
 }
 
-static unsigned int skl_cursor_allocation(int num_active)
-{
-	if (num_active == 1)
-		return 32;
-
-	return 8;
-}
-
 static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
 {
 	entry->start = reg & 0x3ff;
@@ -3166,10 +3155,6 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 						   val);
 		}
 
-		val = I915_READ(CUR_BUF_CFG(pipe));
-		skl_ddb_entry_init_from_hw(&ddb->plane[pipe][PLANE_CURSOR],
-					   val);
-
 		intel_display_power_put(dev_priv, power_domain);
 	}
 }
@@ -3227,8 +3212,6 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 
 	if (!intel_pstate->base.visible)
 		return 0;
-	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
-		return 0;
 	if (y && format != DRM_FORMAT_NV12)
 		return 0;
 
@@ -3386,7 +3369,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	struct drm_plane_state *pstate;
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
-	uint16_t alloc_size, start, cursor_blocks;
+	uint16_t alloc_size, start;
 	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
 	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
 	unsigned int total_data_rate;
@@ -3412,12 +3395,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 	}
 
-	cursor_blocks = skl_cursor_allocation(num_active);
-	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - cursor_blocks;
-	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
-
-	alloc_size -= cursor_blocks;
-
 	/* 1. Allocate the mininum required blocks for each active plane */
 	for_each_plane_in_state(state, plane, pstate, i) {
 		intel_plane = to_intel_plane(plane);
@@ -3431,17 +3408,12 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 			y_minimum[id] = 0;
 			continue;
 		}
-		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
-			minimum[id] = 0;
-			y_minimum[id] = 0;
-			continue;
-		}
 
 		minimum[id] = skl_ddb_min_alloc(pstate, 0);
 		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
 	}
 
-	for (i = 0; i < PLANE_CURSOR; i++) {
+	for (i = 0; i < I915_MAX_PLANES; i++) {
 		alloc_size -= minimum[i];
 		alloc_size -= y_minimum[i];
 	}
@@ -3866,26 +3838,6 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 			    &ddb->y_plane[pipe][plane]);
 }
 
-void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
-			 const struct skl_plane_wm *wm,
-			 const struct skl_ddb_allocation *ddb)
-{
-	struct drm_crtc *crtc = &intel_crtc->base;
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	int level, max_level = ilk_wm_max_level(dev_priv);
-	enum pipe pipe = intel_crtc->pipe;
-
-	for (level = 0; level <= max_level; level++) {
-		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
-				   &wm->wm[level]);
-	}
-	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm);
-
-	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-			    &ddb->plane[pipe][PLANE_CURSOR]);
-}
-
 bool skl_wm_level_equals(const struct skl_wm_level *l1,
 			 const struct skl_wm_level *l2)
 {
@@ -4122,19 +4074,11 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
 			if (skl_ddb_entry_equal(old, new))
 				continue;
 
-			if (id != PLANE_CURSOR) {
-				DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n",
-						 plane->base.id, id + 1,
-						 pipe_name(pipe),
-						 old->start, old->end,
-						 new->start, new->end);
-			} else {
-				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n",
-						 plane->base.id,
-						 pipe_name(pipe),
-						 old->start, old->end,
-						 new->start, new->end);
-			}
+			DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n",
+					 plane->base.id, id + 1,
+					 pipe_name(pipe),
+					 old->start, old->end,
+					 new->start, new->end);
 		}
 	}
 }
@@ -4235,9 +4179,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
 		for_each_universal_plane(dev_priv, pipe, plane)
 			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
 					   &results->ddb, plane);
-
-		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
-				    &results->ddb);
 	}
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
@@ -4350,18 +4291,12 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
 		wm = &out->planes[id];
 
 		for (level = 0; level <= max_level; level++) {
-			if (id != PLANE_CURSOR)
-				val = I915_READ(PLANE_WM(pipe, id, level));
-			else
-				val = I915_READ(CUR_WM(pipe, level));
+			val = I915_READ(PLANE_WM(pipe, id, level));
 
 			skl_wm_level_from_reg_val(val, &wm->wm[level]);
 		}
 
-		if (id != PLANE_CURSOR)
-			val = I915_READ(PLANE_WM_TRANS(pipe, id));
-		else
-			val = I915_READ(CUR_WM_TRANS(pipe));
+		val = I915_READ(PLANE_WM_TRANS(pipe, id));
 
 		skl_wm_level_from_reg_val(val, &wm->trans_wm);
 	}
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 43d0350..9e6406a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -194,7 +194,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 	}
 }
 
-static void
+void
 skl_update_plane(struct drm_plane *drm_plane,
 		 const struct intel_crtc_state *crtc_state,
 		 const struct intel_plane_state *plane_state)
@@ -285,7 +285,7 @@ skl_update_plane(struct drm_plane *drm_plane,
 	POSTING_READ(PLANE_SURF(pipe, plane));
 }
 
-static void
+void
 skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = dplane->dev;
@@ -752,7 +752,7 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	POSTING_READ(DVSSURF(pipe));
 }
 
-static int
+int
 intel_check_sprite_plane(struct drm_plane *plane,
 			 struct intel_crtc_state *crtc_state,
 			 struct intel_plane_state *state)
-- 
2.1.4

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

[RFC 4/4] drm/i915/gen9: Skip debugfs cursor output for universal plane platforms

[Matt Roper]

The universal plane acting as a cursor for userspace purposes still
shows up farther down the output so we still have all the important
information.

Refactor the cursor printing out to a new function while we're at it;
the nesting was getting a bit deep.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0bba472..d105777 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3008,6 +3008,23 @@ static bool cursor_position(struct drm_i915_private *dev_priv,
 	return cursor_active(dev_priv, pipe);
 }
 
+static void intel_cursor_info(struct seq_file *m, struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = node_to_i915(m->private);
+	bool active;
+	int x, y;
+
+	if (!INTEL_INFO(dev_priv)->has_real_cursor)
+		return;
+
+	active = cursor_position(dev_priv, crtc->pipe, &x, &y);
+	seq_printf(m, "\tcursor visible? %s, position (%d, %d), size %dx%d, addr 0x%08x, active? %s\n",
+		   yesno(crtc->cursor_base),
+		   x, y, crtc->base.cursor->state->crtc_w,
+		   crtc->base.cursor->state->crtc_h,
+		   crtc->cursor_addr, yesno(active));
+}
+
 static const char *plane_type(enum drm_plane_type type)
 {
 	switch (type) {
@@ -3130,9 +3147,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
 	seq_printf(m, "CRTC info\n");
 	seq_printf(m, "---------\n");
 	for_each_intel_crtc(dev, crtc) {
-		bool active;
 		struct intel_crtc_state *pipe_config;
-		int x, y;
 
 		pipe_config = to_intel_crtc_state(crtc->base.state);
 
@@ -3145,12 +3160,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
 		if (pipe_config->base.active) {
 			intel_crtc_info(m, crtc);
 
-			active = cursor_position(dev_priv, crtc->pipe, &x, &y);
-			seq_printf(m, "\tcursor visible? %s, position (%d, %d), size %dx%d, addr 0x%08x, active? %s\n",
-				   yesno(crtc->cursor_base),
-				   x, y, crtc->base.cursor->state->crtc_w,
-				   crtc->base.cursor->state->crtc_h,
-				   crtc->cursor_addr, yesno(active));
+			intel_cursor_info(m, crtc);
 			intel_scaler_info(m, crtc);
 			intel_plane_info(m, crtc);
 		}
-- 
2.1.4

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

✓ Fi.CI.BAT: success for Wire gen9 cursor interface to universal plane registers

[Patchwork]

== Series Details ==

Series: Wire gen9 cursor interface to universal plane registers
URL   : https://patchwork.freedesktop.org/series/14440/
State : success

== Summary ==

Series 14440v1 Wire gen9 cursor interface to universal plane registers
https://patchwork.freedesktop.org/api/1.0/series/14440/revisions/1/mbox/


fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
fi-ivb-3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 

5854c608e6354948f1f5e6be53132c897d0d4902 drm-intel-nightly: 2016y-10m-26d-21h-22m-02s UTC integration manifest
8db2854 drm/i915/gen9: Skip debugfs cursor output for universal plane platforms
9f8edab drm/i915/gen9: Expose top-most universal plane as cursor
471d03b drm/i915: Use macro in place of open-coded for_each_universal_plane loop
b7f616e drm/i915: Rename for_each_plane -> for_each_universal_plane

Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2833/

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2833/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 0/4] Wire gen9 cursor interface to universal plane registers

[Chris Wilson]

On Wed, Oct 26, 2016 at 03:51:27PM -0700, Matt Roper wrote:
> Gen9 has a traditional cursor plane that is mutually exclusive with the
> system's top-most "universal" plane; it seems likely that two planes are really
> a single shared hardware unit with two different register interfaces.  Thus far
> i915 has exposed a cursor plane to userspace that's hooked up to the old-style
> cursor registers; we just pretended that the top-most universal plane didn't
> exist and reported one fewer "sprite/overlay" planes for each pipe than the
> platform technically has.  Let's switch this around so that the cursor exposed
> to userspace is actually wired up to the previously-unused top-most universal
> plane registers.  With this change we'll still present the same cursor ABI to
> userspace that we always have, but internally we'll just be programming a
> different set of registers and doing so in a way that's more consistent with
> how all the other gen9 planes work --- less cursor-specific special cases
> throughout the code.

But you still report it as PLANE_TYPE_CURSOR right? Otherwise those that
both expose all the overlay planes and use the legacy cursor abi will be
trying to use that plane simultaneously via two different paths (and
clients). Or is this another cap?
	DRM_CLIENT_CAP_UNIVERSAL_PLANES = no-legacy-cursor-abi
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 0/4] Wire gen9 cursor interface to universal plane registers

[Matt Roper]

On Thu, Oct 27, 2016 at 01:15:03AM +0100, Chris Wilson wrote:
> On Wed, Oct 26, 2016 at 03:51:27PM -0700, Matt Roper wrote:
> > Gen9 has a traditional cursor plane that is mutually exclusive with the
> > system's top-most "universal" plane; it seems likely that two planes are really
> > a single shared hardware unit with two different register interfaces.  Thus far
> > i915 has exposed a cursor plane to userspace that's hooked up to the old-style
> > cursor registers; we just pretended that the top-most universal plane didn't
> > exist and reported one fewer "sprite/overlay" planes for each pipe than the
> > platform technically has.  Let's switch this around so that the cursor exposed
> > to userspace is actually wired up to the previously-unused top-most universal
> > plane registers.  With this change we'll still present the same cursor ABI to
> > userspace that we always have, but internally we'll just be programming a
> > different set of registers and doing so in a way that's more consistent with
> > how all the other gen9 planes work --- less cursor-specific special cases
> > throughout the code.
> 
> But you still report it as PLANE_TYPE_CURSOR right? Otherwise those that
> both expose all the overlay planes and use the legacy cursor abi will be
> trying to use that plane simultaneously via two different paths (and
> clients). Or is this another cap?
> 	DRM_CLIENT_CAP_UNIVERSAL_PLANES = no-legacy-cursor-abi

Right, with this patch it's still reported as PLANE_TYPE_CURSOR so
userspace won't see any difference.  The followup work to this will be
to eventually do one of the following:

 * Add a new capability bit "don't need legacy cursor" that disables the
   legacy cursor interfaces (-EINVAL) and makes these repurposed planes
   flip over to PLANE_TYPE_OVERLAY in the plane list for that specific
   client.  Clients that don't set it will continue to see
   PLANE_TYPE_CURSOR as usual.

or

 * Always leave the plane as DRM_PLANE_TYPE_CURSOR for all clients, but
   add some extra properties to them that hint to userspace that the
   plane might be more capable than a traditional cursor in some ways.
   Userspace could then do atomic TEST_ONLY to see whether the cursor is
   actually featureful enough to meet its general plane needs and just
   use it like an extra overlay if that turns out to be true.  In this
   case userspace should be smart enough to realize it's already using
   the cursor as a full-featured plane and not try to also submit
   conflicting legacy ioctls against it.

We actually have some experimental patches on an internal tree that use
the first approach above, but I'm open to either approach going forward.
Either way, I think the first step is to just make sure we're doing our
internal programming via the universal plane registers, regardless of
how the plane gets exposed to userspace.

Those next steps are probably something to discuss at the general DRM
level; my understanding is that a lot of ARM platforms don't really have
dedicated cursors at all, so *all* of their cursor planes are actually
repurposed sprites that could probably be put to better use in some
use cases.


Matt

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 0/4] Wire gen9 cursor interface to universal plane registers

[Daniel Vetter]

On Wed, Oct 26, 2016 at 05:30:04PM -0700, Matt Roper wrote:
> On Thu, Oct 27, 2016 at 01:15:03AM +0100, Chris Wilson wrote:
> > On Wed, Oct 26, 2016 at 03:51:27PM -0700, Matt Roper wrote:
> > > Gen9 has a traditional cursor plane that is mutually exclusive with the
> > > system's top-most "universal" plane; it seems likely that two planes are really
> > > a single shared hardware unit with two different register interfaces.  Thus far
> > > i915 has exposed a cursor plane to userspace that's hooked up to the old-style
> > > cursor registers; we just pretended that the top-most universal plane didn't
> > > exist and reported one fewer "sprite/overlay" planes for each pipe than the
> > > platform technically has.  Let's switch this around so that the cursor exposed
> > > to userspace is actually wired up to the previously-unused top-most universal
> > > plane registers.  With this change we'll still present the same cursor ABI to
> > > userspace that we always have, but internally we'll just be programming a
> > > different set of registers and doing so in a way that's more consistent with
> > > how all the other gen9 planes work --- less cursor-specific special cases
> > > throughout the code.
> > 
> > But you still report it as PLANE_TYPE_CURSOR right? Otherwise those that
> > both expose all the overlay planes and use the legacy cursor abi will be
> > trying to use that plane simultaneously via two different paths (and
> > clients). Or is this another cap?
> > 	DRM_CLIENT_CAP_UNIVERSAL_PLANES = no-legacy-cursor-abi
> 
> Right, with this patch it's still reported as PLANE_TYPE_CURSOR so
> userspace won't see any difference.  The followup work to this will be
> to eventually do one of the following:
> 
>  * Add a new capability bit "don't need legacy cursor" that disables the
>    legacy cursor interfaces (-EINVAL) and makes these repurposed planes
>    flip over to PLANE_TYPE_OVERLAY in the plane list for that specific
>    client.  Clients that don't set it will continue to see
>    PLANE_TYPE_CURSOR as usual.
> 
> or
> 
>  * Always leave the plane as DRM_PLANE_TYPE_CURSOR for all clients, but
>    add some extra properties to them that hint to userspace that the
>    plane might be more capable than a traditional cursor in some ways.
>    Userspace could then do atomic TEST_ONLY to see whether the cursor is
>    actually featureful enough to meet its general plane needs and just
>    use it like an extra overlay if that turns out to be true.  In this
>    case userspace should be smart enough to realize it's already using
>    the cursor as a full-featured plane and not try to also submit
>    conflicting legacy ioctls against it.

Imo userspace can just do a test-only and figure this out. DRM_PLANE_TYPE
is _purely_ about uabi backwards compat, if you try to infer anything
about what the plane can do you'll get it wrong. Except that such a plane
is probably more useful for one of the old legacy use-cases, e.g. primary
plane should be able to be used full-screen and unscaled.

So yes this is the approach I expect userspace to use.

> We actually have some experimental patches on an internal tree that use
> the first approach above, but I'm open to either approach going forward.
> Either way, I think the first step is to just make sure we're doing our
> internal programming via the universal plane registers, regardless of
> how the plane gets exposed to userspace.
> 
> Those next steps are probably something to discuss at the general DRM
> level; my understanding is that a lot of ARM platforms don't really have
> dedicated cursors at all, so *all* of their cursor planes are actually
> repurposed sprites that could probably be put to better use in some
> use cases.

Yup. Aside: There's some serious hacking going on in drm_hwcomposer, and
no one from intel participates :( We're running a good risk of ending up
with userspace behaviour that's not really great on our hw, instead of
defining what good hw should look like ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/4] drm/i915/gen9: Expose top-most universal plane as cursor

[Paulo Zanoni]

Em Qua, 2016-10-26 às 15:51 -0700, Matt Roper escreveu:
> Gen9 has a traditional cursor plane that is mutually exclusive with
> the
> system's top-most "universal" plane; it seems likely that two planes
> are
> really a single shared hardware unit with two different register
> interfaces.  Thus far i915 has exposed a cursor plane to userspace
> that's hooked up to the old-style cursor registers; we just pretended
> that the top-most universal plane didn't exist and reported one fewer
> "sprite/overlay" planes for each pipe than the platform technically
> has.
> Let's switch this around so that the cursor exposed to userspace is
> actually wired up to top-most universal plane registers.  We'll
> continue
> to present the same cursor ABI to userspace that we always have, but
> internally we'll just be programming a different set of registers and
> doing so in a way that's more consistent with how all the other gen9
> planes work --- less cursor-specific special cases throughout the
> code.
> 
> Aside from making the code a bit simpler (fewer cursor-specific
> special
> cases), this will also pave the way to eventually exposing the top-
> most
> plane in a more full-featured manner to userspace clients that don't
> need a traditional cursor and wish to opt into having access to an
> additional sprite/overlay-style plane instead.
> 
> It's worth noting that a lot of the special-cased cursor-specific
> code
> was in the gen9 watermark programming.  It's good to simplify that
> code,
> but we should keep an eye out for any unwanted side effects of this
> patch; since sprites/overlays tend to get used less than cursors,
> it's
> possible that this could help us uncover additional underruns that
> nobody had run across yet.  Or it could have the opposite effect and
> eliminate some of the underruns that we haven't been able to get rid
> of
> yet.
> 
> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  4 --
>  drivers/gpu/drm/i915/i915_drv.h          | 11 +++-
>  drivers/gpu/drm/i915/intel_device_info.c | 38 +++++++++----
>  drivers/gpu/drm/i915/intel_display.c     | 97 ++++++++++++--------
> ------------
>  drivers/gpu/drm/i915/intel_drv.h         |  7 +++
>  drivers/gpu/drm/i915/intel_pm.c          | 85 ++++----------------
> --------
>  drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
>  7 files changed, 93 insertions(+), 155 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9f5a392..0bba472 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3469,10 +3469,6 @@ static int i915_ddb_info(struct seq_file *m,
> void *unused)
>  				   entry->start, entry->end,
>  				   skl_ddb_entry_size(entry));
>  		}
> -
> -		entry = &ddb->plane[pipe][PLANE_CURSOR];
> -		seq_printf(m, "  %-13s%8u%8u%8u\n", "Cursor", entry-
> >start,
> -			   entry->end, skl_ddb_entry_size(entry));
>  	}
>  
>  	drm_modeset_unlock_all(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 4714051..83aaed2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -178,7 +178,7 @@ enum plane {
>  	PLANE_A = 0,
>  	PLANE_B,
>  	PLANE_C,
> -	PLANE_CURSOR,
> +	PLANE_D,
>  	I915_MAX_PLANES,
>  };
>  #define plane_name(p) ((p) + 'A')
> @@ -316,9 +316,15 @@ struct i915_hotplug {
>  	for ((__p) = 0;						
> 	\
>  	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] +
> 1;	\
>  	     (__p)++)
> +
> +/*
> + * Only iterate over sprites exposed as sprites; omit sprites that
> + * are being repurposed to simulate a cursor.
> + */
>  #define for_each_sprite(__dev_priv, __p, __s)			
> 	\
>  	for ((__s) = 0;						
> 	\
> -	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)];	
> \
> +	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)] -	
> \
> +	             (INTEL_INFO(__dev_priv)->has_real_cursor ? 0 :
> 1);	\
>  	     (__s)++)
>  
>  #define for_each_port_masked(__port, __ports_mask) \
> @@ -687,6 +693,7 @@ struct intel_csr {
>  	func(has_psr); \
>  	func(has_rc6); \
>  	func(has_rc6p); \
> +	func(has_real_cursor); \
>  	func(has_resource_streamer); \
>  	func(has_runtime_pm); \
>  	func(has_snoop); \
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> b/drivers/gpu/drm/i915/intel_device_info.c
> index d6a8f11..a464e0e 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -271,23 +271,39 @@ void intel_device_info_runtime_init(struct
> drm_i915_private *dev_priv)
>  	enum pipe pipe;
>  
>  	/*
> -	 * Skylake and Broxton currently don't expose the topmost
> plane as its
> -	 * use is exclusive with the legacy cursor and we only want
> to expose
> -	 * one of those, not both. Until we can safely expose the
> topmost plane
> -	 * as a DRM_PLANE_TYPE_CURSOR with all the features
> exposed/supported,
> -	 * we don't expose the topmost plane at all to prevent ABI
> breakage
> -	 * down the line.
> +	 * Gen9 platforms have a top-most universal (i.e., sprite)
> plane and a
> +	 * cursor plane that are mutually exclusive.  If we use the
> cursor
> +	 * plane we permanently lose the ability to make use of the
> more
> +	 * full-featured universal plane.  So instead let's use all
> of the
> +	 * universal planes, ignore the cursor plane, but hook the
> top-most
> +	 * universal plane up to the legacy cursor ioctl's and
> expose it to
> +	 * userspace as DRM_PLANE_TYPE_CURSOR.  This won't result in
> any
> +	 * visible behavior change to userspace; we're just
> internally
> +	 * programming a different set of registers.
> +	 *
> +	 * For the purposes of device_info, we're only concerned
> with the
> +	 * number of universal planes we're programming, regardless
> of how they
> +	 * get mapped to userspace interfaces, so we'll report the
> true number of
> +	 * universal planes for gen9.
>  	 */
>  	if (IS_BROXTON(dev_priv)) {
> -		info->num_sprites[PIPE_A] = 2;
> -		info->num_sprites[PIPE_B] = 2;
> -		info->num_sprites[PIPE_C] = 1;
> -	} else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv))
> +		info->has_real_cursor = 0;
> +		info->num_sprites[PIPE_A] = 3;
> +		info->num_sprites[PIPE_B] = 3;
> +		info->num_sprites[PIPE_C] = 2;
> +	} else if (IS_GEN9(dev_priv)) {
> +		info->has_real_cursor = 0;
>  		for_each_pipe(dev_priv, pipe)
>  			info->num_sprites[pipe] = 2;
> -	else
> +	} else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv)) {
> +		info->has_real_cursor = 1;
> +		for_each_pipe(dev_priv, pipe)
> +			info->num_sprites[pipe] = 2;
> +	} else {
> +		info->has_real_cursor = 1;
>  		for_each_pipe(dev_priv, pipe)
>  			info->num_sprites[pipe] = 1;
> +	}
>  
>  	if (i915.disable_display) {
>  		DRM_INFO("Display disabled (module parameter)\n");
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index cb7dd11..9a8c2b1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1232,6 +1232,23 @@ void assert_panel_unlocked(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  	     pipe_name(pipe));
>  }
>  
> +/*
> + * Cursor state for platforms that use a universal plane as a
> cursor.
> + * Primary is universal #0, others are universal #1-
> numsprites.  Cursor
> + * will be the final universal plane for the pipe.
> + */
> +static bool
> +universal_cursor_state(struct drm_i915_private *dev_priv,

-ENOVERB

get_universal_cursor_state()?

> +		       enum pipe pipe)
> +{
> +	unsigned int planenum = INTEL_INFO(dev_priv)-
> >num_sprites[pipe];
> +	u32 val;
> +
> +	val = I915_READ(PLANE_CTL(pipe, planenum));
> +
> +	return val & PLANE_CTL_ENABLE;
> +}
> +
>  static void assert_cursor(struct drm_i915_private *dev_priv,
>  			  enum pipe pipe, bool state)
>  {
> @@ -1239,6 +1256,8 @@ static void assert_cursor(struct
> drm_i915_private *dev_priv,
>  
>  	if (IS_845G(dev_priv) || IS_I865G(dev_priv))
>  		cur_state = I915_READ(CURCNTR(PIPE_A)) &
> CURSOR_ENABLE;
> +	else if (!INTEL_INFO(dev_priv)->has_real_cursor)
> +		cur_state = universal_cursor_state(dev_priv, pipe);
>  	else
>  		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
>  
> @@ -10841,15 +10860,16 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> +	/*
> +	 * Although gen9 has legacy cursor registers, their use is
> mutually
> +	 * exclusive with the top-most universal plane.  We'll just
> use the
> +	 * universal plane to simulate the legacy cursor interface
> instead,
> +	 * which means we'll never enter here on gen9 platforms.
> +	 */
> +	WARN_ON(IS_GEN9(dev_priv));
>  
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
> @@ -13528,56 +13548,6 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
>  		}
>  	}
> -
> -	/*
> -	 * cursor
> -	 * If the cursor plane isn't active, we may not have updated
> it's ddb
> -	 * allocation. In that case since the ddb allocation will be
> updated
> -	 * once the plane becomes visible, we can skip this check
> -	 */
> -	if (intel_crtc->cursor_addr) {
> -		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> -		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> -
> -		/* Watermarks */
> -		for (level = 0; level <= max_level; level++) {
> -			if (skl_wm_level_equals(&hw_plane_wm-
> >wm[level],
> -						&sw_plane_wm-
> >wm[level]))
> -				continue;
> -
> -			DRM_ERROR("mismatch in WM pipe %c cursor
> level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe), level,
> -				  sw_plane_wm->wm[level].plane_en,
> -				  sw_plane_wm-
> >wm[level].plane_res_b,
> -				  sw_plane_wm-
> >wm[level].plane_res_l,
> -				  hw_plane_wm->wm[level].plane_en,
> -				  hw_plane_wm-
> >wm[level].plane_res_b,
> -				  hw_plane_wm-
> >wm[level].plane_res_l);
> -		}
> -
> -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> -					 &sw_plane_wm->trans_wm)) {
> -			DRM_ERROR("mismatch in trans WM pipe %c
> cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe),
> -				  sw_plane_wm->trans_wm.plane_en,
> -				  sw_plane_wm->trans_wm.plane_res_b,
> -				  sw_plane_wm->trans_wm.plane_res_l,
> -				  hw_plane_wm->trans_wm.plane_en,
> -				  hw_plane_wm->trans_wm.plane_res_b,
> -				  hw_plane_wm-
> >trans_wm.plane_res_l);
> -		}
> -
> -		/* DDB */
> -		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> -		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> -
> -		if (!skl_ddb_entry_equal(hw_ddb_entry,
> sw_ddb_entry)) {
> -			DRM_ERROR("mismatch in DDB state pipe %c
> cursor (expected (%u,%u), found (%u,%u))\n",
> -				  pipe_name(pipe),
> -				  sw_ddb_entry->start, sw_ddb_entry-
> >end,
> -				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
> -		}
> -	}
>  }
>  
>  static void
> @@ -15215,11 +15185,18 @@ static struct drm_plane
> *intel_cursor_plane_create(struct drm_device *dev,
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
> -	cursor->plane = pipe;
>  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> -	cursor->check_plane = intel_check_cursor_plane;
> -	cursor->update_plane = intel_update_cursor_plane;
> -	cursor->disable_plane = intel_disable_cursor_plane;
> +	if (INTEL_INFO(dev)->has_real_cursor) {
> +		cursor->plane = pipe;  /* unused? */
> +		cursor->check_plane = intel_check_cursor_plane;
> +		cursor->update_plane = intel_update_cursor_plane;
> +		cursor->disable_plane = intel_disable_cursor_plane;
> +	} else {
> +		cursor->plane = INTEL_INFO(dev)->num_sprites[pipe] -
> 1;
> +		cursor->check_plane = intel_check_sprite_plane;
> +		cursor->update_plane = skl_update_plane;
> +		cursor->disable_plane = skl_disable_plane;
> +	}
>  
>  	ret = drm_universal_plane_init(dev, &cursor->base, 0,
>  				       &intel_plane_funcs,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index c31fddd..50874e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1790,6 +1790,13 @@ int intel_sprite_set_colorkey(struct
> drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  void intel_pipe_update_start(struct intel_crtc *crtc);
>  void intel_pipe_update_end(struct intel_crtc *crtc, struct
> intel_flip_work *work);
> +int intel_check_sprite_plane(struct drm_plane *plane,
> +			     struct intel_crtc_state *crtc_state,
> +			     struct intel_plane_state *state);
> +void skl_update_plane(struct drm_plane *drm_plane,
> +		      const struct intel_crtc_state *crtc_state,
> +		      const struct intel_plane_state *plane_state);
> +void skl_disable_plane(struct drm_plane *dplane, struct drm_crtc
> *crtc);
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 6f19e60..e75f6d8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2863,9 +2863,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
>  
>  /*
>   * Return the index of a plane in the SKL DDB and wm result
> arrays.  Primary
> - * plane is always in slot 0, cursor is always in slot
> I915_MAX_PLANES-1, and
> - * other universal planes are in indices 1..n.  Note that this may
> leave unused
> - * indices between the top "sprite" plane and the cursor.
> + * plane is always in slot 0 and other universal planes are in
> indices 1..n.
>   */
>  static int
>  skl_wm_plane_id(const struct intel_plane *plane)
> @@ -2874,7 +2872,6 @@ skl_wm_plane_id(const struct intel_plane
> *plane)
>  	case DRM_PLANE_TYPE_PRIMARY:
>  		return 0;
>  	case DRM_PLANE_TYPE_CURSOR:
> -		return PLANE_CURSOR;
>  	case DRM_PLANE_TYPE_OVERLAY:
>  		return plane->plane + 1;
>  	default:
> @@ -3128,14 +3125,6 @@ skl_ddb_get_pipe_allocation_limits(struct
> drm_device *dev,
>  	alloc->end = alloc->start + pipe_size;
>  }
>  
> -static unsigned int skl_cursor_allocation(int num_active)
> -{
> -	if (num_active == 1)
> -		return 32;
> -
> -	return 8;
> -}
> -
>  static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry,
> u32 reg)
>  {
>  	entry->start = reg & 0x3ff;
> @@ -3166,10 +3155,6 @@ void skl_ddb_get_hw_state(struct
> drm_i915_private *dev_priv,
>  						   val);
>  		}
>  
> -		val = I915_READ(CUR_BUF_CFG(pipe));
> -		skl_ddb_entry_init_from_hw(&ddb-
> >plane[pipe][PLANE_CURSOR],
> -					   val);
> -
>  		intel_display_power_put(dev_priv, power_domain);
>  	}
>  }
> @@ -3227,8 +3212,6 @@ skl_plane_relative_data_rate(const struct
> intel_crtc_state *cstate,
>  
>  	if (!intel_pstate->base.visible)
>  		return 0;
> -	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
> -		return 0;
>  	if (y && format != DRM_FORMAT_NV12)
>  		return 0;
>  
> @@ -3386,7 +3369,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> -	uint16_t alloc_size, start, cursor_blocks;
> +	uint16_t alloc_size, start;
>  	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
>  	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
>  	unsigned int total_data_rate;
> @@ -3412,12 +3395,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  	}
>  
> -	cursor_blocks = skl_cursor_allocation(num_active);
> -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> cursor_blocks;
> -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> -
> -	alloc_size -= cursor_blocks;

So this means that now in step 2 we're going to try to increase the
cursor DDBs beyond the current fixed sizes of 32 or 8? Aren't we going
to leave the cursor DDB way-too-big and taking space of the other DDBs
when it's just being used as a cursor?

Also, we're not going to leave the cursor DDB at the end of the buffer,
which may cause some extra DDB reallocations that we may not want.
Also, whenever someone disables the cursor we'll do a DDB reallocation,
and as far as I understand we really don't want this specific case
since it's so common.

And there's a sill-not-implemented workaround for watermarks
programming related to the memory bandwidth and it also special-cases
the cursor. Is there a way to know when the cursor is being used as a
cursor or when it's being used as a normal plane? Maybe just check it
by a size threshold?

My anxiety levels always go up whenever I see someone changing the code
in a way that makes it not match with what's written in the spec
anymore. Maybe we'll be fine with the changes, but we need to carefully
analyze the impact here, especially since watermarks are still so
fragile. Maybe we should just ping the spec authors and make them
rewrite the specs in a way that considers the cases where the cursor
plane is being used as a generic plane?

> -
>  	/* 1. Allocate the mininum required blocks for each active
> plane */
>  	for_each_plane_in_state(state, plane, pstate, i) {
>  		intel_plane = to_intel_plane(plane);
> @@ -3431,17 +3408,12 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  			y_minimum[id] = 0;
>  			continue;
>  		}
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> -			minimum[id] = 0;
> -			y_minimum[id] = 0;
> -			continue;
> -		}
>  
>  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
>  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
>  	}
>  
> -	for (i = 0; i < PLANE_CURSOR; i++) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {
>  		alloc_size -= minimum[i];
>  		alloc_size -= y_minimum[i];
>  	}
> @@ -3866,26 +3838,6 @@ void skl_write_plane_wm(struct intel_crtc
> *intel_crtc,
>  			    &ddb->y_plane[pipe][plane]);
>  }
>  
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb)
> -{
> -	struct drm_crtc *crtc = &intel_crtc->base;
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int level, max_level = ilk_wm_max_level(dev_priv);
> -	enum pipe pipe = intel_crtc->pipe;
> -
> -	for (level = 0; level <= max_level; level++) {
> -		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> -				   &wm->wm[level]);
> -	}
> -	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
> >trans_wm);
> -
> -	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> -			    &ddb->plane[pipe][PLANE_CURSOR]);
> -}
> -
>  bool skl_wm_level_equals(const struct skl_wm_level *l1,
>  			 const struct skl_wm_level *l2)
>  {
> @@ -4122,19 +4074,11 @@ skl_print_wm_changes(const struct
> drm_atomic_state *state)
>  			if (skl_ddb_entry_equal(old, new))
>  				continue;
>  
> -			if (id != PLANE_CURSOR) {
> -				DRM_DEBUG_ATOMIC("[PLANE:%d:plane
> %d%c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id, id
> + 1,
> -						 pipe_name(pipe),
> -						 old->start, old-
> >end,
> -						 new->start, new-
> >end);
> -			} else {
> -				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor
> %c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id,
> -						 pipe_name(pipe),
> -						 old->start, old-
> >end,
> -						 new->start, new-
> >end);
> -			}
> +			DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb
> (%d - %d) -> (%d - %d)\n",
> +					 plane->base.id, id + 1,
> +					 pipe_name(pipe),
> +					 old->start, old->end,
> +					 new->start, new->end);
>  		}
>  	}
>  }
> @@ -4235,9 +4179,6 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  		for_each_universal_plane(dev_priv, pipe, plane)
>  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
>  					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> >planes[PLANE_CURSOR],
> -				    &results->ddb);
>  	}
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> @@ -4350,18 +4291,12 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc
> *crtc,
>  		wm = &out->planes[id];
>  
>  		for (level = 0; level <= max_level; level++) {
> -			if (id != PLANE_CURSOR)
> -				val = I915_READ(PLANE_WM(pipe, id,
> level));
> -			else
> -				val = I915_READ(CUR_WM(pipe,
> level));
> +			val = I915_READ(PLANE_WM(pipe, id, level));
>  
>  			skl_wm_level_from_reg_val(val, &wm-
> >wm[level]);
>  		}
>  
> -		if (id != PLANE_CURSOR)
> -			val = I915_READ(PLANE_WM_TRANS(pipe, id));
> -		else
> -			val = I915_READ(CUR_WM_TRANS(pipe));
> +		val = I915_READ(PLANE_WM_TRANS(pipe, id));
>  
>  		skl_wm_level_from_reg_val(val, &wm->trans_wm);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 43d0350..9e6406a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -194,7 +194,7 @@ void intel_pipe_update_end(struct intel_crtc
> *crtc, struct intel_flip_work *work
>  	}
>  }
>  
> -static void
> +void
>  skl_update_plane(struct drm_plane *drm_plane,
>  		 const struct intel_crtc_state *crtc_state,
>  		 const struct intel_plane_state *plane_state)
> @@ -285,7 +285,7 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	POSTING_READ(PLANE_SURF(pipe, plane));
>  }
>  
> -static void
> +void
>  skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = dplane->dev;
> @@ -752,7 +752,7 @@ ilk_disable_plane(struct drm_plane *plane, struct
> drm_crtc *crtc)
>  	POSTING_READ(DVSSURF(pipe));
>  }
>  
> -static int
> +int
>  intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_crtc_state *crtc_state,
>  			 struct intel_plane_state *state)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/4] drm/i915/gen9: Expose top-most universal plane as cursor

[Matt Roper]

On Thu, Oct 27, 2016 at 06:03:54PM -0200, Paulo Zanoni wrote:
> Em Qua, 2016-10-26 às 15:51 -0700, Matt Roper escreveu:
> > Gen9 has a traditional cursor plane that is mutually exclusive with
> > the
> > system's top-most "universal" plane; it seems likely that two planes
> > are
> > really a single shared hardware unit with two different register
> > interfaces.  Thus far i915 has exposed a cursor plane to userspace
> > that's hooked up to the old-style cursor registers; we just pretended
> > that the top-most universal plane didn't exist and reported one fewer
> > "sprite/overlay" planes for each pipe than the platform technically
> > has.
> > Let's switch this around so that the cursor exposed to userspace is
> > actually wired up to top-most universal plane registers.  We'll
> > continue
> > to present the same cursor ABI to userspace that we always have, but
> > internally we'll just be programming a different set of registers and
> > doing so in a way that's more consistent with how all the other gen9
> > planes work --- less cursor-specific special cases throughout the
> > code.
> > 
> > Aside from making the code a bit simpler (fewer cursor-specific
> > special
> > cases), this will also pave the way to eventually exposing the top-
> > most
> > plane in a more full-featured manner to userspace clients that don't
> > need a traditional cursor and wish to opt into having access to an
> > additional sprite/overlay-style plane instead.
> > 
> > It's worth noting that a lot of the special-cased cursor-specific
> > code
> > was in the gen9 watermark programming.  It's good to simplify that
> > code,
> > but we should keep an eye out for any unwanted side effects of this
> > patch; since sprites/overlays tend to get used less than cursors,
> > it's
> > possible that this could help us uncover additional underruns that
> > nobody had run across yet.  Or it could have the opposite effect and
> > eliminate some of the underruns that we haven't been able to get rid
> > of
> > yet.
> > 
> > Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c      |  4 --
> >  drivers/gpu/drm/i915/i915_drv.h          | 11 +++-
> >  drivers/gpu/drm/i915/intel_device_info.c | 38 +++++++++----
> >  drivers/gpu/drm/i915/intel_display.c     | 97 ++++++++++++--------
> > ------------
> >  drivers/gpu/drm/i915/intel_drv.h         |  7 +++
> >  drivers/gpu/drm/i915/intel_pm.c          | 85 ++++----------------
> > --------
> >  drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
> >  7 files changed, 93 insertions(+), 155 deletions(-)
> > 
...
> >  
> > -	cursor_blocks = skl_cursor_allocation(num_active);
> > -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> > cursor_blocks;
> > -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> > -
> > -	alloc_size -= cursor_blocks;
> 
> So this means that now in step 2 we're going to try to increase the
> cursor DDBs beyond the current fixed sizes of 32 or 8? Aren't we going
> to leave the cursor DDB way-too-big and taking space of the other DDBs
> when it's just being used as a cursor?

You're right that we're not using the fixed size allocation anymore.
However the extra blocks that we give the pseudo-cursor should still be
proportional to the size/data rate of the plane.  So if you're using a
64x64 "cursor," that's going to be very small compared to your other
planes (the primary plane is often fullscreen and other universal planes
are a good portion of full size).  In practice it seems to work out that
the extra blocks calculation winds up rounding down to zero or near zero
since the "cursor" is so dwarfed by the primary plane --- on my
1920x1200 display it works out as:

        Primary data rate:  8294400
        Cursor data rate:     16384
        Total data rate:    8310784
        Remaining DDB blocks:   492

        Primary extra blocks = 8294400 * 492 / 8310784
                             = (int)491.03006
                             = 491
        Cursor extra blocks = 16384 * 492 / 8310784
                             = (int)0.969935
                             = 0

Due to rounding we don't actually add any cursor blocks at all.  In fact
we're using _fewer_ blocks for cursor than the 32 fixed allocation we
were previously using for single display setups.

> Also, we're not going to leave the cursor DDB at the end of the buffer,
> which may cause some extra DDB reallocations that we may not want.
> Also, whenever someone disables the cursor we'll do a DDB reallocation,
> and as far as I understand we really don't want this specific case
> since it's so common.

True, but the reallocation in this case is an intra-pipe reallocation
rather than the horrific "lock the whole world" inter-pipe reallocation.
It means we crunch a few more numbers during atomic check and write a
few registers we might have skipped during atomic commit, but there's no
extra locking or major new work happening, so it doesn't seem like it
really adds too much overhead, even if userspace is trying to turn the
cursor on and off nonstop like some DDX's do.

> And there's a sill-not-implemented workaround for watermarks
> programming related to the memory bandwidth and it also special-cases
> the cursor. Is there a way to know when the cursor is being used as a
> cursor or when it's being used as a normal plane? Maybe just check it
> by a size threshold?

Well in this case we're effectively not using the cursor anymore from
the hardware's point of view; it's disabled.  You're just using the
"primary" plane and at least one additional universal (aka "sprite")
plane, which is already a valid display configuration that we encounter
frequently even without this patch.  So the workarounds should already
cover that case properly.

Basically we don't need to use the hardware's idea of what a cursor is
in order to implement userspace's idea of what the cursor is.

> My anxiety levels always go up whenever I see someone changing the code
> in a way that makes it not match with what's written in the spec
> anymore. Maybe we'll be fine with the changes, but we need to carefully
> analyze the impact here, especially since watermarks are still so
> fragile. Maybe we should just ping the spec authors and make them
> rewrite the specs in a way that considers the cases where the cursor
> plane is being used as a generic plane?

I think I may not have done a very good job of explaining what's
happening here --- the code in this patch should still fully match the
spec.  The spec covers how to use all of the planes on the system and
you're free to turn on or turn off any combination of them you want.
The exception is that the topmost universal plane and the cursor can't
be used at the same time.  Previously that meant we just never let the
topmost plane get turned on at all (to ensure no conflict), but the spec
still describes how the top plane should be programmed and accounted for
in workarounds (i.e., the exact same way as the other universal planes);
the changes here should all adhere to that.  Now we're flipping around
how we ensure mututal exclusion to be "we'll never turn on the cursor
plane" instead, which is perfectly legal.

The hardware level details here are independent of the plane list we
expose to userspace.  For ABI purposes we always need *something* that
is identified as a "cursor" and is hooked up to the legacy cursor
ioctls.  There's nothing that says we actually have to implement that
userspace interface using our hardware's dedicated cursor plane ---
we're free to re-use any other plane our hardware happens to have as
well.  That's what I'm doing here...the topmost universal/sprite plane
that was previously never used at all is now reserved for servicing
requests against the plane identified as DRM_PLANE_TYPE_CURSOR.
Userspace never notices a difference as long as the universal plane gets
the cursor content onto the screen the same way as the cursor plane.

I think this is actually pretty similar to what happens a lot in the ARM
world.  My understanding is that most of the ARM guys don't even have a
true "cursor" plane, but rather just a bunch of general purpose planes
that are more similar to what we call universal planes.  They'll
typically reserve one of those planes for cursor ABI compatibility.

Hopefully that's more clear...if not let me know.


Matt

> 
> > -
> >  	/* 1. Allocate the mininum required blocks for each active
> > plane */
> >  	for_each_plane_in_state(state, plane, pstate, i) {
> >  		intel_plane = to_intel_plane(plane);
> > @@ -3431,17 +3408,12 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  			y_minimum[id] = 0;
> >  			continue;
> >  		}
> > -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > -			minimum[id] = 0;
> > -			y_minimum[id] = 0;
> > -			continue;
> > -		}
> >  
> >  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> >  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> >  	}
> >  
> > -	for (i = 0; i < PLANE_CURSOR; i++) {
> > +	for (i = 0; i < I915_MAX_PLANES; i++) {
> >  		alloc_size -= minimum[i];
> >  		alloc_size -= y_minimum[i];
> >  	}
> > @@ -3866,26 +3838,6 @@ void skl_write_plane_wm(struct intel_crtc
> > *intel_crtc,
> >  			    &ddb->y_plane[pipe][plane]);
> >  }
> >  
> > -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > -			 const struct skl_plane_wm *wm,
> > -			 const struct skl_ddb_allocation *ddb)
> > -{
> > -	struct drm_crtc *crtc = &intel_crtc->base;
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	int level, max_level = ilk_wm_max_level(dev_priv);
> > -	enum pipe pipe = intel_crtc->pipe;
> > -
> > -	for (level = 0; level <= max_level; level++) {
> > -		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> > -				   &wm->wm[level]);
> > -	}
> > -	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
> > >trans_wm);
> > -
> > -	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> > -			    &ddb->plane[pipe][PLANE_CURSOR]);
> > -}
> > -
> >  bool skl_wm_level_equals(const struct skl_wm_level *l1,
> >  			 const struct skl_wm_level *l2)
> >  {
> > @@ -4122,19 +4074,11 @@ skl_print_wm_changes(const struct
> > drm_atomic_state *state)
> >  			if (skl_ddb_entry_equal(old, new))
> >  				continue;
> >  
> > -			if (id != PLANE_CURSOR) {
> > -				DRM_DEBUG_ATOMIC("[PLANE:%d:plane
> > %d%c] ddb (%d - %d) -> (%d - %d)\n",
> > -						 plane->base.id, id
> > + 1,
> > -						 pipe_name(pipe),
> > -						 old->start, old-
> > >end,
> > -						 new->start, new-
> > >end);
> > -			} else {
> > -				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor
> > %c] ddb (%d - %d) -> (%d - %d)\n",
> > -						 plane->base.id,
> > -						 pipe_name(pipe),
> > -						 old->start, old-
> > >end,
> > -						 new->start, new-
> > >end);
> > -			}
> > +			DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb
> > (%d - %d) -> (%d - %d)\n",
> > +					 plane->base.id, id + 1,
> > +					 pipe_name(pipe),
> > +					 old->start, old->end,
> > +					 new->start, new->end);
> >  		}
> >  	}
> >  }
> > @@ -4235,9 +4179,6 @@ static void skl_update_wm(struct drm_crtc
> > *crtc)
> >  		for_each_universal_plane(dev_priv, pipe, plane)
> >  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> > >planes[plane],
> >  					   &results->ddb, plane);
> > -
> > -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> > >planes[PLANE_CURSOR],
> > -				    &results->ddb);
> >  	}
> >  
> >  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> > @@ -4350,18 +4291,12 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc
> > *crtc,
> >  		wm = &out->planes[id];
> >  
> >  		for (level = 0; level <= max_level; level++) {
> > -			if (id != PLANE_CURSOR)
> > -				val = I915_READ(PLANE_WM(pipe, id,
> > level));
> > -			else
> > -				val = I915_READ(CUR_WM(pipe,
> > level));
> > +			val = I915_READ(PLANE_WM(pipe, id, level));
> >  
> >  			skl_wm_level_from_reg_val(val, &wm-
> > >wm[level]);
> >  		}
> >  
> > -		if (id != PLANE_CURSOR)
> > -			val = I915_READ(PLANE_WM_TRANS(pipe, id));
> > -		else
> > -			val = I915_READ(CUR_WM_TRANS(pipe));
> > +		val = I915_READ(PLANE_WM_TRANS(pipe, id));
> >  
> >  		skl_wm_level_from_reg_val(val, &wm->trans_wm);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 43d0350..9e6406a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -194,7 +194,7 @@ void intel_pipe_update_end(struct intel_crtc
> > *crtc, struct intel_flip_work *work
> >  	}
> >  }
> >  
> > -static void
> > +void
> >  skl_update_plane(struct drm_plane *drm_plane,
> >  		 const struct intel_crtc_state *crtc_state,
> >  		 const struct intel_plane_state *plane_state)
> > @@ -285,7 +285,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	POSTING_READ(PLANE_SURF(pipe, plane));
> >  }
> >  
> > -static void
> > +void
> >  skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = dplane->dev;
> > @@ -752,7 +752,7 @@ ilk_disable_plane(struct drm_plane *plane, struct
> > drm_crtc *crtc)
> >  	POSTING_READ(DVSSURF(pipe));
> >  }
> >  
> > -static int
> > +int
> >  intel_check_sprite_plane(struct drm_plane *plane,
> >  			 struct intel_crtc_state *crtc_state,
> >  			 struct intel_plane_state *state)

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/4] drm/i915/gen9: Expose top-most universal plane as cursor

[Lyude Paul]

On Wed, 2016-10-26 at 15:51 -0700, Matt Roper wrote:
> Gen9 has a traditional cursor plane that is mutually exclusive with
> the
> system's top-most "universal" plane; it seems likely that two planes
> are
> really a single shared hardware unit with two different register
> interfaces.  Thus far i915 has exposed a cursor plane to userspace
> that's hooked up to the old-style cursor registers; we just pretended
> that the top-most universal plane didn't exist and reported one fewer
> "sprite/overlay" planes for each pipe than the platform technically
> has.
> Let's switch this around so that the cursor exposed to userspace is
> actually wired up to top-most universal plane registers.  We'll
> continue
> to present the same cursor ABI to userspace that we always have, but
> internally we'll just be programming a different set of registers and
> doing so in a way that's more consistent with how all the other gen9
> planes work --- less cursor-specific special cases throughout the
> code.
> 
> Aside from making the code a bit simpler (fewer cursor-specific
> special
> cases), this will also pave the way to eventually exposing the top-
> most
> plane in a more full-featured manner to userspace clients that don't
> need a traditional cursor and wish to opt into having access to an
> additional sprite/overlay-style plane instead.
> 
> It's worth noting that a lot of the special-cased cursor-specific
> code
> was in the gen9 watermark programming.  It's good to simplify that
> code,
> but we should keep an eye out for any unwanted side effects of this
> patch; since sprites/overlays tend to get used less than cursors,
> it's
> possible that this could help us uncover additional underruns that
> nobody had run across yet.  Or it could have the opposite effect and
> eliminate some of the underruns that we haven't been able to get rid
> of
> yet.

Alright, so I had to sit on this patch for a while. I think exposing
this as a normal plane is a good idea: the rest of the world just uses
planes, so we should too. We need to be *really* careful with this
though. Like Paulo said watermarks are still fragile and honestly I
wouldn't be surprised if we found more underruns hiding somewhere.

First off, although this was mentioned in an e-mail down the chain, I'm
pretty sure having a 0 block allocation is definitely not what we want.
The spec itself says a minimum of 8 blocks assuming it's a normal
cursor, so if we're going below that we definitely need to at least
make sure that behavior isn't wrong.

The second thing is that unfortunately this regresses in a rather
strange way. On this X1 yoga w/ a 4K screen (SKL chipset of course), if
I move the cursor in front of a gray background I can see a faintly
visible box starting from the beginning of the cursor and going to the
end of the plane. Reverting the patch fixes the problem.

> 
> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  4 --
>  drivers/gpu/drm/i915/i915_drv.h          | 11 +++-
>  drivers/gpu/drm/i915/intel_device_info.c | 38 +++++++++----
>  drivers/gpu/drm/i915/intel_display.c     | 97 ++++++++++++--------
> ------------
>  drivers/gpu/drm/i915/intel_drv.h         |  7 +++
>  drivers/gpu/drm/i915/intel_pm.c          | 85 ++++----------------
> --------
>  drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
>  7 files changed, 93 insertions(+), 155 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9f5a392..0bba472 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3469,10 +3469,6 @@ static int i915_ddb_info(struct seq_file *m,
> void *unused)
>  				   entry->start, entry->end,
>  				   skl_ddb_entry_size(entry));
>  		}
> -
> -		entry = &ddb->plane[pipe][PLANE_CURSOR];
> -		seq_printf(m, "  %-13s%8u%8u%8u\n", "Cursor", entry-
> >start,
> -			   entry->end, skl_ddb_entry_size(entry));
>  	}
>  
>  	drm_modeset_unlock_all(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 4714051..83aaed2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -178,7 +178,7 @@ enum plane {
>  	PLANE_A = 0,
>  	PLANE_B,
>  	PLANE_C,
> -	PLANE_CURSOR,
> +	PLANE_D,
>  	I915_MAX_PLANES,
>  };
>  #define plane_name(p) ((p) + 'A')
> @@ -316,9 +316,15 @@ struct i915_hotplug {
>  	for ((__p) = 0;						
> 	\
>  	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] +
> 1;	\
>  	     (__p)++)
> +
> +/*
> + * Only iterate over sprites exposed as sprites; omit sprites that
> + * are being repurposed to simulate a cursor.
> + */
>  #define for_each_sprite(__dev_priv, __p, __s)			
> 	\
>  	for ((__s) = 0;						
> 	\
> -	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)];	
> \
> +	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)] -	
> \
> +	             (INTEL_INFO(__dev_priv)->has_real_cursor ? 0 :
> 1);	\
>  	     (__s)++)
>  
>  #define for_each_port_masked(__port, __ports_mask) \
> @@ -687,6 +693,7 @@ struct intel_csr {
>  	func(has_psr); \
>  	func(has_rc6); \
>  	func(has_rc6p); \
> +	func(has_real_cursor); \
>  	func(has_resource_streamer); \
>  	func(has_runtime_pm); \
>  	func(has_snoop); \
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> b/drivers/gpu/drm/i915/intel_device_info.c
> index d6a8f11..a464e0e 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -271,23 +271,39 @@ void intel_device_info_runtime_init(struct
> drm_i915_private *dev_priv)
>  	enum pipe pipe;
>  
>  	/*
> -	 * Skylake and Broxton currently don't expose the topmost
> plane as its
> -	 * use is exclusive with the legacy cursor and we only want
> to expose
> -	 * one of those, not both. Until we can safely expose the
> topmost plane
> -	 * as a DRM_PLANE_TYPE_CURSOR with all the features
> exposed/supported,
> -	 * we don't expose the topmost plane at all to prevent ABI
> breakage
> -	 * down the line.
> +	 * Gen9 platforms have a top-most universal (i.e., sprite)
> plane and a
> +	 * cursor plane that are mutually exclusive.  If we use the
> cursor
> +	 * plane we permanently lose the ability to make use of the
> more
> +	 * full-featured universal plane.  So instead let's use all
> of the
> +	 * universal planes, ignore the cursor plane, but hook the
> top-most
> +	 * universal plane up to the legacy cursor ioctl's and
> expose it to
> +	 * userspace as DRM_PLANE_TYPE_CURSOR.  This won't result in
> any
> +	 * visible behavior change to userspace; we're just
> internally
> +	 * programming a different set of registers.
> +	 *
> +	 * For the purposes of device_info, we're only concerned
> with the
> +	 * number of universal planes we're programming, regardless
> of how they
> +	 * get mapped to userspace interfaces, so we'll report the
> true number of
> +	 * universal planes for gen9.
>  	 */
>  	if (IS_BROXTON(dev_priv)) {
> -		info->num_sprites[PIPE_A] = 2;
> -		info->num_sprites[PIPE_B] = 2;
> -		info->num_sprites[PIPE_C] = 1;
> -	} else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv))
> +		info->has_real_cursor = 0;
> +		info->num_sprites[PIPE_A] = 3;
> +		info->num_sprites[PIPE_B] = 3;
> +		info->num_sprites[PIPE_C] = 2;
> +	} else if (IS_GEN9(dev_priv)) {
> +		info->has_real_cursor = 0;
>  		for_each_pipe(dev_priv, pipe)
>  			info->num_sprites[pipe] = 2;
> -	else
> +	} else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv)) {
> +		info->has_real_cursor = 1;
> +		for_each_pipe(dev_priv, pipe)
> +			info->num_sprites[pipe] = 2;
> +	} else {
> +		info->has_real_cursor = 1;
>  		for_each_pipe(dev_priv, pipe)
>  			info->num_sprites[pipe] = 1;
> +	}
>  
>  	if (i915.disable_display) {
>  		DRM_INFO("Display disabled (module parameter)\n");
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index cb7dd11..9a8c2b1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1232,6 +1232,23 @@ void assert_panel_unlocked(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  	     pipe_name(pipe));
>  }
>  
> +/*
> + * Cursor state for platforms that use a universal plane as a
> cursor.
> + * Primary is universal #0, others are universal #1-
> numsprites.  Cursor
> + * will be the final universal plane for the pipe.
> + */
> +static bool
> +universal_cursor_state(struct drm_i915_private *dev_priv,
> +		       enum pipe pipe)
> +{
> +	unsigned int planenum = INTEL_INFO(dev_priv)-
> >num_sprites[pipe];
> +	u32 val;
> +
> +	val = I915_READ(PLANE_CTL(pipe, planenum));
> +
> +	return val & PLANE_CTL_ENABLE;
> +}
> +
>  static void assert_cursor(struct drm_i915_private *dev_priv,
>  			  enum pipe pipe, bool state)
>  {
> @@ -1239,6 +1256,8 @@ static void assert_cursor(struct
> drm_i915_private *dev_priv,
>  
>  	if (IS_845G(dev_priv) || IS_I865G(dev_priv))
>  		cur_state = I915_READ(CURCNTR(PIPE_A)) &
> CURSOR_ENABLE;
> +	else if (!INTEL_INFO(dev_priv)->has_real_cursor)
> +		cur_state = universal_cursor_state(dev_priv, pipe);
>  	else
>  		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
>  
> @@ -10841,15 +10860,16 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> +	/*
> +	 * Although gen9 has legacy cursor registers, their use is
> mutually
> +	 * exclusive with the top-most universal plane.  We'll just
> use the
> +	 * universal plane to simulate the legacy cursor interface
> instead,
> +	 * which means we'll never enter here on gen9 platforms.
> +	 */
> +	WARN_ON(IS_GEN9(dev_priv));
>  
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
> @@ -13528,56 +13548,6 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
>  		}
>  	}
> -
> -	/*
> -	 * cursor
> -	 * If the cursor plane isn't active, we may not have updated
> it's ddb
> -	 * allocation. In that case since the ddb allocation will be
> updated
> -	 * once the plane becomes visible, we can skip this check
> -	 */
> -	if (intel_crtc->cursor_addr) {
> -		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> -		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> -
> -		/* Watermarks */
> -		for (level = 0; level <= max_level; level++) {
> -			if (skl_wm_level_equals(&hw_plane_wm-
> >wm[level],
> -						&sw_plane_wm-
> >wm[level]))
> -				continue;
> -
> -			DRM_ERROR("mismatch in WM pipe %c cursor
> level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe), level,
> -				  sw_plane_wm->wm[level].plane_en,
> -				  sw_plane_wm-
> >wm[level].plane_res_b,
> -				  sw_plane_wm-
> >wm[level].plane_res_l,
> -				  hw_plane_wm->wm[level].plane_en,
> -				  hw_plane_wm-
> >wm[level].plane_res_b,
> -				  hw_plane_wm-
> >wm[level].plane_res_l);
> -		}
> -
> -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> -					 &sw_plane_wm->trans_wm)) {
> -			DRM_ERROR("mismatch in trans WM pipe %c
> cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> -				  pipe_name(pipe),
> -				  sw_plane_wm->trans_wm.plane_en,
> -				  sw_plane_wm->trans_wm.plane_res_b,
> -				  sw_plane_wm->trans_wm.plane_res_l,
> -				  hw_plane_wm->trans_wm.plane_en,
> -				  hw_plane_wm->trans_wm.plane_res_b,
> -				  hw_plane_wm-
> >trans_wm.plane_res_l);
> -		}
> -
> -		/* DDB */
> -		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> -		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> -
> -		if (!skl_ddb_entry_equal(hw_ddb_entry,
> sw_ddb_entry)) {
> -			DRM_ERROR("mismatch in DDB state pipe %c
> cursor (expected (%u,%u), found (%u,%u))\n",
> -				  pipe_name(pipe),
> -				  sw_ddb_entry->start, sw_ddb_entry-
> >end,
> -				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
> -		}
> -	}
>  }
>  
>  static void
> @@ -15215,11 +15185,18 @@ static struct drm_plane
> *intel_cursor_plane_create(struct drm_device *dev,
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
> -	cursor->plane = pipe;
>  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> -	cursor->check_plane = intel_check_cursor_plane;
> -	cursor->update_plane = intel_update_cursor_plane;
> -	cursor->disable_plane = intel_disable_cursor_plane;
> +	if (INTEL_INFO(dev)->has_real_cursor) {
> +		cursor->plane = pipe;  /* unused? */
> +		cursor->check_plane = intel_check_cursor_plane;
> +		cursor->update_plane = intel_update_cursor_plane;
> +		cursor->disable_plane = intel_disable_cursor_plane;
> +	} else {
> +		cursor->plane = INTEL_INFO(dev)->num_sprites[pipe] -
> 1;
> +		cursor->check_plane = intel_check_sprite_plane;
> +		cursor->update_plane = skl_update_plane;
> +		cursor->disable_plane = skl_disable_plane;
> +	}
>  
>  	ret = drm_universal_plane_init(dev, &cursor->base, 0,
>  				       &intel_plane_funcs,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index c31fddd..50874e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1790,6 +1790,13 @@ int intel_sprite_set_colorkey(struct
> drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  void intel_pipe_update_start(struct intel_crtc *crtc);
>  void intel_pipe_update_end(struct intel_crtc *crtc, struct
> intel_flip_work *work);
> +int intel_check_sprite_plane(struct drm_plane *plane,
> +			     struct intel_crtc_state *crtc_state,
> +			     struct intel_plane_state *state);
> +void skl_update_plane(struct drm_plane *drm_plane,
> +		      const struct intel_crtc_state *crtc_state,
> +		      const struct intel_plane_state *plane_state);
> +void skl_disable_plane(struct drm_plane *dplane, struct drm_crtc
> *crtc);
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 6f19e60..e75f6d8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2863,9 +2863,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
>  
>  /*
>   * Return the index of a plane in the SKL DDB and wm result
> arrays.  Primary
> - * plane is always in slot 0, cursor is always in slot
> I915_MAX_PLANES-1, and
> - * other universal planes are in indices 1..n.  Note that this may
> leave unused
> - * indices between the top "sprite" plane and the cursor.
> + * plane is always in slot 0 and other universal planes are in
> indices 1..n.
>   */
>  static int
>  skl_wm_plane_id(const struct intel_plane *plane)
> @@ -2874,7 +2872,6 @@ skl_wm_plane_id(const struct intel_plane
> *plane)
>  	case DRM_PLANE_TYPE_PRIMARY:
>  		return 0;
>  	case DRM_PLANE_TYPE_CURSOR:
> -		return PLANE_CURSOR;
>  	case DRM_PLANE_TYPE_OVERLAY:
>  		return plane->plane + 1;
>  	default:
> @@ -3128,14 +3125,6 @@ skl_ddb_get_pipe_allocation_limits(struct
> drm_device *dev,
>  	alloc->end = alloc->start + pipe_size;
>  }
>  
> -static unsigned int skl_cursor_allocation(int num_active)
> -{
> -	if (num_active == 1)
> -		return 32;
> -
> -	return 8;
> -}
> -
>  static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry,
> u32 reg)
>  {
>  	entry->start = reg & 0x3ff;
> @@ -3166,10 +3155,6 @@ void skl_ddb_get_hw_state(struct
> drm_i915_private *dev_priv,
>  						   val);
>  		}
>  
> -		val = I915_READ(CUR_BUF_CFG(pipe));
> -		skl_ddb_entry_init_from_hw(&ddb-
> >plane[pipe][PLANE_CURSOR],
> -					   val);
> -
>  		intel_display_power_put(dev_priv, power_domain);
>  	}
>  }
> @@ -3227,8 +3212,6 @@ skl_plane_relative_data_rate(const struct
> intel_crtc_state *cstate,
>  
>  	if (!intel_pstate->base.visible)
>  		return 0;
> -	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
> -		return 0;
>  	if (y && format != DRM_FORMAT_NV12)
>  		return 0;
>  
> @@ -3386,7 +3369,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> -	uint16_t alloc_size, start, cursor_blocks;
> +	uint16_t alloc_size, start;
>  	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
>  	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
>  	unsigned int total_data_rate;
> @@ -3412,12 +3395,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  	}
>  
> -	cursor_blocks = skl_cursor_allocation(num_active);
> -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> cursor_blocks;
> -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> -
> -	alloc_size -= cursor_blocks;
> -
>  	/* 1. Allocate the mininum required blocks for each active
> plane */
>  	for_each_plane_in_state(state, plane, pstate, i) {
>  		intel_plane = to_intel_plane(plane);
> @@ -3431,17 +3408,12 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  			y_minimum[id] = 0;
>  			continue;
>  		}
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> -			minimum[id] = 0;
> -			y_minimum[id] = 0;
> -			continue;
> -		}
>  
>  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
>  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
>  	}
>  
> -	for (i = 0; i < PLANE_CURSOR; i++) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {
>  		alloc_size -= minimum[i];
>  		alloc_size -= y_minimum[i];
>  	}
> @@ -3866,26 +3838,6 @@ void skl_write_plane_wm(struct intel_crtc
> *intel_crtc,
>  			    &ddb->y_plane[pipe][plane]);
>  }
>  
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb)
> -{
> -	struct drm_crtc *crtc = &intel_crtc->base;
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	int level, max_level = ilk_wm_max_level(dev_priv);
> -	enum pipe pipe = intel_crtc->pipe;
> -
> -	for (level = 0; level <= max_level; level++) {
> -		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> -				   &wm->wm[level]);
> -	}
> -	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
> >trans_wm);
> -
> -	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> -			    &ddb->plane[pipe][PLANE_CURSOR]);
> -}
> -
>  bool skl_wm_level_equals(const struct skl_wm_level *l1,
>  			 const struct skl_wm_level *l2)
>  {
> @@ -4122,19 +4074,11 @@ skl_print_wm_changes(const struct
> drm_atomic_state *state)
>  			if (skl_ddb_entry_equal(old, new))
>  				continue;
>  
> -			if (id != PLANE_CURSOR) {
> -				DRM_DEBUG_ATOMIC("[PLANE:%d:plane
> %d%c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id, id
> + 1,
> -						 pipe_name(pipe),
> -						 old->start, old-
> >end,
> -						 new->start, new-
> >end);
> -			} else {
> -				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor
> %c] ddb (%d - %d) -> (%d - %d)\n",
> -						 plane->base.id,
> -						 pipe_name(pipe),
> -						 old->start, old-
> >end,
> -						 new->start, new-
> >end);
> -			}
> +			DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb
> (%d - %d) -> (%d - %d)\n",
> +					 plane->base.id, id + 1,
> +					 pipe_name(pipe),
> +					 old->start, old->end,
> +					 new->start, new->end);
>  		}
>  	}
>  }
> @@ -4235,9 +4179,6 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  		for_each_universal_plane(dev_priv, pipe, plane)
>  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
>  					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> >planes[PLANE_CURSOR],
> -				    &results->ddb);
>  	}
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> @@ -4350,18 +4291,12 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc
> *crtc,
>  		wm = &out->planes[id];
>  
>  		for (level = 0; level <= max_level; level++) {
> -			if (id != PLANE_CURSOR)
> -				val = I915_READ(PLANE_WM(pipe, id,
> level));
> -			else
> -				val = I915_READ(CUR_WM(pipe,
> level));
> +			val = I915_READ(PLANE_WM(pipe, id, level));
>  
>  			skl_wm_level_from_reg_val(val, &wm-
> >wm[level]);
>  		}
>  
> -		if (id != PLANE_CURSOR)
> -			val = I915_READ(PLANE_WM_TRANS(pipe, id));
> -		else
> -			val = I915_READ(CUR_WM_TRANS(pipe));
> +		val = I915_READ(PLANE_WM_TRANS(pipe, id));
>  
>  		skl_wm_level_from_reg_val(val, &wm->trans_wm);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 43d0350..9e6406a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -194,7 +194,7 @@ void intel_pipe_update_end(struct intel_crtc
> *crtc, struct intel_flip_work *work
>  	}
>  }
>  
> -static void
> +void
>  skl_update_plane(struct drm_plane *drm_plane,
>  		 const struct intel_crtc_state *crtc_state,
>  		 const struct intel_plane_state *plane_state)
> @@ -285,7 +285,7 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	POSTING_READ(PLANE_SURF(pipe, plane));
>  }
>  
> -static void
> +void
>  skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = dplane->dev;
> @@ -752,7 +752,7 @@ ilk_disable_plane(struct drm_plane *plane, struct
> drm_crtc *crtc)
>  	POSTING_READ(DVSSURF(pipe));
>  }
>  
> -static int
> +int
>  intel_check_sprite_plane(struct drm_plane *plane,
>  			 struct intel_crtc_state *crtc_state,
>  			 struct intel_plane_state *state)
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/4] drm/i915/gen9: Expose top-most universal plane as cursor

[Matt Roper]

On Thu, Oct 27, 2016 at 06:15:32PM -0400, Lyude Paul wrote:
> On Wed, 2016-10-26 at 15:51 -0700, Matt Roper wrote:
> > Gen9 has a traditional cursor plane that is mutually exclusive with
> > the
> > system's top-most "universal" plane; it seems likely that two planes
> > are
> > really a single shared hardware unit with two different register
> > interfaces.  Thus far i915 has exposed a cursor plane to userspace
> > that's hooked up to the old-style cursor registers; we just pretended
> > that the top-most universal plane didn't exist and reported one fewer
> > "sprite/overlay" planes for each pipe than the platform technically
> > has.
> > Let's switch this around so that the cursor exposed to userspace is
> > actually wired up to top-most universal plane registers.  We'll
> > continue
> > to present the same cursor ABI to userspace that we always have, but
> > internally we'll just be programming a different set of registers and
> > doing so in a way that's more consistent with how all the other gen9
> > planes work --- less cursor-specific special cases throughout the
> > code.
> > 
> > Aside from making the code a bit simpler (fewer cursor-specific
> > special
> > cases), this will also pave the way to eventually exposing the top-
> > most
> > plane in a more full-featured manner to userspace clients that don't
> > need a traditional cursor and wish to opt into having access to an
> > additional sprite/overlay-style plane instead.
> > 
> > It's worth noting that a lot of the special-cased cursor-specific
> > code
> > was in the gen9 watermark programming.  It's good to simplify that
> > code,
> > but we should keep an eye out for any unwanted side effects of this
> > patch; since sprites/overlays tend to get used less than cursors,
> > it's
> > possible that this could help us uncover additional underruns that
> > nobody had run across yet.  Or it could have the opposite effect and
> > eliminate some of the underruns that we haven't been able to get rid
> > of
> > yet.
> 
> Alright, so I had to sit on this patch for a while. I think exposing
> this as a normal plane is a good idea: the rest of the world just uses
> planes, so we should too. We need to be *really* careful with this
> though. Like Paulo said watermarks are still fragile and honestly I
> wouldn't be surprised if we found more underruns hiding somewhere.

Agreed.  We're programming a different subset of the internal hardware
so even if we do it 100% properly according to the spec, we can easily
uncover previously hidden painpoints in the hardware that need new
workarounds.

> First off, although this was mentioned in an e-mail down the chain, I'm
> pretty sure having a 0 block allocation is definitely not what we want.
> The spec itself says a minimum of 8 blocks assuming it's a normal
> cursor, so if we're going below that we definitely need to at least
> make sure that behavior isn't wrong.

DDB allocation should only be necessary if a plane is being used
(otherwise you have no data to fill the buffer with anyway).  If a plane
is off, then it shouldn't need allocation (and this patch guarantees
that the cursor will _never_ be turned on) I'm pretty sure we've
actually had some threads where Art (one of our hardware architects)
mentioned that the cursor fixed allocation is just an optimization and
isn't in any way required even when you do use the cursor hardware
plane.  The only one I can find searching now is this one:

        https://lists.freedesktop.org/archives/intel-gfx/2016-June/099256.html

but I feel like there was another thread (or maybe an IRC conversation)
where he talked about it a bit more.  I'll see if I can find the one I'm
thinking of.

> The second thing is that unfortunately this regresses in a rather
> strange way. On this X1 yoga w/ a 4K screen (SKL chipset of course), if
> I move the cursor in front of a gray background I can see a faintly
> visible box starting from the beginning of the cursor and going to the
> end of the plane. Reverting the patch fixes the problem.

Yeah, I mentioned that in the cover letter...I believe the color
correction settings are different for the universal plane vs the cursor
plane (which causes IGT CRC mismatches at the moment and may be visually
noticeable if you have good eyes); that shouldn't be hard to track down
and fix.


Matt

> 
> > 
> > Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c      |  4 --
> >  drivers/gpu/drm/i915/i915_drv.h          | 11 +++-
> >  drivers/gpu/drm/i915/intel_device_info.c | 38 +++++++++----
> >  drivers/gpu/drm/i915/intel_display.c     | 97 ++++++++++++--------
> > ------------
> >  drivers/gpu/drm/i915/intel_drv.h         |  7 +++
> >  drivers/gpu/drm/i915/intel_pm.c          | 85 ++++----------------
> > --------
> >  drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
> >  7 files changed, 93 insertions(+), 155 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 9f5a392..0bba472 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -3469,10 +3469,6 @@ static int i915_ddb_info(struct seq_file *m,
> > void *unused)
> >  				   entry->start, entry->end,
> >  				   skl_ddb_entry_size(entry));
> >  		}
> > -
> > -		entry = &ddb->plane[pipe][PLANE_CURSOR];
> > -		seq_printf(m, "  %-13s%8u%8u%8u\n", "Cursor", entry-
> > >start,
> > -			   entry->end, skl_ddb_entry_size(entry));
> >  	}
> >  
> >  	drm_modeset_unlock_all(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 4714051..83aaed2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -178,7 +178,7 @@ enum plane {
> >  	PLANE_A = 0,
> >  	PLANE_B,
> >  	PLANE_C,
> > -	PLANE_CURSOR,
> > +	PLANE_D,
> >  	I915_MAX_PLANES,
> >  };
> >  #define plane_name(p) ((p) + 'A')
> > @@ -316,9 +316,15 @@ struct i915_hotplug {
> >  	for ((__p) = 0;						
> > 	\
> >  	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] +
> > 1;	\
> >  	     (__p)++)
> > +
> > +/*
> > + * Only iterate over sprites exposed as sprites; omit sprites that
> > + * are being repurposed to simulate a cursor.
> > + */
> >  #define for_each_sprite(__dev_priv, __p, __s)			
> > 	\
> >  	for ((__s) = 0;						
> > 	\
> > -	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)];	
> > \
> > +	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)] -	
> > \
> > +	             (INTEL_INFO(__dev_priv)->has_real_cursor ? 0 :
> > 1);	\
> >  	     (__s)++)
> >  
> >  #define for_each_port_masked(__port, __ports_mask) \
> > @@ -687,6 +693,7 @@ struct intel_csr {
> >  	func(has_psr); \
> >  	func(has_rc6); \
> >  	func(has_rc6p); \
> > +	func(has_real_cursor); \
> >  	func(has_resource_streamer); \
> >  	func(has_runtime_pm); \
> >  	func(has_snoop); \
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > b/drivers/gpu/drm/i915/intel_device_info.c
> > index d6a8f11..a464e0e 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -271,23 +271,39 @@ void intel_device_info_runtime_init(struct
> > drm_i915_private *dev_priv)
> >  	enum pipe pipe;
> >  
> >  	/*
> > -	 * Skylake and Broxton currently don't expose the topmost
> > plane as its
> > -	 * use is exclusive with the legacy cursor and we only want
> > to expose
> > -	 * one of those, not both. Until we can safely expose the
> > topmost plane
> > -	 * as a DRM_PLANE_TYPE_CURSOR with all the features
> > exposed/supported,
> > -	 * we don't expose the topmost plane at all to prevent ABI
> > breakage
> > -	 * down the line.
> > +	 * Gen9 platforms have a top-most universal (i.e., sprite)
> > plane and a
> > +	 * cursor plane that are mutually exclusive.  If we use the
> > cursor
> > +	 * plane we permanently lose the ability to make use of the
> > more
> > +	 * full-featured universal plane.  So instead let's use all
> > of the
> > +	 * universal planes, ignore the cursor plane, but hook the
> > top-most
> > +	 * universal plane up to the legacy cursor ioctl's and
> > expose it to
> > +	 * userspace as DRM_PLANE_TYPE_CURSOR.  This won't result in
> > any
> > +	 * visible behavior change to userspace; we're just
> > internally
> > +	 * programming a different set of registers.
> > +	 *
> > +	 * For the purposes of device_info, we're only concerned
> > with the
> > +	 * number of universal planes we're programming, regardless
> > of how they
> > +	 * get mapped to userspace interfaces, so we'll report the
> > true number of
> > +	 * universal planes for gen9.
> >  	 */
> >  	if (IS_BROXTON(dev_priv)) {
> > -		info->num_sprites[PIPE_A] = 2;
> > -		info->num_sprites[PIPE_B] = 2;
> > -		info->num_sprites[PIPE_C] = 1;
> > -	} else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv))
> > +		info->has_real_cursor = 0;
> > +		info->num_sprites[PIPE_A] = 3;
> > +		info->num_sprites[PIPE_B] = 3;
> > +		info->num_sprites[PIPE_C] = 2;
> > +	} else if (IS_GEN9(dev_priv)) {
> > +		info->has_real_cursor = 0;
> >  		for_each_pipe(dev_priv, pipe)
> >  			info->num_sprites[pipe] = 2;
> > -	else
> > +	} else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv)) {
> > +		info->has_real_cursor = 1;
> > +		for_each_pipe(dev_priv, pipe)
> > +			info->num_sprites[pipe] = 2;
> > +	} else {
> > +		info->has_real_cursor = 1;
> >  		for_each_pipe(dev_priv, pipe)
> >  			info->num_sprites[pipe] = 1;
> > +	}
> >  
> >  	if (i915.disable_display) {
> >  		DRM_INFO("Display disabled (module parameter)\n");
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index cb7dd11..9a8c2b1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1232,6 +1232,23 @@ void assert_panel_unlocked(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> >  	     pipe_name(pipe));
> >  }
> >  
> > +/*
> > + * Cursor state for platforms that use a universal plane as a
> > cursor.
> > + * Primary is universal #0, others are universal #1-
> > numsprites.  Cursor
> > + * will be the final universal plane for the pipe.
> > + */
> > +static bool
> > +universal_cursor_state(struct drm_i915_private *dev_priv,
> > +		       enum pipe pipe)
> > +{
> > +	unsigned int planenum = INTEL_INFO(dev_priv)-
> > >num_sprites[pipe];
> > +	u32 val;
> > +
> > +	val = I915_READ(PLANE_CTL(pipe, planenum));
> > +
> > +	return val & PLANE_CTL_ENABLE;
> > +}
> > +
> >  static void assert_cursor(struct drm_i915_private *dev_priv,
> >  			  enum pipe pipe, bool state)
> >  {
> > @@ -1239,6 +1256,8 @@ static void assert_cursor(struct
> > drm_i915_private *dev_priv,
> >  
> >  	if (IS_845G(dev_priv) || IS_I865G(dev_priv))
> >  		cur_state = I915_READ(CURCNTR(PIPE_A)) &
> > CURSOR_ENABLE;
> > +	else if (!INTEL_INFO(dev_priv)->has_real_cursor)
> > +		cur_state = universal_cursor_state(dev_priv, pipe);
> >  	else
> >  		cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
> >  
> > @@ -10841,15 +10860,16 @@ static void i9xx_update_cursor(struct
> > drm_crtc *crtc, u32 base,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> > >state);
> > -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> > -	const struct skl_plane_wm *p_wm =
> > -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> >  	int pipe = intel_crtc->pipe;
> >  	uint32_t cntl = 0;
> >  
> > -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > drm_crtc_mask(crtc))
> > -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> > +	/*
> > +	 * Although gen9 has legacy cursor registers, their use is
> > mutually
> > +	 * exclusive with the top-most universal plane.  We'll just
> > use the
> > +	 * universal plane to simulate the legacy cursor interface
> > instead,
> > +	 * which means we'll never enter here on gen9 platforms.
> > +	 */
> > +	WARN_ON(IS_GEN9(dev_priv));
> >  
> >  	if (plane_state && plane_state->base.visible) {
> >  		cntl = MCURSOR_GAMMA_ENABLE;
> > @@ -13528,56 +13548,6 @@ static void verify_wm_state(struct drm_crtc
> > *crtc,
> >  				  hw_ddb_entry->start, hw_ddb_entry-
> > >end);
> >  		}
> >  	}
> > -
> > -	/*
> > -	 * cursor
> > -	 * If the cursor plane isn't active, we may not have updated
> > it's ddb
> > -	 * allocation. In that case since the ddb allocation will be
> > updated
> > -	 * once the plane becomes visible, we can skip this check
> > -	 */
> > -	if (intel_crtc->cursor_addr) {
> > -		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> > -		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> > -
> > -		/* Watermarks */
> > -		for (level = 0; level <= max_level; level++) {
> > -			if (skl_wm_level_equals(&hw_plane_wm-
> > >wm[level],
> > -						&sw_plane_wm-
> > >wm[level]))
> > -				continue;
> > -
> > -			DRM_ERROR("mismatch in WM pipe %c cursor
> > level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > -				  pipe_name(pipe), level,
> > -				  sw_plane_wm->wm[level].plane_en,
> > -				  sw_plane_wm-
> > >wm[level].plane_res_b,
> > -				  sw_plane_wm-
> > >wm[level].plane_res_l,
> > -				  hw_plane_wm->wm[level].plane_en,
> > -				  hw_plane_wm-
> > >wm[level].plane_res_b,
> > -				  hw_plane_wm-
> > >wm[level].plane_res_l);
> > -		}
> > -
> > -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > -					 &sw_plane_wm->trans_wm)) {
> > -			DRM_ERROR("mismatch in trans WM pipe %c
> > cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > -				  pipe_name(pipe),
> > -				  sw_plane_wm->trans_wm.plane_en,
> > -				  sw_plane_wm->trans_wm.plane_res_b,
> > -				  sw_plane_wm->trans_wm.plane_res_l,
> > -				  hw_plane_wm->trans_wm.plane_en,
> > -				  hw_plane_wm->trans_wm.plane_res_b,
> > -				  hw_plane_wm-
> > >trans_wm.plane_res_l);
> > -		}
> > -
> > -		/* DDB */
> > -		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> > -		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> > -
> > -		if (!skl_ddb_entry_equal(hw_ddb_entry,
> > sw_ddb_entry)) {
> > -			DRM_ERROR("mismatch in DDB state pipe %c
> > cursor (expected (%u,%u), found (%u,%u))\n",
> > -				  pipe_name(pipe),
> > -				  sw_ddb_entry->start, sw_ddb_entry-
> > >end,
> > -				  hw_ddb_entry->start, hw_ddb_entry-
> > >end);
> > -		}
> > -	}
> >  }
> >  
> >  static void
> > @@ -15215,11 +15185,18 @@ static struct drm_plane
> > *intel_cursor_plane_create(struct drm_device *dev,
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> > -	cursor->plane = pipe;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> > -	cursor->check_plane = intel_check_cursor_plane;
> > -	cursor->update_plane = intel_update_cursor_plane;
> > -	cursor->disable_plane = intel_disable_cursor_plane;
> > +	if (INTEL_INFO(dev)->has_real_cursor) {
> > +		cursor->plane = pipe;  /* unused? */
> > +		cursor->check_plane = intel_check_cursor_plane;
> > +		cursor->update_plane = intel_update_cursor_plane;
> > +		cursor->disable_plane = intel_disable_cursor_plane;
> > +	} else {
> > +		cursor->plane = INTEL_INFO(dev)->num_sprites[pipe] -
> > 1;
> > +		cursor->check_plane = intel_check_sprite_plane;
> > +		cursor->update_plane = skl_update_plane;
> > +		cursor->disable_plane = skl_disable_plane;
> > +	}
> >  
> >  	ret = drm_universal_plane_init(dev, &cursor->base, 0,
> >  				       &intel_plane_funcs,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index c31fddd..50874e2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1790,6 +1790,13 @@ int intel_sprite_set_colorkey(struct
> > drm_device *dev, void *data,
> >  			      struct drm_file *file_priv);
> >  void intel_pipe_update_start(struct intel_crtc *crtc);
> >  void intel_pipe_update_end(struct intel_crtc *crtc, struct
> > intel_flip_work *work);
> > +int intel_check_sprite_plane(struct drm_plane *plane,
> > +			     struct intel_crtc_state *crtc_state,
> > +			     struct intel_plane_state *state);
> > +void skl_update_plane(struct drm_plane *drm_plane,
> > +		      const struct intel_crtc_state *crtc_state,
> > +		      const struct intel_plane_state *plane_state);
> > +void skl_disable_plane(struct drm_plane *dplane, struct drm_crtc
> > *crtc);
> >  
> >  /* intel_tv.c */
> >  void intel_tv_init(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 6f19e60..e75f6d8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2863,9 +2863,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
> >  
> >  /*
> >   * Return the index of a plane in the SKL DDB and wm result
> > arrays.  Primary
> > - * plane is always in slot 0, cursor is always in slot
> > I915_MAX_PLANES-1, and
> > - * other universal planes are in indices 1..n.  Note that this may
> > leave unused
> > - * indices between the top "sprite" plane and the cursor.
> > + * plane is always in slot 0 and other universal planes are in
> > indices 1..n.
> >   */
> >  static int
> >  skl_wm_plane_id(const struct intel_plane *plane)
> > @@ -2874,7 +2872,6 @@ skl_wm_plane_id(const struct intel_plane
> > *plane)
> >  	case DRM_PLANE_TYPE_PRIMARY:
> >  		return 0;
> >  	case DRM_PLANE_TYPE_CURSOR:
> > -		return PLANE_CURSOR;
> >  	case DRM_PLANE_TYPE_OVERLAY:
> >  		return plane->plane + 1;
> >  	default:
> > @@ -3128,14 +3125,6 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_device *dev,
> >  	alloc->end = alloc->start + pipe_size;
> >  }
> >  
> > -static unsigned int skl_cursor_allocation(int num_active)
> > -{
> > -	if (num_active == 1)
> > -		return 32;
> > -
> > -	return 8;
> > -}
> > -
> >  static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry,
> > u32 reg)
> >  {
> >  	entry->start = reg & 0x3ff;
> > @@ -3166,10 +3155,6 @@ void skl_ddb_get_hw_state(struct
> > drm_i915_private *dev_priv,
> >  						   val);
> >  		}
> >  
> > -		val = I915_READ(CUR_BUF_CFG(pipe));
> > -		skl_ddb_entry_init_from_hw(&ddb-
> > >plane[pipe][PLANE_CURSOR],
> > -					   val);
> > -
> >  		intel_display_power_put(dev_priv, power_domain);
> >  	}
> >  }
> > @@ -3227,8 +3212,6 @@ skl_plane_relative_data_rate(const struct
> > intel_crtc_state *cstate,
> >  
> >  	if (!intel_pstate->base.visible)
> >  		return 0;
> > -	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
> > -		return 0;
> >  	if (y && format != DRM_FORMAT_NV12)
> >  		return 0;
> >  
> > @@ -3386,7 +3369,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  	struct drm_plane_state *pstate;
> >  	enum pipe pipe = intel_crtc->pipe;
> >  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> > -	uint16_t alloc_size, start, cursor_blocks;
> > +	uint16_t alloc_size, start;
> >  	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
> >  	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
> >  	unsigned int total_data_rate;
> > @@ -3412,12 +3395,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  		return 0;
> >  	}
> >  
> > -	cursor_blocks = skl_cursor_allocation(num_active);
> > -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> > cursor_blocks;
> > -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> > -
> > -	alloc_size -= cursor_blocks;
> > -
> >  	/* 1. Allocate the mininum required blocks for each active
> > plane */
> >  	for_each_plane_in_state(state, plane, pstate, i) {
> >  		intel_plane = to_intel_plane(plane);
> > @@ -3431,17 +3408,12 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  			y_minimum[id] = 0;
> >  			continue;
> >  		}
> > -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > -			minimum[id] = 0;
> > -			y_minimum[id] = 0;
> > -			continue;
> > -		}
> >  
> >  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> >  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> >  	}
> >  
> > -	for (i = 0; i < PLANE_CURSOR; i++) {
> > +	for (i = 0; i < I915_MAX_PLANES; i++) {
> >  		alloc_size -= minimum[i];
> >  		alloc_size -= y_minimum[i];
> >  	}
> > @@ -3866,26 +3838,6 @@ void skl_write_plane_wm(struct intel_crtc
> > *intel_crtc,
> >  			    &ddb->y_plane[pipe][plane]);
> >  }
> >  
> > -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > -			 const struct skl_plane_wm *wm,
> > -			 const struct skl_ddb_allocation *ddb)
> > -{
> > -	struct drm_crtc *crtc = &intel_crtc->base;
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	int level, max_level = ilk_wm_max_level(dev_priv);
> > -	enum pipe pipe = intel_crtc->pipe;
> > -
> > -	for (level = 0; level <= max_level; level++) {
> > -		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> > -				   &wm->wm[level]);
> > -	}
> > -	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
> > >trans_wm);
> > -
> > -	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> > -			    &ddb->plane[pipe][PLANE_CURSOR]);
> > -}
> > -
> >  bool skl_wm_level_equals(const struct skl_wm_level *l1,
> >  			 const struct skl_wm_level *l2)
> >  {
> > @@ -4122,19 +4074,11 @@ skl_print_wm_changes(const struct
> > drm_atomic_state *state)
> >  			if (skl_ddb_entry_equal(old, new))
> >  				continue;
> >  
> > -			if (id != PLANE_CURSOR) {
> > -				DRM_DEBUG_ATOMIC("[PLANE:%d:plane
> > %d%c] ddb (%d - %d) -> (%d - %d)\n",
> > -						 plane->base.id, id
> > + 1,
> > -						 pipe_name(pipe),
> > -						 old->start, old-
> > >end,
> > -						 new->start, new-
> > >end);
> > -			} else {
> > -				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor
> > %c] ddb (%d - %d) -> (%d - %d)\n",
> > -						 plane->base.id,
> > -						 pipe_name(pipe),
> > -						 old->start, old-
> > >end,
> > -						 new->start, new-
> > >end);
> > -			}
> > +			DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb
> > (%d - %d) -> (%d - %d)\n",
> > +					 plane->base.id, id + 1,
> > +					 pipe_name(pipe),
> > +					 old->start, old->end,
> > +					 new->start, new->end);
> >  		}
> >  	}
> >  }
> > @@ -4235,9 +4179,6 @@ static void skl_update_wm(struct drm_crtc
> > *crtc)
> >  		for_each_universal_plane(dev_priv, pipe, plane)
> >  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> > >planes[plane],
> >  					   &results->ddb, plane);
> > -
> > -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> > >planes[PLANE_CURSOR],
> > -				    &results->ddb);
> >  	}
> >  
> >  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> > @@ -4350,18 +4291,12 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc
> > *crtc,
> >  		wm = &out->planes[id];
> >  
> >  		for (level = 0; level <= max_level; level++) {
> > -			if (id != PLANE_CURSOR)
> > -				val = I915_READ(PLANE_WM(pipe, id,
> > level));
> > -			else
> > -				val = I915_READ(CUR_WM(pipe,
> > level));
> > +			val = I915_READ(PLANE_WM(pipe, id, level));
> >  
> >  			skl_wm_level_from_reg_val(val, &wm-
> > >wm[level]);
> >  		}
> >  
> > -		if (id != PLANE_CURSOR)
> > -			val = I915_READ(PLANE_WM_TRANS(pipe, id));
> > -		else
> > -			val = I915_READ(CUR_WM_TRANS(pipe));
> > +		val = I915_READ(PLANE_WM_TRANS(pipe, id));
> >  
> >  		skl_wm_level_from_reg_val(val, &wm->trans_wm);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 43d0350..9e6406a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -194,7 +194,7 @@ void intel_pipe_update_end(struct intel_crtc
> > *crtc, struct intel_flip_work *work
> >  	}
> >  }
> >  
> > -static void
> > +void
> >  skl_update_plane(struct drm_plane *drm_plane,
> >  		 const struct intel_crtc_state *crtc_state,
> >  		 const struct intel_plane_state *plane_state)
> > @@ -285,7 +285,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	POSTING_READ(PLANE_SURF(pipe, plane));
> >  }
> >  
> > -static void
> > +void
> >  skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = dplane->dev;
> > @@ -752,7 +752,7 @@ ilk_disable_plane(struct drm_plane *plane, struct
> > drm_crtc *crtc)
> >  	POSTING_READ(DVSSURF(pipe));
> >  }
> >  
> > -static int
> > +int
> >  intel_check_sprite_plane(struct drm_plane *plane,
> >  			 struct intel_crtc_state *crtc_state,
> >  			 struct intel_plane_state *state)
> -- 
> Cheers,
> 	Lyude

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 3/4] drm/i915/gen9: Expose top-most universal plane as cursor

[Lyude Paul]

On Thu, 2016-10-27 at 15:35 -0700, Matt Roper wrote:
> On Thu, Oct 27, 2016 at 06:15:32PM -0400, Lyude Paul wrote:
> > 
> > On Wed, 2016-10-26 at 15:51 -0700, Matt Roper wrote:
> > > 
> > > Gen9 has a traditional cursor plane that is mutually exclusive
> > > with
> > > the
> > > system's top-most "universal" plane; it seems likely that two
> > > planes
> > > are
> > > really a single shared hardware unit with two different register
> > > interfaces.  Thus far i915 has exposed a cursor plane to
> > > userspace
> > > that's hooked up to the old-style cursor registers; we just
> > > pretended
> > > that the top-most universal plane didn't exist and reported one
> > > fewer
> > > "sprite/overlay" planes for each pipe than the platform
> > > technically
> > > has.
> > > Let's switch this around so that the cursor exposed to userspace
> > > is
> > > actually wired up to top-most universal plane registers.  We'll
> > > continue
> > > to present the same cursor ABI to userspace that we always have,
> > > but
> > > internally we'll just be programming a different set of registers
> > > and
> > > doing so in a way that's more consistent with how all the other
> > > gen9
> > > planes work --- less cursor-specific special cases throughout the
> > > code.
> > > 
> > > Aside from making the code a bit simpler (fewer cursor-specific
> > > special
> > > cases), this will also pave the way to eventually exposing the
> > > top-
> > > most
> > > plane in a more full-featured manner to userspace clients that
> > > don't
> > > need a traditional cursor and wish to opt into having access to
> > > an
> > > additional sprite/overlay-style plane instead.
> > > 
> > > It's worth noting that a lot of the special-cased cursor-specific
> > > code
> > > was in the gen9 watermark programming.  It's good to simplify
> > > that
> > > code,
> > > but we should keep an eye out for any unwanted side effects of
> > > this
> > > patch; since sprites/overlays tend to get used less than cursors,
> > > it's
> > > possible that this could help us uncover additional underruns
> > > that
> > > nobody had run across yet.  Or it could have the opposite effect
> > > and
> > > eliminate some of the underruns that we haven't been able to get
> > > rid
> > > of
> > > yet.
> > 
> > Alright, so I had to sit on this patch for a while. I think
> > exposing
> > this as a normal plane is a good idea: the rest of the world just
> > uses
> > planes, so we should too. We need to be *really* careful with this
> > though. Like Paulo said watermarks are still fragile and honestly I
> > wouldn't be surprised if we found more underruns hiding somewhere.
> 
> Agreed.  We're programming a different subset of the internal
> hardware
> so even if we do it 100% properly according to the spec, we can
> easily
> uncover previously hidden painpoints in the hardware that need new
> workarounds.
> 
> > 
> > First off, although this was mentioned in an e-mail down the chain,
> > I'm
> > pretty sure having a 0 block allocation is definitely not what we
> > want.
> > The spec itself says a minimum of 8 blocks assuming it's a normal
> > cursor, so if we're going below that we definitely need to at least
> > make sure that behavior isn't wrong.
> 
> DDB allocation should only be necessary if a plane is being used
> (otherwise you have no data to fill the buffer with anyway).  If a
> plane

Ah whoops, I thought you were implying that we had 0 block ddb
allocation on active planes.

> is off, then it shouldn't need allocation (and this patch guarantees
> that the cursor will _never_ be turned on) I'm pretty sure we've
> actually had some threads where Art (one of our hardware architects)
> mentioned that the cursor fixed allocation is just an optimization
> and
> isn't in any way required even when you do use the cursor hardware
> plane.  The only one I can find searching now is this one:
> 
>         https://lists.freedesktop.org/archives/intel-gfx/2016-June/09
> 9256.html
> 
> but I feel like there was another thread (or maybe an IRC
> conversation)
> where he talked about it a bit more.  I'll see if I can find the one
> I'm
> thinking of.
> 
> > 
> > The second thing is that unfortunately this regresses in a rather
> > strange way. On this X1 yoga w/ a 4K screen (SKL chipset of
> > course), if
> > I move the cursor in front of a gray background I can see a faintly
> > visible box starting from the beginning of the cursor and going to
> > the
> > end of the plane. Reverting the patch fixes the problem.
> 
> Yeah, I mentioned that in the cover letter...I believe the color
> correction settings are different for the universal plane vs the
> cursor
> plane (which causes IGT CRC mismatches at the moment and may be
> visually
> noticeable if you have good eyes); that shouldn't be hard to track 
Ah whoops, must have missed that part. Regardless though, we should
probably get those out of the way first before applying any patches
like this. Otherwise I think the patch looks good, so long as we can
get a couple of people to try it out on their systems for a while and
see if anything breaks.

> down
> and fix.
> 
> 
> Matt
> 
> > 
> > 
> > > 
> > > 
> > > Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c      |  4 --
> > >  drivers/gpu/drm/i915/i915_drv.h          | 11 +++-
> > >  drivers/gpu/drm/i915/intel_device_info.c | 38 +++++++++----
> > >  drivers/gpu/drm/i915/intel_display.c     | 97 ++++++++++++----
> > > ----
> > > ------------
> > >  drivers/gpu/drm/i915/intel_drv.h         |  7 +++
> > >  drivers/gpu/drm/i915/intel_pm.c          | 85 ++++------------
> > > ----
> > > --------
> > >  drivers/gpu/drm/i915/intel_sprite.c      |  6 +-
> > >  7 files changed, 93 insertions(+), 155 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 9f5a392..0bba472 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -3469,10 +3469,6 @@ static int i915_ddb_info(struct seq_file
> > > *m,
> > > void *unused)
> > >  				   entry->start, entry->end,
> > >  				   skl_ddb_entry_size(entry));
> > >  		}
> > > -
> > > -		entry = &ddb->plane[pipe][PLANE_CURSOR];
> > > -		seq_printf(m, "  %-13s%8u%8u%8u\n", "Cursor",
> > > entry-
> > > > 
> > > > start,
> > > -			   entry->end,
> > > skl_ddb_entry_size(entry));
> > >  	}
> > >  
> > >  	drm_modeset_unlock_all(dev);
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4714051..83aaed2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -178,7 +178,7 @@ enum plane {
> > >  	PLANE_A = 0,
> > >  	PLANE_B,
> > >  	PLANE_C,
> > > -	PLANE_CURSOR,
> > > +	PLANE_D,
> > >  	I915_MAX_PLANES,
> > >  };
> > >  #define plane_name(p) ((p) + 'A')
> > > @@ -316,9 +316,15 @@ struct i915_hotplug {
> > >  	for ((__p) = 0;						
> > > 	\
> > >  	     (__p) < INTEL_INFO(__dev_priv)-
> > > >num_sprites[(__pipe)] +
> > > 1;	\
> > >  	     (__p)++)
> > > +
> > > +/*
> > > + * Only iterate over sprites exposed as sprites; omit sprites
> > > that
> > > + * are being repurposed to simulate a cursor.
> > > + */
> > >  #define for_each_sprite(__dev_priv, __p, __s)			
> > > 	\
> > >  	for ((__s) = 0;						
> > > 	\
> > > -	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)];
> > > 	
> > > \
> > > +	     (__s) < INTEL_INFO(__dev_priv)->num_sprites[(__p)]
> > > -	
> > > \
> > > +	             (INTEL_INFO(__dev_priv)->has_real_cursor ?
> > > 0 :
> > > 1);	\
> > >  	     (__s)++)
> > >  
> > >  #define for_each_port_masked(__port, __ports_mask) \
> > > @@ -687,6 +693,7 @@ struct intel_csr {
> > >  	func(has_psr); \
> > >  	func(has_rc6); \
> > >  	func(has_rc6p); \
> > > +	func(has_real_cursor); \
> > >  	func(has_resource_streamer); \
> > >  	func(has_runtime_pm); \
> > >  	func(has_snoop); \
> > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > > b/drivers/gpu/drm/i915/intel_device_info.c
> > > index d6a8f11..a464e0e 100644
> > > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > > @@ -271,23 +271,39 @@ void intel_device_info_runtime_init(struct
> > > drm_i915_private *dev_priv)
> > >  	enum pipe pipe;
> > >  
> > >  	/*
> > > -	 * Skylake and Broxton currently don't expose the
> > > topmost
> > > plane as its
> > > -	 * use is exclusive with the legacy cursor and we only
> > > want
> > > to expose
> > > -	 * one of those, not both. Until we can safely expose
> > > the
> > > topmost plane
> > > -	 * as a DRM_PLANE_TYPE_CURSOR with all the features
> > > exposed/supported,
> > > -	 * we don't expose the topmost plane at all to prevent
> > > ABI
> > > breakage
> > > -	 * down the line.
> > > +	 * Gen9 platforms have a top-most universal (i.e.,
> > > sprite)
> > > plane and a
> > > +	 * cursor plane that are mutually exclusive.  If we use
> > > the
> > > cursor
> > > +	 * plane we permanently lose the ability to make use of
> > > the
> > > more
> > > +	 * full-featured universal plane.  So instead let's use
> > > all
> > > of the
> > > +	 * universal planes, ignore the cursor plane, but hook
> > > the
> > > top-most
> > > +	 * universal plane up to the legacy cursor ioctl's and
> > > expose it to
> > > +	 * userspace as DRM_PLANE_TYPE_CURSOR.  This won't
> > > result in
> > > any
> > > +	 * visible behavior change to userspace; we're just
> > > internally
> > > +	 * programming a different set of registers.
> > > +	 *
> > > +	 * For the purposes of device_info, we're only concerned
> > > with the
> > > +	 * number of universal planes we're programming,
> > > regardless
> > > of how they
> > > +	 * get mapped to userspace interfaces, so we'll report
> > > the
> > > true number of
> > > +	 * universal planes for gen9.
> > >  	 */
> > >  	if (IS_BROXTON(dev_priv)) {
> > > -		info->num_sprites[PIPE_A] = 2;
> > > -		info->num_sprites[PIPE_B] = 2;
> > > -		info->num_sprites[PIPE_C] = 1;
> > > -	} else if (IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv))
> > > +		info->has_real_cursor = 0;
> > > +		info->num_sprites[PIPE_A] = 3;
> > > +		info->num_sprites[PIPE_B] = 3;
> > > +		info->num_sprites[PIPE_C] = 2;
> > > +	} else if (IS_GEN9(dev_priv)) {
> > > +		info->has_real_cursor = 0;
> > >  		for_each_pipe(dev_priv, pipe)
> > >  			info->num_sprites[pipe] = 2;
> > > -	else
> > > +	} else if (IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv)) {
> > > +		info->has_real_cursor = 1;
> > > +		for_each_pipe(dev_priv, pipe)
> > > +			info->num_sprites[pipe] = 2;
> > > +	} else {
> > > +		info->has_real_cursor = 1;
> > >  		for_each_pipe(dev_priv, pipe)
> > >  			info->num_sprites[pipe] = 1;
> > > +	}
> > >  
> > >  	if (i915.disable_display) {
> > >  		DRM_INFO("Display disabled (module
> > > parameter)\n");
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index cb7dd11..9a8c2b1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1232,6 +1232,23 @@ void assert_panel_unlocked(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >  	     pipe_name(pipe));
> > >  }
> > >  
> > > +/*
> > > + * Cursor state for platforms that use a universal plane as a
> > > cursor.
> > > + * Primary is universal #0, others are universal #1-
> > > numsprites.  Cursor
> > > + * will be the final universal plane for the pipe.
> > > + */
> > > +static bool
> > > +universal_cursor_state(struct drm_i915_private *dev_priv,
> > > +		       enum pipe pipe)
> > > +{
> > > +	unsigned int planenum = INTEL_INFO(dev_priv)-
> > > > 
> > > > num_sprites[pipe];
> > > +	u32 val;
> > > +
> > > +	val = I915_READ(PLANE_CTL(pipe, planenum));
> > > +
> > > +	return val & PLANE_CTL_ENABLE;
> > > +}
> > > +
> > >  static void assert_cursor(struct drm_i915_private *dev_priv,
> > >  			  enum pipe pipe, bool state)
> > >  {
> > > @@ -1239,6 +1256,8 @@ static void assert_cursor(struct
> > > drm_i915_private *dev_priv,
> > >  
> > >  	if (IS_845G(dev_priv) || IS_I865G(dev_priv))
> > >  		cur_state = I915_READ(CURCNTR(PIPE_A)) &
> > > CURSOR_ENABLE;
> > > +	else if (!INTEL_INFO(dev_priv)->has_real_cursor)
> > > +		cur_state = universal_cursor_state(dev_priv,
> > > pipe);
> > >  	else
> > >  		cur_state = I915_READ(CURCNTR(pipe)) &
> > > CURSOR_MODE;
> > >  
> > > @@ -10841,15 +10860,16 @@ static void i9xx_update_cursor(struct
> > > drm_crtc *crtc, u32 base,
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > -	struct intel_crtc_state *cstate =
> > > to_intel_crtc_state(crtc-
> > > > 
> > > > state);
> > > -	const struct skl_wm_values *wm = &dev_priv-
> > > >wm.skl_results;
> > > -	const struct skl_plane_wm *p_wm =
> > > -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> > >  	int pipe = intel_crtc->pipe;
> > >  	uint32_t cntl = 0;
> > >  
> > > -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > > drm_crtc_mask(crtc))
> > > -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> > > +	/*
> > > +	 * Although gen9 has legacy cursor registers, their use
> > > is
> > > mutually
> > > +	 * exclusive with the top-most universal plane.  We'll
> > > just
> > > use the
> > > +	 * universal plane to simulate the legacy cursor
> > > interface
> > > instead,
> > > +	 * which means we'll never enter here on gen9 platforms.
> > > +	 */
> > > +	WARN_ON(IS_GEN9(dev_priv));
> > >  
> > >  	if (plane_state && plane_state->base.visible) {
> > >  		cntl = MCURSOR_GAMMA_ENABLE;
> > > @@ -13528,56 +13548,6 @@ static void verify_wm_state(struct
> > > drm_crtc
> > > *crtc,
> > >  				  hw_ddb_entry->start,
> > > hw_ddb_entry-
> > > > 
> > > > end);
> > >  		}
> > >  	}
> > > -
> > > -	/*
> > > -	 * cursor
> > > -	 * If the cursor plane isn't active, we may not have
> > > updated
> > > it's ddb
> > > -	 * allocation. In that case since the ddb allocation
> > > will be
> > > updated
> > > -	 * once the plane becomes visible, we can skip this
> > > check
> > > -	 */
> > > -	if (intel_crtc->cursor_addr) {
> > > -		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> > > -		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> > > -
> > > -		/* Watermarks */
> > > -		for (level = 0; level <= max_level; level++) {
> > > -			if (skl_wm_level_equals(&hw_plane_wm-
> > > > 
> > > > wm[level],
> > > -						&sw_plane_wm-
> > > > 
> > > > wm[level]))
> > > -				continue;
> > > -
> > > -			DRM_ERROR("mismatch in WM pipe %c cursor
> > > level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > > -				  pipe_name(pipe), level,
> > > -				  sw_plane_wm-
> > > >wm[level].plane_en,
> > > -				  sw_plane_wm-
> > > > 
> > > > wm[level].plane_res_b,
> > > -				  sw_plane_wm-
> > > > 
> > > > wm[level].plane_res_l,
> > > -				  hw_plane_wm-
> > > >wm[level].plane_en,
> > > -				  hw_plane_wm-
> > > > 
> > > > wm[level].plane_res_b,
> > > -				  hw_plane_wm-
> > > > 
> > > > wm[level].plane_res_l);
> > > -		}
> > > -
> > > -		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > > -					 &sw_plane_wm-
> > > >trans_wm)) {
> > > -			DRM_ERROR("mismatch in trans WM pipe %c
> > > cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
> > > -				  pipe_name(pipe),
> > > -				  sw_plane_wm-
> > > >trans_wm.plane_en,
> > > -				  sw_plane_wm-
> > > >trans_wm.plane_res_b,
> > > -				  sw_plane_wm-
> > > >trans_wm.plane_res_l,
> > > -				  hw_plane_wm-
> > > >trans_wm.plane_en,
> > > -				  hw_plane_wm-
> > > >trans_wm.plane_res_b,
> > > -				  hw_plane_wm-
> > > > 
> > > > trans_wm.plane_res_l);
> > > -		}
> > > -
> > > -		/* DDB */
> > > -		hw_ddb_entry =
> > > &hw_ddb.plane[pipe][PLANE_CURSOR];
> > > -		sw_ddb_entry = &sw_ddb-
> > > >plane[pipe][PLANE_CURSOR];
> > > -
> > > -		if (!skl_ddb_entry_equal(hw_ddb_entry,
> > > sw_ddb_entry)) {
> > > -			DRM_ERROR("mismatch in DDB state pipe %c
> > > cursor (expected (%u,%u), found (%u,%u))\n",
> > > -				  pipe_name(pipe),
> > > -				  sw_ddb_entry->start,
> > > sw_ddb_entry-
> > > > 
> > > > end,
> > > -				  hw_ddb_entry->start,
> > > hw_ddb_entry-
> > > > 
> > > > end);
> > > -		}
> > > -	}
> > >  }
> > >  
> > >  static void
> > > @@ -15215,11 +15185,18 @@ static struct drm_plane
> > > *intel_cursor_plane_create(struct drm_device *dev,
> > >  	cursor->can_scale = false;
> > >  	cursor->max_downscale = 1;
> > >  	cursor->pipe = pipe;
> > > -	cursor->plane = pipe;
> > >  	cursor->frontbuffer_bit =
> > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > -	cursor->check_plane = intel_check_cursor_plane;
> > > -	cursor->update_plane = intel_update_cursor_plane;
> > > -	cursor->disable_plane = intel_disable_cursor_plane;
> > > +	if (INTEL_INFO(dev)->has_real_cursor) {
> > > +		cursor->plane = pipe;  /* unused? */
> > > +		cursor->check_plane = intel_check_cursor_plane;
> > > +		cursor->update_plane =
> > > intel_update_cursor_plane;
> > > +		cursor->disable_plane =
> > > intel_disable_cursor_plane;
> > > +	} else {
> > > +		cursor->plane = INTEL_INFO(dev)-
> > > >num_sprites[pipe] -
> > > 1;
> > > +		cursor->check_plane = intel_check_sprite_plane;
> > > +		cursor->update_plane = skl_update_plane;
> > > +		cursor->disable_plane = skl_disable_plane;
> > > +	}
> > >  
> > >  	ret = drm_universal_plane_init(dev, &cursor->base, 0,
> > >  				       &intel_plane_funcs,
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index c31fddd..50874e2 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1790,6 +1790,13 @@ int intel_sprite_set_colorkey(struct
> > > drm_device *dev, void *data,
> > >  			      struct drm_file *file_priv);
> > >  void intel_pipe_update_start(struct intel_crtc *crtc);
> > >  void intel_pipe_update_end(struct intel_crtc *crtc, struct
> > > intel_flip_work *work);
> > > +int intel_check_sprite_plane(struct drm_plane *plane,
> > > +			     struct intel_crtc_state
> > > *crtc_state,
> > > +			     struct intel_plane_state *state);
> > > +void skl_update_plane(struct drm_plane *drm_plane,
> > > +		      const struct intel_crtc_state *crtc_state,
> > > +		      const struct intel_plane_state
> > > *plane_state);
> > > +void skl_disable_plane(struct drm_plane *dplane, struct drm_crtc
> > > *crtc);
> > >  
> > >  /* intel_tv.c */
> > >  void intel_tv_init(struct drm_device *dev);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 6f19e60..e75f6d8 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2863,9 +2863,7 @@ bool ilk_disable_lp_wm(struct drm_device
> > > *dev)
> > >  
> > >  /*
> > >   * Return the index of a plane in the SKL DDB and wm result
> > > arrays.  Primary
> > > - * plane is always in slot 0, cursor is always in slot
> > > I915_MAX_PLANES-1, and
> > > - * other universal planes are in indices 1..n.  Note that this
> > > may
> > > leave unused
> > > - * indices between the top "sprite" plane and the cursor.
> > > + * plane is always in slot 0 and other universal planes are in
> > > indices 1..n.
> > >   */
> > >  static int
> > >  skl_wm_plane_id(const struct intel_plane *plane)
> > > @@ -2874,7 +2872,6 @@ skl_wm_plane_id(const struct intel_plane
> > > *plane)
> > >  	case DRM_PLANE_TYPE_PRIMARY:
> > >  		return 0;
> > >  	case DRM_PLANE_TYPE_CURSOR:
> > > -		return PLANE_CURSOR;
> > >  	case DRM_PLANE_TYPE_OVERLAY:
> > >  		return plane->plane + 1;
> > >  	default:
> > > @@ -3128,14 +3125,6 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >  	alloc->end = alloc->start + pipe_size;
> > >  }
> > >  
> > > -static unsigned int skl_cursor_allocation(int num_active)
> > > -{
> > > -	if (num_active == 1)
> > > -		return 32;
> > > -
> > > -	return 8;
> > > -}
> > > -
> > >  static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry
> > > *entry,
> > > u32 reg)
> > >  {
> > >  	entry->start = reg & 0x3ff;
> > > @@ -3166,10 +3155,6 @@ void skl_ddb_get_hw_state(struct
> > > drm_i915_private *dev_priv,
> > >  						   val);
> > >  		}
> > >  
> > > -		val = I915_READ(CUR_BUF_CFG(pipe));
> > > -		skl_ddb_entry_init_from_hw(&ddb-
> > > > 
> > > > plane[pipe][PLANE_CURSOR],
> > > -					   val);
> > > -
> > >  		intel_display_power_put(dev_priv, power_domain);
> > >  	}
> > >  }
> > > @@ -3227,8 +3212,6 @@ skl_plane_relative_data_rate(const struct
> > > intel_crtc_state *cstate,
> > >  
> > >  	if (!intel_pstate->base.visible)
> > >  		return 0;
> > > -	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
> > > -		return 0;
> > >  	if (y && format != DRM_FORMAT_NV12)
> > >  		return 0;
> > >  
> > > @@ -3386,7 +3369,7 @@ skl_allocate_pipe_ddb(struct
> > > intel_crtc_state
> > > *cstate,
> > >  	struct drm_plane_state *pstate;
> > >  	enum pipe pipe = intel_crtc->pipe;
> > >  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> > > -	uint16_t alloc_size, start, cursor_blocks;
> > > +	uint16_t alloc_size, start;
> > >  	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
> > >  	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
> > >  	unsigned int total_data_rate;
> > > @@ -3412,12 +3395,6 @@ skl_allocate_pipe_ddb(struct
> > > intel_crtc_state
> > > *cstate,
> > >  		return 0;
> > >  	}
> > >  
> > > -	cursor_blocks = skl_cursor_allocation(num_active);
> > > -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> > > cursor_blocks;
> > > -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> > > -
> > > -	alloc_size -= cursor_blocks;
> > > -
> > >  	/* 1. Allocate the mininum required blocks for each
> > > active
> > > plane */
> > >  	for_each_plane_in_state(state, plane, pstate, i) {
> > >  		intel_plane = to_intel_plane(plane);
> > > @@ -3431,17 +3408,12 @@ skl_allocate_pipe_ddb(struct
> > > intel_crtc_state
> > > *cstate,
> > >  			y_minimum[id] = 0;
> > >  			continue;
> > >  		}
> > > -		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > > -			minimum[id] = 0;
> > > -			y_minimum[id] = 0;
> > > -			continue;
> > > -		}
> > >  
> > >  		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> > >  		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> > >  	}
> > >  
> > > -	for (i = 0; i < PLANE_CURSOR; i++) {
> > > +	for (i = 0; i < I915_MAX_PLANES; i++) {
> > >  		alloc_size -= minimum[i];
> > >  		alloc_size -= y_minimum[i];
> > >  	}
> > > @@ -3866,26 +3838,6 @@ void skl_write_plane_wm(struct intel_crtc
> > > *intel_crtc,
> > >  			    &ddb->y_plane[pipe][plane]);
> > >  }
> > >  
> > > -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > > -			 const struct skl_plane_wm *wm,
> > > -			 const struct skl_ddb_allocation *ddb)
> > > -{
> > > -	struct drm_crtc *crtc = &intel_crtc->base;
> > > -	struct drm_device *dev = crtc->dev;
> > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	int level, max_level = ilk_wm_max_level(dev_priv);
> > > -	enum pipe pipe = intel_crtc->pipe;
> > > -
> > > -	for (level = 0; level <= max_level; level++) {
> > > -		skl_write_wm_level(dev_priv, CUR_WM(pipe,
> > > level),
> > > -				   &wm->wm[level]);
> > > -	}
> > > -	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
> > > > 
> > > > trans_wm);
> > > -
> > > -	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> > > -			    &ddb->plane[pipe][PLANE_CURSOR]);
> > > -}
> > > -
> > >  bool skl_wm_level_equals(const struct skl_wm_level *l1,
> > >  			 const struct skl_wm_level *l2)
> > >  {
> > > @@ -4122,19 +4074,11 @@ skl_print_wm_changes(const struct
> > > drm_atomic_state *state)
> > >  			if (skl_ddb_entry_equal(old, new))
> > >  				continue;
> > >  
> > > -			if (id != PLANE_CURSOR) {
> > > -				DRM_DEBUG_ATOMIC("[PLANE:%d:plan
> > > e
> > > %d%c] ddb (%d - %d) -> (%d - %d)\n",
> > > -						 plane->base.id, 
> > > id
> > > + 1,
> > > -						 pipe_name(pipe)
> > > ,
> > > -						 old->start,
> > > old-
> > > > 
> > > > end,
> > > -						 new->start,
> > > new-
> > > > 
> > > > end);
> > > -			} else {
> > > -				DRM_DEBUG_ATOMIC("[PLANE:%d:curs
> > > or
> > > %c] ddb (%d - %d) -> (%d - %d)\n",
> > > -						 plane->base.id,
> > > -						 pipe_name(pipe)
> > > ,
> > > -						 old->start,
> > > old-
> > > > 
> > > > end,
> > > -						 new->start,
> > > new-
> > > > 
> > > > end);
> > > -			}
> > > +			DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c]
> > > ddb
> > > (%d - %d) -> (%d - %d)\n",
> > > +					 plane->base.id, id + 1,
> > > +					 pipe_name(pipe),
> > > +					 old->start, old->end,
> > > +					 new->start, new->end);
> > >  		}
> > >  	}
> > >  }
> > > @@ -4235,9 +4179,6 @@ static void skl_update_wm(struct drm_crtc
> > > *crtc)
> > >  		for_each_universal_plane(dev_priv, pipe, plane)
> > >  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> > > > 
> > > > planes[plane],
> > >  					   &results->ddb,
> > > plane);
> > > -
> > > -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> > > > 
> > > > planes[PLANE_CURSOR],
> > > -				    &results->ddb);
> > >  	}
> > >  
> > >  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> > > @@ -4350,18 +4291,12 @@ void skl_pipe_wm_get_hw_state(struct
> > > drm_crtc
> > > *crtc,
> > >  		wm = &out->planes[id];
> > >  
> > >  		for (level = 0; level <= max_level; level++) {
> > > -			if (id != PLANE_CURSOR)
> > > -				val = I915_READ(PLANE_WM(pipe,
> > > id,
> > > level));
> > > -			else
> > > -				val = I915_READ(CUR_WM(pipe,
> > > level));
> > > +			val = I915_READ(PLANE_WM(pipe, id,
> > > level));
> > >  
> > >  			skl_wm_level_from_reg_val(val, &wm-
> > > > 
> > > > wm[level]);
> > >  		}
> > >  
> > > -		if (id != PLANE_CURSOR)
> > > -			val = I915_READ(PLANE_WM_TRANS(pipe,
> > > id));
> > > -		else
> > > -			val = I915_READ(CUR_WM_TRANS(pipe));
> > > +		val = I915_READ(PLANE_WM_TRANS(pipe, id));
> > >  
> > >  		skl_wm_level_from_reg_val(val, &wm->trans_wm);
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 43d0350..9e6406a 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -194,7 +194,7 @@ void intel_pipe_update_end(struct intel_crtc
> > > *crtc, struct intel_flip_work *work
> > >  	}
> > >  }
> > >  
> > > -static void
> > > +void
> > >  skl_update_plane(struct drm_plane *drm_plane,
> > >  		 const struct intel_crtc_state *crtc_state,
> > >  		 const struct intel_plane_state *plane_state)
> > > @@ -285,7 +285,7 @@ skl_update_plane(struct drm_plane *drm_plane,
> > >  	POSTING_READ(PLANE_SURF(pipe, plane));
> > >  }
> > >  
> > > -static void
> > > +void
> > >  skl_disable_plane(struct drm_plane *dplane, struct drm_crtc
> > > *crtc)
> > >  {
> > >  	struct drm_device *dev = dplane->dev;
> > > @@ -752,7 +752,7 @@ ilk_disable_plane(struct drm_plane *plane,
> > > struct
> > > drm_crtc *crtc)
> > >  	POSTING_READ(DVSSURF(pipe));
> > >  }
> > >  
> > > -static int
> > > +int
> > >  intel_check_sprite_plane(struct drm_plane *plane,
> > >  			 struct intel_crtc_state *crtc_state,
> > >  			 struct intel_plane_state *state)
> > -- 
> > Cheers,
> > 	Lyude
> 
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/4] drm/i915: Rename for_each_plane -> for_each_universal_plane

[Paulo Zanoni]

Em Qua, 2016-10-26 às 15:51 -0700, Matt Roper escreveu:
> This macro's name is a bit misleading; it doesn't actually iterate
> over
> all planes since it omits the cursor plane.  Its only uses are in
> gen9
> code which is using it to iterate over the universal planes (which we
> treat as primary+sprites); in these cases the legacy cursor registers
> are programmed independently if necessary.  The macro's iterator
> value
> (0 for primary plane, spritenum+1 for each secondary plane) also
> isn't
> meaningful outside the gen9 context where the hardware considers them
> to
> all be "universal" planes that follow this numbering.
> 
> This is just a renaming/clarification patch with no functional
> change.
> However it will make the subsequent patches more clear.

I really like the rename. Also, this seems to apply even on top of
Maarten's series. IMHO we should merge this soon, before patch 3 reach
its final form.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 2 +-
>  drivers/gpu/drm/i915/i915_drv.h      | 2 +-
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index be92efe..9f5a392 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3463,7 +3463,7 @@ static int i915_ddb_info(struct seq_file *m,
> void *unused)
>  	for_each_pipe(dev_priv, pipe) {
>  		seq_printf(m, "Pipe %c\n", pipe_name(pipe));
>  
> -		for_each_plane(dev_priv, pipe, plane) {
> +		for_each_universal_plane(dev_priv, pipe, plane) {
>  			entry = &ddb->plane[pipe][plane];
>  			seq_printf(m, "  Plane%-8d%8u%8u%8u\n",
> plane + 1,
>  				   entry->start, entry->end,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 55afb66..4714051 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -312,7 +312,7 @@ struct i915_hotplug {
>  #define for_each_pipe_masked(__dev_priv, __p, __mask) \
>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> (__p)++) \
>  		for_each_if ((__mask) & (1 << (__p)))
> -#define for_each_plane(__dev_priv, __pipe, __p)			
> 	\
> +#define for_each_universal_plane(__dev_priv, __pipe, __p)		
> \
>  	for ((__p) = 0;						
> 	\
>  	     (__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] +
> 1;	\
>  	     (__p)++)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 895b3dc..cb7dd11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13485,7 +13485,7 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
>  
>  	/* planes */
> -	for_each_plane(dev_priv, pipe, plane) {
> +	for_each_universal_plane(dev_priv, pipe, plane) {
>  		hw_plane_wm = &hw_wm.planes[plane];
>  		sw_plane_wm = &sw_wm->planes[plane];
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 9e0e874..58d3ba0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3160,7 +3160,7 @@ void skl_ddb_get_hw_state(struct
> drm_i915_private *dev_priv,
>  		if (!intel_display_power_get_if_enabled(dev_priv,
> power_domain))
>  			continue;
>  
> -		for_each_plane(dev_priv, pipe, plane) {
> +		for_each_universal_plane(dev_priv, pipe, plane) {
>  			val = I915_READ(PLANE_BUF_CFG(pipe, plane));
>  			skl_ddb_entry_init_from_hw(&ddb-
> >plane[pipe][plane],
>  						   val);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 2/4] drm/i915: Use macro in place of open-coded for_each_universal_plane loop

[Paulo Zanoni]

Em Qua, 2016-10-26 às 15:51 -0700, Matt Roper escreveu:
> This was the only use of (misleadingly-named) intel_num_planes()
> function, so we can remove it as well.

This one has a trivial conflict with Maarten's series. Same comment as
p1 regarding merging.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 9 ---------
>  drivers/gpu/drm/i915/intel_pm.c  | 2 +-
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index c2f3863..c31fddd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1108,15 +1108,6 @@ hdmi_to_dig_port(struct intel_hdmi
> *intel_hdmi)
>  	return container_of(intel_hdmi, struct intel_digital_port,
> hdmi);
>  }
>  
> -/*
> - * Returns the number of planes for this pipe, ie the number of
> sprites + 1
> - * (primary plane). This doesn't count the cursor plane then.
> - */
> -static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> -{
> -	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] +
> 1;
> -}
> -
>  /* intel_fifo_underrun.c */
>  bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private
> *dev_priv,
>  					   enum pipe pipe, bool
> enable);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 58d3ba0..6f19e60 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4232,7 +4232,7 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  	if (crtc->state->active_changed) {
>  		int plane;
>  
> -		for (plane = 0; plane <
> intel_num_planes(intel_crtc); plane++)
> +		for_each_universal_plane(dev_priv, pipe, plane)
>  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
>  					   &results->ddb, plane);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 2/4] drm/i915: Use macro in place of open-coded for_each_universal_plane loop

[Matt Roper]

On Fri, Oct 28, 2016 at 04:17:46PM -0200, Paulo Zanoni wrote:
> Em Qua, 2016-10-26 às 15:51 -0700, Matt Roper escreveu:
> > This was the only use of (misleadingly-named) intel_num_planes()
> > function, so we can remove it as well.
> 
> This one has a trivial conflict with Maarten's series. Same comment as
> p1 regarding merging.
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for the reviews.  Pushed these two trivial prep patches to dinq.


Matt


> 
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h | 9 ---------
> >  drivers/gpu/drm/i915/intel_pm.c  | 2 +-
> >  2 files changed, 1 insertion(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index c2f3863..c31fddd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1108,15 +1108,6 @@ hdmi_to_dig_port(struct intel_hdmi
> > *intel_hdmi)
> >  	return container_of(intel_hdmi, struct intel_digital_port,
> > hdmi);
> >  }
> >  
> > -/*
> > - * Returns the number of planes for this pipe, ie the number of
> > sprites + 1
> > - * (primary plane). This doesn't count the cursor plane then.
> > - */
> > -static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> > -{
> > -	return INTEL_INFO(crtc->base.dev)->num_sprites[crtc->pipe] +
> > 1;
> > -}
> > -
> >  /* intel_fifo_underrun.c */
> >  bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private
> > *dev_priv,
> >  					   enum pipe pipe, bool
> > enable);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 58d3ba0..6f19e60 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4232,7 +4232,7 @@ static void skl_update_wm(struct drm_crtc
> > *crtc)
> >  	if (crtc->state->active_changed) {
> >  		int plane;
> >  
> > -		for (plane = 0; plane <
> > intel_num_planes(intel_crtc); plane++)
> > +		for_each_universal_plane(dev_priv, pipe, plane)
> >  			skl_write_plane_wm(intel_crtc, &pipe_wm-
> > >planes[plane],
> >  					   &results->ddb, plane);
> >  

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx