[PATCH 00/15] drm/i915/gen11: Implement planar format support.

[PATCH 00/15] drm/i915/gen11: Implement planar format support.

[Maarten Lankhorst]

Gen11 has to program 2 planes to enable planar formats. One scans out
the Y plane, the other the UV plane.

The UV plane is the plane that matters for Z-order and performs color
correction etc. The Y plane is always sprite 4 or 5, and only feeds Y
pixels directly to the chroma upsampler on the UV plane, which will
convert it to YUV444 that will be processed further without scaler.
If there's no chroma upsampler on a plane, it will be done by a scaler,
which needs to know the Y plane to take pixels from.

Maarten Lankhorst (15):
  drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
  drm/i915: Handle cursor updating active_planes correctly.
  drm/i915: Make intel_crtc_disable_planes() use active planes mask.
  drm/i915: Replace call to commit_planes_on_crtc with internal update,
    v2.
  drm/i915: Clean up scaler setup.
  drm/i915: Force NV12 coordinates to be a multiple of 2
  drm/i915: Unconditionally clear plane_state->visible flag
  drm/i915/gen11: Enable 6 sprites on gen11
  drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v2.
  drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.
  drm/i915: Move programming plane scaler to its own function.
  drm/i915/gen11: Program the scalers correctly for planar formats.
  drm/i915/gen11: Program the chroma upsampler for HDR planes.
  drm/i915/gen11: Program the Y and UV plane for planar mode correctly.
  drm/i915/gen11: Expose planar format support on gen11.

 drivers/gpu/drm/i915/i915_reg.h           |  32 +++-
 drivers/gpu/drm/i915/intel_atomic.c       | 112 +++++++-----
 drivers/gpu/drm/i915/intel_atomic_plane.c | 155 ++++++++++++-----
 drivers/gpu/drm/i915/intel_device_info.c  |   5 +-
 drivers/gpu/drm/i915/intel_display.c      | 200 ++++++++++++++++------
 drivers/gpu/drm/i915/intel_display.h      |   3 +
 drivers/gpu/drm/i915/intel_drv.h          |  65 +++++++
 drivers/gpu/drm/i915/intel_pm.c           | 134 +++++++++------
 drivers/gpu/drm/i915/intel_sprite.c       | 161 +++++++++++------
 9 files changed, 618 insertions(+), 249 deletions(-)

-- 
2.18.0

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

[PATCH 01/15] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()

[Maarten Lankhorst]

Use old/new_intel_crtc_state, and get rid of all the conversion casts
where they don't add anything.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eb25037d7b38..2ac381eb8103 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12665,8 +12665,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
 	struct drm_crtc *crtc;
-	struct intel_crtc_state *intel_cstate;
+	struct intel_crtc *intel_crtc;
 	u64 put_domains[I915_MAX_PIPES] = {};
 	int i;
 
@@ -12678,21 +12679,22 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+		intel_crtc = to_intel_crtc(crtc);
 
 		if (needs_modeset(new_crtc_state) ||
 		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
 
-			put_domains[to_intel_crtc(crtc)->pipe] =
+			put_domains[intel_crtc->pipe] =
 				modeset_get_crtc_power_domains(crtc,
-					to_intel_crtc_state(new_crtc_state));
+					new_intel_crtc_state);
 		}
 
 		if (!needs_modeset(new_crtc_state))
 			continue;
 
-		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
-				       to_intel_crtc_state(new_crtc_state));
+		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
 
 		if (old_crtc_state->active) {
 			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
@@ -12703,7 +12705,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 			 */
 			intel_crtc_disable_pipe_crc(intel_crtc);
 
-			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
+			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
 			intel_crtc->active = false;
 			intel_fbc_disable(intel_crtc);
 			intel_disable_shared_dpll(intel_crtc);
@@ -12724,7 +12726,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 				 */
 				if (INTEL_GEN(dev_priv) >= 9)
 					dev_priv->display.initial_watermarks(intel_state,
-									     to_intel_crtc_state(new_crtc_state));
+									     new_intel_crtc_state);
 			}
 		}
 	}
@@ -12784,11 +12786,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	 * TODO: Move this (and other cleanup) to an async worker eventually.
 	 */
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-		intel_cstate = to_intel_crtc_state(new_crtc_state);
+		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
 
 		if (dev_priv->display.optimize_watermarks)
 			dev_priv->display.optimize_watermarks(intel_state,
-							      intel_cstate);
+							      new_intel_crtc_state);
 	}
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-- 
2.18.0

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

[PATCH 02/15] drm/i915: Handle cursor updating active_planes correctly.

[Maarten Lankhorst]

While we may not update new_crtc_state, we may clear active_planes
if the new cursor update state will disable the cursor, but we fail
after. If this is immediately followed by a modeset disable, we may
soon not disable the planes correctly when we start depending on
active_planes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ac381eb8103..7a7ff1d76031 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13517,14 +13517,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *old_fb;
-	struct drm_crtc_state *crtc_state = crtc->state;
+	struct intel_crtc_state *crtc_state =
+		to_intel_crtc_state(crtc->state);
+	struct intel_crtc_state *new_crtc_state;
 
 	/*
 	 * When crtc is inactive or there is a modeset pending,
 	 * wait for it to complete in the slowpath
 	 */
-	if (!crtc_state->active || needs_modeset(crtc_state) ||
-	    to_intel_crtc_state(crtc_state)->update_pipe)
+	if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
+	    crtc_state->update_pipe)
 		goto slow;
 
 	old_plane_state = plane->state;
@@ -13554,6 +13556,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	if (!new_plane_state)
 		return -ENOMEM;
 
+	new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
+	if (!new_crtc_state) {
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
 	drm_atomic_set_fb_for_plane(new_plane_state, fb);
 
 	new_plane_state->src_x = src_x;
@@ -13565,9 +13573,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	new_plane_state->crtc_w = crtc_w;
 	new_plane_state->crtc_h = crtc_h;
 
-	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
-						  to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
-						  to_intel_plane_state(plane->state),
+	ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
+						  to_intel_plane_state(old_plane_state),
 						  to_intel_plane_state(new_plane_state));
 	if (ret)
 		goto out_free;
@@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 
 	/* Swap plane state */
 	plane->state = new_plane_state;
+	crtc_state->active_planes = new_crtc_state->active_planes;
 
 	if (plane->state->visible) {
 		trace_intel_update_plane(plane, to_intel_crtc(crtc));
-		intel_plane->update_plane(intel_plane,
-					  to_intel_crtc_state(crtc->state),
+		intel_plane->update_plane(intel_plane, crtc_state,
 					  to_intel_plane_state(plane->state));
 	} else {
 		trace_intel_disable_plane(plane, to_intel_crtc(crtc));
@@ -13604,6 +13611,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 out_unlock:
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 out_free:
+	if (new_crtc_state)
+		intel_crtc_destroy_state(crtc, &new_crtc_state->base);
 	if (ret)
 		intel_plane_destroy_state(plane, new_plane_state);
 	else
-- 
2.18.0

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

[PATCH 03/15] drm/i915: Make intel_crtc_disable_planes() use active planes mask.

[Maarten Lankhorst]

This will only disable planes we actually had marked as visible in
crtc_state->visible_planes and cleans up intel_crtc_disable_plane()
slightly.

This is also useful for when we start enabling NV12 support, in which
we will make the separate Y plane visible, but ignore the Y plane's state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7a7ff1d76031..f7982f18b8fe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5402,24 +5402,23 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
 		intel_update_watermarks(crtc);
 }
 
-static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
+static void intel_crtc_disable_planes(struct intel_crtc *crtc, unsigned plane_mask)
 {
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_plane *p;
-	int pipe = intel_crtc->pipe;
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_plane *plane;
+	unsigned fb_bits = 0;
 
-	intel_crtc_dpms_overlay_disable(intel_crtc);
+	intel_crtc_dpms_overlay_disable(crtc);
 
-	drm_for_each_plane_mask(p, dev, plane_mask)
-		to_intel_plane(p)->disable_plane(to_intel_plane(p), intel_crtc);
+	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		if (plane_mask & BIT(plane->id)) {
+			plane->disable_plane(plane, crtc);
 
-	/*
-	 * FIXME: Once we grow proper nuclear flip support out of this we need
-	 * to compute the mask of flip planes precisely. For the time being
-	 * consider this a flip to a NULL plane.
-	 */
-	intel_frontbuffer_flip(to_i915(dev), INTEL_FRONTBUFFER_ALL_MASK(pipe));
+			fb_bits |= plane->frontbuffer_bit;
+		}
+	}
+
+	intel_frontbuffer_flip(to_i915(dev), fb_bits);
 }
 
 static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
