[PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2)

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2)
@ 2016-11-22 16:01 ville.syrjala
  2016-11-22 16:01 ` [PATCH 1/9] drm/i915: Add per-pipe plane identifier ville.syrjala
                   ` (10 more replies)
  0 siblings, 11 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:01 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I ended up tweaking quite a few of the patches (and even adding a new one)
based on Paulo's review, so figured I'd repost the entire thing.

I didn't do the _ID_ rename thing, at least not yet. I'll have to
think about it more if I could come up with some nice way to deal
with the per-pipe id vs. the A/B/C plane id.

Anyways, enough of these got r-bs so that I can at least move
forward with VLV/CHV watermark rewrite #4 now.

Ville Syrjälä (9):
  drm/i915: Add per-pipe plane identifier
  drm/i915: Add crtc->plane_ids_mask
  drm/i915: Use enum plane_id in SKL wm code
  drm/i915: Use enum plane_id in SKL plane code
  drm/i915: Use enum plane_id in VLV/CHV sprite code
  drm/i915: Use enum plane_id in VLV/CHV wm code
  drm/i915: s/plane/plane_id/ in skl+ plane register defines
  drm/i915: Don't populate plane->plane for cursors and sprites
  drm/i915: Rename the local 'plane' variable to 'plane_id' in primary
    plane code

 drivers/gpu/drm/i915/i915_drv.h      |  30 ++++-
 drivers/gpu/drm/i915/i915_reg.h      | 104 +++++++-------
 drivers/gpu/drm/i915/intel_display.c |  96 ++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 253 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_sprite.c  | 118 ++++++++--------
 6 files changed, 304 insertions(+), 303 deletions(-)

-- 
2.7.4

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

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

* [PATCH 1/9] drm/i915: Add per-pipe plane identifier
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
@ 2016-11-22 16:01 ` ville.syrjala
  2016-11-22 16:01 ` [PATCH 2/9] drm/i915: Add crtc->plane_ids_mask ville.syrjala
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:01 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

As I told people in [1] we really should not be confusing enum plane
as a per-pipe plane identifier. Looks like that happened nonetheless, so
let's fix it up by splitting the two into two enums.

We'll also want something we just directly pass to various register
offset macros and whatnot on SKL+. So let's make this new thing work for that.
Currently we pass intel_plane->plane for the "sprites" and just a
hardcoded zero for the "primary" planes. We want to get rid of that
hardocoding so that we can share the same code for all planes (apart
from the legacy cursor of course).

[1] https://lists.freedesktop.org/archives/intel-gfx/2015-September/076082.html

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 28 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_display.c |  2 ++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_sprite.c  |  1 +
 4 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c7d5f7a30fe8..6c9864287b29 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -180,22 +180,36 @@ static inline bool transcoder_is_dsi(enum transcoder transcoder)
 }
 
 /*
- * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
- * number of planes per CRTC.  Not all platforms really have this many planes,
- * which means some arrays of size I915_MAX_PLANES may have unused entries
- * between the topmost sprite plane and the cursor plane.
+ * Global legacy plane identifier. Valid only for primary/sprite
+ * planes on pre-g4x, and only for primary planes on g4x+.
  */
 enum plane {
-	PLANE_A = 0,
+	PLANE_A,
 	PLANE_B,
 	PLANE_C,
-	PLANE_CURSOR,
-	I915_MAX_PLANES,
 };
 #define plane_name(p) ((p) + 'A')
 
 #define sprite_name(p, s) ((p) * INTEL_INFO(dev_priv)->num_sprites[(p)] + (s) + 'A')
 
+/*
+ * Per-pipe plane identifier.
+ * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
+ * number of planes per CRTC.  Not all platforms really have this many planes,
+ * which means some arrays of size I915_MAX_PLANES may have unused entries
+ * between the topmost sprite plane and the cursor plane.
+ *
+ * This is expected to be passed to various register macros
+ * (eg. PLANE_CTL(), PS_PLANE_SEL(), etc.) so adjust with care.
+ */
+enum plane_id {
+	PLANE_PRIMARY,
+	PLANE_SPRITE0,
+	PLANE_SPRITE1,
+	PLANE_CURSOR,
+	I915_MAX_PLANES,
+};
+
 enum port {
 	PORT_NONE = -1,
 	PORT_A = 0,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b7a7ed82c325..dfd3dbe6a92a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14988,6 +14988,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 		primary->plane = (enum plane) !pipe;
 	else
 		primary->plane = (enum plane) pipe;
+	primary->id = PLANE_PRIMARY;
 	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
 	primary->check_plane = intel_check_primary_plane;
 
@@ -15187,6 +15188,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
 	cursor->plane = pipe;
+	cursor->id = PLANE_CURSOR;
 	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
 	cursor->check_plane = intel_check_cursor_plane;
 	cursor->update_plane = intel_update_cursor_plane;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cd132c216a67..b2a0409330a9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -766,7 +766,8 @@ struct intel_plane_wm_parameters {
 
 struct intel_plane {
 	struct drm_plane base;
-	int plane;
+	u8 plane;
+	enum plane_id id;
 	enum pipe pipe;
 	bool can_scale;
 	int max_downscale;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8f131a08d440..b5f750364e99 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1112,6 +1112,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 
 	intel_plane->pipe = pipe;
 	intel_plane->plane = plane;
+	intel_plane->id = PLANE_SPRITE0 + plane;
 	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
 	intel_plane->check_plane = intel_check_sprite_plane;
 
-- 
2.7.4

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

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

* [PATCH 2/9] drm/i915: Add crtc->plane_ids_mask
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
  2016-11-22 16:01 ` [PATCH 1/9] drm/i915: Add per-pipe plane identifier ville.syrjala
@ 2016-11-22 16:01 ` ville.syrjala
  2016-11-22 16:01 ` [PATCH 3/9] drm/i915: Use enum plane_id in SKL wm code ville.syrjala
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:01 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mask of which planes are available for each pipe. This doesn't
quite work for old platforms with dynamic plane<->pipe assignment, but
as we don't support that sort of stuff (yet) we can get away with it.

The main use I have for this is the for_each_plane_id_on_crtc() macro
for iterating over all possible planes on the crtc. I suppose we could
not add the mask, and instead iterate by comparing intel_plane->pipe
but then we'd need a local intel_plane variable which is just
unnecessary clutter in some cases. But I'm not hung up on this, so if
people prefer the other option I could be convinced to use it.

v2: Use BIT() in the iterator macro too (Paulo)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 4 ++++
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 drivers/gpu/drm/i915/intel_drv.h     | 3 ++-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6c9864287b29..90bd31d71a81 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -210,6 +210,10 @@ enum plane_id {
 	I915_MAX_PLANES,
 };
 
+#define for_each_plane_id_on_crtc(__crtc, __p) \
+	for ((__p) = PLANE_PRIMARY; (__p) < I915_MAX_PLANES; (__p)++) \
+		for_each_if ((__crtc)->plane_ids_mask & BIT(__p))
+
 enum port {
 	PORT_NONE = -1,
 	PORT_A = 0,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dfd3dbe6a92a..6c3032f04d54 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15277,6 +15277,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 		ret = PTR_ERR(primary);
 		goto fail;
 	}
+	intel_crtc->plane_ids_mask |= BIT(primary->id);
 
 	for_each_sprite(dev_priv, pipe, sprite) {
 		struct intel_plane *plane;
@@ -15286,6 +15287,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 			ret = PTR_ERR(plane);
 			goto fail;
 		}
+		intel_crtc->plane_ids_mask |= BIT(plane->id);
 	}
 
 	cursor = intel_cursor_plane_create(dev_priv, pipe);
@@ -15293,6 +15295,7 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 		ret = PTR_ERR(cursor);
 		goto fail;
 	}
+	intel_crtc->plane_ids_mask |= BIT(cursor->id);
 
 	ret = drm_crtc_init_with_planes(&dev_priv->drm, &intel_crtc->base,
 					&primary->base, &cursor->base,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b2a0409330a9..c1b245853ba9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -691,8 +691,9 @@ struct intel_crtc {
 	 * some outputs connected to this crtc.
 	 */
 	bool active;
-	unsigned long enabled_power_domains;
 	bool lowfreq_avail;
+	u8 plane_ids_mask;
+	unsigned long enabled_power_domains;
 	struct intel_overlay *overlay;
 	struct intel_flip_work *flip_work;
 
-- 
2.7.4

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

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

* [PATCH 3/9] drm/i915: Use enum plane_id in SKL wm code
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
  2016-11-22 16:01 ` [PATCH 1/9] drm/i915: Add per-pipe plane identifier ville.syrjala
  2016-11-22 16:01 ` [PATCH 2/9] drm/i915: Add crtc->plane_ids_mask ville.syrjala
@ 2016-11-22 16:01 ` ville.syrjala
  2016-11-22 20:27   ` Lyude Paul
  2016-11-22 16:01 ` [PATCH 4/9] drm/i915: Use enum plane_id in SKL plane code ville.syrjala
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:01 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Nuke skl_wm_plane_id() and just use the new intel_plane->id.

v2: Convert skl_write_plane_wm() as well
v3: Convert skl_pipe_wm_get_hw_state() correctly
v4: Rebase due to changes in the wm code
    Drop the cursor FIXME from the total data rate calc (Paulo)
    Use the "[PLANE:%d:%s]" format in debug print (Paulo)

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Lyude <cpaul@redhat.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 180 +++++++++++++++++-----------------------
 1 file changed, 77 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bbb1eaf1e6db..e6351445016d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2867,28 +2867,6 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
 #define SKL_SAGV_BLOCK_TIME	30 /* µs */
 
 /*
- * 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.
- */
-static int
-skl_wm_plane_id(const struct intel_plane *plane)
-{
-	switch (plane->base.type) {
-	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:
-		MISSING_CASE(plane->base.type);
-		return plane->plane;
-	}
-}
-
-/*
  * FIXME: We still don't have the proper code detect if we need to apply the WA,
  * so assume we'll always need it in order to avoid underruns.
  */
@@ -3026,7 +3004,6 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	struct intel_crtc *crtc;
 	struct intel_plane *plane;
 	struct intel_crtc_state *cstate;
-	struct skl_plane_wm *wm;
 	enum pipe pipe;
 	int level, latency;
 
@@ -3053,7 +3030,8 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 		return false;
 
 	for_each_intel_plane_on_crtc(dev, crtc, plane) {
-		wm = &cstate->wm.skl.optimal.planes[skl_wm_plane_id(plane)];
+		struct skl_plane_wm *wm =
+			&cstate->wm.skl.optimal.planes[plane->id];
 
 		/* Skip this plane if it's not enabled */
 		if (!wm->wm[0].plane_en)
@@ -3156,28 +3134,29 @@ static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */)
 {
-	enum pipe pipe;
-	int plane;
-	u32 val;
+	struct intel_crtc *crtc;
 
 	memset(ddb, 0, sizeof(*ddb));
 
-	for_each_pipe(dev_priv, pipe) {
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		enum intel_display_power_domain power_domain;
+		enum plane_id plane_id;
+		enum pipe pipe = crtc->pipe;
 
 		power_domain = POWER_DOMAIN_PIPE(pipe);
 		if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 			continue;
 
-		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);
-		}
+		for_each_plane_id_on_crtc(crtc, plane_id) {
+			u32 val;
+
+			if (plane_id != PLANE_CURSOR)
+				val = I915_READ(PLANE_BUF_CFG(pipe, plane_id));
+			else
+				val = I915_READ(CUR_BUF_CFG(pipe));
 
-		val = I915_READ(CUR_BUF_CFG(pipe));
-		skl_ddb_entry_init_from_hw(&ddb->plane[pipe][PLANE_CURSOR],
-					   val);
+			skl_ddb_entry_init_from_hw(&ddb->plane[pipe][plane_id], val);
+		}
 
 		intel_display_power_put(dev_priv, power_domain);
 	}
@@ -3278,30 +3257,28 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
 	struct drm_plane *plane;
-	const struct intel_plane *intel_plane;
 	const struct drm_plane_state *pstate;
-	unsigned int rate, total_data_rate = 0;
-	int id;
+	unsigned int total_data_rate = 0;
 
 	if (WARN_ON(!state))
 		return 0;
 
 	/* Calculate and cache data rate for each plane */
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
-		id = skl_wm_plane_id(to_intel_plane(plane));
-		intel_plane = to_intel_plane(plane);
+		enum plane_id plane_id = to_intel_plane(plane)->id;
+		unsigned int rate;
 
 		/* packed/uv */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 0);
-		plane_data_rate[id] = rate;
+		plane_data_rate[plane_id] = rate;
 
 		total_data_rate += rate;
 
 		/* y-plane */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 1);
-		plane_y_data_rate[id] = rate;
+		plane_y_data_rate[plane_id] = rate;
 
 		total_data_rate += rate;
 	}
@@ -3380,17 +3357,16 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
 	struct drm_plane *plane;
 
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
-		struct intel_plane *intel_plane = to_intel_plane(plane);
-		int id = skl_wm_plane_id(intel_plane);
+		enum plane_id plane_id = to_intel_plane(plane)->id;
 
-		if (id == PLANE_CURSOR)
+		if (plane_id == PLANE_CURSOR)
 			continue;
 
 		if (!pstate->visible)
 			continue;
 
-		minimum[id] = skl_ddb_min_alloc(pstate, 0);
-		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
+		minimum[plane_id] = skl_ddb_min_alloc(pstate, 0);
+		y_minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
 	}
 
 	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
@@ -3410,8 +3386,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t y_minimum[I915_MAX_PLANES] = {};
 	unsigned int total_data_rate;
+	enum plane_id plane_id;
 	int num_active;
-	int id, i;
 	unsigned plane_data_rate[I915_MAX_PLANES] = {};
 	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
 
@@ -3442,9 +3418,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	 * proportional to the data rate.
 	 */
 
-	for (i = 0; i < I915_MAX_PLANES; i++) {
-		alloc_size -= minimum[i];
-		alloc_size -= y_minimum[i];
+	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+		alloc_size -= minimum[plane_id];
+		alloc_size -= y_minimum[plane_id];
 	}
 
 	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
@@ -3463,28 +3439,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 
 	start = alloc->start;
-	for (id = 0; id < I915_MAX_PLANES; id++) {
+	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
 		unsigned int data_rate, y_data_rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
 
-		if (id == PLANE_CURSOR)
+		if (plane_id == PLANE_CURSOR)
 			continue;
 
-		data_rate = plane_data_rate[id];
+		data_rate = plane_data_rate[plane_id];
 
 		/*
 		 * allocation for (packed formats) or (uv-plane part of planar format):
 		 * promote the expression to 64 bits to avoid overflowing, the
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
-		plane_blocks = minimum[id];
+		plane_blocks = minimum[plane_id];
 		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
 					total_data_rate);
 
 		/* Leave disabled planes at (0,0) */
 		if (data_rate) {
-			ddb->plane[pipe][id].start = start;
-			ddb->plane[pipe][id].end = start + plane_blocks;
+			ddb->plane[pipe][plane_id].start = start;
+			ddb->plane[pipe][plane_id].end = start + plane_blocks;
 		}
 
 		start += plane_blocks;
@@ -3492,15 +3468,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		/*
 		 * allocation for y_plane part of planar format:
 		 */
-		y_data_rate = plane_y_data_rate[id];
+		y_data_rate = plane_y_data_rate[plane_id];
 
-		y_plane_blocks = y_minimum[id];
+		y_plane_blocks = y_minimum[plane_id];
 		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
 					total_data_rate);
 
 		if (y_data_rate) {
-			ddb->y_plane[pipe][id].start = start;
-			ddb->y_plane[pipe][id].end = start + y_plane_blocks;
+			ddb->y_plane[pipe][plane_id].start = start;
+			ddb->y_plane[pipe][plane_id].end = start + y_plane_blocks;
 		}
 
 		start += y_plane_blocks;
@@ -3692,12 +3668,12 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		if (level) {
 			return 0;
 		} else {
+			struct drm_plane *plane = pstate->plane;
+
 			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
-			DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u, lines required = %u/31\n",
-				      to_intel_crtc(cstate->base.crtc)->pipe,
-				      skl_wm_plane_id(to_intel_plane(pstate->plane)),
+			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
+				      plane->base.id, plane->name,
 				      res_blocks, ddb_allocation, res_lines);
-
 			return -EINVAL;
 		}
 	}
@@ -3724,7 +3700,6 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 	uint16_t ddb_blocks;
 	enum pipe pipe = intel_crtc->pipe;
 	int ret;
-	int i = skl_wm_plane_id(intel_plane);
 
 	if (state)
 		intel_pstate =
@@ -3747,7 +3722,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 
 	WARN_ON(!intel_pstate->base.fb);
 
-	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
+	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][intel_plane->id]);
 
 	ret = skl_compute_plane_wm(dev_priv,
 				   cstate,
@@ -3810,7 +3785,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 	for_each_intel_plane_mask(&dev_priv->drm,
 				  intel_plane,
 				  cstate->base.plane_mask) {
-		wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
+		wm = &pipe_wm->planes[intel_plane->id];
 
 		for (level = 0; level <= max_level; level++) {
 			ret = skl_compute_wm_level(dev_priv, ddb, cstate,
@@ -3854,7 +3829,7 @@ static void skl_write_wm_level(struct drm_i915_private *dev_priv,
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 			const struct skl_plane_wm *wm,
 			const struct skl_ddb_allocation *ddb,
-			int plane)
+			enum plane_id plane_id)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
 	struct drm_device *dev = crtc->dev;
@@ -3863,16 +3838,16 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 	enum pipe pipe = intel_crtc->pipe;
 
 	for (level = 0; level <= max_level; level++) {
-		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane, level),
+		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level),
 				   &wm->wm[level]);
 	}
-	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane),
+	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
 			   &wm->trans_wm);
 
-	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
-			    &ddb->plane[pipe][plane]);
-	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
-			    &ddb->y_plane[pipe][plane]);
+	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
+			    &ddb->plane[pipe][plane_id]);
+	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane_id),
+			    &ddb->y_plane[pipe][plane_id]);
 }
 
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
@@ -3977,17 +3952,16 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
 	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	enum pipe pipe = intel_crtc->pipe;
-	int id;
 
 	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
 
 	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
-		id = skl_wm_plane_id(to_intel_plane(plane));
+		enum plane_id plane_id = to_intel_plane(plane)->id;
 
-		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id],
-					&new_ddb->plane[pipe][id]) &&
-		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][id],
-					&new_ddb->y_plane[pipe][id]))
+		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
+					&new_ddb->plane[pipe][plane_id]) &&
+		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][plane_id],
+					&new_ddb->y_plane[pipe][plane_id]))
 			continue;
 
 		plane_state = drm_atomic_get_plane_state(state, plane);
@@ -4099,7 +4073,6 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
 	const struct intel_plane *intel_plane;
 	const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
 	const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
-	int id;
 	int i;
 
 	for_each_crtc_in_state(state, crtc, cstate, i) {
@@ -4107,11 +4080,11 @@ skl_print_wm_changes(const struct drm_atomic_state *state)
 		enum pipe pipe = intel_crtc->pipe;
 
 		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+			enum plane_id plane_id = intel_plane->id;
 			const struct skl_ddb_entry *old, *new;
 
-			id = skl_wm_plane_id(intel_plane);
-			old = &old_ddb->plane[pipe][id];
-			new = &new_ddb->plane[pipe][id];
+			old = &old_ddb->plane[pipe][plane_id];
+			new = &new_ddb->plane[pipe][plane_id];
 
 			if (skl_ddb_entry_equal(old, new))
 				continue;
@@ -4201,17 +4174,21 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
 	enum pipe pipe = crtc->pipe;
-	int plane;
+	enum plane_id plane_id;
 
 	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
 		return;
 
 	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
 
-	for_each_universal_plane(dev_priv, pipe, plane)
-		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
-
-	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
+	for_each_plane_id_on_crtc(crtc, plane_id) {
+		if (plane_id != PLANE_CURSOR)
+			skl_write_plane_wm(crtc, &pipe_wm->planes[plane_id],
+					   ddb, plane_id);
+		else
+			skl_write_cursor_wm(crtc, &pipe_wm->planes[plane_id],
+					    ddb);
+	}
 }
 
 static void skl_initial_wm(struct intel_atomic_state *state,
@@ -4326,32 +4303,29 @@ static inline void skl_wm_level_from_reg_val(uint32_t val,
 void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
 			      struct skl_pipe_wm *out)
 {
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-	struct skl_plane_wm *wm;
 	enum pipe pipe = intel_crtc->pipe;
-	int level, id, max_level;
+	int level, max_level;
+	enum plane_id plane_id;
 	uint32_t val;
 
 	max_level = ilk_wm_max_level(dev_priv);
 
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		id = skl_wm_plane_id(intel_plane);
-		wm = &out->planes[id];
+	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+		struct skl_plane_wm *wm = &out->planes[plane_id];
 
 		for (level = 0; level <= max_level; level++) {
-			if (id != PLANE_CURSOR)
-				val = I915_READ(PLANE_WM(pipe, id, level));
+			if (plane_id != PLANE_CURSOR)
+				val = I915_READ(PLANE_WM(pipe, plane_id, level));
 			else
 				val = I915_READ(CUR_WM(pipe, level));
 
 			skl_wm_level_from_reg_val(val, &wm->wm[level]);
 		}
 
-		if (id != PLANE_CURSOR)
-			val = I915_READ(PLANE_WM_TRANS(pipe, id));
+		if (plane_id != PLANE_CURSOR)
+			val = I915_READ(PLANE_WM_TRANS(pipe, plane_id));
 		else
 			val = I915_READ(CUR_WM_TRANS(pipe));
 
-- 
2.7.4

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

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

* [PATCH 4/9] drm/i915: Use enum plane_id in SKL plane code
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (2 preceding siblings ...)
  2016-11-22 16:01 ` [PATCH 3/9] drm/i915: Use enum plane_id in SKL wm code ville.syrjala
@ 2016-11-22 16:01 ` ville.syrjala
  2016-11-22 16:02 ` [PATCH 5/9] drm/i915: Use enum plane_id in VLV/CHV sprite code ville.syrjala
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:01 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the intel_plane->plane and hardcoded 0 usage in the SKL plane
code with intel_plane->id.

This should make the SKL "primary" and "sprite" code virtually
identical, so the next logical step would likely be dropping one
of the copies.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_sprite.c  | 42 ++++++++++++++++++------------------
 2 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6c3032f04d54..0a98989b8663 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3378,7 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	int pipe = intel_crtc->pipe;
+	enum plane_id plane_id = to_intel_plane(plane)->id;
+	enum pipe pipe = to_intel_plane(plane)->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
 	u32 stride = skl_plane_stride(fb, 0, rotation);
@@ -3413,30 +3414,30 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = src_x;
 	intel_crtc->adjusted_y = src_y;
 
-	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
-	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
-	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
-	I915_WRITE(PLANE_SIZE(pipe, 0), (src_h << 16) | src_w);
+	I915_WRITE(PLANE_CTL(pipe, plane_id), plane_ctl);
+	I915_WRITE(PLANE_OFFSET(pipe, plane_id), (src_y << 16) | src_x);
+	I915_WRITE(PLANE_STRIDE(pipe, plane_id), stride);
+	I915_WRITE(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w);
 
 	if (scaler_id >= 0) {
 		uint32_t ps_ctrl = 0;
 
 		WARN_ON(!dst_w || !dst_h);
-		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) |
+		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane_id) |
 			crtc_state->scaler_state.scalers[scaler_id].mode;
 		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
 		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
 		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (dst_x << 16) | dst_y);
 		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h);
-		I915_WRITE(PLANE_POS(pipe, 0), 0);
+		I915_WRITE(PLANE_POS(pipe, plane_id), 0);
 	} else {
-		I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x);
+		I915_WRITE(PLANE_POS(pipe, plane_id), (dst_y << 16) | dst_x);
 	}
 
-	I915_WRITE(PLANE_SURF(pipe, 0),
+	I915_WRITE(PLANE_SURF(pipe, plane_id),
 		   intel_fb_gtt_offset(fb, rotation) + surf_addr);
 
-	POSTING_READ(PLANE_SURF(pipe, 0));
+	POSTING_READ(PLANE_SURF(pipe, plane_id));
 }
 
 static void skylake_disable_primary_plane(struct drm_plane *primary,
@@ -3444,12 +3445,12 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
+	enum plane_id plane_id = to_intel_plane(primary)->id;
+	enum pipe pipe = to_intel_plane(primary)->pipe;
 
-	I915_WRITE(PLANE_CTL(pipe, 0), 0);
-	I915_WRITE(PLANE_SURF(pipe, 0), 0);
-	POSTING_READ(PLANE_SURF(pipe, 0));
+	I915_WRITE(PLANE_CTL(pipe, plane_id), 0);
+	I915_WRITE(PLANE_SURF(pipe, plane_id), 0);
+	POSTING_READ(PLANE_SURF(pipe, plane_id));
 }
 
 /* Assume fb object is pinned & idle & fenced and just update base pointers */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index b5f750364e99..93158061f790 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,8 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const int pipe = intel_plane->pipe;
-	const int plane = intel_plane->plane + 1;
+	enum plane_id plane_id = intel_plane->id;
+	enum pipe pipe = intel_plane->pipe;
 	u32 plane_ctl;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 surf_addr = plane_state->main.offset;
@@ -229,9 +229,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
 	if (key->flags) {
-		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
-		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-		I915_WRITE(PLANE_KEYMSK(pipe, plane), key->channel_mask);
+		I915_WRITE(PLANE_KEYVAL(pipe, plane_id), key->min_value);
+		I915_WRITE(PLANE_KEYMAX(pipe, plane_id), key->max_value);
+		I915_WRITE(PLANE_KEYMSK(pipe, plane_id), key->channel_mask);
 	}
 
 	if (key->flags & I915_SET_COLORKEY_DESTINATION)
@@ -245,36 +245,36 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
-	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
-	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
-	I915_WRITE(PLANE_SIZE(pipe, plane), (src_h << 16) | src_w);
+	I915_WRITE(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
+	I915_WRITE(PLANE_STRIDE(pipe, plane_id), stride);
+	I915_WRITE(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w);
 
 	/* program plane scaler */
 	if (plane_state->scaler_id >= 0) {
 		int scaler_id = plane_state->scaler_id;
 		const struct intel_scaler *scaler;
 
-		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
-			PS_PLANE_SEL(plane));
+		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n",
+			      plane_id, PS_PLANE_SEL(plane_id));
 
 		scaler = &crtc_state->scaler_state.scalers[scaler_id];
 
 		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id),
-			   PS_SCALER_EN | PS_PLANE_SEL(plane) | scaler->mode);
+			   PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
 		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
 		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
 		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id),
 			((crtc_w + 1) << 16)|(crtc_h + 1));
 
-		I915_WRITE(PLANE_POS(pipe, plane), 0);
+		I915_WRITE(PLANE_POS(pipe, plane_id), 0);
 	} else {
-		I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
+		I915_WRITE(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 	}
 
-	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
-	I915_WRITE(PLANE_SURF(pipe, plane),
+	I915_WRITE(PLANE_CTL(pipe, plane_id), plane_ctl);
+	I915_WRITE(PLANE_SURF(pipe, plane_id),
 		   intel_fb_gtt_offset(fb, rotation) + surf_addr);
-	POSTING_READ(PLANE_SURF(pipe, plane));
+	POSTING_READ(PLANE_SURF(pipe, plane_id));
 }
 
 static void
@@ -283,13 +283,13 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
-	const int pipe = intel_plane->pipe;
-	const int plane = intel_plane->plane + 1;
+	enum plane_id plane_id = intel_plane->id;
+	enum pipe pipe = intel_plane->pipe;
 
-	I915_WRITE(PLANE_CTL(pipe, plane), 0);
+	I915_WRITE(PLANE_CTL(pipe, plane_id), 0);
 
-	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-	POSTING_READ(PLANE_SURF(pipe, plane));
+	I915_WRITE(PLANE_SURF(pipe, plane_id), 0);
+	POSTING_READ(PLANE_SURF(pipe, plane_id));
 }
 
 static void
-- 
2.7.4

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

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

* [PATCH 5/9] drm/i915: Use enum plane_id in VLV/CHV sprite code
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (3 preceding siblings ...)
  2016-11-22 16:01 ` [PATCH 4/9] drm/i915: Use enum plane_id in SKL plane code ville.syrjala
@ 2016-11-22 16:02 ` ville.syrjala
  2016-11-22 16:02 ` [PATCH 6/9] drm/i915: Use enum plane_id in VLV/CHV wm code ville.syrjala
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:02 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use intel_plane->id to derive the VLV/CHV sprite register offsets
instead of abusing plane->plane which is really meant to for
primary planes only.

v2: Convert assert_sprites_disabled() over as well
v3: Rename the reg macro parameter to 'plane_id' as well (Paulo)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 58 +++++++++++++++-------------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 74 ++++++++++++++++++------------------
 3 files changed, 70 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c70c07a7b586..d03491089f9c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5374,18 +5374,21 @@ enum {
 #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
 #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
 
-#define SPCNTR(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPACNTR, _SPBCNTR)
-#define SPLINOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPALINOFF, _SPBLINOFF)
-#define SPSTRIDE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPASTRIDE, _SPBSTRIDE)
-#define SPPOS(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAPOS, _SPBPOS)
-#define SPSIZE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPASIZE, _SPBSIZE)
-#define SPKEYMINVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAKEYMINVAL, _SPBKEYMINVAL)
-#define SPKEYMSK(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAKEYMSK, _SPBKEYMSK)
-#define SPSURF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPASURF, _SPBSURF)
-#define SPKEYMAXVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
-#define SPTILEOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPATILEOFF, _SPBTILEOFF)
-#define SPCONSTALPHA(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPACONSTALPHA, _SPBCONSTALPHA)
-#define SPGAMC(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAGAMC, _SPBGAMC)
+#define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
+	_MMIO_PIPE((pipe) * 2 + (plane_id) - PLANE_SPRITE0, (reg_a), (reg_b))
+
+#define SPCNTR(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPACNTR, _SPBCNTR)
+#define SPLINOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPALINOFF, _SPBLINOFF)
+#define SPSTRIDE(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPASTRIDE, _SPBSTRIDE)
+#define SPPOS(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAPOS, _SPBPOS)
+#define SPSIZE(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPASIZE, _SPBSIZE)
+#define SPKEYMINVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMINVAL, _SPBKEYMINVAL)
+#define SPKEYMSK(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMSK, _SPBKEYMSK)
+#define SPSURF(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPASURF, _SPBSURF)
+#define SPKEYMAXVAL(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
+#define SPTILEOFF(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
+#define SPCONSTALPHA(pipe, plane_id)	_MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
+#define SPGAMC(pipe, plane_id)		_MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
 
 /*
  * CHV pipe B sprite CSC
@@ -5394,29 +5397,32 @@ enum {
  * |yg| = |c3 c4 c5| x |yg + yg_ioff| + |yg_ooff|
  * |cb|   |c6 c7 c8|   |cb + cr_ioff|   |cb_ooff|
  */
-#define SPCSCYGOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d900 + (sprite) * 0x1000)
-#define SPCSCCBOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d904 + (sprite) * 0x1000)
-#define SPCSCCROFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d908 + (sprite) * 0x1000)
+#define _MMIO_CHV_SPCSC(plane_id, reg) \
+	_MMIO(VLV_DISPLAY_BASE + ((plane_id) - PLANE_SPRITE0) * 0x1000 + (reg))
+
+#define SPCSCYGOFF(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d900)
+#define SPCSCCBOFF(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d904)
+#define SPCSCCROFF(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d908)
 #define  SPCSC_OOFF(x)		(((x) & 0x7ff) << 16) /* s11 */
 #define  SPCSC_IOFF(x)		(((x) & 0x7ff) << 0) /* s11 */
 
-#define SPCSCC01(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d90c + (sprite) * 0x1000)
-#define SPCSCC23(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d910 + (sprite) * 0x1000)
-#define SPCSCC45(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d914 + (sprite) * 0x1000)
-#define SPCSCC67(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d918 + (sprite) * 0x1000)
-#define SPCSCC8(sprite)		_MMIO(VLV_DISPLAY_BASE + 0x6d91c + (sprite) * 0x1000)
+#define SPCSCC01(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d90c)
+#define SPCSCC23(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d910)
+#define SPCSCC45(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d914)
+#define SPCSCC67(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d918)
+#define SPCSCC8(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d91c)
 #define  SPCSC_C1(x)		(((x) & 0x7fff) << 16) /* s3.12 */
 #define  SPCSC_C0(x)		(((x) & 0x7fff) << 0) /* s3.12 */
 
-#define SPCSCYGICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d920 + (sprite) * 0x1000)
-#define SPCSCCBICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d924 + (sprite) * 0x1000)
-#define SPCSCCRICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d928 + (sprite) * 0x1000)
+#define SPCSCYGICLAMP(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d920)
+#define SPCSCCBICLAMP(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d924)
+#define SPCSCCRICLAMP(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d928)
 #define  SPCSC_IMAX(x)		(((x) & 0x7ff) << 16) /* s11 */
 #define  SPCSC_IMIN(x)		(((x) & 0x7ff) << 0) /* s11 */
 
-#define SPCSCYGOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d92c + (sprite) * 0x1000)
-#define SPCSCCBOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d930 + (sprite) * 0x1000)
-#define SPCSCCROCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d934 + (sprite) * 0x1000)
+#define SPCSCYGOCLAMP(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d92c)
+#define SPCSCCBOCLAMP(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d930)
+#define SPCSCCROCLAMP(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d934)
 #define  SPCSC_OMAX(x)		((x) << 16) /* u10 */
 #define  SPCSC_OMIN(x)		((x) << 0) /* u10 */
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0a98989b8663..b6d5d5e5cc99 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1327,7 +1327,7 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 		}
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		for_each_sprite(dev_priv, pipe, sprite) {
-			u32 val = I915_READ(SPCNTR(pipe, sprite));
+			u32 val = I915_READ(SPCNTR(pipe, PLANE_SPRITE0 + sprite));
 			I915_STATE_WARN(val & SP_ENABLE,
 			     "sprite %c assertion failure, should be off on pipe %c but is still active\n",
 			     sprite_name(pipe, sprite), pipe_name(pipe));
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 93158061f790..63154a5a9305 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -296,7 +296,7 @@ static void
 chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_plane->base.dev);
-	int plane = intel_plane->plane;
+	enum plane_id plane_id = intel_plane->id;
 
 	/* Seems RGB data bypasses the CSC always */
 	if (!format_is_yuv(format))
@@ -312,23 +312,23 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 	 * Cb and Cr apparently come in as signed already, so no
 	 * need for any offset. For Y we need to remove the offset.
 	 */
-	I915_WRITE(SPCSCYGOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
-	I915_WRITE(SPCSCCBOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(0));
-	I915_WRITE(SPCSCCROFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(0));
-
-	I915_WRITE(SPCSCC01(plane), SPCSC_C1(4769) | SPCSC_C0(6537));
-	I915_WRITE(SPCSCC23(plane), SPCSC_C1(-3330) | SPCSC_C0(0));
-	I915_WRITE(SPCSCC45(plane), SPCSC_C1(-1605) | SPCSC_C0(4769));
-	I915_WRITE(SPCSCC67(plane), SPCSC_C1(4769) | SPCSC_C0(0));
-	I915_WRITE(SPCSCC8(plane), SPCSC_C0(8263));
-
-	I915_WRITE(SPCSCYGICLAMP(plane), SPCSC_IMAX(940) | SPCSC_IMIN(64));
-	I915_WRITE(SPCSCCBICLAMP(plane), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
-	I915_WRITE(SPCSCCRICLAMP(plane), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
-
-	I915_WRITE(SPCSCYGOCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
-	I915_WRITE(SPCSCCBOCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
-	I915_WRITE(SPCSCCROCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
+	I915_WRITE(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
+	I915_WRITE(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
+	I915_WRITE(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
+
+	I915_WRITE(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
+	I915_WRITE(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
+	I915_WRITE(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
+	I915_WRITE(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
+	I915_WRITE(SPCSCC8(plane_id), SPCSC_C0(8263));
+
+	I915_WRITE(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
+	I915_WRITE(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+	I915_WRITE(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+
+	I915_WRITE(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
+	I915_WRITE(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
+	I915_WRITE(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 }
 
 static void
@@ -340,8 +340,8 @@ vlv_update_plane(struct drm_plane *dplane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	int pipe = intel_plane->pipe;
-	int plane = intel_plane->plane;
+	enum pipe pipe = intel_plane->pipe;
+	enum plane_id plane_id = intel_plane->id;
 	u32 sprctl;
 	u32 sprsurf_offset, linear_offset;
 	unsigned int rotation = plane_state->base.rotation;
@@ -434,9 +434,9 @@ vlv_update_plane(struct drm_plane *dplane,
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
 
 	if (key->flags) {
-		I915_WRITE(SPKEYMINVAL(pipe, plane), key->min_value);
-		I915_WRITE(SPKEYMAXVAL(pipe, plane), key->max_value);
-		I915_WRITE(SPKEYMSK(pipe, plane), key->channel_mask);
+		I915_WRITE(SPKEYMINVAL(pipe, plane_id), key->min_value);
+		I915_WRITE(SPKEYMAXVAL(pipe, plane_id), key->max_value);
+		I915_WRITE(SPKEYMSK(pipe, plane_id), key->channel_mask);
 	}
 
 	if (key->flags & I915_SET_COLORKEY_SOURCE)
@@ -445,21 +445,21 @@ vlv_update_plane(struct drm_plane *dplane,
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
 		chv_update_csc(intel_plane, fb->pixel_format);
 
-	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
-	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
+	I915_WRITE(SPSTRIDE(pipe, plane_id), fb->pitches[0]);
+	I915_WRITE(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 
 	if (fb->modifier == I915_FORMAT_MOD_X_TILED)
-		I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
+		I915_WRITE(SPTILEOFF(pipe, plane_id), (y << 16) | x);
 	else
-		I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
+		I915_WRITE(SPLINOFF(pipe, plane_id), linear_offset);
 
-	I915_WRITE(SPCONSTALPHA(pipe, plane), 0);
+	I915_WRITE(SPCONSTALPHA(pipe, plane_id), 0);
 
-	I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w);
-	I915_WRITE(SPCNTR(pipe, plane), sprctl);
-	I915_WRITE(SPSURF(pipe, plane),
+	I915_WRITE(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
+	I915_WRITE(SPCNTR(pipe, plane_id), sprctl);
+	I915_WRITE(SPSURF(pipe, plane_id),
 		   intel_fb_gtt_offset(fb, rotation) + sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+	POSTING_READ(SPSURF(pipe, plane_id));
 }
 
 static void
@@ -468,13 +468,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
-	int pipe = intel_plane->pipe;
-	int plane = intel_plane->plane;
+	enum pipe pipe = intel_plane->pipe;
+	enum plane_id plane_id = intel_plane->id;
 
-	I915_WRITE(SPCNTR(pipe, plane), 0);
+	I915_WRITE(SPCNTR(pipe, plane_id), 0);
 
-	I915_WRITE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+	I915_WRITE(SPSURF(pipe, plane_id), 0);
+	POSTING_READ(SPSURF(pipe, plane_id));
 }
 
 static void
-- 
2.7.4

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

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

* [PATCH 6/9] drm/i915: Use enum plane_id in VLV/CHV wm code
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (4 preceding siblings ...)
  2016-11-22 16:02 ` [PATCH 5/9] drm/i915: Use enum plane_id in VLV/CHV sprite code ville.syrjala
@ 2016-11-22 16:02 ` ville.syrjala
  2016-11-22 16:02 ` [PATCH 7/9] drm/i915: s/plane/plane_id/ in skl+ plane register defines ville.syrjala
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:02 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's try not to abuse plane->plane for sprites on VLV/CHV and instead
use plane->id. Since out watermark structures aren't entirely plane type
agnostic (for now) and start indexing sprites from 0  we'll add a small
helper to convert between the two bases.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 73 ++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e6351445016d..2b4bdb3abb10 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -370,12 +370,15 @@ static const int pessimal_latency_ns = 5000;
 #define VLV_FIFO_START(dsparb, dsparb2, lo_shift, hi_shift) \
 	((((dsparb) >> (lo_shift)) & 0xff) | ((((dsparb2) >> (hi_shift)) & 0x1) << 8))
 
-static int vlv_get_fifo_size(struct drm_i915_private *dev_priv,
-			      enum pipe pipe, int plane)
+static int vlv_get_fifo_size(struct intel_plane *plane)
 {
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	int sprite0_start, sprite1_start, size;
 
-	switch (pipe) {
+	if (plane->id == PLANE_CURSOR)
+		return 63;
+
+	switch (plane->pipe) {
 		uint32_t dsparb, dsparb2, dsparb3;
 	case PIPE_A:
 		dsparb = I915_READ(DSPARB);
@@ -399,24 +402,21 @@ static int vlv_get_fifo_size(struct drm_i915_private *dev_priv,
 		return 0;
 	}
 
-	switch (plane) {
-	case 0:
+	switch (plane->id) {
+	case PLANE_PRIMARY:
 		size = sprite0_start;
 		break;
-	case 1:
+	case PLANE_SPRITE0:
 		size = sprite1_start - sprite0_start;
 		break;
-	case 2:
+	case PLANE_SPRITE1:
 		size = 512 - 1 - sprite1_start;
 		break;
 	default:
 		return 0;
 	}
 
-	DRM_DEBUG_KMS("Pipe %c %s %c FIFO size: %d\n",
-		      pipe_name(pipe), plane == 0 ? "primary" : "sprite",
-		      plane == 0 ? plane_name(pipe) : sprite_name(pipe, plane - 1),
-		      size);
+	DRM_DEBUG_KMS("%s FIFO size: %d\n", plane->base.name, size);
 
 	return size;
 }
@@ -1053,6 +1053,12 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
 	WARN_ON(fifo_left != 0);
 }
 
+/* FIXME kill me */
+static inline int vlv_sprite_id(enum plane_id plane_id)
+{
+	return plane_id - PLANE_SPRITE0;
+}
+
 static void vlv_invert_wms(struct intel_crtc *crtc)
 {
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
@@ -1079,7 +1085,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 					wm_state->wm[level].primary;
 				break;
 			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = plane->plane;
+				sprite = vlv_sprite_id(plane->id);
 				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
 					wm_state->wm[level].sprite[sprite];
 				break;
@@ -1144,7 +1150,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 				wm_state->wm[level].primary = wm;
 				break;
 			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = plane->plane;
+				sprite = vlv_sprite_id(plane->id);
 				wm_state->wm[level].sprite[sprite] = wm;
 				break;
 			}
@@ -1170,7 +1176,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 					    wm_state->wm[level].primary);
 			break;
 		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = plane->plane;
+			sprite = vlv_sprite_id(plane->id);
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].plane =
 					min(wm_state->sr[level].plane,
@@ -1199,17 +1205,23 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 	int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
 
 	for_each_intel_plane_on_crtc(dev, crtc, plane) {
-		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
-			WARN_ON(plane->wm.fifo_size != 63);
-			continue;
-		}
-
-		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
+		switch (plane->id) {
+		case PLANE_PRIMARY:
 			sprite0_start = plane->wm.fifo_size;
-		else if (plane->plane == 0)
+			break;
+		case PLANE_SPRITE0:
 			sprite1_start = sprite0_start + plane->wm.fifo_size;
-		else
+			break;
+		case PLANE_SPRITE1:
 			fifo_size = sprite1_start + plane->wm.fifo_size;
+			break;
+		case PLANE_CURSOR:
+			WARN_ON(plane->wm.fifo_size != 63);
+			break;
+		default:
+			MISSING_CASE(plane->id);
+			break;
+		}
 	}
 
 	WARN_ON(fifo_size != 512 - 1);
@@ -4510,21 +4522,8 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 
 	vlv_read_wm_values(dev_priv, wm);
 
-	for_each_intel_plane(dev, plane) {
-		switch (plane->base.type) {
-			int sprite;
-		case DRM_PLANE_TYPE_CURSOR:
-			plane->wm.fifo_size = 63;
-			break;
-		case DRM_PLANE_TYPE_PRIMARY:
-			plane->wm.fifo_size = vlv_get_fifo_size(dev_priv, plane->pipe, 0);
-			break;
-		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = plane->plane;
-			plane->wm.fifo_size = vlv_get_fifo_size(dev_priv, plane->pipe, sprite + 1);
-			break;
-		}
-	}
+	for_each_intel_plane(dev, plane)
+		plane->wm.fifo_size = vlv_get_fifo_size(plane);
 
 	wm->cxsr = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
 	wm->level = VLV_WM_LEVEL_PM2;
-- 
2.7.4

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

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

* [PATCH 7/9] drm/i915: s/plane/plane_id/ in skl+ plane register defines
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (5 preceding siblings ...)
  2016-11-22 16:02 ` [PATCH 6/9] drm/i915: Use enum plane_id in VLV/CHV wm code ville.syrjala
@ 2016-11-22 16:02 ` ville.syrjala
  2016-12-15 18:33   ` Paulo Zanoni
  2016-11-22 16:02 ` [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites ville.syrjala
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:02 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rename the SKL plane register define 'plane' parameter to 'plane_id' to
reflect the fact that you're supposed to pass in the enum plane_id
rather than enum plane.

Do the same for the scaler plane selector bits.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Suggested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 46 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d03491089f9c..8ceea23ba63b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5502,8 +5502,8 @@ enum {
 #define _PLANE_CTL_1(pipe)	_PIPE(pipe, _PLANE_CTL_1_A, _PLANE_CTL_1_B)
 #define _PLANE_CTL_2(pipe)	_PIPE(pipe, _PLANE_CTL_2_A, _PLANE_CTL_2_B)
 #define _PLANE_CTL_3(pipe)	_PIPE(pipe, _PLANE_CTL_3_A, _PLANE_CTL_3_B)
-#define PLANE_CTL(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_CTL_1(pipe), _PLANE_CTL_2(pipe))
+#define PLANE_CTL(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_CTL_1(pipe), _PLANE_CTL_2(pipe))
 
 #define _PLANE_STRIDE_1_B			0x71188
 #define _PLANE_STRIDE_2_B			0x71288
@@ -5514,8 +5514,8 @@ enum {
 	_PIPE(pipe, _PLANE_STRIDE_2_A, _PLANE_STRIDE_2_B)
 #define _PLANE_STRIDE_3(pipe)	\
 	_PIPE(pipe, _PLANE_STRIDE_3_A, _PLANE_STRIDE_3_B)
-#define PLANE_STRIDE(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_STRIDE_1(pipe), _PLANE_STRIDE_2(pipe))
+#define PLANE_STRIDE(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_STRIDE_1(pipe), _PLANE_STRIDE_2(pipe))
 
 #define _PLANE_POS_1_B				0x7118c
 #define _PLANE_POS_2_B				0x7128c
@@ -5523,8 +5523,8 @@ enum {
 #define _PLANE_POS_1(pipe)	_PIPE(pipe, _PLANE_POS_1_A, _PLANE_POS_1_B)
 #define _PLANE_POS_2(pipe)	_PIPE(pipe, _PLANE_POS_2_A, _PLANE_POS_2_B)
 #define _PLANE_POS_3(pipe)	_PIPE(pipe, _PLANE_POS_3_A, _PLANE_POS_3_B)
-#define PLANE_POS(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_POS_1(pipe), _PLANE_POS_2(pipe))
+#define PLANE_POS(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_POS_1(pipe), _PLANE_POS_2(pipe))
 
 #define _PLANE_SIZE_1_B				0x71190
 #define _PLANE_SIZE_2_B				0x71290
@@ -5532,8 +5532,8 @@ enum {
 #define _PLANE_SIZE_1(pipe)	_PIPE(pipe, _PLANE_SIZE_1_A, _PLANE_SIZE_1_B)
 #define _PLANE_SIZE_2(pipe)	_PIPE(pipe, _PLANE_SIZE_2_A, _PLANE_SIZE_2_B)
 #define _PLANE_SIZE_3(pipe)	_PIPE(pipe, _PLANE_SIZE_3_A, _PLANE_SIZE_3_B)
-#define PLANE_SIZE(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_SIZE_1(pipe), _PLANE_SIZE_2(pipe))
+#define PLANE_SIZE(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_SIZE_1(pipe), _PLANE_SIZE_2(pipe))
 
 #define _PLANE_SURF_1_B				0x7119c
 #define _PLANE_SURF_2_B				0x7129c
@@ -5541,36 +5541,36 @@ enum {
 #define _PLANE_SURF_1(pipe)	_PIPE(pipe, _PLANE_SURF_1_A, _PLANE_SURF_1_B)
 #define _PLANE_SURF_2(pipe)	_PIPE(pipe, _PLANE_SURF_2_A, _PLANE_SURF_2_B)
 #define _PLANE_SURF_3(pipe)	_PIPE(pipe, _PLANE_SURF_3_A, _PLANE_SURF_3_B)
-#define PLANE_SURF(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_SURF_1(pipe), _PLANE_SURF_2(pipe))
+#define PLANE_SURF(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_SURF_1(pipe), _PLANE_SURF_2(pipe))
 
 #define _PLANE_OFFSET_1_B			0x711a4
 #define _PLANE_OFFSET_2_B			0x712a4
 #define _PLANE_OFFSET_1(pipe) _PIPE(pipe, _PLANE_OFFSET_1_A, _PLANE_OFFSET_1_B)
 #define _PLANE_OFFSET_2(pipe) _PIPE(pipe, _PLANE_OFFSET_2_A, _PLANE_OFFSET_2_B)
-#define PLANE_OFFSET(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_OFFSET_1(pipe), _PLANE_OFFSET_2(pipe))
+#define PLANE_OFFSET(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_OFFSET_1(pipe), _PLANE_OFFSET_2(pipe))
 
 #define _PLANE_KEYVAL_1_B			0x71194
 #define _PLANE_KEYVAL_2_B			0x71294
 #define _PLANE_KEYVAL_1(pipe) _PIPE(pipe, _PLANE_KEYVAL_1_A, _PLANE_KEYVAL_1_B)
 #define _PLANE_KEYVAL_2(pipe) _PIPE(pipe, _PLANE_KEYVAL_2_A, _PLANE_KEYVAL_2_B)
-#define PLANE_KEYVAL(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_KEYVAL_1(pipe), _PLANE_KEYVAL_2(pipe))
+#define PLANE_KEYVAL(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_KEYVAL_1(pipe), _PLANE_KEYVAL_2(pipe))
 
 #define _PLANE_KEYMSK_1_B			0x71198
 #define _PLANE_KEYMSK_2_B			0x71298
 #define _PLANE_KEYMSK_1(pipe) _PIPE(pipe, _PLANE_KEYMSK_1_A, _PLANE_KEYMSK_1_B)
 #define _PLANE_KEYMSK_2(pipe) _PIPE(pipe, _PLANE_KEYMSK_2_A, _PLANE_KEYMSK_2_B)
-#define PLANE_KEYMSK(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_KEYMSK_1(pipe), _PLANE_KEYMSK_2(pipe))
+#define PLANE_KEYMSK(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_KEYMSK_1(pipe), _PLANE_KEYMSK_2(pipe))
 
 #define _PLANE_KEYMAX_1_B			0x711a0
 #define _PLANE_KEYMAX_2_B			0x712a0
 #define _PLANE_KEYMAX_1(pipe) _PIPE(pipe, _PLANE_KEYMAX_1_A, _PLANE_KEYMAX_1_B)
 #define _PLANE_KEYMAX_2(pipe) _PIPE(pipe, _PLANE_KEYMAX_2_A, _PLANE_KEYMAX_2_B)
-#define PLANE_KEYMAX(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_KEYMAX_1(pipe), _PLANE_KEYMAX_2(pipe))
+#define PLANE_KEYMAX(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_KEYMAX_1(pipe), _PLANE_KEYMAX_2(pipe))
 
 #define _PLANE_BUF_CFG_1_B			0x7127c
 #define _PLANE_BUF_CFG_2_B			0x7137c
@@ -5578,8 +5578,8 @@ enum {
 	_PIPE(pipe, _PLANE_BUF_CFG_1_A, _PLANE_BUF_CFG_1_B)
 #define _PLANE_BUF_CFG_2(pipe)	\
 	_PIPE(pipe, _PLANE_BUF_CFG_2_A, _PLANE_BUF_CFG_2_B)
-#define PLANE_BUF_CFG(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_BUF_CFG_1(pipe), _PLANE_BUF_CFG_2(pipe))
+#define PLANE_BUF_CFG(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_BUF_CFG_1(pipe), _PLANE_BUF_CFG_2(pipe))
 
 #define _PLANE_NV12_BUF_CFG_1_B		0x71278
 #define _PLANE_NV12_BUF_CFG_2_B		0x71378
@@ -5587,8 +5587,8 @@ enum {
 	_PIPE(pipe, _PLANE_NV12_BUF_CFG_1_A, _PLANE_NV12_BUF_CFG_1_B)
 #define _PLANE_NV12_BUF_CFG_2(pipe)	\
 	_PIPE(pipe, _PLANE_NV12_BUF_CFG_2_A, _PLANE_NV12_BUF_CFG_2_B)
-#define PLANE_NV12_BUF_CFG(pipe, plane)	\
-	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
+#define PLANE_NV12_BUF_CFG(pipe, plane_id)	\
+	_MMIO_PLANE(plane_id, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
 
 /* SKL new cursor registers */
 #define _CUR_BUF_CFG_A				0x7017c
@@ -5737,7 +5737,7 @@ enum {
 #define PS_SCALER_MODE_DYN  (0 << 28)
 #define PS_SCALER_MODE_HQ  (1 << 28)
 #define PS_PLANE_SEL_MASK  (7 << 25)
-#define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
+#define PS_PLANE_SEL(plane_id) (((plane_id) + 1) << 25)
 #define PS_FILTER_MASK         (3 << 23)
 #define PS_FILTER_MEDIUM       (0 << 23)
 #define PS_FILTER_EDGE_ENHANCE (2 << 23)
-- 
2.7.4

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

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

* [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (6 preceding siblings ...)
  2016-11-22 16:02 ` [PATCH 7/9] drm/i915: s/plane/plane_id/ in skl+ plane register defines ville.syrjala
@ 2016-11-22 16:02 ` ville.syrjala
  2016-12-15 19:05   ` Paulo Zanoni
  2016-11-22 16:02 ` [PATCH 9/9] drm/i915: Rename the local 'plane' variable to 'plane_id' in primary plane code ville.syrjala
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:02 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

With plane->plane now purely reserved for the primary planes, let's
not even populate it for cursors and sprites. Let's switch the type
to enum plane as well since it's no longer being abused for anything
else.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 1 -
 drivers/gpu/drm/i915/intel_drv.h     | 2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b6d5d5e5cc99..f180f14fcf3a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
-	cursor->plane = pipe;
 	cursor->id = PLANE_CURSOR;
 	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
 	cursor->check_plane = intel_check_cursor_plane;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c1b245853ba9..d14718e09911 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
 
 struct intel_plane {
 	struct drm_plane base;
-	u8 plane;
+	enum plane plane;
 	enum plane_id id;
 	enum pipe pipe;
 	bool can_scale;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 63154a5a9305..1044095d0084 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 	}
 
 	intel_plane->pipe = pipe;
-	intel_plane->plane = plane;
 	intel_plane->id = PLANE_SPRITE0 + plane;
 	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
 	intel_plane->check_plane = intel_check_sprite_plane;
-- 
2.7.4

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

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

* [PATCH 9/9] drm/i915: Rename the local 'plane' variable to 'plane_id' in primary plane code
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (7 preceding siblings ...)
  2016-11-22 16:02 ` [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites ville.syrjala
@ 2016-11-22 16:02 ` ville.syrjala
  2016-11-22 16:45 ` ✓ Fi.CI.BAT: success for drm/i915: Add a per-pipe plane identifier enum (rev5) Patchwork
  2016-11-23 20:16 ` [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) Ville Syrjälä
  10 siblings, 0 replies; 22+ messages in thread
From: ville.syrjala @ 2016-11-22 16:02 UTC (permalink / raw)
  To: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now we've rename the local plane id variable as 'plane_id' everywhere
except the pre-SKL primary plane code. Let's do the rename there as well
so that we'll free up the name 'plane' for use with struct intel_plane*.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 57 +++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f180f14fcf3a..a9b88749e897 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3001,10 +3001,9 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
 	struct drm_i915_private *dev_priv = to_i915(primary->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	int plane = intel_crtc->plane;
+	enum plane plane_id = to_intel_plane(primary)->plane;
 	u32 linear_offset;
 	u32 dspcntr;
-	i915_reg_t reg = DSPCNTR(plane);
 	unsigned int rotation = plane_state->base.rotation;
 	int x = plane_state->base.src.x1 >> 16;
 	int y = plane_state->base.src.y1 >> 16;
@@ -3020,16 +3019,16 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
 		/* pipesrc and dspsize control the size that is scaled from,
 		 * which should always be the user's requested size.
 		 */
-		I915_WRITE(DSPSIZE(plane),
+		I915_WRITE(DSPSIZE(plane_id),
 			   ((crtc_state->pipe_src_h - 1) << 16) |
 			   (crtc_state->pipe_src_w - 1));
-		I915_WRITE(DSPPOS(plane), 0);
-	} else if (IS_CHERRYVIEW(dev_priv) && plane == PLANE_B) {
-		I915_WRITE(PRIMSIZE(plane),
+		I915_WRITE(DSPPOS(plane_id), 0);
+	} else if (IS_CHERRYVIEW(dev_priv) && plane_id == PLANE_B) {
+		I915_WRITE(PRIMSIZE(plane_id),
 			   ((crtc_state->pipe_src_h - 1) << 16) |
 			   (crtc_state->pipe_src_w - 1));
-		I915_WRITE(PRIMPOS(plane), 0);
-		I915_WRITE(PRIMCNSTALPHA(plane), 0);
+		I915_WRITE(PRIMPOS(plane_id), 0);
+		I915_WRITE(PRIMCNSTALPHA(plane_id), 0);
 	}
 
 	switch (fb->pixel_format) {
@@ -3092,21 +3091,21 @@ static void i9xx_update_primary_plane(struct drm_plane *primary,
 	intel_crtc->adjusted_x = x;
 	intel_crtc->adjusted_y = y;
 
-	I915_WRITE(reg, dspcntr);
+	I915_WRITE(DSPCNTR(plane_id), dspcntr);
 
-	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
+	I915_WRITE(DSPSTRIDE(plane_id), fb->pitches[0]);
 	if (INTEL_GEN(dev_priv) >= 4) {
-		I915_WRITE(DSPSURF(plane),
+		I915_WRITE(DSPSURF(plane_id),
 			   intel_fb_gtt_offset(fb, rotation) +
 			   intel_crtc->dspaddr_offset);
-		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
-		I915_WRITE(DSPLINOFF(plane), linear_offset);
+		I915_WRITE(DSPTILEOFF(plane_id), (y << 16) | x);
+		I915_WRITE(DSPLINOFF(plane_id), linear_offset);
 	} else {
-		I915_WRITE(DSPADDR(plane),
+		I915_WRITE(DSPADDR(plane_id),
 			   intel_fb_gtt_offset(fb, rotation) +
 			   intel_crtc->dspaddr_offset);
 	}
-	POSTING_READ(reg);
+	POSTING_READ(DSPCNTR(plane_id));
 }
 
 static void i9xx_disable_primary_plane(struct drm_plane *primary,
@@ -3114,15 +3113,14 @@ static void i9xx_disable_primary_plane(struct drm_plane *primary,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int plane = intel_crtc->plane;
+	enum plane plane_id = to_intel_plane(primary)->plane;
 
-	I915_WRITE(DSPCNTR(plane), 0);
+	I915_WRITE(DSPCNTR(plane_id), 0);
 	if (INTEL_INFO(dev_priv)->gen >= 4)
-		I915_WRITE(DSPSURF(plane), 0);
+		I915_WRITE(DSPSURF(plane_id), 0);
 	else
-		I915_WRITE(DSPADDR(plane), 0);
-	POSTING_READ(DSPCNTR(plane));
+		I915_WRITE(DSPADDR(plane_id), 0);
+	POSTING_READ(DSPCNTR(plane_id));
 }
 
 static void ironlake_update_primary_plane(struct drm_plane *primary,
@@ -3133,10 +3131,9 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	int plane = intel_crtc->plane;
+	enum plane plane_id = to_intel_plane(primary)->plane;
 	u32 linear_offset;
 	u32 dspcntr;
-	i915_reg_t reg = DSPCNTR(plane);
 	unsigned int rotation = plane_state->base.rotation;
 	int x = plane_state->base.src.x1 >> 16;
 	int y = plane_state->base.src.y1 >> 16;
@@ -3196,19 +3193,19 @@ static void ironlake_update_primary_plane(struct drm_plane *primary,
 	intel_crtc->adjusted_x = x;
 	intel_crtc->adjusted_y = y;
 
-	I915_WRITE(reg, dspcntr);
+	I915_WRITE(DSPCNTR(plane_id), dspcntr);
 
-	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
-	I915_WRITE(DSPSURF(plane),
+	I915_WRITE(DSPSTRIDE(plane_id), fb->pitches[0]);
+	I915_WRITE(DSPSURF(plane_id),
 		   intel_fb_gtt_offset(fb, rotation) +
 		   intel_crtc->dspaddr_offset);
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		I915_WRITE(DSPOFFSET(plane), (y << 16) | x);
+		I915_WRITE(DSPOFFSET(plane_id), (y << 16) | x);
 	} else {
-		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
-		I915_WRITE(DSPLINOFF(plane), linear_offset);
+		I915_WRITE(DSPTILEOFF(plane_id), (y << 16) | x);
+		I915_WRITE(DSPLINOFF(plane_id), linear_offset);
 	}
-	POSTING_READ(reg);
+	POSTING_READ(DSPCNTR(plane_id));
 }
 
 u32 intel_fb_stride_alignment(const struct drm_i915_private *dev_priv,
-- 
2.7.4

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

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

* ✓ Fi.CI.BAT: success for drm/i915: Add a per-pipe plane identifier enum (rev5)
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (8 preceding siblings ...)
  2016-11-22 16:02 ` [PATCH 9/9] drm/i915: Rename the local 'plane' variable to 'plane_id' in primary plane code ville.syrjala
@ 2016-11-22 16:45 ` Patchwork
  2016-11-23 20:16 ` [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) Ville Syrjälä
  10 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2016-11-22 16:45 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Add a per-pipe plane identifier enum (rev5)
URL   : https://patchwork.freedesktop.org/series/14978/
State : success

== Summary ==

Series 14978v5 drm/i915: Add a per-pipe plane identifier enum
https://patchwork.freedesktop.org/api/1.0/series/14978/revisions/5/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

eeec5e7742b23082dd20523c8baa08fe495175e4 drm-intel-nightly: 2016y-11m-21d-18h-22m-22s UTC integration manifest
dc45b91 drm/i915: Rename the local 'plane' variable to 'plane_id' in primary plane code
3e642c7 drm/i915: Don't populate plane->plane for cursors and sprites
fa55355 drm/i915: s/plane/plane_id/ in skl+ plane register defines
87d3a43 drm/i915: Use enum plane_id in VLV/CHV wm code
1e0e553 drm/i915: Use enum plane_id in VLV/CHV sprite code
b861dac drm/i915: Use enum plane_id in SKL plane code
2b8c0dd drm/i915: Use enum plane_id in SKL wm code
baf65ea drm/i915: Add crtc->plane_ids_mask
0d733576 drm/i915: Add per-pipe plane identifier

== Logs ==

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

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

* Re: [PATCH 3/9] drm/i915: Use enum plane_id in SKL wm code
  2016-11-22 16:01 ` [PATCH 3/9] drm/i915: Use enum plane_id in SKL wm code ville.syrjala
@ 2016-11-22 20:27   ` Lyude Paul
  0 siblings, 0 replies; 22+ messages in thread
From: Lyude Paul @ 2016-11-22 20:27 UTC (permalink / raw)
  To: ville.syrjala, intel-gfx; +Cc: Paulo Zanoni

Looks good to me

Reviewed-by: Lyude <lyude@redhat.com>

On Tue, 2016-11-22 at 18:01 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Nuke skl_wm_plane_id() and just use the new intel_plane->id.
> 
> v2: Convert skl_write_plane_wm() as well
> v3: Convert skl_pipe_wm_get_hw_state() correctly
> v4: Rebase due to changes in the wm code
>     Drop the cursor FIXME from the total data rate calc (Paulo)
>     Use the "[PLANE:%d:%s]" format in debug print (Paulo)
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Lyude <cpaul@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 180 +++++++++++++++++-------------
> ----------
>  1 file changed, 77 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index bbb1eaf1e6db..e6351445016d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2867,28 +2867,6 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
>  #define SKL_SAGV_BLOCK_TIME	30 /* µs */
>  
>  /*
> - * 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.
> - */
> -static int
> -skl_wm_plane_id(const struct intel_plane *plane)
> -{
> -	switch (plane->base.type) {
> -	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:
> -		MISSING_CASE(plane->base.type);
> -		return plane->plane;
> -	}
> -}
> -
> -/*
>   * FIXME: We still don't have the proper code detect if we need to
> apply the WA,
>   * so assume we'll always need it in order to avoid underruns.
>   */
> @@ -3026,7 +3004,6 @@ bool intel_can_enable_sagv(struct
> drm_atomic_state *state)
>  	struct intel_crtc *crtc;
>  	struct intel_plane *plane;
>  	struct intel_crtc_state *cstate;
> -	struct skl_plane_wm *wm;
>  	enum pipe pipe;
>  	int level, latency;
>  
> @@ -3053,7 +3030,8 @@ bool intel_can_enable_sagv(struct
> drm_atomic_state *state)
>  		return false;
>  
>  	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -		wm = &cstate-
> >wm.skl.optimal.planes[skl_wm_plane_id(plane)];
> +		struct skl_plane_wm *wm =
> +			&cstate->wm.skl.optimal.planes[plane->id];
>  
>  		/* Skip this plane if it's not enabled */
>  		if (!wm->wm[0].plane_en)
> @@ -3156,28 +3134,29 @@ static void skl_ddb_entry_init_from_hw(struct
> skl_ddb_entry *entry, u32 reg)
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */)
>  {
> -	enum pipe pipe;
> -	int plane;
> -	u32 val;
> +	struct intel_crtc *crtc;
>  
>  	memset(ddb, 0, sizeof(*ddb));
>  
> -	for_each_pipe(dev_priv, pipe) {
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		enum intel_display_power_domain power_domain;
> +		enum plane_id plane_id;
> +		enum pipe pipe = crtc->pipe;
>  
>  		power_domain = POWER_DOMAIN_PIPE(pipe);
>  		if (!intel_display_power_get_if_enabled(dev_priv,
> power_domain))
>  			continue;
>  
> -		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);
> -		}
> +		for_each_plane_id_on_crtc(crtc, plane_id) {
> +			u32 val;
> +
> +			if (plane_id != PLANE_CURSOR)
> +				val = I915_READ(PLANE_BUF_CFG(pipe,
> plane_id));
> +			else
> +				val = I915_READ(CUR_BUF_CFG(pipe));
>  
> -		val = I915_READ(CUR_BUF_CFG(pipe));
> -		skl_ddb_entry_init_from_hw(&ddb-
> >plane[pipe][PLANE_CURSOR],
> -					   val);
> +			skl_ddb_entry_init_from_hw(&ddb-
> >plane[pipe][plane_id], val);
> +		}
>  
>  		intel_display_power_put(dev_priv, power_domain);
>  	}
> @@ -3278,30 +3257,28 @@ skl_get_total_relative_data_rate(struct
> intel_crtc_state *intel_cstate,
>  	struct drm_crtc_state *cstate = &intel_cstate->base;
>  	struct drm_atomic_state *state = cstate->state;
>  	struct drm_plane *plane;
> -	const struct intel_plane *intel_plane;
>  	const struct drm_plane_state *pstate;
> -	unsigned int rate, total_data_rate = 0;
> -	int id;
> +	unsigned int total_data_rate = 0;
>  
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	/* Calculate and cache data rate for each plane */
>  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> cstate) {
> -		id = skl_wm_plane_id(to_intel_plane(plane));
> -		intel_plane = to_intel_plane(plane);
> +		enum plane_id plane_id = to_intel_plane(plane)->id;
> +		unsigned int rate;
>  
>  		/* packed/uv */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 0);
> -		plane_data_rate[id] = rate;
> +		plane_data_rate[plane_id] = rate;
>  
>  		total_data_rate += rate;
>  
>  		/* y-plane */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 1);
> -		plane_y_data_rate[id] = rate;
> +		plane_y_data_rate[plane_id] = rate;
>  
>  		total_data_rate += rate;
>  	}
> @@ -3380,17 +3357,16 @@ skl_ddb_calc_min(const struct
> intel_crtc_state *cstate, int num_active,
>  	struct drm_plane *plane;
>  
>  	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> &cstate->base) {
> -		struct intel_plane *intel_plane =
> to_intel_plane(plane);
> -		int id = skl_wm_plane_id(intel_plane);
> +		enum plane_id plane_id = to_intel_plane(plane)->id;
>  
> -		if (id == PLANE_CURSOR)
> +		if (plane_id == PLANE_CURSOR)
>  			continue;
>  
>  		if (!pstate->visible)
>  			continue;
>  
> -		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> -		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> +		minimum[plane_id] = skl_ddb_min_alloc(pstate, 0);
> +		y_minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
>  	}
>  
>  	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> @@ -3410,8 +3386,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	uint16_t minimum[I915_MAX_PLANES] = {};
>  	uint16_t y_minimum[I915_MAX_PLANES] = {};
>  	unsigned int total_data_rate;
> +	enum plane_id plane_id;
>  	int num_active;
> -	int id, i;
>  	unsigned plane_data_rate[I915_MAX_PLANES] = {};
>  	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
>  
> @@ -3442,9 +3418,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	 * proportional to the data rate.
>  	 */
>  
> -	for (i = 0; i < I915_MAX_PLANES; i++) {
> -		alloc_size -= minimum[i];
> -		alloc_size -= y_minimum[i];
> +	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> +		alloc_size -= minimum[plane_id];
> +		alloc_size -= y_minimum[plane_id];
>  	}
>  
>  	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> minimum[PLANE_CURSOR];
> @@ -3463,28 +3439,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  
>  	start = alloc->start;
> -	for (id = 0; id < I915_MAX_PLANES; id++) {
> +	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
>  		unsigned int data_rate, y_data_rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
>  
> -		if (id == PLANE_CURSOR)
> +		if (plane_id == PLANE_CURSOR)
>  			continue;
>  
> -		data_rate = plane_data_rate[id];
> +		data_rate = plane_data_rate[plane_id];
>  
>  		/*
>  		 * allocation for (packed formats) or (uv-plane part
> of planar format):
>  		 * promote the expression to 64 bits to avoid
> overflowing, the
>  		 * result is < available as data_rate /
> total_data_rate < 1
>  		 */
> -		plane_blocks = minimum[id];
> +		plane_blocks = minimum[plane_id];
>  		plane_blocks += div_u64((uint64_t)alloc_size *
> data_rate,
>  					total_data_rate);
>  
>  		/* Leave disabled planes at (0,0) */
>  		if (data_rate) {
> -			ddb->plane[pipe][id].start = start;
> -			ddb->plane[pipe][id].end = start +
> plane_blocks;
> +			ddb->plane[pipe][plane_id].start = start;
> +			ddb->plane[pipe][plane_id].end = start +
> plane_blocks;
>  		}
>  
>  		start += plane_blocks;
> @@ -3492,15 +3468,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		/*
>  		 * allocation for y_plane part of planar format:
>  		 */
> -		y_data_rate = plane_y_data_rate[id];
> +		y_data_rate = plane_y_data_rate[plane_id];
>  
> -		y_plane_blocks = y_minimum[id];
> +		y_plane_blocks = y_minimum[plane_id];
>  		y_plane_blocks += div_u64((uint64_t)alloc_size *
> y_data_rate,
>  					total_data_rate);
>  
>  		if (y_data_rate) {
> -			ddb->y_plane[pipe][id].start = start;
> -			ddb->y_plane[pipe][id].end = start +
> y_plane_blocks;
> +			ddb->y_plane[pipe][plane_id].start = start;
> +			ddb->y_plane[pipe][plane_id].end = start +
> y_plane_blocks;
>  		}
>  
>  		start += y_plane_blocks;
> @@ -3692,12 +3668,12 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  		if (level) {
>  			return 0;
>  		} else {
> +			struct drm_plane *plane = pstate->plane;
> +
>  			DRM_DEBUG_KMS("Requested display
> configuration exceeds system watermark limitations\n");
> -			DRM_DEBUG_KMS("Plane %d.%d: blocks required
> = %u/%u, lines required = %u/31\n",
> -				      to_intel_crtc(cstate-
> >base.crtc)->pipe,
> -				      skl_wm_plane_id(to_intel_plane
> (pstate->plane)),
> +			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required
> = %u/%u, lines required = %u/31\n",
> +				      plane->base.id, plane->name,
>  				      res_blocks, ddb_allocation,
> res_lines);
> -
>  			return -EINVAL;
>  		}
>  	}
> @@ -3724,7 +3700,6 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  	uint16_t ddb_blocks;
>  	enum pipe pipe = intel_crtc->pipe;
>  	int ret;
> -	int i = skl_wm_plane_id(intel_plane);
>  
>  	if (state)
>  		intel_pstate =
> @@ -3747,7 +3722,7 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  
>  	WARN_ON(!intel_pstate->base.fb);
>  
> -	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
> +	ddb_blocks = skl_ddb_entry_size(&ddb-
> >plane[pipe][intel_plane->id]);
>  
>  	ret = skl_compute_plane_wm(dev_priv,
>  				   cstate,
> @@ -3810,7 +3785,7 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *cstate,
>  	for_each_intel_plane_mask(&dev_priv->drm,
>  				  intel_plane,
>  				  cstate->base.plane_mask) {
> -		wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
> +		wm = &pipe_wm->planes[intel_plane->id];
>  
>  		for (level = 0; level <= max_level; level++) {
>  			ret = skl_compute_wm_level(dev_priv, ddb,
> cstate,
> @@ -3854,7 +3829,7 @@ static void skl_write_wm_level(struct
> drm_i915_private *dev_priv,
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>  			const struct skl_plane_wm *wm,
>  			const struct skl_ddb_allocation *ddb,
> -			int plane)
> +			enum plane_id plane_id)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
>  	struct drm_device *dev = crtc->dev;
> @@ -3863,16 +3838,16 @@ void skl_write_plane_wm(struct intel_crtc
> *intel_crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	for (level = 0; level <= max_level; level++) {
> -		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane,
> level),
> +		skl_write_wm_level(dev_priv, PLANE_WM(pipe,
> plane_id, level),
>  				   &wm->wm[level]);
>  	}
> -	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane),
> +	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
>  			   &wm->trans_wm);
>  
> -	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
> -			    &ddb->plane[pipe][plane]);
> -	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe,
> plane),
> -			    &ddb->y_plane[pipe][plane]);
> +	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> +			    &ddb->plane[pipe][plane_id]);
> +	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe,
> plane_id),
> +			    &ddb->y_plane[pipe][plane_id]);
>  }
>  
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> @@ -3977,17 +3952,16 @@ skl_ddb_add_affected_planes(struct
> intel_crtc_state *cstate)
>  	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	enum pipe pipe = intel_crtc->pipe;
> -	int id;
>  
>  	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>  
>  	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) 
> {
> -		id = skl_wm_plane_id(to_intel_plane(plane));
> +		enum plane_id plane_id = to_intel_plane(plane)->id;
>  
> -		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id],
> -					&new_ddb->plane[pipe][id])
> &&
> -		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][id],
> -					&new_ddb-
> >y_plane[pipe][id]))
> +		if (skl_ddb_entry_equal(&cur_ddb-
> >plane[pipe][plane_id],
> +					&new_ddb-
> >plane[pipe][plane_id]) &&
> +		    skl_ddb_entry_equal(&cur_ddb-
> >y_plane[pipe][plane_id],
> +					&new_ddb-
> >y_plane[pipe][plane_id]))
>  			continue;
>  
>  		plane_state = drm_atomic_get_plane_state(state,
> plane);
> @@ -4099,7 +4073,6 @@ skl_print_wm_changes(const struct
> drm_atomic_state *state)
>  	const struct intel_plane *intel_plane;
>  	const struct skl_ddb_allocation *old_ddb = &dev_priv-
> >wm.skl_hw.ddb;
>  	const struct skl_ddb_allocation *new_ddb = &intel_state-
> >wm_results.ddb;
> -	int id;
>  	int i;
>  
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
> @@ -4107,11 +4080,11 @@ skl_print_wm_changes(const struct
> drm_atomic_state *state)
>  		enum pipe pipe = intel_crtc->pipe;
>  
>  		for_each_intel_plane_on_crtc(dev, intel_crtc,
> intel_plane) {
> +			enum plane_id plane_id = intel_plane->id;
>  			const struct skl_ddb_entry *old, *new;
>  
> -			id = skl_wm_plane_id(intel_plane);
> -			old = &old_ddb->plane[pipe][id];
> -			new = &new_ddb->plane[pipe][id];
> +			old = &old_ddb->plane[pipe][plane_id];
> +			new = &new_ddb->plane[pipe][plane_id];
>  
>  			if (skl_ddb_entry_equal(old, new))
>  				continue;
> @@ -4201,17 +4174,21 @@ static void skl_atomic_update_crtc_wm(struct
> intel_atomic_state *state,
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	const struct skl_ddb_allocation *ddb = &state-
> >wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> -	int plane;
> +	enum plane_id plane_id;
>  
>  	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc-
> >base)))
>  		return;
>  
>  	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
>  
> -	for_each_universal_plane(dev_priv, pipe, plane)
> -		skl_write_plane_wm(crtc, &pipe_wm->planes[plane],
> ddb, plane);
> -
> -	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR],
> ddb);
> +	for_each_plane_id_on_crtc(crtc, plane_id) {
> +		if (plane_id != PLANE_CURSOR)
> +			skl_write_plane_wm(crtc, &pipe_wm-
> >planes[plane_id],
> +					   ddb, plane_id);
> +		else
> +			skl_write_cursor_wm(crtc, &pipe_wm-
> >planes[plane_id],
> +					    ddb);
> +	}
>  }
>  
>  static void skl_initial_wm(struct intel_atomic_state *state,
> @@ -4326,32 +4303,29 @@ static inline void
> skl_wm_level_from_reg_val(uint32_t val,
>  void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
>  			      struct skl_pipe_wm *out)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -	struct skl_plane_wm *wm;
>  	enum pipe pipe = intel_crtc->pipe;
> -	int level, id, max_level;
> +	int level, max_level;
> +	enum plane_id plane_id;
>  	uint32_t val;
>  
>  	max_level = ilk_wm_max_level(dev_priv);
>  
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		id = skl_wm_plane_id(intel_plane);
> -		wm = &out->planes[id];
> +	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> +		struct skl_plane_wm *wm = &out->planes[plane_id];
>  
>  		for (level = 0; level <= max_level; level++) {
> -			if (id != PLANE_CURSOR)
> -				val = I915_READ(PLANE_WM(pipe, id,
> level));
> +			if (plane_id != PLANE_CURSOR)
> +				val = I915_READ(PLANE_WM(pipe,
> plane_id, level));
>  			else
>  				val = I915_READ(CUR_WM(pipe,
> level));
>  
>  			skl_wm_level_from_reg_val(val, &wm-
> >wm[level]);
>  		}
>  
> -		if (id != PLANE_CURSOR)
> -			val = I915_READ(PLANE_WM_TRANS(pipe, id));
> +		if (plane_id != PLANE_CURSOR)
> +			val = I915_READ(PLANE_WM_TRANS(pipe,
> plane_id));
>  		else
>  			val = I915_READ(CUR_WM_TRANS(pipe));
>  
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2)
  2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
                   ` (9 preceding siblings ...)
  2016-11-22 16:45 ` ✓ Fi.CI.BAT: success for drm/i915: Add a per-pipe plane identifier enum (rev5) Patchwork
@ 2016-11-23 20:16 ` Ville Syrjälä
  10 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2016-11-23 20:16 UTC (permalink / raw)
  To: intel-gfx

On Tue, Nov 22, 2016 at 06:01:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I ended up tweaking quite a few of the patches (and even adding a new one)
> based on Paulo's review, so figured I'd repost the entire thing.
> 
> I didn't do the _ID_ rename thing, at least not yet. I'll have to
> think about it more if I could come up with some nice way to deal
> with the per-pipe id vs. the A/B/C plane id.
> 
> Anyways, enough of these got r-bs so that I can at least move
> forward with VLV/CHV watermark rewrite #4 now.
> 
> Ville Syrjälä (9):
>   drm/i915: Add per-pipe plane identifier
>   drm/i915: Add crtc->plane_ids_mask
>   drm/i915: Use enum plane_id in SKL wm code
>   drm/i915: Use enum plane_id in SKL plane code
>   drm/i915: Use enum plane_id in VLV/CHV sprite code
>   drm/i915: Use enum plane_id in VLV/CHV wm code

Pushed upto here. Thanks for the reviews.

>   drm/i915: s/plane/plane_id/ in skl+ plane register defines
>   drm/i915: Don't populate plane->plane for cursors and sprites
>   drm/i915: Rename the local 'plane' variable to 'plane_id' in primary
>     plane code
> 
>  drivers/gpu/drm/i915/i915_drv.h      |  30 ++++-
>  drivers/gpu/drm/i915/i915_reg.h      | 104 +++++++-------
>  drivers/gpu/drm/i915/intel_display.c |  96 ++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 253 ++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_sprite.c  | 118 ++++++++--------
>  6 files changed, 304 insertions(+), 303 deletions(-)
> 
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 7/9] drm/i915: s/plane/plane_id/ in skl+ plane register defines
  2016-11-22 16:02 ` [PATCH 7/9] drm/i915: s/plane/plane_id/ in skl+ plane register defines ville.syrjala
@ 2016-12-15 18:33   ` Paulo Zanoni
  0 siblings, 0 replies; 22+ messages in thread
From: Paulo Zanoni @ 2016-12-15 18:33 UTC (permalink / raw)
  To: ville.syrjala, intel-gfx

Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Rename the SKL plane register define 'plane' parameter to 'plane_id'
> to
> reflect the fact that you're supposed to pass in the enum plane_id
> rather than enum plane.
> 
> Do the same for the scaler plane selector bits.
> 

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

> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Suggested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 46 ++++++++++++++++++++-----------
> ----------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index d03491089f9c..8ceea23ba63b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5502,8 +5502,8 @@ enum {
>  #define _PLANE_CTL_1(pipe)	_PIPE(pipe, _PLANE_CTL_1_A,
> _PLANE_CTL_1_B)
>  #define _PLANE_CTL_2(pipe)	_PIPE(pipe, _PLANE_CTL_2_A,
> _PLANE_CTL_2_B)
>  #define _PLANE_CTL_3(pipe)	_PIPE(pipe, _PLANE_CTL_3_A,
> _PLANE_CTL_3_B)
> -#define PLANE_CTL(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_CTL_1(pipe), _PLANE_CTL_2(pipe))
> +#define PLANE_CTL(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_CTL_1(pipe),
> _PLANE_CTL_2(pipe))
>  
>  #define _PLANE_STRIDE_1_B			0x71188
>  #define _PLANE_STRIDE_2_B			0x71288
> @@ -5514,8 +5514,8 @@ enum {
>  	_PIPE(pipe, _PLANE_STRIDE_2_A, _PLANE_STRIDE_2_B)
>  #define _PLANE_STRIDE_3(pipe)	\
>  	_PIPE(pipe, _PLANE_STRIDE_3_A, _PLANE_STRIDE_3_B)
> -#define PLANE_STRIDE(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_STRIDE_1(pipe),
> _PLANE_STRIDE_2(pipe))
> +#define PLANE_STRIDE(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_STRIDE_1(pipe),
> _PLANE_STRIDE_2(pipe))
>  
>  #define _PLANE_POS_1_B				0x7118c
>  #define _PLANE_POS_2_B				0x7128c
> @@ -5523,8 +5523,8 @@ enum {
>  #define _PLANE_POS_1(pipe)	_PIPE(pipe, _PLANE_POS_1_A,
> _PLANE_POS_1_B)
>  #define _PLANE_POS_2(pipe)	_PIPE(pipe, _PLANE_POS_2_A,
> _PLANE_POS_2_B)
>  #define _PLANE_POS_3(pipe)	_PIPE(pipe, _PLANE_POS_3_A,
> _PLANE_POS_3_B)
> -#define PLANE_POS(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_POS_1(pipe), _PLANE_POS_2(pipe))
> +#define PLANE_POS(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_POS_1(pipe),
> _PLANE_POS_2(pipe))
>  
>  #define _PLANE_SIZE_1_B				0x71190
>  #define _PLANE_SIZE_2_B				0x71290
> @@ -5532,8 +5532,8 @@ enum {
>  #define _PLANE_SIZE_1(pipe)	_PIPE(pipe, _PLANE_SIZE_1_A,
> _PLANE_SIZE_1_B)
>  #define _PLANE_SIZE_2(pipe)	_PIPE(pipe, _PLANE_SIZE_2_A,
> _PLANE_SIZE_2_B)
>  #define _PLANE_SIZE_3(pipe)	_PIPE(pipe, _PLANE_SIZE_3_A,
> _PLANE_SIZE_3_B)
> -#define PLANE_SIZE(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_SIZE_1(pipe), _PLANE_SIZE_2(pipe))
> +#define PLANE_SIZE(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_SIZE_1(pipe),
> _PLANE_SIZE_2(pipe))
>  
>  #define _PLANE_SURF_1_B				0x7119c
>  #define _PLANE_SURF_2_B				0x7129c
> @@ -5541,36 +5541,36 @@ enum {
>  #define _PLANE_SURF_1(pipe)	_PIPE(pipe, _PLANE_SURF_1_A,
> _PLANE_SURF_1_B)
>  #define _PLANE_SURF_2(pipe)	_PIPE(pipe, _PLANE_SURF_2_A,
> _PLANE_SURF_2_B)
>  #define _PLANE_SURF_3(pipe)	_PIPE(pipe, _PLANE_SURF_3_A,
> _PLANE_SURF_3_B)
> -#define PLANE_SURF(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_SURF_1(pipe), _PLANE_SURF_2(pipe))
> +#define PLANE_SURF(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_SURF_1(pipe),
> _PLANE_SURF_2(pipe))
>  
>  #define _PLANE_OFFSET_1_B			0x711a4
>  #define _PLANE_OFFSET_2_B			0x712a4
>  #define _PLANE_OFFSET_1(pipe) _PIPE(pipe, _PLANE_OFFSET_1_A,
> _PLANE_OFFSET_1_B)
>  #define _PLANE_OFFSET_2(pipe) _PIPE(pipe, _PLANE_OFFSET_2_A,
> _PLANE_OFFSET_2_B)
> -#define PLANE_OFFSET(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_OFFSET_1(pipe),
> _PLANE_OFFSET_2(pipe))
> +#define PLANE_OFFSET(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_OFFSET_1(pipe),
> _PLANE_OFFSET_2(pipe))
>  
>  #define _PLANE_KEYVAL_1_B			0x71194
>  #define _PLANE_KEYVAL_2_B			0x71294
>  #define _PLANE_KEYVAL_1(pipe) _PIPE(pipe, _PLANE_KEYVAL_1_A,
> _PLANE_KEYVAL_1_B)
>  #define _PLANE_KEYVAL_2(pipe) _PIPE(pipe, _PLANE_KEYVAL_2_A,
> _PLANE_KEYVAL_2_B)
> -#define PLANE_KEYVAL(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_KEYVAL_1(pipe),
> _PLANE_KEYVAL_2(pipe))
> +#define PLANE_KEYVAL(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_KEYVAL_1(pipe),
> _PLANE_KEYVAL_2(pipe))
>  
>  #define _PLANE_KEYMSK_1_B			0x71198
>  #define _PLANE_KEYMSK_2_B			0x71298
>  #define _PLANE_KEYMSK_1(pipe) _PIPE(pipe, _PLANE_KEYMSK_1_A,
> _PLANE_KEYMSK_1_B)
>  #define _PLANE_KEYMSK_2(pipe) _PIPE(pipe, _PLANE_KEYMSK_2_A,
> _PLANE_KEYMSK_2_B)
> -#define PLANE_KEYMSK(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_KEYMSK_1(pipe),
> _PLANE_KEYMSK_2(pipe))
> +#define PLANE_KEYMSK(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_KEYMSK_1(pipe),
> _PLANE_KEYMSK_2(pipe))
>  
>  #define _PLANE_KEYMAX_1_B			0x711a0
>  #define _PLANE_KEYMAX_2_B			0x712a0
>  #define _PLANE_KEYMAX_1(pipe) _PIPE(pipe, _PLANE_KEYMAX_1_A,
> _PLANE_KEYMAX_1_B)
>  #define _PLANE_KEYMAX_2(pipe) _PIPE(pipe, _PLANE_KEYMAX_2_A,
> _PLANE_KEYMAX_2_B)
> -#define PLANE_KEYMAX(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_KEYMAX_1(pipe),
> _PLANE_KEYMAX_2(pipe))
> +#define PLANE_KEYMAX(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_KEYMAX_1(pipe),
> _PLANE_KEYMAX_2(pipe))
>  
>  #define _PLANE_BUF_CFG_1_B			0x7127c
>  #define _PLANE_BUF_CFG_2_B			0x7137c
> @@ -5578,8 +5578,8 @@ enum {
>  	_PIPE(pipe, _PLANE_BUF_CFG_1_A, _PLANE_BUF_CFG_1_B)
>  #define _PLANE_BUF_CFG_2(pipe)	\
>  	_PIPE(pipe, _PLANE_BUF_CFG_2_A, _PLANE_BUF_CFG_2_B)
> -#define PLANE_BUF_CFG(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_BUF_CFG_1(pipe),
> _PLANE_BUF_CFG_2(pipe))
> +#define PLANE_BUF_CFG(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_BUF_CFG_1(pipe),
> _PLANE_BUF_CFG_2(pipe))
>  
>  #define _PLANE_NV12_BUF_CFG_1_B		0x71278
>  #define _PLANE_NV12_BUF_CFG_2_B		0x71378
> @@ -5587,8 +5587,8 @@ enum {
>  	_PIPE(pipe, _PLANE_NV12_BUF_CFG_1_A,
> _PLANE_NV12_BUF_CFG_1_B)
>  #define _PLANE_NV12_BUF_CFG_2(pipe)	\
>  	_PIPE(pipe, _PLANE_NV12_BUF_CFG_2_A,
> _PLANE_NV12_BUF_CFG_2_B)
> -#define PLANE_NV12_BUF_CFG(pipe, plane)	\
> -	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe),
> _PLANE_NV12_BUF_CFG_2(pipe))
> +#define PLANE_NV12_BUF_CFG(pipe, plane_id)	\
> +	_MMIO_PLANE(plane_id, _PLANE_NV12_BUF_CFG_1(pipe),
> _PLANE_NV12_BUF_CFG_2(pipe))
>  
>  /* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
> @@ -5737,7 +5737,7 @@ enum {
>  #define PS_SCALER_MODE_DYN  (0 << 28)
>  #define PS_SCALER_MODE_HQ  (1 << 28)
>  #define PS_PLANE_SEL_MASK  (7 << 25)
> -#define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
> +#define PS_PLANE_SEL(plane_id) (((plane_id) + 1) << 25)
>  #define PS_FILTER_MASK         (3 << 23)
>  #define PS_FILTER_MEDIUM       (0 << 23)
>  #define PS_FILTER_EDGE_ENHANCE (2 << 23)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-11-22 16:02 ` [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites ville.syrjala
@ 2016-12-15 19:05   ` Paulo Zanoni
  2016-12-15 19:11     ` Ville Syrjälä
  0 siblings, 1 reply; 22+ messages in thread
From: Paulo Zanoni @ 2016-12-15 19:05 UTC (permalink / raw)
  To: ville.syrjala, intel-gfx

Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> With plane->plane now purely reserved for the primary planes, let's
> not even populate it for cursors and sprites. Let's switch the type
> to enum plane as well since it's no longer being abused for anything
> else.

Since it's a primary plane thing, I think it makes more sense to just
leave it at struct intel_crtc instead of having a field that's unused
and doesn't make sense in some cases. The crtc struct already includes
some fields that are specific to primary planes, so I think it's a good
place. Or we could create a new class: struct intel_primary_plane {
struct intel_plane base; enum plane legacy_plane };

Following is a patch suggestion (probably impossible to apply due to my
editor breaking long lines). Any arguments against it?

--8<----------------------------------------------------------

diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index bc1af87..c54b1a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
drm_i915_private *dev_priv, enum pipe pipe)
 		state->scaler_id = -1;
 	}
 	primary->pipe = pipe;
-	/*
-	 * On gen2/3 only plane A can do FBC, but the panel fitter and
LVDS
-	 * port is hooked to pipe B. Hence we want plane A feeding
pipe B.
-	 */
-	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
-		primary->plane = (enum plane) !pipe;
-	else
-		primary->plane = (enum plane) pipe;
 	primary->id = PLANE_PRIMARY;
 	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
 	primary->check_plane = intel_check_primary_plane;
@@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
drm_i915_private *dev_priv, enum pipe pipe)
 					       0, &intel_plane_funcs,
 					       intel_primary_formats,
num_formats,
 					       DRM_PLANE_TYPE_PRIMARY,
-					       "plane %c",
plane_name(primary->plane));
+					       "plane %c",
pipe_name(pipe)); /* FIXME */
 	if (ret)
 		goto fail;
 
@@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
drm_i915_private *dev_priv, enum pipe pipe)
 	cursor->can_scale = false;
 	cursor->max_downscale = 1;
 	cursor->pipe = pipe;
-	cursor->plane = pipe;
 	cursor->id = PLANE_CURSOR;
 	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
 	cursor->check_plane = intel_check_cursor_plane;
@@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
drm_i915_private *dev_priv, enum pipe pipe)
 		goto fail;
 
 	intel_crtc->pipe = pipe;
-	intel_crtc->plane = primary->plane;
+	/*
+	 * On gen2/3 only plane A can do FBC, but the panel fitter and
LVDS
+	 * port is hooked to pipe B. Hence we want plane A feeding
pipe B.
+	 */
+	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
+		intel_crtc->plane = (enum plane) !pipe;
+	else
+		intel_crtc->plane = (enum plane) pipe;
 
 	intel_crtc->cursor_base = ~0;
 	intel_crtc->cursor_cntl = ~0;
@@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
drm_i915_private *dev_priv)
 	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
 }
 