@@ -12697,7 +12696,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
 
 		if (old_crtc_state->active) {
-			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+			intel_crtc_disable_planes(intel_crtc, old_intel_crtc_state->active_planes);
 
 			/*
 			 * We need to disable pipe CRC before disabling the pipe,
-- 
2.18.0

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

[PATCH 04/15] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.

[Maarten Lankhorst]

drm_atomic_helper_commit_planes_on_crtc calls begin_commit,
then plane_update hooks, then flush_commit. Because we keep our own
visibility tracking through plane_state->visible there's no need to
rely on the atomic hooks for this.

By explicitly writing our own helper, we can update visible planes
as needed, which is useful to make NV12 support work as intended.

Changes since v1:
- Reword commit message. (Matt Roper)
- Rename to intel_update_planes_on_crtc(). (Matt)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 41 ++++++++++++-----------
 drivers/gpu/drm/i915/intel_display.c      | 10 ++++--
 drivers/gpu/drm/i915/intel_drv.h          |  4 +++
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index aabebe0d2e9b..e067fb531a29 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -164,29 +164,33 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 						   to_intel_plane_state(new_plane_state));
 }
 
-static void intel_plane_atomic_update(struct drm_plane *plane,
-				      struct drm_plane_state *old_state)
+void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
+				 struct intel_crtc *crtc,
+				 struct intel_crtc_state *old_crtc_state,
+				 struct intel_crtc_state *new_crtc_state)
 {
-	struct intel_atomic_state *state = to_intel_atomic_state(old_state->state);
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	const struct intel_plane_state *new_plane_state =
-		intel_atomic_get_new_plane_state(state, intel_plane);
-	struct drm_crtc *crtc = new_plane_state->base.crtc ?: old_state->crtc;
+	struct intel_plane_state *new_plane_state;
+	struct intel_plane *plane;
+	u32 update_mask;
+	int i;
+
+	update_mask = old_crtc_state->active_planes;
+	update_mask |= new_crtc_state->active_planes;
 
-	if (new_plane_state->base.visible) {
-		const struct intel_crtc_state *new_crtc_state =
-			intel_atomic_get_new_crtc_state(state, to_intel_crtc(crtc));
+	for_each_new_intel_plane_in_state(old_state, plane, new_plane_state, i) {
+		if (crtc->pipe != plane->pipe ||
+		    !(update_mask & BIT(plane->id)))
+			continue;
 
-		trace_intel_update_plane(plane,
-					 to_intel_crtc(crtc));
+		if (new_plane_state->base.visible) {
+			trace_intel_update_plane(&plane->base, crtc);
 
-		intel_plane->update_plane(intel_plane,
-					  new_crtc_state, new_plane_state);
-	} else {
-		trace_intel_disable_plane(plane,
-					  to_intel_crtc(crtc));
+			plane->update_plane(plane, new_crtc_state, new_plane_state);
+		} else {
+			trace_intel_disable_plane(&plane->base, crtc);
 
-		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
+			plane->disable_plane(plane, crtc);
+		}
 	}
 }
 
@@ -194,7 +198,6 @@ const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
 	.prepare_fb = intel_prepare_plane_fb,
 	.cleanup_fb = intel_cleanup_plane_fb,
 	.atomic_check = intel_plane_atomic_check,
-	.atomic_update = intel_plane_atomic_update,
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f7982f18b8fe..3975f3af4fb4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10813,8 +10813,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs intel_helper_funcs = {
-	.atomic_begin = intel_begin_crtc_commit,
-	.atomic_flush = intel_finish_crtc_commit,
 	.atomic_check = intel_crtc_atomic_check,
 };
 
@@ -12483,6 +12481,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *old_intel_cstate = to_intel_crtc_state(old_crtc_state);
 	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
 	bool modeset = needs_modeset(new_crtc_state);
 	struct intel_plane_state *new_plane_state =
@@ -12503,7 +12502,12 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	if (new_plane_state)
 		intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
 
-	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
+	intel_begin_crtc_commit(crtc, old_crtc_state);
+
+	intel_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc,
+				    old_intel_cstate, pipe_config);
+
+	intel_finish_crtc_commit(crtc, old_crtc_state);
 }
 
 static void intel_update_crtcs(struct drm_atomic_state *state)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf1c38728a59..8073a85d7178 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2188,6 +2188,10 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
 void intel_plane_destroy_state(struct drm_plane *plane,
 			       struct drm_plane_state *state);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
+				 struct intel_crtc *crtc,
+				 struct intel_crtc_state *old_crtc_state,
+				 struct intel_crtc_state *new_crtc_state);
 int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
 					struct intel_crtc_state *crtc_state,
 					const struct intel_plane_state *old_plane_state,
-- 
2.18.0

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

[PATCH 05/15] drm/i915: Clean up scaler setup.

[Maarten Lankhorst]

On skylake we can switch to a high quality scaler mode when only 1 out
of 2 scalers are used, but on GLK and later bit 28 has a different
meaning. Don't set it, and make clear the distinction between
SKL and later PS values.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   7 +-
 drivers/gpu/drm/i915/intel_atomic.c  | 108 +++++++++++++++------------
 drivers/gpu/drm/i915/intel_display.c |   2 +-
 3 files changed, 66 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4948b352bf4c..e7e6ca7f9665 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6850,11 +6850,12 @@ enum {
 #define _PS_2B_CTRL      0x68A80
 #define _PS_1C_CTRL      0x69180
 #define PS_SCALER_EN        (1 << 31)
-#define PS_SCALER_MODE_MASK (3 << 28)
-#define PS_SCALER_MODE_DYN  (0 << 28)
-#define PS_SCALER_MODE_HQ  (1 << 28)
+#define SKL_PS_SCALER_MODE_MASK (3 << 28)
+#define SKL_PS_SCALER_MODE_DYN  (0 << 28)
+#define SKL_PS_SCALER_MODE_HQ  (1 << 28)
 #define SKL_PS_SCALER_MODE_NV12 (2 << 28)
 #define PS_SCALER_MODE_PLANAR (1 << 29)
+#define PS_SCALER_MODE_PACKED (0 << 29)
 #define PS_PLANE_SEL_MASK  (7 << 25)
 #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
 #define PS_FILTER_MASK         (3 << 23)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index b04952bacf77..40fba3f22f87 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -203,6 +203,62 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
 	drm_atomic_helper_crtc_destroy_state(crtc, state);
 }
 
+static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
+				      int num_scalers_need, struct intel_crtc *intel_crtc,
+				      const char *name, int idx,
+				      struct intel_plane_state *plane_state,
+				      int *scaler_id)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	int j;
+	u32 mode;
+
+	if (*scaler_id < 0) {
+		/* find a free scaler */
+		for (j = 0; j < intel_crtc->num_scalers; j++) {
+			if (scaler_state->scalers[j].in_use)
+				continue;
+
+			*scaler_id = j;
+			scaler_state->scalers[*scaler_id].in_use = 1;
+		}
+	}
+
+	if (WARN(*scaler_id < 0, "Cannot find scaler for %s:%d\n", name, idx))
+		return;
+
+	/* set scaler mode */
+	if (plane_state && plane_state->base.fb &&
+	    plane_state->base.fb->format->is_yuv &&
+	    plane_state->base.fb->format->num_planes > 1) {
+		if (INTEL_GEN(dev_priv) == 9 &&
+		    !IS_GEMINILAKE(dev_priv))
+			mode = SKL_PS_SCALER_MODE_NV12;
+		else
+			mode = PS_SCALER_MODE_PLANAR;
+
+	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
+		mode = PS_SCALER_MODE_PACKED;
+	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
+		/*
+		 * when only 1 scaler is in use on a pipe with 2 scalers
+		 * scaler 0 operates in high quality (HQ) mode.
+		 * In this case use scaler 0 to take advantage of HQ mode
+		 */
+		scaler_state->scalers[*scaler_id].in_use = 0;
+		*scaler_id = 0;
+		scaler_state->scalers[0].in_use = 1;
+		mode = SKL_PS_SCALER_MODE_HQ;
+	} else {
+		mode = SKL_PS_SCALER_MODE_DYN;
+	}
+
+	DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
+		      intel_crtc->pipe, *scaler_id, name, idx);
+	scaler_state->scalers[*scaler_id].mode = mode;
+}
+
+
 /**
  * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
  * @dev_priv: i915 device
@@ -232,7 +288,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 	struct drm_atomic_state *drm_state = crtc_state->base.state;
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
 	int num_scalers_need;
-	int i, j;
+	int i;
 
 	num_scalers_need = hweight32(scaler_state->scaler_users);
 
@@ -304,59 +360,17 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 			idx = plane->base.id;
 
 			/* plane on different crtc cannot be a scaler user of this crtc */
-			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
+			if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
 				continue;
-			}
 
 			plane_state = intel_atomic_get_new_plane_state(intel_state,
 								       intel_plane);
 			scaler_id = &plane_state->scaler_id;
 		}
 
-		if (*scaler_id < 0) {
-			/* find a free scaler */
-			for (j = 0; j < intel_crtc->num_scalers; j++) {
-				if (!scaler_state->scalers[j].in_use) {
-					scaler_state->scalers[j].in_use = 1;
-					*scaler_id = j;
-					DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
-						intel_crtc->pipe, *scaler_id, name, idx);
-					break;
-				}
-			}
-		}
-
-		if (WARN_ON(*scaler_id < 0)) {
-			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
-			continue;
-		}
-
-		/* set scaler mode */
-		if ((INTEL_GEN(dev_priv) >= 9) &&
-		    plane_state && plane_state->base.fb &&
-		    plane_state->base.fb->format->format ==
-		    DRM_FORMAT_NV12) {
-			if (INTEL_GEN(dev_priv) == 9 &&
-			    !IS_GEMINILAKE(dev_priv) &&
-			    !IS_SKYLAKE(dev_priv))
-				scaler_state->scalers[*scaler_id].mode =
-					SKL_PS_SCALER_MODE_NV12;
-			else
-				scaler_state->scalers[*scaler_id].mode =
-					PS_SCALER_MODE_PLANAR;
-		} else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
-			/*
-			 * when only 1 scaler is in use on either pipe A or B,
-			 * scaler 0 operates in high quality (HQ) mode.
-			 * In this case use scaler 0 to take advantage of HQ mode
-			 */
-			*scaler_id = 0;
-			scaler_state->scalers[0].in_use = 1;
-			scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
-			scaler_state->scalers[1].in_use = 0;
-		} else {
-			scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_DYN;
-		}
+		intel_atomic_setup_scaler(scaler_state, num_scalers_need,
+					  intel_crtc, name, idx,
+					  plane_state, scaler_id);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3975f3af4fb4..5a77d7615fa0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13954,7 +13954,7 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
 		struct intel_scaler *scaler = &scaler_state->scalers[i];
 
 		scaler->in_use = 0;
-		scaler->mode = PS_SCALER_MODE_DYN;
+		scaler->mode = 0;
 	}
 
 	scaler_state->scaler_id = -1;
-- 
2.18.0

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

[PATCH 06/15] drm/i915: Force NV12 coordinates to be a multiple of 2

[Maarten Lankhorst]