-static bool primary_get_hw_state(struct intel_plane *plane)
+static bool primary_get_hw_state(struct intel_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
-	return I915_READ(DSPCNTR(plane->plane)) &
DISPLAY_PLANE_ENABLE;
+	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
 }
 
 /* FIXME read out full plane state for all planes */
@@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
intel_crtc *crtc)
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(primary->state);
 
-	plane_state->base.visible = crtc->active &&
-		primary_get_hw_state(to_intel_plane(primary));
+	plane_state->base.visible = crtc->active &&
primary_get_hw_state(crtc);
 
 	if (plane_state->base.visible)
 		crtc->base.state->plane_mask |= 1 <<
drm_plane_index(primary);
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index 362f698..b2bdd49 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
 
 struct intel_plane {
 	struct drm_plane base;
-	u8 plane;
 	enum plane_id id;
 	enum pipe pipe;
 	bool can_scale;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 63154a5..1044095 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct drm_i915_private
*dev_priv,
 	}
 
 	intel_plane->pipe = pipe;
-	intel_plane->plane = plane;
 	intel_plane->id = PLANE_SPRITE0 + plane;
 	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe,
plane);
 	intel_plane->check_plane = intel_check_sprite_plane;

--8<----------------------------------------------------------

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 1 -
>  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
>  3 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b6d5d5e5cc99..f180f14fcf3a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
> -	cursor->plane = pipe;
>  	cursor->id = PLANE_CURSOR;
>  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
>  	cursor->check_plane = intel_check_cursor_plane;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index c1b245853ba9..d14718e09911 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
>  
>  struct intel_plane {
>  	struct drm_plane base;
> -	u8 plane;
> +	enum plane plane;
>  	enum plane_id id;
>  	enum pipe pipe;
>  	bool can_scale;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 63154a5a9305..1044095d0084 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> drm_i915_private *dev_priv,
>  	}
>  
>  	intel_plane->pipe = pipe;
> -	intel_plane->plane = plane;
>  	intel_plane->id = PLANE_SPRITE0 + plane;
>  	intel_plane->frontbuffer_bit =
> INTEL_FRONTBUFFER_SPRITE(pipe, plane);
>  	intel_plane->check_plane = intel_check_sprite_plane;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-15 19:05   ` Paulo Zanoni
@ 2016-12-15 19:11     ` Ville Syrjälä
  2016-12-15 19:50       ` Paulo Zanoni
  0 siblings, 1 reply; 22+ messages in thread
From: Ville Syrjälä @ 2016-12-15 19:11 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > With plane->plane now purely reserved for the primary planes, let's
> > not even populate it for cursors and sprites. Let's switch the type
> > to enum plane as well since it's no longer being abused for anything
> > else.
> 
> Since it's a primary plane thing, I think it makes more sense to just
> leave it at struct intel_crtc instead of having a field that's unused
> and doesn't make sense in some cases.

Primary plane aren't really primary planes on old platforms though.
That's just a software concept that has no bearing on the hardware.

> The crtc struct already includes
> some fields that are specific to primary planes, so I think it's a good
> place. Or we could create a new class: struct intel_primary_plane {
> struct intel_plane base; enum plane legacy_plane };
> 
> Following is a patch suggestion (probably impossible to apply due to my
> editor breaking long lines). Any arguments against it?
> 
> --8<----------------------------------------------------------
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index bc1af87..c54b1a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  		state->scaler_id = -1;
>  	}
>  	primary->pipe = pipe;
> -	/*
> -	 * On gen2/3 only plane A can do FBC, but the panel fitter and
> LVDS
> -	 * port is hooked to pipe B. Hence we want plane A feeding
> pipe B.
> -	 */
> -	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> -		primary->plane = (enum plane) !pipe;
> -	else
> -		primary->plane = (enum plane) pipe;
>  	primary->id = PLANE_PRIMARY;
>  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
>  	primary->check_plane = intel_check_primary_plane;
> @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  					       0, &intel_plane_funcs,
>  					       intel_primary_formats,
> num_formats,
>  					       DRM_PLANE_TYPE_PRIMARY,
> -					       "plane %c",
> plane_name(primary->plane));
> +					       "plane %c",
> pipe_name(pipe)); /* FIXME */
>  	if (ret)
>  		goto fail;
>  
> @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  	cursor->can_scale = false;
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
> -	cursor->plane = pipe;
>  	cursor->id = PLANE_CURSOR;
>  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
>  	cursor->check_plane = intel_check_cursor_plane;
> @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  		goto fail;
>  
>  	intel_crtc->pipe = pipe;
> -	intel_crtc->plane = primary->plane;
> +	/*
> +	 * On gen2/3 only plane A can do FBC, but the panel fitter and
> LVDS
> +	 * port is hooked to pipe B. Hence we want plane A feeding
> pipe B.
> +	 */
> +	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> +		intel_crtc->plane = (enum plane) !pipe;
> +	else
> +		intel_crtc->plane = (enum plane) pipe;
>  
>  	intel_crtc->cursor_base = ~0;
>  	intel_crtc->cursor_cntl = ~0;
> @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> drm_i915_private *dev_priv)
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
>  }
>  
> -static bool primary_get_hw_state(struct intel_plane *plane)
> +static bool primary_get_hw_state(struct intel_crtc *crtc)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  
> -	return I915_READ(DSPCNTR(plane->plane)) &
> DISPLAY_PLANE_ENABLE;
> +	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>  }
>  
>  /* FIXME read out full plane state for all planes */
> @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> intel_crtc *crtc)
>  	struct intel_plane_state *plane_state =
>  		to_intel_plane_state(primary->state);
>  
> -	plane_state->base.visible = crtc->active &&
> -		primary_get_hw_state(to_intel_plane(primary));
> +	plane_state->base.visible = crtc->active &&
> primary_get_hw_state(crtc);
>  
>  	if (plane_state->base.visible)
>  		crtc->base.state->plane_mask |= 1 <<
> drm_plane_index(primary);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 362f698..b2bdd49 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
>  
>  struct intel_plane {
>  	struct drm_plane base;
> -	u8 plane;
>  	enum plane_id id;
>  	enum pipe pipe;
>  	bool can_scale;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 63154a5..1044095 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct drm_i915_private
> *dev_priv,
>  	}
>  
>  	intel_plane->pipe = pipe;
> -	intel_plane->plane = plane;
>  	intel_plane->id = PLANE_SPRITE0 + plane;
>  	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe,
> plane);
>  	intel_plane->check_plane = intel_check_sprite_plane;
> 
> --8<----------------------------------------------------------
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 1 -
> >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
> >  3 files changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index b6d5d5e5cc99..f180f14fcf3a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> > -	cursor->plane = pipe;
> >  	cursor->id = PLANE_CURSOR;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >  	cursor->check_plane = intel_check_cursor_plane;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index c1b245853ba9..d14718e09911 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> >  
> >  struct intel_plane {
> >  	struct drm_plane base;
> > -	u8 plane;
> > +	enum plane plane;
> >  	enum plane_id id;
> >  	enum pipe pipe;
> >  	bool can_scale;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 63154a5a9305..1044095d0084 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > drm_i915_private *dev_priv,
> >  	}
> >  
> >  	intel_plane->pipe = pipe;
> > -	intel_plane->plane = plane;
> >  	intel_plane->id = PLANE_SPRITE0 + plane;
> >  	intel_plane->frontbuffer_bit =
> > INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> >  	intel_plane->check_plane = intel_check_sprite_plane;

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-15 19:11     ` Ville Syrjälä
@ 2016-12-15 19:50       ` Paulo Zanoni
  2016-12-15 19:59         ` Ville Syrjälä
  0 siblings, 1 reply; 22+ messages in thread
From: Paulo Zanoni @ 2016-12-15 19:50 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> > 
> > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
> > escreveu:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > With plane->plane now purely reserved for the primary planes,
> > > let's
> > > not even populate it for cursors and sprites. Let's switch the
> > > type
> > > to enum plane as well since it's no longer being abused for
> > > anything
> > > else.
> > 
> > Since it's a primary plane thing, I think it makes more sense to
> > just
> > leave it at struct intel_crtc instead of having a field that's
> > unused
> > and doesn't make sense in some cases.
> 
> Primary plane aren't really primary planes on old platforms though.
> That's just a software concept that has no bearing on the hardware.

Please explain more since that's not my understanding. I just opened
the gen 2 and 3 spec docs and they do contain tons of "primary plane"
references. I know that the hardware is a little more flexible there,
but as far as I understand we don't even implement that. Besides, both
our driver and our API do have the concept of a primary plane, so it
makes sense for the code to be organized that way, IMHO. Besides, I
think our code organizations should always better fit the new hardware,
since otherwise we'll make i915.ko development even harder than it is
for new contributors/employees.

(And when I see those specs and realize how different the HW was, then
I remember that in order to explain to the new people why some things
are the way they are in our code I have to explain how the HW was 10
years ago, I start thinking again that maybe we should just have i915-
old.ko and i915-new.ko, since either we make our abstractions/design
fit one thing or we make it fit another...)