We can't make NV12 work any other way. The scaler doesn't handle odd
coordinates well, and we will get visual corruption on the screen.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5a77d7615fa0..122f3f4cfc53 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3072,6 +3072,15 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
 static int
 skl_check_nv12_surface(struct intel_plane_state *plane_state)
 {
+	if ((plane_state->base.src.x1 >> 16) % 2 ||
+	    (plane_state->base.src.y1 >> 16) % 2 ||
+	    (drm_rect_width(&plane_state->base.src) >> 16) % 2 ||
+	    (drm_rect_height(&plane_state->base.src) >> 16) % 2) {
+		DRM_DEBUG_KMS("src coordinates and width/height must be a multiple of 2\n");
+
+		return -EINVAL;
+	}
+
 	/* Display WA #1106 */
 	if (plane_state->base.rotation !=
 	    (DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_90) &&
-- 
2.18.0

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

[PATCH 07/15] drm/i915: Unconditionally clear plane_state->visible flag

[Maarten Lankhorst]

If we ever hit bugs in our readout code we may have set
plane_state->visible without setting a fb/crtc. Our code implicitly
assumes that plane_state->visible is only true when both are set,
so we will either null deref in intel_fbc_choose_crtc() or
do something bad during the actual commit which cares even more.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index e067fb531a29..d754a8ba9ffb 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -117,10 +117,10 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int ret;
 
+	intel_state->base.visible = false;
 	if (!intel_state->base.crtc && !old_plane_state->base.crtc)
 		return 0;
 
-	intel_state->base.visible = false;
 	ret = intel_plane->check_plane(crtc_state, intel_state);
 	if (ret)
 		return ret;
@@ -152,6 +152,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	const struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
 
+	new_plane_state->visible = false;
 	if (!crtc)
 		return 0;
 
-- 
2.18.0

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

[PATCH 08/15] drm/i915/gen11: Enable 6 sprites on gen11

[Maarten Lankhorst]

Gen11 supports 7 planes + 1 cursor on each pipe. Bump
I915_MAX_PLANES to 8, and set num_sprites correctly.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_device_info.c | 5 ++++-
 drivers/gpu/drm/i915/intel_display.h     | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 0ef0c6448d53..7863cf8eb3e6 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -761,7 +761,10 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
 	 * we don't expose the topmost plane at all to prevent ABI breakage
 	 * down the line.
 	 */
-	if (IS_GEN10(dev_priv) || IS_GEMINILAKE(dev_priv))
+	if (IS_GEN11(dev_priv))
+		for_each_pipe(dev_priv, pipe)
+			info->num_sprites[pipe] = 6;
+	else if (IS_GEN10(dev_priv) || IS_GEMINILAKE(dev_priv))
 		for_each_pipe(dev_priv, pipe)
 			info->num_sprites[pipe] = 3;
 	else if (IS_BROXTON(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index ed474da6c200..3f81876bd07c 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -120,6 +120,9 @@ enum plane_id {
 	PLANE_SPRITE0,
 	PLANE_SPRITE1,
 	PLANE_SPRITE2,
+	PLANE_SPRITE3,
+	PLANE_SPRITE4,
+	PLANE_SPRITE5,
 	PLANE_CURSOR,
 
 	I915_MAX_PLANES,
-- 
2.18.0

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

[PATCH 09/15] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v2.

[Maarten Lankhorst]

To make NV12 working on icl, we need to update 2 planes simultaneously.
I've chosen to do this in the CRTC step after plane validation is done,
so we know what planes are (in)visible. The linked Y plane will get
updated in intel_plane_update_planes_on_crtc(), by the call to
update_slave, which gets the master's plane_state as argument.

The link requires both planes for atomic_update to work,
so make sure skl_ddb_add_affected_planes() adds both states.

Changes since v1:
- Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers.
- Put all the state updating login in intel_plane_atomic_check_with_state().
- Clean up changes in intel_plane_atomic_check().

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 107 ++++++++++++++++++----
 drivers/gpu/drm/i915/intel_display.c      |  57 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          |  53 +++++++++++
 drivers/gpu/drm/i915/intel_pm.c           |  12 ++-
 4 files changed, 210 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index d754a8ba9ffb..4f224f71d50e 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -117,7 +117,12 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int ret;
 
+	crtc_state->active_planes &= ~BIT(intel_plane->id);
 	intel_state->base.visible = false;
+	intel_state->linked_plane = NULL;
+	intel_state->slave = false;
+
+	/* If this is a cursor or Y plane, no further checks are needed. */
 	if (!intel_state->base.crtc && !old_plane_state->base.crtc)
 		return 0;
 
@@ -128,8 +133,6 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	/* FIXME pre-g4x don't work like this */
 	if (state->visible)
 		crtc_state->active_planes |= BIT(intel_plane->id);
-	else
-		crtc_state->active_planes &= ~BIT(intel_plane->id);
 
 	if (state->visible && state->fb->format->format == DRM_FORMAT_NV12)
 		crtc_state->nv12_planes |= BIT(intel_plane->id);
@@ -142,27 +145,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 					       state);
 }
 
-static int intel_plane_atomic_check(struct drm_plane *plane,
-				    struct drm_plane_state *new_plane_state)
+static int intel_plane_atomic_check(struct drm_plane *drm_plane,
+				    struct drm_plane_state *new_drm_plane_state)
 {
-	struct drm_atomic_state *state = new_plane_state->state;
-	const struct drm_plane_state *old_plane_state =
-		drm_atomic_get_old_plane_state(state, plane);
-	struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
-	const struct drm_crtc_state *old_crtc_state;
-	struct drm_crtc_state *new_crtc_state;
-
-	new_plane_state->visible = false;
+	struct intel_atomic_state *state =
+		to_intel_atomic_state(new_drm_plane_state->state);
+	struct intel_plane *plane = to_intel_plane(drm_plane);
+	const struct intel_plane_state *old_plane_state =
+		intel_atomic_get_old_plane_state(state, plane);
+	struct intel_plane_state *new_plane_state =
+		to_intel_plane_state(new_drm_plane_state);
+	struct intel_crtc *crtc = to_intel_crtc(
+		new_plane_state->base.crtc ?:
+		old_plane_state->base.crtc);
+	const struct intel_crtc_state *old_crtc_state;
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_plane *linked = old_plane_state->linked_plane;
+	int ret;
+	const struct intel_plane_state *old_linked_state;
+	struct intel_plane_state *new_linked_state = NULL;
+
+	if (linked) {
+		/*
+		* Make sure a previously linked plane (and implicitly, the CRTC)
+		* is part of the atomic commit.
+		*/
+		if (!intel_atomic_get_new_plane_state(state, linked)) {
+			new_linked_state = intel_atomic_get_plane_state(state, linked);
+			if (IS_ERR(new_linked_state))
+				return PTR_ERR(new_linked_state);
+		}
+
+		old_linked_state =
+			intel_atomic_get_old_plane_state(state, linked);
+
+		/*
+		 * This will happen when we're the Y plane. In which case
+		 * old/new_state->crtc are both NULL. We still need to perform
+		 * updates on the linked plane.
+		 */
+		if (!crtc)
+			crtc = to_intel_crtc(old_linked_state->base.crtc);
+
+		WARN_ON(!crtc);
+	}
+
+	new_plane_state->base.visible = false;
 	if (!crtc)
 		return 0;
 
-	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
-	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
+	new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+
+	if (new_linked_state &&
+	    drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) {
+		/*
+		 * This function is called from drm_atomic_helper_check_planes(), which
+		 * will normally check the newly added plane for us, but since we're
+		 * already in that function, it won't check the plane if our index
+		 * is bigger than the linked index because of the
+		 * for_each_oldnew_plane_in_state() call.
+		 */
+		new_crtc_state->base.planes_changed = true;
+		ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state,
+							  old_linked_state, new_linked_state);
+		if (ret)
+			return ret;
+	}
 
-	return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state),
-						   to_intel_crtc_state(new_crtc_state),
-						   to_intel_plane_state(old_plane_state),
-						   to_intel_plane_state(new_plane_state));
+	return intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state,
+						   old_plane_state, new_plane_state);
 }
 
 void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
@@ -187,6 +239,25 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
 			trace_intel_update_plane(&plane->base, crtc);
 
 			plane->update_plane(plane, new_crtc_state, new_plane_state);
+		} else if (new_plane_state->slave) {
+			struct intel_plane *master =
+				new_plane_state->linked_plane;
+
+			/*
+			 * We update the slave plane from this function because
+			 * programming it from the master plane's update_plane
+			 * callback runs into issues when the Y plane is
+			 * reassigned, disabled or used by a different plane.
+			 *
+			 * The slave plane is updated with the master plane's
+			 * plane_state.
+			 */
+			new_plane_state =
+				intel_atomic_get_new_plane_state(old_state, master);
+
+			trace_intel_update_plane(&plane->base, crtc);
+
+			plane->update_slave(plane, new_crtc_state, new_plane_state);
 		} else {
 			trace_intel_disable_plane(&plane->base, crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 122f3f4cfc53..a53953df6c9f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10736,6 +10736,60 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_check_nv12_planes(struct drm_i915_private *dev_priv,
+				 struct intel_crtc *crtc,
+				 struct intel_crtc_state *crtc_state)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state);
+	struct intel_plane *plane, *aux;
+
+	if (!crtc_state->nv12_planes)
+		return 0;
+
+	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
+		struct intel_plane_state *plane_state, *aux_state;
+		struct drm_plane_state *drm_aux_state = NULL;
+
+		if (!(crtc_state->nv12_planes & BIT(plane->id)))
+			continue;
+
+		plane_state = intel_atomic_get_new_plane_state(state, plane);
+		if (!plane_state)
+			continue;
+
+		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, aux) {
+			if (!icl_is_nv12_y_plane(aux->id))
+				continue;
+
+			if (crtc_state->active_planes & BIT(aux->id))
+				continue;
+
+			drm_aux_state = drm_atomic_get_plane_state(&state->base, &aux->base);
+			if (IS_ERR(drm_aux_state))
+				return PTR_ERR(drm_aux_state);
+
+			break;
+		}
+
+		if (!drm_aux_state) {
+			DRM_DEBUG_KMS("Need %d free Y planes for NV12\n",
+				      hweight8(crtc_state->nv12_planes));
+
+			return -EINVAL;
+		}
+
+		plane_state->linked_plane = aux;
+
+		aux_state = to_intel_plane_state(drm_aux_state);
+		aux_state->slave = true;
+		aux_state->linked_plane = plane;
+		crtc_state->active_planes |= BIT(aux->id);
+		DRM_DEBUG_KMS("Using %s as Y plane for %s\n", aux->base.name, plane->base.name);
+	}
+
+	return 0;
+}
+
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
@@ -10807,6 +10861,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		if (mode_changed)
 			ret = skl_update_scaler_crtc(pipe_config);
 
+		if (!ret)
+			ret = icl_check_nv12_planes(dev_priv, intel_crtc,
+						    pipe_config);
 		if (!ret)
 			ret = skl_check_pipe_max_pixel_rate(intel_crtc,
 							    pipe_config);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8073a85d7178..dd9600065eff 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -539,6 +539,26 @@ struct intel_plane_state {
 	 */
 	int scaler_id;
 
+	/*
+	 * linked_plane:
+	 *
+	 * ICL planar formats require 2 planes that are updated as pairs.
+	 * This member is used to make sure the other plane is also updated
+	 * when required, and for update_slave() to find the correct
+	 * plane_state to pass as argument.
+	 */
+	struct intel_plane *linked_plane;
+
+	/*
+	 * slave:
+	 * If set don't update use the linked plane's state for updating
+	 * this plane during atomic commit with the update_slave() callback.
+	 *
+	 * It's also used by the watermark code to ignore wm calculations on
+	 * this plane. They're calculated by the linked plane's wm code.
+	 */
+	bool slave;
+
 	struct drm_intel_sprite_colorkey ckey;
 };
 
@@ -973,6 +993,9 @@ struct intel_plane {
 	void (*update_plane)(struct intel_plane *plane,
 			     const struct intel_crtc_state *crtc_state,
 			     const struct intel_plane_state *plane_state);
+	void (*update_slave)(struct intel_plane *plane,
+			     const struct intel_crtc_state *crtc_state,
+			     const struct intel_plane_state *plane_state);
 	void (*disable_plane)(struct intel_plane *plane,
 			      struct intel_crtc *crtc);
 	bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe);
@@ -1330,6 +1353,27 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
 	return container_of(intel_hdmi, struct intel_digital_port, hdmi);
 }
 
+static inline struct intel_plane_state *
+intel_atomic_get_plane_state(struct intel_atomic_state *state,
+				 struct intel_plane *plane)
+{
+	struct drm_plane_state *ret =
+		drm_atomic_get_plane_state(&state->base, &plane->base);
+
+	if (IS_ERR(ret))
+		return ERR_CAST(ret);
+
+	return to_intel_plane_state(ret);
+}
+
+static inline struct intel_plane_state *
+intel_atomic_get_old_plane_state(struct intel_atomic_state *state,
+				 struct intel_plane *plane)
+{
+	return to_intel_plane_state(drm_atomic_get_new_plane_state(&state->base,
+								   &plane->base));
+}
+
 static inline struct intel_plane_state *
 intel_atomic_get_new_plane_state(struct intel_atomic_state *state,
 				 struct intel_plane *plane)
@@ -2143,6 +2187,15 @@ int skl_plane_check(struct intel_crtc_state *crtc_state,
 int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state);
 int chv_plane_check_rotation(const struct intel_plane_state *plane_state);
 
+static inline bool icl_is_nv12_y_plane(enum plane_id id)
+{
+	/* Don't need to do a gen check, these planes are only available on gen11 */
+	if (id == PLANE_SPRITE4 || id == PLANE_SPRITE5)
+		return true;
+
+	return false;
+}
+
 /* intel_tv.c */
 void intel_tv_init(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1db9b8328275..d76d93452137 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5137,11 +5137,12 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
 	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
-	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	enum pipe pipe = intel_crtc->pipe;
 
 	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
+		struct drm_plane_state *plane_state;
+		struct intel_plane *linked;
 		enum plane_id plane_id = to_intel_plane(plane)->id;
 
 		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
@@ -5153,6 +5154,15 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state))
 			return PTR_ERR(plane_state);
+
+		/* Make sure linked plane is updated too */
+		linked = to_intel_plane_state(plane_state)->linked_plane;
+		if (!linked)
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, &linked->base);
+		if (IS_ERR(plane_state))
+			return PTR_ERR(plane_state);
 	}
 
 	return 0;
-- 
2.18.0

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

[PATCH 10/15] drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.

[Maarten Lankhorst]

Skylake style watermarks program the UV parameters into wm->uv_wm,
and have a separate DDB allocation for UV blocks into the same plane.

Gen11 watermarks have a separate plane for Y and UV, with separate
mechanisms. The simplest way to make it work is to keep the current
way of programming watermarks and calculate the Y and UV plane
watermarks from the master plane.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 122 +++++++++++++++++++-------------
 1 file changed, 72 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d76d93452137..b85403798593 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3945,14 +3945,9 @@ skl_ddb_get_hw_plane_state(struct drm_i915_private *dev_priv,
 				      val & PLANE_CTL_ALPHA_MASK);
 
 	val = I915_READ(PLANE_BUF_CFG(pipe, plane_id));