> 
> > 
> > The crtc struct already includes
> > some fields that are specific to primary planes, so I think it's a
> > good
> > place. Or we could create a new class: struct intel_primary_plane {
> > struct intel_plane base; enum plane legacy_plane };
> > 
> > Following is a patch suggestion (probably impossible to apply due
> > to my
> > editor breaking long lines). Any arguments against it?
> > 
> > --8<----------------------------------------------------------
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index bc1af87..c54b1a7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> >  		state->scaler_id = -1;
> >  	}
> >  	primary->pipe = pipe;
> > -	/*
> > -	 * On gen2/3 only plane A can do FBC, but the panel fitter
> > and
> > LVDS
> > -	 * port is hooked to pipe B. Hence we want plane A feeding
> > pipe B.
> > -	 */
> > -	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > -		primary->plane = (enum plane) !pipe;
> > -	else
> > -		primary->plane = (enum plane) pipe;
> >  	primary->id = PLANE_PRIMARY;
> >  	primary->frontbuffer_bit =
> > INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> >  					       0,
> > &intel_plane_funcs,
> >  					       intel_primary_forma
> > ts,
> > num_formats,
> >  					       DRM_PLANE_TYPE_PRIM
> > ARY,
> > -					       "plane %c",
> > plane_name(primary->plane));
> > +					       "plane %c",
> > pipe_name(pipe)); /* FIXME */
> >  	if (ret)
> >  		goto fail;
> >  
> > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> > -	cursor->plane = pipe;
> >  	cursor->id = PLANE_CURSOR;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >  	cursor->check_plane = intel_check_cursor_plane;
> > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> >  		goto fail;
> >  
> >  	intel_crtc->pipe = pipe;
> > -	intel_crtc->plane = primary->plane;
> > +	/*
> > +	 * On gen2/3 only plane A can do FBC, but the panel fitter
> > and
> > LVDS
> > +	 * port is hooked to pipe B. Hence we want plane A feeding
> > pipe B.
> > +	 */
> > +	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > +		intel_crtc->plane = (enum plane) !pipe;
> > +	else
> > +		intel_crtc->plane = (enum plane) pipe;
> >  
> >  	intel_crtc->cursor_base = ~0;
> >  	intel_crtc->cursor_cntl = ~0;
> > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> > drm_i915_private *dev_priv)
> >  	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
> >  }
> >  
> > -static bool primary_get_hw_state(struct intel_plane *plane)
> > +static bool primary_get_hw_state(struct intel_crtc *crtc)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(plane-
> > >base.dev);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc-
> > >base.dev);
> >  
> > -	return I915_READ(DSPCNTR(plane->plane)) &
> > DISPLAY_PLANE_ENABLE;
> > +	return I915_READ(DSPCNTR(crtc->plane)) &
> > DISPLAY_PLANE_ENABLE;
> >  }
> >  
> >  /* FIXME read out full plane state for all planes */
> > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> > intel_crtc *crtc)
> >  	struct intel_plane_state *plane_state =
> >  		to_intel_plane_state(primary->state);
> >  
> > -	plane_state->base.visible = crtc->active &&
> > -		primary_get_hw_state(to_intel_plane(primary));
> > +	plane_state->base.visible = crtc->active &&
> > primary_get_hw_state(crtc);
> >  
> >  	if (plane_state->base.visible)
> >  		crtc->base.state->plane_mask |= 1 <<
> > drm_plane_index(primary);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 362f698..b2bdd49 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
> >  
> >  struct intel_plane {
> >  	struct drm_plane base;
> > -	u8 plane;
> >  	enum plane_id id;
> >  	enum pipe pipe;
> >  	bool can_scale;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 63154a5..1044095 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > drm_i915_private
> > *dev_priv,
> >  	}
> >  
> >  	intel_plane->pipe = pipe;
> > -	intel_plane->plane = plane;
> >  	intel_plane->id = PLANE_SPRITE0 + plane;
> >  	intel_plane->frontbuffer_bit =
> > INTEL_FRONTBUFFER_SPRITE(pipe,
> > plane);
> >  	intel_plane->check_plane = intel_check_sprite_plane;
> > 
> > --8<----------------------------------------------------------
> > 
> > > 
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 1 -
> > >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
> > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index b6d5d5e5cc99..f180f14fcf3a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >  	cursor->can_scale = false;
> > >  	cursor->max_downscale = 1;
> > >  	cursor->pipe = pipe;
> > > -	cursor->plane = pipe;
> > >  	cursor->id = PLANE_CURSOR;
> > >  	cursor->frontbuffer_bit =
> > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > >  	cursor->check_plane = intel_check_cursor_plane;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index c1b245853ba9..d14718e09911 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> > >  
> > >  struct intel_plane {
> > >  	struct drm_plane base;
> > > -	u8 plane;
> > > +	enum plane plane;
> > >  	enum plane_id id;
> > >  	enum pipe pipe;
> > >  	bool can_scale;
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 63154a5a9305..1044095d0084 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > drm_i915_private *dev_priv,
> > >  	}
> > >  
> > >  	intel_plane->pipe = pipe;
> > > -	intel_plane->plane = plane;
> > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > >  	intel_plane->frontbuffer_bit =
> > > INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> > >  	intel_plane->check_plane = intel_check_sprite_plane;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-15 19:50       ` Paulo Zanoni
@ 2016-12-15 19:59         ` Ville Syrjälä
  2016-12-16 14:07           ` Paulo Zanoni
  2016-12-19 18:24           ` Matt Roper
  0 siblings, 2 replies; 22+ messages in thread