-	/*
-	 * FIXME: add proper NV12 support for ICL. Avoid reading unclaimed
-	 * registers for now.
-	 */
-	if (INTEL_GEN(dev_priv) < 11)
+	if (fourcc == DRM_FORMAT_NV12 && INTEL_GEN(dev_priv) < 11) {
 		val2 = I915_READ(PLANE_NV12_BUF_CFG(pipe, plane_id));
 
-	if (fourcc == DRM_FORMAT_NV12) {
 		skl_ddb_entry_init_from_hw(dev_priv,
 					   &ddb->plane[pipe][plane_id], val2);
 		skl_ddb_entry_init_from_hw(dev_priv,
@@ -4207,19 +4202,29 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
 		enum plane_id plane_id = to_intel_plane(plane)->id;
 		unsigned int rate;
+		struct intel_plane_state *plane_state =
+			to_intel_plane_state(pstate);
+		enum plane_id plane0_id =
+			plane_state->linked_plane ?
+			plane_state->linked_plane->id : plane_id;
+		unsigned int *uv_data_rate =
+			plane_state->linked_plane ?
+			&plane_data_rate[plane_id] :
+			&uv_plane_data_rate[plane_id];
+
+		if (plane_state->slave)
+			continue;
 
 		/* packed/y */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 0);
-		plane_data_rate[plane_id] = rate;
-
+		plane_data_rate[plane0_id] = rate;
 		total_data_rate += rate;
 
 		/* uv-plane */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 1);
-		uv_plane_data_rate[plane_id] = rate;
-
+		*uv_data_rate = rate;
 		total_data_rate += rate;
 	}
 
@@ -4298,6 +4303,7 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
 
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
 		enum plane_id plane_id = to_intel_plane(plane)->id;
+		struct intel_plane_state *plane_state = to_intel_plane_state(pstate);
 
 		if (plane_id == PLANE_CURSOR)
 			continue;
@@ -4305,8 +4311,14 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
 		if (!pstate->visible)
 			continue;
 
-		minimum[plane_id] = skl_ddb_min_alloc(pstate, 0);
-		uv_minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
+		if (!plane_state->linked_plane) {
+			minimum[plane_id] = skl_ddb_min_alloc(pstate, 0);
+			uv_minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
+		} else {
+			enum plane_id aux = plane_state->linked_plane->id;
+			minimum[aux] = skl_ddb_min_alloc(pstate, 0);
+			minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
+		}
 	}
 
 	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
@@ -4780,36 +4792,20 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 		      struct skl_ddb_allocation *ddb,
 		      struct intel_crtc_state *cstate,
 		      const struct intel_plane_state *intel_pstate,
+		      uint16_t ddb_blocks,
 		      const struct skl_wm_params *wm_params,
 		      struct skl_plane_wm *wm,
-		      int plane_id)
+		      struct skl_wm_level *levels)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_plane *plane = intel_pstate->base.plane;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	uint16_t ddb_blocks;
-	enum pipe pipe = intel_crtc->pipe;
 	int level, max_level = ilk_wm_max_level(dev_priv);
-	enum plane_id intel_plane_id = intel_plane->id;
+	struct skl_wm_level *result_prev = &levels[0];
 	int ret;
 
 	if (WARN_ON(!intel_pstate->base.fb))
 		return -EINVAL;
 
-	ddb_blocks = plane_id ?
-		     skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
-		     skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
-
 	for (level = 0; level <= max_level; level++) {
-		struct skl_wm_level *result = plane_id ? &wm->uv_wm[level] :
-							  &wm->wm[level];
-		struct skl_wm_level *result_prev;
-
-		if (level)
-			result_prev = plane_id ? &wm->uv_wm[level - 1] :
-						  &wm->wm[level - 1];
-		else
-			result_prev = plane_id ? &wm->uv_wm[0] : &wm->wm[0];
+		struct skl_wm_level *result = &levels[level];
 
 		ret = skl_compute_plane_wm(dev_priv,
 					   cstate,
@@ -4821,6 +4817,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
 					   result);
 		if (ret)
 			return ret;
+
+		result_prev = result;
 	}
 
 	if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
@@ -4929,20 +4927,29 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 		const struct intel_plane_state *intel_pstate =
 						to_intel_plane_state(pstate);
 		enum plane_id plane_id = to_intel_plane(plane)->id;
+		enum plane_id plane0_id =
+			intel_pstate->linked_plane ?
+			intel_pstate->linked_plane->id : plane_id;
 		struct skl_wm_params wm_params;
 		enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
 		uint16_t ddb_blocks;
 
-		wm = &pipe_wm->planes[plane_id];
-		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
+		/* Watermarks calculated in master */
+		if (intel_pstate->slave)
+			continue;
+
+		wm = &pipe_wm->planes[plane0_id];
+		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane0_id]);
 
 		ret = skl_compute_plane_wm_params(dev_priv, cstate,
-						  intel_pstate, &wm_params, 0);
+						  intel_pstate,
+						  &wm_params, 0);
 		if (ret)
 			return ret;
 
 		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
-					    intel_pstate, &wm_params, wm, 0);
+					    intel_pstate, ddb_blocks,
+					    &wm_params, wm, wm->wm);
 		if (ret)
 			return ret;
 
@@ -4960,11 +4967,32 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 			if (ret)
 				return ret;
 
-			ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
-						    intel_pstate, &wm_params,
-						    wm, 1);
-			if (ret)
-				return ret;
+			if (intel_pstate->linked_plane) {
+				ddb_blocks =
+					skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
+
+				/* write out wm on the UV plane */
+				wm = &pipe_wm->planes[plane_id];
+				wm->is_planar = true;
+
+				ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
+							    intel_pstate, ddb_blocks,
+							    &wm_params, wm, wm->wm);
+				if (ret)
+					return ret;
+
+				skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
+							  ddb_blocks, &wm->trans_wm);
+			} else {
+				ddb_blocks =
+					skl_ddb_entry_size(&ddb->uv_plane[pipe][plane_id]);
+
+				ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
+							    intel_pstate, ddb_blocks,
+							    &wm_params, wm, wm->uv_wm);
+				if (ret)
+					return ret;
+			}
 		}
 	}
 
@@ -5016,14 +5044,7 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 	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_id),
-			    &ddb->plane[pipe][plane_id]);
-	/* FIXME: add proper NV12 support for ICL. */
-	if (INTEL_GEN(dev_priv) >= 11)
-		return skl_ddb_entry_write(dev_priv,
-					   PLANE_BUF_CFG(pipe, plane_id),
-					   &ddb->plane[pipe][plane_id]);
-	if (wm->is_planar) {
+	if (wm->is_planar && INTEL_GEN(dev_priv) < 11) {
 		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
 				    &ddb->uv_plane[pipe][plane_id]);
 		skl_ddb_entry_write(dev_priv,
@@ -5032,7 +5053,8 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 	} else {
 		skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
 				    &ddb->plane[pipe][plane_id]);
-		I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
+		if (INTEL_GEN(dev_priv) < 11)
+			I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
 	}
 }
 
-- 
2.18.0

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

[PATCH 11/15] drm/i915: Move programming plane scaler to its own function.

[Maarten Lankhorst]

This cleans the code up slightly, and will make other changes easier.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 90 +++++++++++++++++------------
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d4c8e10fc90b..7d3c7469d271 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
 		return min(8192 * cpp, 32768);
 }
 
+static void
+skl_program_scaler(struct drm_i915_private *dev_priv,
+		   struct intel_plane *plane,
+		   const struct intel_crtc_state *crtc_state,
+		   const struct intel_plane_state *plane_state)
+{
+	enum plane_id plane_id = plane->id;
+	enum pipe pipe = plane->pipe;
+	int scaler_id = plane_state->scaler_id;
+	const struct intel_scaler *scaler =
+		&crtc_state->scaler_state.scalers[scaler_id];
+	int crtc_x = plane_state->base.dst.x1;
+	int crtc_y = plane_state->base.dst.y1;
+	uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
+	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
+	u16 y_hphase, uv_rgb_hphase;
+	u16 y_vphase, uv_rgb_vphase;
+
+	/* Sizes are 0 based */
+	crtc_w--;
+	crtc_h--;
+
+	/* TODO: handle sub-pixel coordinates */
+	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
+		y_hphase = skl_scaler_calc_phase(1, false);
+		y_vphase = skl_scaler_calc_phase(1, false);
+
+		/* MPEG2 chroma siting convention */
+		uv_rgb_hphase = skl_scaler_calc_phase(2, true);
+		uv_rgb_vphase = skl_scaler_calc_phase(2, false);
+	} else {
+		/* not used */
+		y_hphase = 0;
+		y_vphase = 0;
+
+		uv_rgb_hphase = skl_scaler_calc_phase(1, false);
+		uv_rgb_vphase = skl_scaler_calc_phase(1, false);
+	}
+
+	I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
+		      PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
+	I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
+	I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
+		      PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
+	I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
+		      PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
+	I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
+	I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
+		      ((crtc_w + 1) << 16)|(crtc_h + 1));
+}
+
 void
 skl_update_plane(struct intel_plane *plane,
 		 const struct intel_crtc_state *crtc_state,
 		 const struct intel_plane_state *plane_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-	const struct drm_framebuffer *fb = plane_state->base.fb;
 	enum plane_id plane_id = plane->id;
 	enum pipe pipe = plane->pipe;
 	u32 plane_ctl = plane_state->ctl;
@@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
 	u32 aux_stride = skl_plane_stride(plane_state, 1);
 	int crtc_x = plane_state->base.dst.x1;
 	int crtc_y = plane_state->base.dst.y1;
-	uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
-	uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
 	uint32_t x = plane_state->color_plane[0].x;
 	uint32_t y = plane_state->color_plane[0].y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
@@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
-	crtc_w--;
-	crtc_h--;
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
@@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
 
 	/* program plane scaler */
 	if (plane_state->scaler_id >= 0) {
-		int scaler_id = plane_state->scaler_id;
-		const struct intel_scaler *scaler =
-			&crtc_state->scaler_state.scalers[scaler_id];
-		u16 y_hphase, uv_rgb_hphase;
-		u16 y_vphase, uv_rgb_vphase;
-
-		/* TODO: handle sub-pixel coordinates */
-		if (fb->format->format == DRM_FORMAT_NV12) {
-			y_hphase = skl_scaler_calc_phase(1, false);
-			y_vphase = skl_scaler_calc_phase(1, false);
-
-			/* MPEG2 chroma siting convention */
-			uv_rgb_hphase = skl_scaler_calc_phase(2, true);
-			uv_rgb_vphase = skl_scaler_calc_phase(2, false);
-		} else {
-			/* not used */
-			y_hphase = 0;
-			y_vphase = 0;
-
-			uv_rgb_hphase = skl_scaler_calc_phase(1, false);
-			uv_rgb_vphase = skl_scaler_calc_phase(1, false);
-		}
-
-		I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
-			      PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
-		I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
-		I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
-			      PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
-		I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
-			      PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
-		I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
-		I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
-			      ((crtc_w + 1) << 16)|(crtc_h + 1));
+		skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
 
 		I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0);
 	} else {
-- 
2.18.0

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

[PATCH 12/15] drm/i915/gen11: Program the scalers correctly for planar formats.

[Maarten Lankhorst]

The first 3 planes (primary, sprite 0 and 1) have a dedicated chroma
upsampler to upscale YUV420 to YUV444 and the scaler should only be
used for upscaling. Because of this we shouldn't program the scalers
in planar mode if NV12 and the chroma upsampler are used. Instead
program the scalers like on normal planes.

Sprite 2 and 3 have no dedicated scaler, and need to program the
selected Y plane in the scaler mode.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  2 ++
 drivers/gpu/drm/i915/intel_atomic.c  |  6 +++++-
 drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |  3 ++-
 5 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e7e6ca7f9665..1b59d15aaf59 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6872,6 +6872,8 @@ enum {
 #define PS_VADAPT_MODE_LEAST_ADAPT (0 << 5)
 #define PS_VADAPT_MODE_MOD_ADAPT   (1 << 5)
 #define PS_VADAPT_MODE_MOST_ADAPT  (3 << 5)
+#define PS_PLANE_Y_SEL_MASK  (7 << 5)
+#define PS_PLANE_Y_SEL(plane) (((plane) + 1) << 5)
 
 #define _PS_PWR_GATE_1A     0x68160
 #define _PS_PWR_GATE_2A     0x68260
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 40fba3f22f87..fd92c710d291 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -234,9 +234,13 @@ static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_sta
 		if (INTEL_GEN(dev_priv) == 9 &&
 		    !IS_GEMINILAKE(dev_priv))
 			mode = SKL_PS_SCALER_MODE_NV12;
-		else
+		else if (!icl_is_hdr_plane(to_intel_plane(plane_state->base.plane))) {
 			mode = PS_SCALER_MODE_PLANAR;
 
+			if (plane_state->linked_plane)
+				mode |= PS_PLANE_Y_SEL(plane_state->linked_plane->id);
+		} else
+			mode = PS_SCALER_MODE_PACKED;
 	} else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
 		mode = PS_SCALER_MODE_PACKED;
 	} else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a53953df6c9f..5a82946d0d97 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4842,8 +4842,7 @@ static int
 skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 		  unsigned int scaler_user, int *scaler_id,
 		  int src_w, int src_h, int dst_w, int dst_h,
-		  bool plane_scaler_check,
-		  uint32_t pixel_format)
+		  const struct drm_format_info *format, bool need_scaling)
 {
 	struct intel_crtc_scaler_state *scaler_state =
 		&crtc_state->scaler_state;
@@ -4852,18 +4851,14 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->base.adjusted_mode;
-	int need_scaling;
 
 	/*
 	 * Src coordinates are already rotated by 270 degrees for
 	 * the 90/270 degree plane rotation cases (to match the
 	 * GTT mapping), hence no need to account for rotation here.
 	 */
-	need_scaling = src_w != dst_w || src_h != dst_h;
-
-	if (plane_scaler_check)
-		if (pixel_format == DRM_FORMAT_NV12)
-			need_scaling = true;
+	if (src_w != dst_w || src_h != dst_h)
+		need_scaling = true;
 
 	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
 		need_scaling = true;
@@ -4904,7 +4899,7 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 		return 0;
 	}
 
-	if (plane_scaler_check && pixel_format == DRM_FORMAT_NV12 &&
+	if (format && format->format == DRM_FORMAT_NV12 &&
 	    (src_h < SKL_MIN_YUV_420_SRC_H || src_w < SKL_MIN_YUV_420_SRC_W)) {
 		DRM_DEBUG_KMS("NV12: src dimensions not met\n");
 		return -EINVAL;
@@ -4952,7 +4947,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
 				 &state->scaler_state.scaler_id,
 				 state->pipe_src_w, state->pipe_src_h,
 				 adjusted_mode->crtc_hdisplay,
-				 adjusted_mode->crtc_vdisplay, false, 0);
+				 adjusted_mode->crtc_vdisplay, NULL, 0);
 }
 
 /**
@@ -4967,13 +4962,22 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
 static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 				   struct intel_plane_state *plane_state)
 {
-
 	struct intel_plane *intel_plane =
 		to_intel_plane(plane_state->base.plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	int ret;
-
 	bool force_detach = !fb || !plane_state->base.visible;
+	bool need_scaling = false;
+
+	if (fb && fb->format->format == DRM_FORMAT_NV12) {
+		/*
+		 * Gen10- and sprite 2 and 3 always need the scaler.
+		 * On gen11 we use the chroma upsampler when available,
+		 * and only use the scaler for normal scaling.
+		 */
+		if (!icl_is_hdr_plane(intel_plane))
+			need_scaling = true;
+	}
 
 	ret = skl_update_scaler(crtc_state, force_detach,
 				drm_plane_index(&intel_plane->base),
@@ -4982,7 +4986,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 				drm_rect_height(&plane_state->base.src) >> 16,
 				drm_rect_width(&plane_state->base.dst),
 				drm_rect_height(&plane_state->base.dst),
-				fb ? true : false, fb ? fb->format->format : 0);
+				fb ? fb->format : NULL, need_scaling);
 
 	if (ret || plane_state->scaler_id < 0)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dd9600065eff..d1d9e0a0e60a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2196,6 +2196,14 @@ static inline bool icl_is_nv12_y_plane(enum plane_id id)
 	return false;
 }
 