From: Ville Syrjälä @ 2016-12-15 19:59 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote:
> Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> > > 
> > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
> > > escreveu:
> > > > 
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > With plane->plane now purely reserved for the primary planes,
> > > > let's
> > > > not even populate it for cursors and sprites. Let's switch the
> > > > type
> > > > to enum plane as well since it's no longer being abused for
> > > > anything
> > > > else.
> > > 
> > > Since it's a primary plane thing, I think it makes more sense to
> > > just
> > > leave it at struct intel_crtc instead of having a field that's
> > > unused
> > > and doesn't make sense in some cases.
> > 
> > Primary plane aren't really primary planes on old platforms though.
> > That's just a software concept that has no bearing on the hardware.
> 
> Please explain more since that's not my understanding. I just opened
> the gen 2 and 3 spec docs and they do contain tons of "primary plane"
> references. I know that the hardware is a little more flexible there,

Yep. Planes can be moved to any pipe in general.

> but as far as I understand we don't even implement that. 

Not at the moment. But I have plans...

> Besides, both
> our driver and our API do have the concept of a primary plane, so it
> makes sense for the code to be organized that way, IMHO.

A plane is a plane, a pipe is a pipe. IMO you're trying to make it
more confusing by mixing up the two.

> Besides, I
> think our code organizations should always better fit the new hardware,
> since otherwise we'll make i915.ko development even harder than it is
> for new contributors/employees.

If you don't mix planes and pipes there can't be any confusion.

> 
> (And when I see those specs and realize how different the HW was, then
> I remember that in order to explain to the new people why some things
> are the way they are in our code I have to explain how the HW was 10
> years ago, I start thinking again that maybe we should just have i915-
> old.ko and i915-new.ko, since either we make our abstractions/design
> fit one thing or we make it fit another...)

From the register POV hw was almost identical up to BDW at least. It's
just a few bits (the pipe selector) that vanished from the registers.
SKL+ is a little more different but still the registers even live in the
same address.

Also given what recent history has shown us, I wouldn't be at all
surprised if we would go back to a flexible plane<->pipe assignment
in the hardware at some point in the future. A lot of the other
features that were present in old hardware has been making a comeback
in recent years.