+static inline bool icl_is_hdr_plane(struct intel_plane *plane)
+{
+	if (INTEL_GEN(to_i915(plane->base.dev)) < 11)
+		return false;
+
+	return plane->id < PLANE_SPRITE2;
+}
+
 /* intel_tv.c */
 void intel_tv_init(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7d3c7469d271..0b41e0d3b79e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -303,7 +303,8 @@ skl_program_scaler(struct drm_i915_private *dev_priv,
 	crtc_h--;
 
 	/* TODO: handle sub-pixel coordinates */
-	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
+	if (plane_state->base.fb->format->format == DRM_FORMAT_NV12 &&
+	    !icl_is_hdr_plane(plane)) {
 		y_hphase = skl_scaler_calc_phase(1, false);
 		y_vphase = skl_scaler_calc_phase(1, false);
 
-- 
2.18.0

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

[PATCH 13/15] drm/i915/gen11: Program the chroma upsampler for HDR planes.

[Maarten Lankhorst]

We configure the chroma upsampler with the same chroma siting as
used by the scaler for consistency, the chroma upsampler is used
instead of the scaler for YUV 4:2:0 on ICL's HDR planes.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c | 22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1b59d15aaf59..b614a06b66c4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6560,6 +6560,19 @@ enum {
 #define _PLANE_AUX_DIST_2_A			0x702c0
 #define _PLANE_AUX_OFFSET_1_A			0x701c4
 #define _PLANE_AUX_OFFSET_2_A			0x702c4
+#define _PLANE_CUS_CTL_1_A			0x701c8
+#define _PLANE_CUS_CTL_2_A			0x702c8
+#define  PLANE_CUS_ENABLE			(1 << 31)
+#define  PLANE_CUS_PLANE_6			(0 << 30)
+#define  PLANE_CUS_PLANE_7			(1 << 30)
+#define  PLANE_CUS_HPHASE_SIGN_NEGATIVE		(1 << 19)
+#define  PLANE_CUS_HPHASE_0			(0 << 16)
+#define  PLANE_CUS_HPHASE_0_25			(1 << 16)
+#define  PLANE_CUS_HPHASE_0_5			(2 << 16)
+#define  PLANE_CUS_VPHASE_SIGN_NEGATIVE		(1 << 15)
+#define  PLANE_CUS_VPHASE_0			(0 << 12)
+#define  PLANE_CUS_VPHASE_0_25			(1 << 12)
+#define  PLANE_CUS_VPHASE_0_5			(2 << 12)
 #define _PLANE_COLOR_CTL_1_A			0x701CC /* GLK+ */
 #define _PLANE_COLOR_CTL_2_A			0x702CC /* GLK+ */
 #define _PLANE_COLOR_CTL_3_A			0x703CC /* GLK+ */
@@ -6697,6 +6710,15 @@ enum {
 #define PLANE_AUX_OFFSET(pipe, plane)   \
 	_MMIO_PLANE(plane, _PLANE_AUX_OFFSET_1(pipe), _PLANE_AUX_OFFSET_2(pipe))
 
+#define _PLANE_CUS_CTL_1_B		0x711c8
+#define _PLANE_CUS_CTL_2_B		0x712c8
+#define _PLANE_CUS_CTL_1(pipe)       \
+		_PIPE(pipe, _PLANE_CUS_CTL_1_A, _PLANE_CUS_CTL_1_B)
+#define _PLANE_CUS_CTL_2(pipe)       \
+		_PIPE(pipe, _PLANE_CUS_CTL_2_A, _PLANE_CUS_CTL_2_B)
+#define PLANE_CUS_CTL(pipe, plane)   \
+	_MMIO_PLANE(plane, _PLANE_CUS_CTL_1(pipe), _PLANE_CUS_CTL_2(pipe))
+
 #define _PLANE_COLOR_CTL_1_B			0x711CC
 #define _PLANE_COLOR_CTL_2_B			0x712CC
 #define _PLANE_COLOR_CTL_3_B			0x713CC
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0b41e0d3b79e..6e3b3ce36d11 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -351,6 +351,7 @@ skl_update_plane(struct intel_plane *plane,
 	uint32_t y = plane_state->color_plane[0].y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
+	struct intel_plane *linked = plane_state->linked_plane;
 	unsigned long irqflags;
 
 	/* Sizes are 0 based */
@@ -378,6 +379,27 @@ skl_update_plane(struct intel_plane *plane,
 		      (plane_state->color_plane[1].y << 16) |
 		      plane_state->color_plane[1].x);
 
+	if (icl_is_hdr_plane(plane)) {
+		u32 cus_ctl = 0;
+
+		if (linked) {
+			/* Enable and use MPEG-2 chroma siting */
+			cus_ctl = PLANE_CUS_ENABLE |
+				PLANE_CUS_HPHASE_0 |
+				PLANE_CUS_VPHASE_SIGN_NEGATIVE |
+				PLANE_CUS_VPHASE_0_25;
+
+			if (linked->id == PLANE_SPRITE5)
+				cus_ctl |= PLANE_CUS_PLANE_7;
+			else if (linked->id == PLANE_SPRITE4)
+				cus_ctl |= PLANE_CUS_PLANE_6;
+			else
+				MISSING_CASE(linked->id);
+		}
+
+		I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
+	}
+
 	/* program plane scaler */
 	if (plane_state->scaler_id >= 0) {
 		skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
-- 
2.18.0

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

[PATCH 14/15] drm/i915/gen11: Program the Y and UV plane for planar mode correctly.

[Maarten Lankhorst]

The UV plane is the master plane that does all color correction etc.
It needs to be programmed with the dimensions for color plane 1 (UV).

The Y plane just feeds the Y pixels to it. Program the scaler from the
master only, and set PLANE_CTL_YUV420_Y_PLANE on the slave plane.

Changes since v1:
- Make a common skl_program_plane, and use it for both plane updates.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  1 +
 drivers/gpu/drm/i915/intel_sprite.c | 50 +++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b614a06b66c4..a3129a4c15cc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6511,6 +6511,7 @@ enum {
 #define   PLANE_CTL_KEY_ENABLE_DESTINATION	(2 << 21)
 #define   PLANE_CTL_ORDER_BGRX			(0 << 20)
 #define   PLANE_CTL_ORDER_RGBX			(1 << 20)
+#define   PLANE_CTL_YUV420_Y_PLANE		(1 << 19)
 #define   PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709	(1 << 18)
 #define   PLANE_CTL_YUV422_ORDER_MASK		(0x3 << 16)
 #define   PLANE_CTL_YUV422_YUYV			(0 << 16)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 6e3b3ce36d11..b9033313b60a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -332,23 +332,23 @@ skl_program_scaler(struct drm_i915_private *dev_priv,
 		      ((crtc_w + 1) << 16)|(crtc_h + 1));
 }
 
-void
-skl_update_plane(struct intel_plane *plane,
-		 const struct intel_crtc_state *crtc_state,
-		 const struct intel_plane_state *plane_state)
+static void
+skl_program_plane(struct intel_plane *plane,
+		  const struct intel_crtc_state *crtc_state,
+		  const struct intel_plane_state *plane_state,
+		  int color_plane, bool slave, u32 plane_ctl)
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum plane_id plane_id = plane->id;
 	enum pipe pipe = plane->pipe;
-	u32 plane_ctl = plane_state->ctl;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
-	u32 surf_addr = plane_state->color_plane[0].offset;
-	u32 stride = skl_plane_stride(plane_state, 0);
+	u32 surf_addr = plane_state->color_plane[color_plane].offset;
+	u32 stride = skl_plane_stride(plane_state, color_plane);
 	u32 aux_stride = skl_plane_stride(plane_state, 1);
 	int crtc_x = plane_state->base.dst.x1;
 	int crtc_y = plane_state->base.dst.y1;
-	uint32_t x = plane_state->color_plane[0].x;
-	uint32_t y = plane_state->color_plane[0].y;
+	uint32_t x = plane_state->color_plane[color_plane].x;
+	uint32_t y = plane_state->color_plane[color_plane].y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 	struct intel_plane *linked = plane_state->linked_plane;
@@ -402,7 +402,9 @@ skl_update_plane(struct intel_plane *plane,
 
 	/* program plane scaler */
 	if (plane_state->scaler_id >= 0) {
-		skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
+		if (!slave)
+			skl_program_scaler(dev_priv, plane,
+					   crtc_state, plane_state);
 
 		I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0);
 	} else {
@@ -417,6 +419,25 @@ skl_update_plane(struct intel_plane *plane,
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
+void
+skl_update_plane(struct intel_plane *plane,
+		 const struct intel_crtc_state *crtc_state,
+		 const struct intel_plane_state *plane_state)
+{
+	skl_program_plane(plane, crtc_state, plane_state,
+			  !!plane_state->linked_plane, false,
+			  plane_state->ctl);
+}
+
+static void
+icl_update_slave(struct intel_plane *plane,
+		 const struct intel_crtc_state *crtc_state,
+		 const struct intel_plane_state *plane_state)
+{
+	skl_program_plane(plane, crtc_state, plane_state, 0, true,
+			  plane_state->ctl | PLANE_CTL_YUV420_Y_PLANE);
+}
+
 void
 skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 {
@@ -1768,6 +1789,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 	const uint64_t *modifiers;
 	unsigned int supported_rotations;
 	int num_plane_formats;
+	enum plane_id plane_id = PLANE_SPRITE0 + plane;
 	int ret;
 
 	intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
@@ -1787,16 +1809,18 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 		state->scaler_id = -1;
 
 		intel_plane->has_ccs = skl_plane_has_ccs(dev_priv, pipe,
-							 PLANE_SPRITE0 + plane);
+							 plane_id);
 
 		intel_plane->max_stride = skl_plane_max_stride;
 		intel_plane->update_plane = skl_update_plane;
+		if (icl_is_nv12_y_plane(plane_id))
+			intel_plane->update_slave = icl_update_slave;
 		intel_plane->disable_plane = skl_disable_plane;
 		intel_plane->get_hw_state = skl_plane_get_hw_state;
 		intel_plane->check_plane = skl_plane_check;
 
 		if (skl_plane_has_planar(dev_priv, pipe,
-					 PLANE_SPRITE0 + plane)) {
+					 plane_id)) {
 			plane_formats = skl_planar_formats;
 			num_plane_formats = ARRAY_SIZE(skl_planar_formats);
 		} else {
@@ -1870,7 +1894,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 
 	intel_plane->pipe = pipe;
 	intel_plane->i9xx_plane = plane;
-	intel_plane->id = PLANE_SPRITE0 + plane;
+	intel_plane->id = plane_id;
 	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, intel_plane->id);
 
 	possible_crtcs = (1 << pipe);
-- 
2.18.0

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

[PATCH 15/15] drm/i915/gen11: Expose planar format support on gen11.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5a82946d0d97..beeaae072f2b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13738,12 +13738,14 @@ static bool skl_plane_has_fbc(struct drm_i915_private *dev_priv,
 bool skl_plane_has_planar(struct drm_i915_private *dev_priv,
 			  enum pipe pipe, enum plane_id plane_id)
 {
-	/*
-	 * FIXME: ICL requires two hardware planes for scanning out NV12
-	 * framebuffers. Do not advertize support until this is implemented.
-	 */
-	if (INTEL_GEN(dev_priv) >= 11)
-		return false;
+	if (INTEL_GEN(dev_priv) >= 11) {
+		/*
+		 * Sprite 4 and 5 are used for scanning out the planar Y plane,
+		 * but it's the UV plane that counts for the Z-order, so don't
+		 * advertise planar YUV support on it.
+		 */
+		return plane_id <= PLANE_SPRITE3;
+	}
 
 	if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))
 		return false;
@@ -14653,7 +14655,7 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		break;
 	case DRM_FORMAT_NV12:
 		if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv) ||
-		    IS_BROXTON(dev_priv) || INTEL_GEN(dev_priv) >= 11) {
+		    IS_BROXTON(dev_priv)) {
 			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
 				      drm_get_format_name(mode_cmd->pixel_format,
 							  &format_name));
-- 
2.18.0

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/gen11: Implement planar format support.

[Patchwork]

== Series Details ==

Series: drm/i915/gen11: Implement planar format support.
URL   : https://patchwork.freedesktop.org/series/49910/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
a4eaf927a004 drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
0fd609b3d9b6 drm/i915: Handle cursor updating active_planes correctly.
ce778e3b4ca8 drm/i915: Make intel_crtc_disable_planes() use active planes mask.
-:25: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#25: FILE: drivers/gpu/drm/i915/intel_display.c:5405:
+static void intel_crtc_disable_planes(struct intel_crtc *crtc, unsigned plane_mask)

-:33: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#33: FILE: drivers/gpu/drm/i915/intel_display.c:5409:
+	unsigned fb_bits = 0;

total: 0 errors, 2 warnings, 0 checks, 45 lines checked
16a4dc462ded drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
2bf67e55f211 drm/i915: Clean up scaler setup.
-:96: CHECK:LINE_SPACING: Please don't use multiple blank lines
#96: FILE: drivers/gpu/drm/i915/intel_atomic.c:261:
+
+

total: 0 errors, 0 warnings, 1 checks, 156 lines checked
d79e4ec625b1 drm/i915: Force NV12 coordinates to be a multiple of 2
b7730c8c44ca drm/i915: Unconditionally clear plane_state->visible flag
59f05301361b drm/i915/gen11: Enable 6 sprites on gen11
36eaf29d48e4 drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v2.
-:18: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#18: 
- Put all the state updating login in intel_plane_atomic_check_with_state().

-:73: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#73: FILE: drivers/gpu/drm/i915/intel_atomic_plane.c:158:
+	struct intel_crtc *crtc = to_intel_crtc(

-:85: WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each line
#85: FILE: drivers/gpu/drm/i915/intel_atomic_plane.c:170:
+		/*
+		* Make sure a previously linked plane (and implicitly, the CRTC)

-:269: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#269: FILE: drivers/gpu/drm/i915/intel_drv.h:560:
+	bool slave;

-:290: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#290: FILE: drivers/gpu/drm/i915/intel_drv.h:1358:
+intel_atomic_get_plane_state(struct intel_atomic_state *state,
+				 struct intel_plane *plane)

total: 0 errors, 2 warnings, 3 checks, 311 lines checked
7dfbc98f126e drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.
-:90: WARNING:LINE_SPACING: Missing a blank line after declarations
#90: FILE: drivers/gpu/drm/i915/intel_pm.c:4319:
+			enum plane_id aux = plane_state->linked_plane->id;
+			minimum[aux] = skl_ddb_min_alloc(pstate, 0);

total: 0 errors, 1 warnings, 0 checks, 213 lines checked
ec1e8d42067e drm/i915: Move programming plane scaler to its own function.
-:66: CHECK:SPACING: spaces preferred around that '|' (ctx:VxV)
#66: FILE: drivers/gpu/drm/i915/intel_sprite.c:331:
+		      ((crtc_w + 1) << 16)|(crtc_h + 1));
 		                          ^

total: 0 errors, 0 warnings, 1 checks, 120 lines checked
33fed8c42f7c drm/i915/gen11: Program the scalers correctly for planar formats.
-:45: CHECK:BRACES: Unbalanced braces around else statement
#45: FILE: drivers/gpu/drm/i915/intel_atomic.c:242:
+		} else

total: 0 errors, 0 warnings, 1 checks, 122 lines checked
e308838371e3 drm/i915/gen11: Program the chroma upsampler for HDR planes.
-:46: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'pipe' - possible side-effects?
#46: FILE: drivers/gpu/drm/i915/i915_reg.h:6719:
+#define PLANE_CUS_CTL(pipe, plane)   \
+	_MMIO_PLANE(plane, _PLANE_CUS_CTL_1(pipe), _PLANE_CUS_CTL_2(pipe))

total: 0 errors, 0 warnings, 1 checks, 68 lines checked
64f5cc32aebb drm/i915/gen11: Program the Y and UV plane for planar mode correctly.
2f4cfd3509d5 drm/i915/gen11: Expose planar format support on gen11.
-:7: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 28 lines checked

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

✗ Fi.CI.SPARSE: warning for drm/i915/gen11: Implement planar format support.

[Patchwork]

== Series Details ==

Series: drm/i915/gen11: Implement planar format support.
URL   : https://patchwork.freedesktop.org/series/49910/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
Okay!

Commit: drm/i915: Handle cursor updating active_planes correctly.
Okay!

Commit: drm/i915: Make intel_crtc_disable_planes() use active planes mask.
Okay!

Commit: drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
Okay!

Commit: drm/i915: Clean up scaler setup.
Okay!

Commit: drm/i915: Force NV12 coordinates to be a multiple of 2
Okay!

Commit: drm/i915: Unconditionally clear plane_state->visible flag
Okay!

Commit: drm/i915/gen11: Enable 6 sprites on gen11
Okay!

Commit: drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v2.
Okay!

Commit: drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.
Okay!

Commit: drm/i915: Move programming plane scaler to its own function.
-O:drivers/gpu/drm/i915/intel_sprite.c:280:24: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_sprite.c:280:24: warning: expression using sizeof(void)

Commit: drm/i915/gen11: Program the scalers correctly for planar formats.
Okay!

Commit: drm/i915/gen11: Program the chroma upsampler for HDR planes.
Okay!

Commit: drm/i915/gen11: Program the Y and UV plane for planar mode correctly.
Okay!

Commit: drm/i915/gen11: Expose planar format support on gen11.
Okay!

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

✗ Fi.CI.BAT: failure for drm/i915/gen11: Implement planar format support.

[Patchwork]

== Series Details ==

Series: drm/i915/gen11: Implement planar format support.
URL   : https://patchwork.freedesktop.org/series/49910/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4843 -> Patchwork_10225 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10225 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10225, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49910/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10225:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_module_reload@basic-reload:
      fi-skl-guc:         PASS -> DMESG-WARN +41

    igt@drv_module_reload@basic-reload-inject:
      fi-skl-6260u:       PASS -> DMESG-WARN +25
      fi-byt-j1900:       PASS -> DMESG-WARN +20
      fi-kbl-7560u:       PASS -> DMESG-WARN +28
      fi-bwr-2160:        PASS -> DMESG-WARN +16

    igt@gem_exec_suspend@basic-s4-devices:
      fi-cfl-s3:          PASS -> DMESG-WARN +29
      fi-byt-n2820:       PASS -> DMESG-WARN +18
      fi-elk-e7500:       PASS -> DMESG-WARN +17

    igt@kms_busy@basic-flip-a:
      fi-kbl-7567u:       PASS -> DMESG-WARN +24
      fi-whl-u:           PASS -> DMESG-WARN +29

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       PASS -> DMESG-WARN +42

    igt@kms_chamelium@hdmi-crc-fast:
      fi-skl-6700k2:      PASS -> DMESG-WARN +44

    igt@kms_flip@basic-flip-vs-dpms:
      fi-ilk-650:         PASS -> DMESG-WARN +33
      fi-pnv-d510:        PASS -> DMESG-WARN +18
      fi-skl-6770hq:      PASS -> DMESG-WARN +25
      fi-ivb-3520m:       PASS -> DMESG-WARN +24

    igt@kms_flip@basic-flip-vs-modeset:
      fi-cnl-u:           PASS -> DMESG-WARN +25
      fi-blb-e6850:       PASS -> DMESG-WARN +18
      fi-ivb-3770:        PASS -> DMESG-WARN +24

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-icl-u:           PASS -> DMESG-WARN +29
      fi-hsw-peppy:       PASS -> DMESG-WARN +21
      fi-snb-2600:        PASS -> DMESG-WARN +20

    igt@kms_flip@basic-plain-flip:
      fi-bsw-n3050:       PASS -> DMESG-WARN +11
      fi-cfl-guc:         PASS -> DMESG-WARN +25

    igt@kms_force_connector_basic@force-load-detect:
      fi-hsw-4770:        PASS -> DMESG-WARN +26

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-bxt-j4205:       PASS -> DMESG-WARN +24

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-bsw-kefka:       PASS -> DMESG-WARN +25
      fi-bdw-5557u:       PASS -> DMESG-WARN +25
      fi-byt-clapper:     PASS -> DMESG-WARN +16

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-bdw-samus:       NOTRUN -> DMESG-WARN +17
      fi-bdw-gvtdvm:      PASS -> DMESG-WARN +22

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-skl-gvtdvm:      PASS -> DMESG-WARN +22

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
      fi-glk-dsi:         PASS -> DMESG-WARN +25

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-cfl-8109u:       PASS -> DMESG-WARN +41

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-snb-2520m:       PASS -> DMESG-WARN +17

    igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence:
      fi-bxt-dsi:         PASS -> DMESG-WARN +24

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-hsw-4770r:       PASS -> DMESG-WARN +24
      fi-cfl-8700k:       PASS -> DMESG-WARN +25

    igt@kms_psr@cursor_plane_move:
      fi-kbl-r:           PASS -> DMESG-WARN +27

    igt@kms_psr@sprite_plane_onoff:
      fi-skl-6700hq:      PASS -> DMESG-WARN +28

    igt@pm_rpm@basic-pci-d3-state:
      fi-skl-6600u:       PASS -> DMESG-WARN +29

    igt@pm_rpm@basic-rte:
      fi-glk-j4005:       PASS -> DMESG-WARN +23

    
    ==== Warnings ====

    igt@gem_exec_suspend@basic-s3:
      fi-bdw-samus:       INCOMPLETE (fdo#107773) -> DMESG-WARN

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#107362) -> DMESG-WARN

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           FAIL (fdo#107336) -> DMESG-WARN

    
== Known issues ==

  Here are the changes found in Patchwork_10225 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

    igt@drv_module_reload@basic-reload-inject:
      fi-bxt-j4205:       PASS -> DMESG-WARN (fdo#107821)
      fi-bxt-dsi:         PASS -> DMESG-WARN (fdo#107821)
      fi-hsw-4770:        PASS -> DMESG-WARN (fdo#107924)
      fi-hsw-peppy:       PASS -> DMESG-WARN (fdo#102614, fdo#107924)
      fi-hsw-4770r:       PASS -> DMESG-WARN (fdo#107924)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7567u:       PASS -> DMESG-WARN (fdo#107139)
      fi-kbl-r:           PASS -> DMESG-WARN (fdo#107139)
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#107139)
      fi-kbl-7560u:       PASS -> DMESG-WARN (fdo#107139)

    igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
      fi-bdw-samus:       NOTRUN -> DMESG-WARN (fdo#107733) +2

    igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
      fi-byt-clapper:     PASS -> DMESG-WARN (fdo#107751) +2
      fi-byt-j1900:       PASS -> DMESG-WARN (fdo#107751)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-WARN (fdo#102614) +2
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-bdw-samus:       NOTRUN -> INCOMPLETE (fdo#107773)

    
    ==== Warnings ====

    igt@kms_psr@primary_mmap_gtt:
      fi-cnl-u:           FAIL (fdo#107383) -> DMESG-FAIL (fdo#107383) +3

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383
  fdo#107733 https://bugs.freedesktop.org/show_bug.cgi?id=107733
  fdo#107751 https://bugs.freedesktop.org/show_bug.cgi?id=107751
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107821 https://bugs.freedesktop.org/show_bug.cgi?id=107821
  fdo#107924 https://bugs.freedesktop.org/show_bug.cgi?id=107924


== Participating hosts (52 -> 46) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-skl-iommu 


== Build changes ==

    * Linux: CI_DRM_4843 -> Patchwork_10225

  CI_DRM_4843: c8db7511254acf102934bbc2dcf6ca069a4281e8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4646: d409cc6f234fbc0122c64be27ba85b5603658de5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10225: 2f4cfd3509d5fdf1ce419eaaafe20bbb3c410ad5 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2f4cfd3509d5 drm/i915/gen11: Expose planar format support on gen11.
64f5cc32aebb drm/i915/gen11: Program the Y and UV plane for planar mode correctly.
e308838371e3 drm/i915/gen11: Program the chroma upsampler for HDR planes.
33fed8c42f7c drm/i915/gen11: Program the scalers correctly for planar formats.
ec1e8d42067e drm/i915: Move programming plane scaler to its own function.
7dfbc98f126e drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.
36eaf29d48e4 drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v2.
59f05301361b drm/i915/gen11: Enable 6 sprites on gen11
b7730c8c44ca drm/i915: Unconditionally clear plane_state->visible flag
d79e4ec625b1 drm/i915: Force NV12 coordinates to be a multiple of 2
2bf67e55f211 drm/i915: Clean up scaler setup.
16a4dc462ded drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
ce778e3b4ca8 drm/i915: Make intel_crtc_disable_planes() use active planes mask.
0fd609b3d9b6 drm/i915: Handle cursor updating active_planes correctly.
a4eaf927a004 drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10225/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/15] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()

[Matt Roper]

On Wed, Sep 19, 2018 at 03:56:30PM +0200, Maarten Lankhorst wrote:
> Use old/new_intel_crtc_state, and get rid of all the conversion casts
> where they don't add anything.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb25037d7b38..2ac381eb8103 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12665,8 +12665,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
>  	struct drm_crtc *crtc;
> -	struct intel_crtc_state *intel_cstate;
> +	struct intel_crtc *intel_crtc;
>  	u64 put_domains[I915_MAX_PIPES] = {};
>  	int i;
>  
> @@ -12678,21 +12679,22 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>  
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {

It looks like we have a for_each_oldnew_intel_plane_in_state, but not a
for_each_oldnew_intel_crtc_in_state yet.  If we added the Intel-specific
iterator, then we could simplify this function a bit further by dropping
the base versions of crtc, old_crtc_state, and new_crtc_state here and
renaming the Intel-specific subtypes.  I believe Ville mentioned that on
the other thread.

Anyway, the general cleanup here looks fine, so

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

even if you decide to defer further cleanup via
for_each_oldnew_intel_crtc_in_state to later.


Matt

> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
> +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> +		intel_crtc = to_intel_crtc(crtc);
>  
>  		if (needs_modeset(new_crtc_state) ||
>  		    to_intel_crtc_state(new_crtc_state)->update_pipe) {
>  
> -			put_domains[to_intel_crtc(crtc)->pipe] =
> +			put_domains[intel_crtc->pipe] =
>  				modeset_get_crtc_power_domains(crtc,
> -					to_intel_crtc_state(new_crtc_state));
> +					new_intel_crtc_state);
>  		}
>  
>  		if (!needs_modeset(new_crtc_state))
>  			continue;
>  
> -		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
> -				       to_intel_crtc_state(new_crtc_state));
> +		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
>  
>  		if (old_crtc_state->active) {
>  			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> @@ -12703,7 +12705,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  			 */
>  			intel_crtc_disable_pipe_crc(intel_crtc);
>  
> -			dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
> +			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
>  			intel_crtc->active = false;
>  			intel_fbc_disable(intel_crtc);
>  			intel_disable_shared_dpll(intel_crtc);
> @@ -12724,7 +12726,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  				 */
>  				if (INTEL_GEN(dev_priv) >= 9)
>  					dev_priv->display.initial_watermarks(intel_state,
> -									     to_intel_crtc_state(new_crtc_state));
> +									     new_intel_crtc_state);
>  			}
>  		}
>  	}
> @@ -12784,11 +12786,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	 * TODO: Move this (and other cleanup) to an async worker eventually.
>  	 */
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> -		intel_cstate = to_intel_crtc_state(new_crtc_state);
> +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
>  
>  		if (dev_priv->display.optimize_watermarks)
>  			dev_priv->display.optimize_watermarks(intel_state,
> -							      intel_cstate);
> +							      new_intel_crtc_state);
>  	}
>  
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -- 
> 2.18.0
> 
> _______________________________________________
> 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

Re: [PATCH 02/15] drm/i915: Handle cursor updating active_planes correctly.

[Matt Roper]

On Wed, Sep 19, 2018 at 03:56:31PM +0200, Maarten Lankhorst wrote:
> While we may not update new_crtc_state, we may clear active_planes
> if the new cursor update state will disable the cursor, but we fail
> after. If this is immediately followed by a modeset disable, we may
> soon not disable the planes correctly when we start depending on
> active_planes.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Fixes: b2b55502d683c ("drm/i915: Pass proper old/new states to
intel_plane_atomic_check_with_state()")

I think?  So basically we were clobbering the current cstate's
active_planes field (and anything else in cstate that
intel_plane_atomic_check_with_state() might touch in the future) if the
atomic transaction failed.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ac381eb8103..7a7ff1d76031 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13517,14 +13517,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *old_fb;
> -	struct drm_crtc_state *crtc_state = crtc->state;
> +	struct intel_crtc_state *crtc_state =
> +		to_intel_crtc_state(crtc->state);
> +	struct intel_crtc_state *new_crtc_state;
>  
>  	/*
>  	 * When crtc is inactive or there is a modeset pending,
>  	 * wait for it to complete in the slowpath
>  	 */
> -	if (!crtc_state->active || needs_modeset(crtc_state) ||
> -	    to_intel_crtc_state(crtc_state)->update_pipe)
> +	if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
> +	    crtc_state->update_pipe)
>  		goto slow;
>  
>  	old_plane_state = plane->state;
> @@ -13554,6 +13556,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	if (!new_plane_state)
>  		return -ENOMEM;
>  
> +	new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
> +	if (!new_crtc_state) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
>  	drm_atomic_set_fb_for_plane(new_plane_state, fb);
>  
>  	new_plane_state->src_x = src_x;
> @@ -13565,9 +13573,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	new_plane_state->crtc_w = crtc_w;
>  	new_plane_state->crtc_h = crtc_h;
>  
> -	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> -						  to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
> -						  to_intel_plane_state(plane->state),
> +	ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
> +						  to_intel_plane_state(old_plane_state),
>  						  to_intel_plane_state(new_plane_state));
>  	if (ret)
>  		goto out_free;
> @@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  
>  	/* Swap plane state */
>  	plane->state = new_plane_state;
> +	crtc_state->active_planes = new_crtc_state->active_planes;

At the moment this is the only crtc_state field that our plane check
might update for a cursor, but is there a reason why we don't just swap
in the new state and destroy the old one farther down?  If our driver
ever did start updating other fields, it would be really easy to
overlook the need to copy those new fields over here.  I realize a lot
of the likely candidates for fields that plane check might want to
update (e.g., watermark stuff) would also disqualify us from taking this
legacy cursor fastpath in the first place, but I'm a bit nervous about
assuming that we'll never be updating any other fields at all.


Matt