> 
> > 
> > > 
> > > The crtc struct already includes
> > > some fields that are specific to primary planes, so I think it's a
> > > good
> > > place. Or we could create a new class: struct intel_primary_plane {
> > > struct intel_plane base; enum plane legacy_plane };
> > > 
> > > Following is a patch suggestion (probably impossible to apply due
> > > to my
> > > editor breaking long lines). Any arguments against it?
> > > 
> > > --8<----------------------------------------------------------
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index bc1af87..c54b1a7 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >  		state->scaler_id = -1;
> > >  	}
> > >  	primary->pipe = pipe;
> > > -	/*
> > > -	 * On gen2/3 only plane A can do FBC, but the panel fitter
> > > and
> > > LVDS
> > > -	 * port is hooked to pipe B. Hence we want plane A feeding
> > > pipe B.
> > > -	 */
> > > -	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > -		primary->plane = (enum plane) !pipe;
> > > -	else
> > > -		primary->plane = (enum plane) pipe;
> > >  	primary->id = PLANE_PRIMARY;
> > >  	primary->frontbuffer_bit =
> > > INTEL_FRONTBUFFER_PRIMARY(pipe);
> > >  	primary->check_plane = intel_check_primary_plane;
> > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >  					       0,
> > > &intel_plane_funcs,
> > >  					       intel_primary_forma
> > > ts,
> > > num_formats,
> > >  					       DRM_PLANE_TYPE_PRIM
> > > ARY,
> > > -					       "plane %c",
> > > plane_name(primary->plane));
> > > +					       "plane %c",
> > > pipe_name(pipe)); /* FIXME */
> > >  	if (ret)
> > >  		goto fail;
> > >  
> > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >  	cursor->can_scale = false;
> > >  	cursor->max_downscale = 1;
> > >  	cursor->pipe = pipe;
> > > -	cursor->plane = pipe;
> > >  	cursor->id = PLANE_CURSOR;
> > >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> > >  	cursor->check_plane = intel_check_cursor_plane;
> > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >  		goto fail;
> > >  
> > >  	intel_crtc->pipe = pipe;
> > > -	intel_crtc->plane = primary->plane;
> > > +	/*
> > > +	 * On gen2/3 only plane A can do FBC, but the panel fitter
> > > and
> > > LVDS
> > > +	 * port is hooked to pipe B. Hence we want plane A feeding
> > > pipe B.
> > > +	 */
> > > +	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > +		intel_crtc->plane = (enum plane) !pipe;
> > > +	else
> > > +		intel_crtc->plane = (enum plane) pipe;
> > >  
> > >  	intel_crtc->cursor_base = ~0;
> > >  	intel_crtc->cursor_cntl = ~0;
> > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> > > drm_i915_private *dev_priv)
> > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
> > >  }
> > >  
> > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > +static bool primary_get_hw_state(struct intel_crtc *crtc)
> > >  {
> > > -	struct drm_i915_private *dev_priv = to_i915(plane-
> > > >base.dev);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > >base.dev);
> > >  
> > > -	return I915_READ(DSPCNTR(plane->plane)) &
> > > DISPLAY_PLANE_ENABLE;
> > > +	return I915_READ(DSPCNTR(crtc->plane)) &
> > > DISPLAY_PLANE_ENABLE;
> > >  }
> > >  
> > >  /* FIXME read out full plane state for all planes */
> > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> > > intel_crtc *crtc)
> > >  	struct intel_plane_state *plane_state =
> > >  		to_intel_plane_state(primary->state);
> > >  
> > > -	plane_state->base.visible = crtc->active &&
> > > -		primary_get_hw_state(to_intel_plane(primary));
> > > +	plane_state->base.visible = crtc->active &&
> > > primary_get_hw_state(crtc);
> > >  
> > >  	if (plane_state->base.visible)
> > >  		crtc->base.state->plane_mask |= 1 <<
> > > drm_plane_index(primary);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 362f698..b2bdd49 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
> > >  
> > >  struct intel_plane {
> > >  	struct drm_plane base;
> > > -	u8 plane;
> > >  	enum plane_id id;
> > >  	enum pipe pipe;
> > >  	bool can_scale;
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 63154a5..1044095 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > drm_i915_private
> > > *dev_priv,
> > >  	}
> > >  
> > >  	intel_plane->pipe = pipe;
> > > -	intel_plane->plane = plane;
> > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > >  	intel_plane->frontbuffer_bit =
> > > INTEL_FRONTBUFFER_SPRITE(pipe,
> > > plane);
> > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > > 
> > > --8<----------------------------------------------------------
> > > 
> > > > 
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 1 -
> > > >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> > > >  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
> > > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index b6d5d5e5cc99..f180f14fcf3a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  	cursor->can_scale = false;
> > > >  	cursor->max_downscale = 1;
> > > >  	cursor->pipe = pipe;
> > > > -	cursor->plane = pipe;
> > > >  	cursor->id = PLANE_CURSOR;
> > > >  	cursor->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index c1b245853ba9..d14718e09911 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> > > >  
> > > >  struct intel_plane {
> > > >  	struct drm_plane base;
> > > > -	u8 plane;
> > > > +	enum plane plane;
> > > >  	enum plane_id id;
> > > >  	enum pipe pipe;
> > > >  	bool can_scale;
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 63154a5a9305..1044095d0084 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > drm_i915_private *dev_priv,
> > > >  	}
> > > >  
> > > >  	intel_plane->pipe = pipe;
> > > > -	intel_plane->plane = plane;
> > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > >  	intel_plane->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-15 19:59         ` Ville Syrjälä
@ 2016-12-16 14:07           ` Paulo Zanoni
  2016-12-16 14:56             ` Ander Conselvan De Oliveira
  2016-12-19 18:24           ` Matt Roper
  1 sibling, 1 reply; 22+ messages in thread
From: Paulo Zanoni @ 2016-12-16 14:07 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Em Qui, 2016-12-15 às 21:59 +0200, Ville Syrjälä escreveu:
> On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote:
> > 
> > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> > > 
> > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> > > > 
> > > > 
> > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.co
> > > > m
> > > > escreveu:
> > > > > 
> > > > > 
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > With plane->plane now purely reserved for the primary planes,
> > > > > let's
> > > > > not even populate it for cursors and sprites. Let's switch
> > > > > the
> > > > > type
> > > > > to enum plane as well since it's no longer being abused for
> > > > > anything
> > > > > else.
> > > > 
> > > > Since it's a primary plane thing, I think it makes more sense
> > > > to
> > > > just
> > > > leave it at struct intel_crtc instead of having a field that's
> > > > unused
> > > > and doesn't make sense in some cases.
> > > 
> > > Primary plane aren't really primary planes on old platforms
> > > though.
> > > That's just a software concept that has no bearing on the
> > > hardware.
> > 
> > Please explain more since that's not my understanding. I just
> > opened
> > the gen 2 and 3 spec docs and they do contain tons of "primary
> > plane"
> > references. I know that the hardware is a little more flexible
> > there,
> 
> Yep. Planes can be moved to any pipe in general.
> 
> > 
> > but as far as I understand we don't even implement that. 
> 
> Not at the moment. But I have plans...
> 
> > 
> > Besides, both
> > our driver and our API do have the concept of a primary plane, so
> > it
> > makes sense for the code to be organized that way, IMHO.
> 
> A plane is a plane, a pipe is a pipe. IMO you're trying to make it
> more confusing by mixing up the two.

I think that goes against your original argument to remove the plane-
>plane assignment for everything except for primary planes.

Anyway, your argumentation is very short and dismissive, it's hard to
understand what exactly you are thinking and try to move the discussion
forward.

I suppose we'll just have to agree to disagree here. In one solution we
create an inconsistency where the plane struct has a field that's only
applicable and can only be used to some specific planes (even though
"every plane is a plane"). In the other solution we create the
inconsistency where the crtc<->plane connection is assigned in the way
not chosen by the hardware and this may be a problem if we ever get to
implement the plane assignment flexibility that's allowed by the
hardware. Maybe having a 3rd opinion here would help.

> 
> > 
> > Besides, I
> > think our code organizations should always better fit the new
> > hardware,
> > since otherwise we'll make i915.ko development even harder than it
> > is
> > for new contributors/employees.
> 
> If you don't mix planes and pipes there can't be any confusion.
> 
> > 
> > 
> > (And when I see those specs and realize how different the HW was,
> > then
> > I remember that in order to explain to the new people why some
> > things
> > are the way they are in our code I have to explain how the HW was
> > 10
> > years ago, I start thinking again that maybe we should just have
> > i915-
> > old.ko and i915-new.ko, since either we make our
> > abstractions/design
> > fit one thing or we make it fit another...)
> 
> From the register POV hw was almost identical up to BDW at least.
> It's
> just a few bits (the pipe selector) that vanished from the registers.
> SKL+ is a little more different but still the registers even live in
> the
> same address.
> 
> Also given what recent history has shown us, I wouldn't be at all
> surprised if we would go back to a flexible plane<->pipe assignment
> in the hardware at some point in the future. A lot of the other
> features that were present in old hardware has been making a comeback
> in recent years.
> 
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > The crtc struct already includes
> > > > some fields that are specific to primary planes, so I think
> > > > it's a
> > > > good
> > > > place. Or we could create a new class: struct
> > > > intel_primary_plane {
> > > > struct intel_plane base; enum plane legacy_plane };
> > > > 
> > > > Following is a patch suggestion (probably impossible to apply
> > > > due
> > > > to my
> > > > editor breaking long lines). Any arguments against it?
> > > > 
> > > > --8<----------------------------------------------------------
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index bc1af87..c54b1a7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  		state->scaler_id = -1;
> > > >  	}
> > > >  	primary->pipe = pipe;
> > > > -	/*
> > > > -	 * On gen2/3 only plane A can do FBC, but the panel
> > > > fitter
> > > > and
> > > > LVDS
> > > > -	 * port is hooked to pipe B. Hence we want plane A
> > > > feeding
> > > > pipe B.
> > > > -	 */
> > > > -	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > > -		primary->plane = (enum plane) !pipe;
> > > > -	else
> > > > -		primary->plane = (enum plane) pipe;
> > > >  	primary->id = PLANE_PRIMARY;
> > > >  	primary->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > >  	primary->check_plane = intel_check_primary_plane;
> > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  					       0,
> > > > &intel_plane_funcs,
> > > >  					       intel_primary_f
> > > > orma
> > > > ts,
> > > > num_formats,
> > > >  					       DRM_PLANE_TYPE_
> > > > PRIM
> > > > ARY,
> > > > -					       "plane %c",
> > > > plane_name(primary->plane));
> > > > +					       "plane %c",
> > > > pipe_name(pipe)); /* FIXME */
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  	cursor->can_scale = false;
> > > >  	cursor->max_downscale = 1;
> > > >  	cursor->pipe = pipe;
> > > > -	cursor->plane = pipe;
> > > >  	cursor->id = PLANE_CURSOR;
> > > >  	cursor->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  		goto fail;
> > > >  
> > > >  	intel_crtc->pipe = pipe;
> > > > -	intel_crtc->plane = primary->plane;
> > > > +	/*
> > > > +	 * On gen2/3 only plane A can do FBC, but the panel
> > > > fitter
> > > > and
> > > > LVDS
> > > > +	 * port is hooked to pipe B. Hence we want plane A
> > > > feeding
> > > > pipe B.
> > > > +	 */
> > > > +	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > > +		intel_crtc->plane = (enum plane) !pipe;
> > > > +	else
> > > > +		intel_crtc->plane = (enum plane) pipe;
> > > >  
> > > >  	intel_crtc->cursor_base = ~0;
> > > >  	intel_crtc->cursor_cntl = ~0;
> > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> > > > drm_i915_private *dev_priv)
> > > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
> > > >  }
> > > >  
> > > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > > +static bool primary_get_hw_state(struct intel_crtc *crtc)
> > > >  {
> > > > -	struct drm_i915_private *dev_priv = to_i915(plane-
> > > > > 
> > > > > base.dev);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > > 
> > > > > base.dev);
> > > >  
> > > > -	return I915_READ(DSPCNTR(plane->plane)) &
> > > > DISPLAY_PLANE_ENABLE;
> > > > +	return I915_READ(DSPCNTR(crtc->plane)) &
> > > > DISPLAY_PLANE_ENABLE;
> > > >  }
> > > >  
> > > >  /* FIXME read out full plane state for all planes */
> > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> > > > intel_crtc *crtc)
> > > >  	struct intel_plane_state *plane_state =
> > > >  		to_intel_plane_state(primary->state);
> > > >  
> > > > -	plane_state->base.visible = crtc->active &&
> > > > -		primary_get_hw_state(to_intel_plane(primary));
> > > > +	plane_state->base.visible = crtc->active &&
> > > > primary_get_hw_state(crtc);
> > > >  
> > > >  	if (plane_state->base.visible)
> > > >  		crtc->base.state->plane_mask |= 1 <<
> > > > drm_plane_index(primary);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 362f698..b2bdd49 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
> > > >  
> > > >  struct intel_plane {
> > > >  	struct drm_plane base;
> > > > -	u8 plane;
> > > >  	enum plane_id id;
> > > >  	enum pipe pipe;
> > > >  	bool can_scale;
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 63154a5..1044095 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > >  	}
> > > >  
> > > >  	intel_plane->pipe = pipe;
> > > > -	intel_plane->plane = plane;
> > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > >  	intel_plane->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_SPRITE(pipe,
> > > > plane);
> > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > > > 
> > > > --8<----------------------------------------------------------
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 1 -
> > > > >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
> > > > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index b6d5d5e5cc99..f180f14fcf3a 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > >  	cursor->can_scale = false;
> > > > >  	cursor->max_downscale = 1;
> > > > >  	cursor->pipe = pipe;
> > > > > -	cursor->plane = pipe;
> > > > >  	cursor->id = PLANE_CURSOR;
> > > > >  	cursor->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index c1b245853ba9..d14718e09911 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> > > > >  
> > > > >  struct intel_plane {
> > > > >  	struct drm_plane base;
> > > > > -	u8 plane;
> > > > > +	enum plane plane;
> > > > >  	enum plane_id id;
> > > > >  	enum pipe pipe;
> > > > >  	bool can_scale;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index 63154a5a9305..1044095d0084 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	}
> > > > >  
> > > > >  	intel_plane->pipe = pipe;
> > > > > -	intel_plane->plane = plane;
> > > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > > >  	intel_plane->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> > > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-16 14:07           ` Paulo Zanoni
@ 2016-12-16 14:56             ` Ander Conselvan De Oliveira
  0 siblings, 0 replies; 22+ messages in thread
From: Ander Conselvan De Oliveira @ 2016-12-16 14:56 UTC (permalink / raw)
  To: Paulo Zanoni, Ville Syrjälä; +Cc: intel-gfx

On Fri, 2016-12-16 at 12:07 -0200, Paulo Zanoni wrote:
> Em Qui, 2016-12-15 às 21:59 +0200, Ville Syrjälä escreveu:
> > 
> > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote:
> > > 
> > > 
> > > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> > > > 
> > > > 
> > > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.co
> > > > > m
> > > > > escreveu:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > With plane->plane now purely reserved for the primary planes,
> > > > > > let's
> > > > > > not even populate it for cursors and sprites. Let's switch
> > > > > > the
> > > > > > type
> > > > > > to enum plane as well since it's no longer being abused for
> > > > > > anything
> > > > > > else.
> > > > > Since it's a primary plane thing, I think it makes more sense
> > > > > to
> > > > > just
> > > > > leave it at struct intel_crtc instead of having a field that's
> > > > > unused
> > > > > and doesn't make sense in some cases.
> > > > Primary plane aren't really primary planes on old platforms
> > > > though.
> > > > That's just a software concept that has no bearing on the
> > > > hardware.
> > > Please explain more since that's not my understanding. I just
> > > opened
> > > the gen 2 and 3 spec docs and they do contain tons of "primary
> > > plane"
> > > references. I know that the hardware is a little more flexible
> > > there,
> > Yep. Planes can be moved to any pipe in general.
> > 
> > > 
> > > 
> > > but as far as I understand we don't even implement that. 
> > Not at the moment. But I have plans...
> > 
> > > 
> > > 
> > > Besides, both
> > > our driver and our API do have the concept of a primary plane, so
> > > it
> > > makes sense for the code to be organized that way, IMHO.
> > A plane is a plane, a pipe is a pipe. IMO you're trying to make it
> > more confusing by mixing up the two.
> I think that goes against your original argument to remove the plane-
> > 
> > plane assignment for everything except for primary planes.
> Anyway, your argumentation is very short and dismissive, it's hard to
> understand what exactly you are thinking and try to move the discussion
> forward.
> 
> I suppose we'll just have to agree to disagree here. In one solution we
> create an inconsistency where the plane struct has a field that's only
> applicable and can only be used to some specific planes (even though
> "every plane is a plane"). In the other solution we create the
> inconsistency where the crtc<->plane connection is assigned in the way
> not chosen by the hardware and this may be a problem if we ever get to
> implement the plane assignment flexibility that's allowed by the
> hardware. Maybe having a 3rd opinion here would help.

I find the two plane fields in struct intel_plane a bit confusing, but I don't
think the right solution is to leave that in intel_crtc. If I understand
correctly, we have a problem of uniquely identifying a plane, since that takes
different forms in different platforms. Neither of the generically named enums,
enum plane and enum plane_id, is sufficiently generic.

Would it make sense to turn things up-side down? Now we are trying to make a
crtc be able to find the proper registers for a plane. Instead we could just not
do it and define everything in terms of functions that take planes. Then we
could hide the details of mapping some bit of information in intel_plane to the
appropriate registers in platform-dependent vfuncs. And perhaps give those enums
a platform-dependent name, i.e., enum i8xx_plane_id, enum skl_plane_id, etc.

Anyway, I haven't looked much into it, so just ignore me if I'm way off.

Ander
 