>  
>  	if (plane->state->visible) {
>  		trace_intel_update_plane(plane, to_intel_crtc(crtc));
> -		intel_plane->update_plane(intel_plane,
> -					  to_intel_crtc_state(crtc->state),
> +		intel_plane->update_plane(intel_plane, crtc_state,
>  					  to_intel_plane_state(plane->state));
>  	} else {
>  		trace_intel_disable_plane(plane, to_intel_crtc(crtc));
> @@ -13604,6 +13611,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  out_free:
> +	if (new_crtc_state)
> +		intel_crtc_destroy_state(crtc, &new_crtc_state->base);
>  	if (ret)
>  		intel_plane_destroy_state(plane, new_plane_state);
>  	else
> -- 
> 2.18.0
> 
> _______________________________________________
> 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

Re: [PATCH 03/15] drm/i915: Make intel_crtc_disable_planes() use active planes mask.

[Matt Roper]

On Wed, Sep 19, 2018 at 03:56:32PM +0200, Maarten Lankhorst wrote:
> This will only disable planes we actually had marked as visible in
> crtc_state->visible_planes and cleans up intel_crtc_disable_plane()
> slightly.
> 
> This is also useful for when we start enabling NV12 support, in which
> we will make the separate Y plane visible, but ignore the Y plane's state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7a7ff1d76031..f7982f18b8fe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5402,24 +5402,23 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>  		intel_update_watermarks(crtc);
>  }
>  
> -static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> +static void intel_crtc_disable_planes(struct intel_crtc *crtc, unsigned plane_mask)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct drm_plane *p;
> -	int pipe = intel_crtc->pipe;
> +	struct drm_device *dev = crtc->base.dev;
> +	struct intel_plane *plane;
> +	unsigned fb_bits = 0;
>  
> -	intel_crtc_dpms_overlay_disable(intel_crtc);
> +	intel_crtc_dpms_overlay_disable(crtc);
>  
> -	drm_for_each_plane_mask(p, dev, plane_mask)
> -		to_intel_plane(p)->disable_plane(to_intel_plane(p), intel_crtc);
> +	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		if (plane_mask & BIT(plane->id)) {
> +			plane->disable_plane(plane, crtc);
>  
> -	/*
> -	 * FIXME: Once we grow proper nuclear flip support out of this we need
> -	 * to compute the mask of flip planes precisely. For the time being
> -	 * consider this a flip to a NULL plane.
> -	 */
> -	intel_frontbuffer_flip(to_i915(dev), INTEL_FRONTBUFFER_ALL_MASK(pipe));
> +			fb_bits |= plane->frontbuffer_bit;
> +		}
> +	}
> +
> +	intel_frontbuffer_flip(to_i915(dev), fb_bits);
>  }
>  
>  static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
> @@ -12697,7 +12696,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
>  
>  		if (old_crtc_state->active) {
> -			intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
> +			intel_crtc_disable_planes(intel_crtc, old_intel_crtc_state->active_planes);
>  
>  			/*
>  			 * We need to disable pipe CRC before disabling the pipe,
> -- 
> 2.18.0
> 
> _______________________________________________
> 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

Re: [PATCH 04/15] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.

[Matt Roper]

On Wed, Sep 19, 2018 at 03:56:33PM +0200, Maarten Lankhorst wrote:
> drm_atomic_helper_commit_planes_on_crtc calls begin_commit,
> then plane_update hooks, then flush_commit. Because we keep our own
> visibility tracking through plane_state->visible there's no need to
> rely on the atomic hooks for this.
> 
> By explicitly writing our own helper, we can update visible planes
> as needed, which is useful to make NV12 support work as intended.
> 
> Changes since v1:
> - Reword commit message. (Matt Roper)
> - Rename to intel_update_planes_on_crtc(). (Matt)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

This patch does leave drm_atomic_helper_commit_planes_on_crtc() unused
by any driver.  Not sure if we need to follow up with a patch to remove
it as dead code, or whether we anticipate other drivers wanting to use
it soon.


Matt

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 41 ++++++++++++-----------
>  drivers/gpu/drm/i915/intel_display.c      | 10 ++++--
>  drivers/gpu/drm/i915/intel_drv.h          |  4 +++
>  3 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index aabebe0d2e9b..e067fb531a29 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -164,29 +164,33 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  						   to_intel_plane_state(new_plane_state));
>  }
>  
> -static void intel_plane_atomic_update(struct drm_plane *plane,
> -				      struct drm_plane_state *old_state)
> +void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> +				 struct intel_crtc *crtc,
> +				 struct intel_crtc_state *old_crtc_state,
> +				 struct intel_crtc_state *new_crtc_state)
>  {
> -	struct intel_atomic_state *state = to_intel_atomic_state(old_state->state);
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	const struct intel_plane_state *new_plane_state =
> -		intel_atomic_get_new_plane_state(state, intel_plane);
> -	struct drm_crtc *crtc = new_plane_state->base.crtc ?: old_state->crtc;
> +	struct intel_plane_state *new_plane_state;
> +	struct intel_plane *plane;
> +	u32 update_mask;
> +	int i;
> +
> +	update_mask = old_crtc_state->active_planes;
> +	update_mask |= new_crtc_state->active_planes;
>  
> -	if (new_plane_state->base.visible) {
> -		const struct intel_crtc_state *new_crtc_state =
> -			intel_atomic_get_new_crtc_state(state, to_intel_crtc(crtc));
> +	for_each_new_intel_plane_in_state(old_state, plane, new_plane_state, i) {
> +		if (crtc->pipe != plane->pipe ||
> +		    !(update_mask & BIT(plane->id)))
> +			continue;
>  
> -		trace_intel_update_plane(plane,
> -					 to_intel_crtc(crtc));
> +		if (new_plane_state->base.visible) {
> +			trace_intel_update_plane(&plane->base, crtc);
>  
> -		intel_plane->update_plane(intel_plane,
> -					  new_crtc_state, new_plane_state);
> -	} else {
> -		trace_intel_disable_plane(plane,
> -					  to_intel_crtc(crtc));
> +			plane->update_plane(plane, new_crtc_state, new_plane_state);
> +		} else {
> +			trace_intel_disable_plane(&plane->base, crtc);
>  
> -		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
> +			plane->disable_plane(plane, crtc);
> +		}
>  	}
>  }
>  
> @@ -194,7 +198,6 @@ const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
>  	.prepare_fb = intel_prepare_plane_fb,
>  	.cleanup_fb = intel_cleanup_plane_fb,
>  	.atomic_check = intel_plane_atomic_check,
> -	.atomic_update = intel_plane_atomic_update,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f7982f18b8fe..3975f3af4fb4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10813,8 +10813,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> -	.atomic_begin = intel_begin_crtc_commit,
> -	.atomic_flush = intel_finish_crtc_commit,
>  	.atomic_check = intel_crtc_atomic_check,
>  };
>  
> @@ -12483,6 +12481,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *old_intel_cstate = to_intel_crtc_state(old_crtc_state);
>  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
>  	bool modeset = needs_modeset(new_crtc_state);
>  	struct intel_plane_state *new_plane_state =
> @@ -12503,7 +12502,12 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	if (new_plane_state)
>  		intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
>  
> -	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
> +	intel_begin_crtc_commit(crtc, old_crtc_state);
> +
> +	intel_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc,
> +				    old_intel_cstate, pipe_config);
> +
> +	intel_finish_crtc_commit(crtc, old_crtc_state);
>  }
>  
>  static void intel_update_crtcs(struct drm_atomic_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf1c38728a59..8073a85d7178 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2188,6 +2188,10 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
>  void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> +void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> +				 struct intel_crtc *crtc,
> +				 struct intel_crtc_state *old_crtc_state,
> +				 struct intel_crtc_state *new_crtc_state);
>  int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
>  					struct intel_crtc_state *crtc_state,
>  					const struct intel_plane_state *old_plane_state,
> -- 
> 2.18.0
> 
> _______________________________________________
> 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

Re: [PATCH 02/15] drm/i915: Handle cursor updating active_planes correctly.

[Maarten Lankhorst]

Op 20-09-18 om 02:12 schreef Matt Roper:
> On Wed, Sep 19, 2018 at 03:56:31PM +0200, Maarten Lankhorst wrote:
>> While we may not update new_crtc_state, we may clear active_planes
>> if the new cursor update state will disable the cursor, but we fail
>> after. If this is immediately followed by a modeset disable, we may
>> soon not disable the planes correctly when we start depending on
>> active_planes.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: b2b55502d683c ("drm/i915: Pass proper old/new states to
> intel_plane_atomic_check_with_state()")
>
> I think?  So basically we were clobbering the current cstate's
> active_planes field (and anything else in cstate that
> intel_plane_atomic_check_with_state() might touch in the future) if the
> atomic transaction failed.

This is in theory a fix, but up until now we don't depend on it. It only
starts mattering further down where we start to updating/disabling planes
based on this field.

This is why I didn't add -fixes.

>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++--------
>>  1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 2ac381eb8103..7a7ff1d76031 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13517,14 +13517,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>>  	struct drm_framebuffer *old_fb;
>> -	struct drm_crtc_state *crtc_state = crtc->state;
>> +	struct intel_crtc_state *crtc_state =
>> +		to_intel_crtc_state(crtc->state);
>> +	struct intel_crtc_state *new_crtc_state;
>>  
>>  	/*
>>  	 * When crtc is inactive or there is a modeset pending,
>>  	 * wait for it to complete in the slowpath
>>  	 */
>> -	if (!crtc_state->active || needs_modeset(crtc_state) ||
>> -	    to_intel_crtc_state(crtc_state)->update_pipe)
>> +	if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
>> +	    crtc_state->update_pipe)
>>  		goto slow;
>>  
>>  	old_plane_state = plane->state;
>> @@ -13554,6 +13556,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>  	if (!new_plane_state)
>>  		return -ENOMEM;
>>  
>> +	new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
>> +	if (!new_crtc_state) {
>> +		ret = -ENOMEM;
>> +		goto out_free;
>> +	}
>> +
>>  	drm_atomic_set_fb_for_plane(new_plane_state, fb);
>>  
>>  	new_plane_state->src_x = src_x;
>> @@ -13565,9 +13573,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>  	new_plane_state->crtc_w = crtc_w;
>>  	new_plane_state->crtc_h = crtc_h;
>>  
>> -	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
>> -						  to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
>> -						  to_intel_plane_state(plane->state),
>> +	ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
>> +						  to_intel_plane_state(old_plane_state),
>>  						  to_intel_plane_state(new_plane_state));
>>  	if (ret)
>>  		goto out_free;
>> @@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>  
>>  	/* Swap plane state */
>>  	plane->state = new_plane_state;
>> +	crtc_state->active_planes = new_crtc_state->active_planes;
> At the moment this is the only crtc_state field that our plane check
> might update for a cursor, but is there a reason why we don't just swap
> in the new state and destroy the old one farther down?  If our driver
> ever did start updating other fields, it would be really easy to
> overlook the need to copy those new fields over here.  I realize a lot
> of the likely candidates for fields that plane check might want to
> update (e.g., watermark stuff) would also disqualify us from taking this
> legacy cursor fastpath in the first place, but I'm a bit nervous about
> assuming that we'll never be updating any other fields at all.

I wish! This was what I first tried. Then realized it was a very very bad idea.
We wait for plane_state->hw_done to be set, but an atomic update may be in
progress. In which case our old_crtc_state is its new_crtc_state.

If we start freeing that.. really bad things happen. (TM)

I'll add a comment here to explain why we only update crtc_state.

In theory it could be a problem that we update crtc_state->active_planes life,
but the place that will depend on it checks if the plane is part of the atomic
commit, and we ruled out that's the case. So even if we change it, either value
will cause the same behavior when running the atomic plane update part. We've
already ruled out modesets, so we won't race with disabling all planes.

~Maarten

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