> > 
> > 
> > > 
> > > 
> > > Besides, I
> > > think our code organizations should always better fit the new
> > > hardware,
> > > since otherwise we'll make i915.ko development even harder than it
> > > is
> > > for new contributors/employees.
> > If you don't mix planes and pipes there can't be any confusion.
> > 
> > > 
> > > 
> > > 
> > > (And when I see those specs and realize how different the HW was,
> > > then
> > > I remember that in order to explain to the new people why some
> > > things
> > > are the way they are in our code I have to explain how the HW was
> > > 10
> > > years ago, I start thinking again that maybe we should just have
> > > i915-
> > > old.ko and i915-new.ko, since either we make our
> > > abstractions/design
> > > fit one thing or we make it fit another...)
> > From the register POV hw was almost identical up to BDW at least.
> > It's
> > just a few bits (the pipe selector) that vanished from the registers.
> > SKL+ is a little more different but still the registers even live in
> > the
> > same address.
> > 
> > Also given what recent history has shown us, I wouldn't be at all
> > surprised if we would go back to a flexible plane<->pipe assignment
> > in the hardware at some point in the future. A lot of the other
> > features that were present in old hardware has been making a comeback
> > in recent years.
> > 
> > > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > The crtc struct already includes
> > > > > some fields that are specific to primary planes, so I think
> > > > > it's a
> > > > > good
> > > > > place. Or we could create a new class: struct
> > > > > intel_primary_plane {
> > > > > struct intel_plane base; enum plane legacy_plane };
> > > > > 
> > > > > Following is a patch suggestion (probably impossible to apply
> > > > > due
> > > > > to my
> > > > > editor breaking long lines). Any arguments against it?
> > > > > 
> > > > > --8<----------------------------------------------------------
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index bc1af87..c54b1a7 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > >  		state->scaler_id = -1;
> > > > >  	}
> > > > >  	primary->pipe = pipe;
> > > > > -	/*
> > > > > -	 * On gen2/3 only plane A can do FBC, but the panel
> > > > > fitter
> > > > > and
> > > > > LVDS
> > > > > -	 * port is hooked to pipe B. Hence we want plane A
> > > > > feeding
> > > > > pipe B.
> > > > > -	 */
> > > > > -	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > > > -		primary->plane = (enum plane) !pipe;
> > > > > -	else
> > > > > -		primary->plane = (enum plane) pipe;
> > > > >  	primary->id = PLANE_PRIMARY;
> > > > >  	primary->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > > >  	primary->check_plane = intel_check_primary_plane;
> > > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > >  					       0,
> > > > > &intel_plane_funcs,
> > > > >  					       intel_primary_f
> > > > > orma
> > > > > ts,
> > > > > num_formats,
> > > > >  					       DRM_PLANE_TYPE_
> > > > > PRIM
> > > > > ARY,
> > > > > -					       "plane %c",
> > > > > plane_name(primary->plane));
> > > > > +					       "plane %c",
> > > > > pipe_name(pipe)); /* FIXME */
> > > > >  	if (ret)
> > > > >  		goto fail;
> > > > >  
> > > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > >  	cursor->can_scale = false;
> > > > >  	cursor->max_downscale = 1;
> > > > >  	cursor->pipe = pipe;
> > > > > -	cursor->plane = pipe;
> > > > >  	cursor->id = PLANE_CURSOR;
> > > > >  	cursor->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > >  		goto fail;
> > > > >  
> > > > >  	intel_crtc->pipe = pipe;
> > > > > -	intel_crtc->plane = primary->plane;
> > > > > +	/*
> > > > > +	 * On gen2/3 only plane A can do FBC, but the panel
> > > > > fitter
> > > > > and
> > > > > LVDS
> > > > > +	 * port is hooked to pipe B. Hence we want plane A
> > > > > feeding
> > > > > pipe B.
> > > > > +	 */
> > > > > +	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > > > +		intel_crtc->plane = (enum plane) !pipe;
> > > > > +	else
> > > > > +		intel_crtc->plane = (enum plane) pipe;
> > > > >  
> > > > >  	intel_crtc->cursor_base = ~0;
> > > > >  	intel_crtc->cursor_cntl = ~0;
> > > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
> > > > >  }
> > > > >  
> > > > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > > > +static bool primary_get_hw_state(struct intel_crtc *crtc)
> > > > >  {
> > > > > -	struct drm_i915_private *dev_priv = to_i915(plane-
> > > > > > 
> > > > > > 
> > > > > > base.dev);
> > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > > > 
> > > > > > 
> > > > > > base.dev);
> > > > >  
> > > > > -	return I915_READ(DSPCNTR(plane->plane)) &
> > > > > DISPLAY_PLANE_ENABLE;
> > > > > +	return I915_READ(DSPCNTR(crtc->plane)) &
> > > > > DISPLAY_PLANE_ENABLE;
> > > > >  }
> > > > >  
> > > > >  /* FIXME read out full plane state for all planes */
> > > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> > > > > intel_crtc *crtc)
> > > > >  	struct intel_plane_state *plane_state =
> > > > >  		to_intel_plane_state(primary->state);
> > > > >  
> > > > > -	plane_state->base.visible = crtc->active &&
> > > > > -		primary_get_hw_state(to_intel_plane(primary));
> > > > > +	plane_state->base.visible = crtc->active &&
> > > > > primary_get_hw_state(crtc);
> > > > >  
> > > > >  	if (plane_state->base.visible)
> > > > >  		crtc->base.state->plane_mask |= 1 <<
> > > > > drm_plane_index(primary);
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 362f698..b2bdd49 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
> > > > >  
> > > > >  struct intel_plane {
> > > > >  	struct drm_plane base;
> > > > > -	u8 plane;
> > > > >  	enum plane_id id;
> > > > >  	enum pipe pipe;
> > > > >  	bool can_scale;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index 63154a5..1044095 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > >  	}
> > > > >  
> > > > >  	intel_plane->pipe = pipe;
> > > > > -	intel_plane->plane = plane;
> > > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > > >  	intel_plane->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_SPRITE(pipe,
> > > > > plane);
> > > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > > > > 
> > > > > --8<----------------------------------------------------------
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.c | 1 -
> > > > > >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> > > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
> > > > > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index b6d5d5e5cc99..f180f14fcf3a 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > > >  	cursor->can_scale = false;
> > > > > >  	cursor->max_downscale = 1;
> > > > > >  	cursor->pipe = pipe;
> > > > > > -	cursor->plane = pipe;
> > > > > >  	cursor->id = PLANE_CURSOR;
> > > > > >  	cursor->frontbuffer_bit =
> > > > > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index c1b245853ba9..d14718e09911 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> > > > > >  
> > > > > >  struct intel_plane {
> > > > > >  	struct drm_plane base;
> > > > > > -	u8 plane;
> > > > > > +	enum plane plane;
> > > > > >  	enum plane_id id;
> > > > > >  	enum pipe pipe;
> > > > > >  	bool can_scale;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > index 63154a5a9305..1044095d0084 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  	}
> > > > > >  
> > > > > >  	intel_plane->pipe = pipe;
> > > > > > -	intel_plane->plane = plane;
> > > > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > > > >  	intel_plane->frontbuffer_bit =
> > > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> > > > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-15 19:59         ` Ville Syrjälä
  2016-12-16 14:07           ` Paulo Zanoni
@ 2016-12-19 18:24           ` Matt Roper
  2016-12-19 18:55             ` Ville Syrjälä
  1 sibling, 1 reply; 22+ messages in thread
From: Matt Roper @ 2016-12-19 18:24 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni

On Thu, Dec 15, 2016 at 09:59:51PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote:
> > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> > > > 
> > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
> > > > escreveu:
> > > > > 
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > With plane->plane now purely reserved for the primary planes,
> > > > > let's
> > > > > not even populate it for cursors and sprites. Let's switch the
> > > > > type
> > > > > to enum plane as well since it's no longer being abused for
> > > > > anything
> > > > > else.
> > > > 
> > > > Since it's a primary plane thing, I think it makes more sense to
> > > > just
> > > > leave it at struct intel_crtc instead of having a field that's
> > > > unused
> > > > and doesn't make sense in some cases.
> > > 
> > > Primary plane aren't really primary planes on old platforms though.
> > > That's just a software concept that has no bearing on the hardware.
> > 
> > Please explain more since that's not my understanding. I just opened
> > the gen 2 and 3 spec docs and they do contain tons of "primary plane"
> > references. I know that the hardware is a little more flexible there,
> 
> Yep. Planes can be moved to any pipe in general.
> 
> > but as far as I understand we don't even implement that. 
> 
> Not at the moment. But I have plans...

I'm not even sure where to find bspecs for the really old
platforms...how flexible were those primary planes?  Can we drive both
"primary" planes at the same time on a single CRTC if we're not using
the second pipe?  If not, is there some other benefit we get from not
enforcing a fixed pipe-plane association?


Matt

> 
> > Besides, both
> > our driver and our API do have the concept of a primary plane, so it
> > makes sense for the code to be organized that way, IMHO.
> 
> A plane is a plane, a pipe is a pipe. IMO you're trying to make it
> more confusing by mixing up the two.
> 
> > Besides, I
> > think our code organizations should always better fit the new hardware,
> > since otherwise we'll make i915.ko development even harder than it is
> > for new contributors/employees.
> 
> If you don't mix planes and pipes there can't be any confusion.
> 
> > 
> > (And when I see those specs and realize how different the HW was, then
> > I remember that in order to explain to the new people why some things
> > are the way they are in our code I have to explain how the HW was 10
> > years ago, I start thinking again that maybe we should just have i915-
> > old.ko and i915-new.ko, since either we make our abstractions/design
> > fit one thing or we make it fit another...)
> 
> From the register POV hw was almost identical up to BDW at least. It's
> just a few bits (the pipe selector) that vanished from the registers.
> SKL+ is a little more different but still the registers even live in the
> same address.
> 
> Also given what recent history has shown us, I wouldn't be at all
> surprised if we would go back to a flexible plane<->pipe assignment
> in the hardware at some point in the future. A lot of the other
> features that were present in old hardware has been making a comeback
> in recent years.
> 
> > 
> > > 
> > > > 
> > > > The crtc struct already includes
> > > > some fields that are specific to primary planes, so I think it's a
> > > > good
> > > > place. Or we could create a new class: struct intel_primary_plane {
> > > > struct intel_plane base; enum plane legacy_plane };
> > > > 
> > > > Following is a patch suggestion (probably impossible to apply due
> > > > to my
> > > > editor breaking long lines). Any arguments against it?
> > > > 
> > > > --8<----------------------------------------------------------
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index bc1af87..c54b1a7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  		state->scaler_id = -1;
> > > >  	}
> > > >  	primary->pipe = pipe;
> > > > -	/*
> > > > -	 * On gen2/3 only plane A can do FBC, but the panel fitter
> > > > and
> > > > LVDS
> > > > -	 * port is hooked to pipe B. Hence we want plane A feeding
> > > > pipe B.
> > > > -	 */
> > > > -	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > > -		primary->plane = (enum plane) !pipe;
> > > > -	else
> > > > -		primary->plane = (enum plane) pipe;
> > > >  	primary->id = PLANE_PRIMARY;
> > > >  	primary->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > >  	primary->check_plane = intel_check_primary_plane;
> > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  					       0,
> > > > &intel_plane_funcs,
> > > >  					       intel_primary_forma
> > > > ts,
> > > > num_formats,
> > > >  					       DRM_PLANE_TYPE_PRIM
> > > > ARY,
> > > > -					       "plane %c",
> > > > plane_name(primary->plane));
> > > > +					       "plane %c",
> > > > pipe_name(pipe)); /* FIXME */
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  	cursor->can_scale = false;
> > > >  	cursor->max_downscale = 1;
> > > >  	cursor->pipe = pipe;
> > > > -	cursor->plane = pipe;
> > > >  	cursor->id = PLANE_CURSOR;
> > > >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > >  		goto fail;
> > > >  
> > > >  	intel_crtc->pipe = pipe;
> > > > -	intel_crtc->plane = primary->plane;
> > > > +	/*
> > > > +	 * On gen2/3 only plane A can do FBC, but the panel fitter
> > > > and
> > > > LVDS
> > > > +	 * port is hooked to pipe B. Hence we want plane A feeding
> > > > pipe B.
> > > > +	 */
> > > > +	if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > > > +		intel_crtc->plane = (enum plane) !pipe;
> > > > +	else
> > > > +		intel_crtc->plane = (enum plane) pipe;
> > > >  
> > > >  	intel_crtc->cursor_base = ~0;
> > > >  	intel_crtc->cursor_cntl = ~0;
> > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> > > > drm_i915_private *dev_priv)
> > > >  	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
> > > >  }
> > > >  
> > > > -static bool primary_get_hw_state(struct intel_plane *plane)
> > > > +static bool primary_get_hw_state(struct intel_crtc *crtc)
> > > >  {
> > > > -	struct drm_i915_private *dev_priv = to_i915(plane-
> > > > >base.dev);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc-
> > > > >base.dev);
> > > >  
> > > > -	return I915_READ(DSPCNTR(plane->plane)) &
> > > > DISPLAY_PLANE_ENABLE;
> > > > +	return I915_READ(DSPCNTR(crtc->plane)) &
> > > > DISPLAY_PLANE_ENABLE;
> > > >  }
> > > >  
> > > >  /* FIXME read out full plane state for all planes */
> > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> > > > intel_crtc *crtc)
> > > >  	struct intel_plane_state *plane_state =
> > > >  		to_intel_plane_state(primary->state);
> > > >  
> > > > -	plane_state->base.visible = crtc->active &&
> > > > -		primary_get_hw_state(to_intel_plane(primary));
> > > > +	plane_state->base.visible = crtc->active &&
> > > > primary_get_hw_state(crtc);
> > > >  
> > > >  	if (plane_state->base.visible)
> > > >  		crtc->base.state->plane_mask |= 1 <<
> > > > drm_plane_index(primary);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 362f698..b2bdd49 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
> > > >  
> > > >  struct intel_plane {
> > > >  	struct drm_plane base;
> > > > -	u8 plane;
> > > >  	enum plane_id id;
> > > >  	enum pipe pipe;
> > > >  	bool can_scale;
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 63154a5..1044095 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > >  	}
> > > >  
> > > >  	intel_plane->pipe = pipe;
> > > > -	intel_plane->plane = plane;
> > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > >  	intel_plane->frontbuffer_bit =
> > > > INTEL_FRONTBUFFER_SPRITE(pipe,
> > > > plane);
> > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > > > 
> > > > --8<----------------------------------------------------------
> > > > 
> > > > > 
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 1 -
> > > > >  drivers/gpu/drm/i915/intel_drv.h     | 2 +-
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  | 1 -
> > > > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index b6d5d5e5cc99..f180f14fcf3a 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > > > > drm_i915_private *dev_priv, enum pipe pipe)
> > > > >  	cursor->can_scale = false;
> > > > >  	cursor->max_downscale = 1;
> > > > >  	cursor->pipe = pipe;
> > > > > -	cursor->plane = pipe;
> > > > >  	cursor->id = PLANE_CURSOR;
> > > > >  	cursor->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_CURSOR(pipe);
> > > > >  	cursor->check_plane = intel_check_cursor_plane;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index c1b245853ba9..d14718e09911 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> > > > >  
> > > > >  struct intel_plane {
> > > > >  	struct drm_plane base;
> > > > > -	u8 plane;
> > > > > +	enum plane plane;
> > > > >  	enum plane_id id;
> > > > >  	enum pipe pipe;
> > > > >  	bool can_scale;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > index 63154a5a9305..1044095d0084 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	}
> > > > >  
> > > > >  	intel_plane->pipe = pipe;
> > > > > -	intel_plane->plane = plane;
> > > > >  	intel_plane->id = PLANE_SPRITE0 + plane;
> > > > >  	intel_plane->frontbuffer_bit =
> > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> > > > >  	intel_plane->check_plane = intel_check_sprite_plane;
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

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

* Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
  2016-12-19 18:24           ` Matt Roper
@ 2016-12-19 18:55             ` Ville Syrjälä
  0 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2016-12-19 18:55 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, Paulo Zanoni

On Mon, Dec 19, 2016 at 10:24:15AM -0800, Matt Roper wrote:
> On Thu, Dec 15, 2016 at 09:59:51PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote:
> > > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> > > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> > > > > 
> > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com
> > > > > escreveu:
> > > > > > 
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > With plane->plane now purely reserved for the primary planes,
> > > > > > let's
> > > > > > not even populate it for cursors and sprites. Let's switch the
> > > > > > type
> > > > > > to enum plane as well since it's no longer being abused for
> > > > > > anything
> > > > > > else.
> > > > > 
> > > > > Since it's a primary plane thing, I think it makes more sense to
> > > > > just
> > > > > leave it at struct intel_crtc instead of having a field that's
> > > > > unused
> > > > > and doesn't make sense in some cases.
> > > > 
> > > > Primary plane aren't really primary planes on old platforms though.
> > > > That's just a software concept that has no bearing on the hardware.
> > > 
> > > Please explain more since that's not my understanding. I just opened
> > > the gen 2 and 3 spec docs and they do contain tons of "primary plane"
> > > references. I know that the hardware is a little more flexible there,
> > 
> > Yep. Planes can be moved to any pipe in general.
> > 
> > > but as far as I understand we don't even implement that. 
> > 
> > Not at the moment. But I have plans...
> 
> I'm not even sure where to find bspecs for the really old
> platforms...how flexible were those primary planes?  Can we drive both
> "primary" planes at the same time on a single CRTC if we're not using
> the second pipe? 

There are three "primary" planes, two cursors, and one video overlay.
Any one can be assigned to either pipe more or less freely (some
restrctions do apply). The planes aren't uniform (not even the
"primary" planes) and instead have different capabilities between them.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2016-12-19 18:55 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-22 16:01 [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) ville.syrjala
2016-11-22 16:01 ` [PATCH 1/9] drm/i915: Add per-pipe plane identifier ville.syrjala
2016-11-22 16:01 ` [PATCH 2/9] drm/i915: Add crtc->plane_ids_mask ville.syrjala
2016-11-22 16:01 ` [PATCH 3/9] drm/i915: Use enum plane_id in SKL wm code ville.syrjala
2016-11-22 20:27   ` Lyude Paul
2016-11-22 16:01 ` [PATCH 4/9] drm/i915: Use enum plane_id in SKL plane code ville.syrjala
2016-11-22 16:02 ` [PATCH 5/9] drm/i915: Use enum plane_id in VLV/CHV sprite code ville.syrjala
2016-11-22 16:02 ` [PATCH 6/9] drm/i915: Use enum plane_id in VLV/CHV wm code ville.syrjala
2016-11-22 16:02 ` [PATCH 7/9] drm/i915: s/plane/plane_id/ in skl+ plane register defines ville.syrjala
2016-12-15 18:33   ` Paulo Zanoni
2016-11-22 16:02 ` [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites ville.syrjala
2016-12-15 19:05   ` Paulo Zanoni
2016-12-15 19:11     ` Ville Syrjälä
2016-12-15 19:50       ` Paulo Zanoni
2016-12-15 19:59         ` Ville Syrjälä
2016-12-16 14:07           ` Paulo Zanoni
2016-12-16 14:56             ` Ander Conselvan De Oliveira
2016-12-19 18:24           ` Matt Roper
2016-12-19 18:55             ` Ville Syrjälä
2016-11-22 16:02 ` [PATCH 9/9] drm/i915: Rename the local 'plane' variable to 'plane_id' in primary plane code ville.syrjala
2016-11-22 16:45 ` ✓ Fi.CI.BAT: success for drm/i915: Add a per-pipe plane identifier enum (rev5) Patchwork
2016-11-23 20:16 ` [PATCH v2 0/9] drm/i915: Add a per-pipe plane identifier enum (v2) Ville Syrjälä

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