[PATCH 0/7] drm/i915/gen11: Enable planar format support on gen11.

[PATCH 0/7] drm/i915/gen11: Enable planar format support on gen11.

[Maarten Lankhorst]

With all the preparation patches upstreamed, time to enable NV12. :)

Maarten Lankhorst (7):
  drm/i915/gen11: Enable 6 sprites on gen11
  drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.
  drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.
  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           |  25 ++++
 drivers/gpu/drm/i915/intel_atomic.c       |   6 +-
 drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++---
 drivers/gpu/drm/i915/intel_device_info.c  |   5 +-
 drivers/gpu/drm/i915/intel_display.c      | 103 +++++++++++++----
 drivers/gpu/drm/i915/intel_display.h      |   3 +
 drivers/gpu/drm/i915/intel_drv.h          |  61 ++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 134 ++++++++++++++--------
 drivers/gpu/drm/i915/intel_sprite.c       |  75 +++++++++---
 9 files changed, 414 insertions(+), 104 deletions(-)

-- 
2.18.0

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

[PATCH 1/7] 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 9fac67e31205..47000b25f1ff 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

Re: [PATCH 1/7] drm/i915/gen11: Enable 6 sprites on gen11

[Matt Roper]

On Fri, Sep 21, 2018 at 07:39:39PM +0200, Maarten Lankhorst wrote:
> 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>

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

I notcie the comment above this if statement is a bit stale since it
only mentions SKL and BXT, but not KBL, CFL, WHL, AML.  Just to avoid
confusion, we might want to update that to indicate the mutual exclusion
applies to all gen9 except for GLK and that the mutual exclusion is no
longer relevant on gen10+.


Matt

> ---
>  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 9fac67e31205..47000b25f1ff 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

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

[PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[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().
Changes since v2:
- Fix intel_atomic_get_old_plane_state() to actually return old state.
- Move visibility changes to preparation patch.
- Only try to find a Y plane on gen11, earlier platforms only require a single plane.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

fixup Y/UV Linkage

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
 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(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 984bc1f26625..522699085a59 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
 	intel_state->base.visible = false;
 
-	/* If this is a cursor plane, no further checks are needed. */
+	/* Destroy the link */
+	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;
 
@@ -142,27 +146,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 +240,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 390907d77ecd..19cd6bbb43c4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10726,6 +10726,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 (INTEL_GEN(dev_priv) < 11 || !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)
 {
@@ -10797,6 +10851,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..29c7a4bb484d 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_old_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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> 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().
> Changes since v2:
> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> - Move visibility changes to preparation patch.
> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> fixup Y/UV Linkage
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
>  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(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 984bc1f26625..522699085a59 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
>  	intel_state->base.visible = false;
>  
> -	/* If this is a cursor plane, no further checks are needed. */
> +	/* Destroy the link */
> +	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;
>  
> @@ -142,27 +146,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;
> +	}

This is all rather confusing. Can't we just do a preprocessing step
before check_planes() to add the linked planes as needed, and then
let the normal check_planes() do its thing?

>  
> -	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 +240,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 390907d77ecd..19cd6bbb43c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10726,6 +10726,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 (INTEL_GEN(dev_priv) < 11 || !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);

Don't really like the aux name here. Each plane can have an aux surface
so now having aux planes is getting a bit overloady. You called it slave
elsewhere so I'd stick to that everywhere.

> +		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)
>  {
> @@ -10797,6 +10851,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..29c7a4bb484d 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;

Wondering if separate *slave and *master pointers would be make things
more obvioius which one we're looking at in each piece of code.

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

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> > 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().
> > Changes since v2:
> > - Fix intel_atomic_get_old_plane_state() to actually return old state.
> > - Move visibility changes to preparation patch.
> > - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> > fixup Y/UV Linkage
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> >  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(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 984bc1f26625..522699085a59 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> >  	intel_state->base.visible = false;
> >  
> > -	/* If this is a cursor plane, no further checks are needed. */
> > +	/* Destroy the link */
> > +	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;
> >  
> > @@ -142,27 +146,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;
> > +	}
> 
> This is all rather confusing. Can't we just do a preprocessing step
> before check_planes() to add the linked planes as needed, and then
> let the normal check_planes() do its thing?

Also one thing that slightly worries me is what happens if someone adds
more planes to the state after this has all been done. I guess
currently those cases would either come from the tail end of
intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
would appear that wm/ddb is the only piece of code that can do this
sort of thing.

Some ideas how to solve it perhaps:
- Move the linked plane pointer to the core so that get_plane_state()
  can immediately pick up both planes
- Add some kind of flag into the top level atomic state that after this
  point no new planes. But I guess duplicate_state() is the only thing
  we have and that doesn't know anything about the the top level state
- Live with it and tell everyone to be careful? Some kind of
  intel_add_affected_planes() might be helpful here to at least give
  people one thing to call in the late stages. Wouldn't help if the
  call comes from some helper/core function though. And as there is
  no use for such a function currently no point adding it I suppose.
  The wm stuff has its own thing, which you covered already.

Dunno, maybe I'm just a bit too paranoid here.

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Maarten Lankhorst]

Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
>> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
>>> 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().
>>> Changes since v2:
>>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
>>> - Move visibility changes to preparation patch.
>>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>
>>> fixup Y/UV Linkage
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
>>>  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(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> index 984bc1f26625..522699085a59 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
>>>  	intel_state->base.visible = false;
>>>  
>>> -	/* If this is a cursor plane, no further checks are needed. */
>>> +	/* Destroy the link */
>>> +	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;
>>>  
>>> @@ -142,27 +146,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;
>>> +	}
>> This is all rather confusing. Can't we just do a preprocessing step
>> before check_planes() to add the linked planes as needed, and then
>> let the normal check_planes() do its thing?
> Also one thing that slightly worries me is what happens if someone adds
> more planes to the state after this has all been done. I guess
> currently those cases would either come from the tail end of
> intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
> would appear that wm/ddb is the only piece of code that can do this
> sort of thing.
I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
Otherwise you can't rerun any validation as required.

I've now added a function icl_add_linked_planes helper that iterates over all planes in
the state, and adds any linked planes to the transaction.

This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
planes are added, without doing it from intel_plane_atomic_check()

WM will continue to do its own thing, since it's a design error IMO that it even adds
planes to the state to begin with. :)

Which is the last option you mentioned below.

> Some ideas how to solve it perhaps:
> - Move the linked plane pointer to the core so that get_plane_state()
>   can immediately pick up both planes
> - Add some kind of flag into the top level atomic state that after this
>   point no new planes. But I guess duplicate_state() is the only thing
>   we have and that doesn't know anything about the the top level state
> - Live with it and tell everyone to be careful? Some kind of
>   intel_add_affected_planes() might be helpful here to at least give
>   people one thing to call in the late stages. Wouldn't help if the
>   call comes from some helper/core function though. And as there is
>   no use for such a function currently no point adding it I suppose.
>   The wm stuff has its own thing, which you covered already.
>
> Dunno, maybe I'm just a bit too paranoid here.
>

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> >>> 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().
> >>> Changes since v2:
> >>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> >>> - Move visibility changes to preparation patch.
> >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>
> >>> fixup Y/UV Linkage
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> >>>  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(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> index 984bc1f26625..522699085a59 100644
> >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> >>>  	intel_state->base.visible = false;
> >>>  
> >>> -	/* If this is a cursor plane, no further checks are needed. */
> >>> +	/* Destroy the link */
> >>> +	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;
> >>>  
> >>> @@ -142,27 +146,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;
> >>> +	}
> >> This is all rather confusing. Can't we just do a preprocessing step
> >> before check_planes() to add the linked planes as needed, and then
> >> let the normal check_planes() do its thing?
> > Also one thing that slightly worries me is what happens if someone adds
> > more planes to the state after this has all been done. I guess
> > currently those cases would either come from the tail end of
> > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
> > would appear that wm/ddb is the only piece of code that can do this
> > sort of thing.
> I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
> Otherwise you can't rerun any validation as required.

You shouldn't need validation for eg. dpms on/off. I guess we currently
do that although we shouldn't have to.

> 
> I've now added a function icl_add_linked_planes helper that iterates over all planes in
> the state, and adds any linked planes to the transaction.
> 
> This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
> planes are added, without doing it from intel_plane_atomic_check()
> 
> WM will continue to do its own thing, since it's a design error IMO that it even adds
> planes to the state to begin with. :)

It pretty much has to. The design error we have at the moment is not
programming the watermarks from the update_plane()/disable_plane().
That one I've attempted to fix in:
git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence

And supposedly that one does fix bugs related to watermarks vs.
plane updates.

> 
> Which is the last option you mentioned below.
> 
> > Some ideas how to solve it perhaps:
> > - Move the linked plane pointer to the core so that get_plane_state()
> >   can immediately pick up both planes
> > - Add some kind of flag into the top level atomic state that after this
> >   point no new planes. But I guess duplicate_state() is the only thing
> >   we have and that doesn't know anything about the the top level state
> > - Live with it and tell everyone to be careful? Some kind of
> >   intel_add_affected_planes() might be helpful here to at least give
> >   people one thing to call in the late stages. Wouldn't help if the
> >   call comes from some helper/core function though. And as there is
> >   no use for such a function currently no point adding it I suppose.
> >   The wm stuff has its own thing, which you covered already.
> >
> > Dunno, maybe I'm just a bit too paranoid here.
> >

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Maarten Lankhorst]

Op 24-09-18 om 15:18 schreef Ville Syrjälä:
> On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
>> Op 21-09-18 om 21:31 schreef Ville Syrjälä:
>>> On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
>>>> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
>>>>> 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().
>>>>> Changes since v2:
>>>>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
>>>>> - Move visibility changes to preparation patch.
>>>>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>
>>>>> fixup Y/UV Linkage
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
>>>>>  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(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>> index 984bc1f26625..522699085a59 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>>>>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
>>>>>  	intel_state->base.visible = false;
>>>>>  
>>>>> -	/* If this is a cursor plane, no further checks are needed. */
>>>>> +	/* Destroy the link */
>>>>> +	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;
>>>>>  
>>>>> @@ -142,27 +146,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;
>>>>> +	}
>>>> This is all rather confusing. Can't we just do a preprocessing step
>>>> before check_planes() to add the linked planes as needed, and then
>>>> let the normal check_planes() do its thing?
>>> Also one thing that slightly worries me is what happens if someone adds
>>> more planes to the state after this has all been done. I guess
>>> currently those cases would either come from the tail end of
>>> intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
>>> would appear that wm/ddb is the only piece of code that can do this
>>> sort of thing.
>> I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
>> Otherwise you can't rerun any validation as required.
> You shouldn't need validation for eg. dpms on/off. I guess we currently
> do that although we shouldn't have to.
We should, if we ever have 2 crtc's active and disable 1. Watermarks should be
distributed over active planes only, which dpms toggle affects. Only way around setting
plane_state->visible when plane is inactive and calculate minimum requirements, then
calculate max requirements.

We would have to fix up all of wm programming and plane programming to make it work.

I don't think the extra complexity is worth the effort, tbh..
>> I've now added a function icl_add_linked_planes helper that iterates over all planes in
>> the state, and adds any linked planes to the transaction.
>>
>> This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
>> planes are added, without doing it from intel_plane_atomic_check()
>>
>> WM will continue to do its own thing, since it's a design error IMO that it even adds
>> planes to the state to begin with. :)
> It pretty much has to. The design error we have at the moment is not
> programming the watermarks from the update_plane()/disable_plane().
> That one I've attempted to fix in:
> git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
>
> And supposedly that one does fix bugs related to watermarks vs.
> plane updates.

Yeah, fortunately it shouldn't affect this code much, should be easy to rebase. :)
Though I would like to get rid of skl_next_plane_to_commit, ugh..

Which is probably a confirmation that the nv12 gen11 series isn't making plane programming
that much more complicated, fortunately.

I would really like to simplify the locking first at some point. It can't be good to sync write everything.

each plane update now does:

spin_lock()
I915_WRITE_FW(...)

POSTING_READ()
spin_unlock()

Would be nice if we 

~Maarten

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 12:03:47PM +0200, Maarten Lankhorst wrote:
> Op 24-09-18 om 15:18 schreef Ville Syrjälä:
> > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
> >> Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> >>> On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> >>>> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> >>>>> 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().
> >>>>> Changes since v2:
> >>>>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> >>>>> - Move visibility changes to preparation patch.
> >>>>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>
> >>>>> fixup Y/UV Linkage
> >>>>>
> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> >>>>>  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(+), 18 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> index 984bc1f26625..522699085a59 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >>>>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> >>>>>  	intel_state->base.visible = false;
> >>>>>  
> >>>>> -	/* If this is a cursor plane, no further checks are needed. */
> >>>>> +	/* Destroy the link */
> >>>>> +	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;
> >>>>>  
> >>>>> @@ -142,27 +146,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;
> >>>>> +	}
> >>>> This is all rather confusing. Can't we just do a preprocessing step
> >>>> before check_planes() to add the linked planes as needed, and then
> >>>> let the normal check_planes() do its thing?
> >>> Also one thing that slightly worries me is what happens if someone adds
> >>> more planes to the state after this has all been done. I guess
> >>> currently those cases would either come from the tail end of
> >>> intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
> >>> would appear that wm/ddb is the only piece of code that can do this
> >>> sort of thing.
> >> I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
> >> Otherwise you can't rerun any validation as required.
> > You shouldn't need validation for eg. dpms on/off. I guess we currently
> > do that although we shouldn't have to.
> We should, if we ever have 2 crtc's active and disable 1. Watermarks should be
> distributed over active planes only, which dpms toggle affects.

I think the rule we want aim for is .enable controls watermarks,
.active does not matter. That will guarantee that dpms on will never
fail on account of watermarks. The current way of doing things is
IMO just wrong and we should eventually fix it.

> Only way around setting
> plane_state->visible when plane is inactive and calculate minimum requirements, then
> calculate max requirements.
> 
> We would have to fix up all of wm programming and plane programming to make it work.
> 
> I don't think the extra complexity is worth the effort, tbh..
> >> I've now added a function icl_add_linked_planes helper that iterates over all planes in
> >> the state, and adds any linked planes to the transaction.
> >>
> >> This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
> >> planes are added, without doing it from intel_plane_atomic_check()
> >>
> >> WM will continue to do its own thing, since it's a design error IMO that it even adds
> >> planes to the state to begin with. :)
> > It pretty much has to. The design error we have at the moment is not
> > programming the watermarks from the update_plane()/disable_plane().
> > That one I've attempted to fix in:
> > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
> >
> > And supposedly that one does fix bugs related to watermarks vs.
> > plane updates.
> 
> Yeah, fortunately it shouldn't affect this code much, should be easy to rebase. :)
> Though I would like to get rid of skl_next_plane_to_commit, ugh..

That thing is pretty much whole point of the series. It could of
course be written in a different way, but the idea of ddb
allocations driving the plane commit order is key.

> 
> Which is probably a confirmation that the nv12 gen11 series isn't making plane programming
> that much more complicated, fortunately.
> 
> I would really like to simplify the locking first at some point. It can't be good to sync write everything.
> 
> each plane update now does:
> 
> spin_lock()
> I915_WRITE_FW(...)
> 
> POSTING_READ()

Posting reads we should just nuke I think.

> spin_unlock()
> 
> Would be nice if we 

Cut off, but I'll assume you wanted to suggest grabbing the lock just
once for the entire pipe commit. I do agree that it would probably be
nicer. But I still maintain that splitting the lock to display engine
vs. "the rest" is probably something we want to do as well. The
annoying thing with that was that I couldn't trick the compiler select
the right lock at compile time, so I think this would involve splitting
I915_READ/WRITE & co. for display engine vs. the rest as well. I have
a feeling I even typed up something like that at one point, just never
had the time to test it.

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Matt Roper]

On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
> > Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> > >>> 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().
> > >>> Changes since v2:
> > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> > >>> - Move visibility changes to preparation patch.
> > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> > >>>
> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>>
> > >>> fixup Y/UV Linkage
> > >>>
> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>> ---
> > >>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> > >>>  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(+), 18 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > >>> index 984bc1f26625..522699085a59 100644
> > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> > >>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> > >>>  	intel_state->base.visible = false;
> > >>>  
> > >>> -	/* If this is a cursor plane, no further checks are needed. */
> > >>> +	/* Destroy the link */
> > >>> +	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;
> > >>>  
> > >>> @@ -142,27 +146,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;
> > >>> +	}
> > >> This is all rather confusing. Can't we just do a preprocessing step
> > >> before check_planes() to add the linked planes as needed, and then
> > >> let the normal check_planes() do its thing?
> > > Also one thing that slightly worries me is what happens if someone adds
> > > more planes to the state after this has all been done. I guess
> > > currently those cases would either come from the tail end of
> > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
> > > would appear that wm/ddb is the only piece of code that can do this
> > > sort of thing.
> > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
> > Otherwise you can't rerun any validation as required.
> 
> You shouldn't need validation for eg. dpms on/off. I guess we currently
> do that although we shouldn't have to.
> 
> > 
> > I've now added a function icl_add_linked_planes helper that iterates over all planes in
> > the state, and adds any linked planes to the transaction.
> > 
> > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
> > planes are added, without doing it from intel_plane_atomic_check()
> > 
> > WM will continue to do its own thing, since it's a design error IMO that it even adds
> > planes to the state to begin with. :)
> 
> It pretty much has to. The design error we have at the moment is not
> programming the watermarks from the update_plane()/disable_plane().
> That one I've attempted to fix in:
> git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
> 
> And supposedly that one does fix bugs related to watermarks vs.
> plane updates.

Where did the bugs arise?  Were we unsuccessful at actually evading the
vblank (leading to the planes and watermarks taking effect on different
vblanks) or is it something else?


Matt

> 
> > 
> > Which is the last option you mentioned below.
> > 
> > > Some ideas how to solve it perhaps:
> > > - Move the linked plane pointer to the core so that get_plane_state()
> > >   can immediately pick up both planes
> > > - Add some kind of flag into the top level atomic state that after this
> > >   point no new planes. But I guess duplicate_state() is the only thing
> > >   we have and that doesn't know anything about the the top level state
> > > - Live with it and tell everyone to be careful? Some kind of
> > >   intel_add_affected_planes() might be helpful here to at least give
> > >   people one thing to call in the late stages. Wouldn't help if the
> > >   call comes from some helper/core function though. And as there is
> > >   no use for such a function currently no point adding it I suppose.
> > >   The wm stuff has its own thing, which you covered already.
> > >
> > > Dunno, maybe I'm just a bit too paranoid here.
> > >
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> 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 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote:
> On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
> > > Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> > > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> > > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> > > >>> 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().
> > > >>> Changes since v2:
> > > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> > > >>> - Move visibility changes to preparation patch.
> > > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> > > >>>
> > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > >>>
> > > >>> fixup Y/UV Linkage
> > > >>>
> > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > >>> ---
> > > >>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> > > >>>  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(+), 18 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > >>> index 984bc1f26625..522699085a59 100644
> > > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> > > >>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> > > >>>  	intel_state->base.visible = false;
> > > >>>  
> > > >>> -	/* If this is a cursor plane, no further checks are needed. */
> > > >>> +	/* Destroy the link */
> > > >>> +	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;
> > > >>>  
> > > >>> @@ -142,27 +146,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;
> > > >>> +	}
> > > >> This is all rather confusing. Can't we just do a preprocessing step
> > > >> before check_planes() to add the linked planes as needed, and then
> > > >> let the normal check_planes() do its thing?
> > > > Also one thing that slightly worries me is what happens if someone adds
> > > > more planes to the state after this has all been done. I guess
> > > > currently those cases would either come from the tail end of
> > > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
> > > > would appear that wm/ddb is the only piece of code that can do this
> > > > sort of thing.
> > > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
> > > Otherwise you can't rerun any validation as required.
> > 
> > You shouldn't need validation for eg. dpms on/off. I guess we currently
> > do that although we shouldn't have to.
> > 
> > > 
> > > I've now added a function icl_add_linked_planes helper that iterates over all planes in
> > > the state, and adds any linked planes to the transaction.
> > > 
> > > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
> > > planes are added, without doing it from intel_plane_atomic_check()
> > > 
> > > WM will continue to do its own thing, since it's a design error IMO that it even adds
> > > planes to the state to begin with. :)
> > 
> > It pretty much has to. The design error we have at the moment is not
> > programming the watermarks from the update_plane()/disable_plane().
> > That one I've attempted to fix in:
> > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
> > 
> > And supposedly that one does fix bugs related to watermarks vs.
> > plane updates.
> 
> Where did the bugs arise?  Were we unsuccessful at actually evading the
> vblank (leading to the planes and watermarks taking effect on different
> vblanks) or is it something else?

There are a few issues here:
- write to any plane registers apart from SURF will cancel an already
  pending plane update. Well, it's not 100% cancelled as some registers
  aren't part of the SURF based arming mechanism but IIRC they still
  cause a cancellation. This means doing a watermark update before a
  pending plane update was latched cancels most of the plane update.
  This at least caused the cursor to remain on in when it should have
  been turned off.
- overlapping DDB allocations can hard hang the box. So any vblank that
  sneaks in while we've partiially reprogrammed the ddb could be a
  death sentence. A suggested solution was to turn off the planes
  before ddb reprogramming but that didn't work out so well when I
  tried it due to the whole cancellation thing.

So I pulled in the ddb/wm programming into the normal plane update.
That means no more accidental cancellations from elsewhere.

And to avoid any ddb overlaps we simply sequence the plane updates
carefully. It's pretty much the same algorithm that we use to avoid
ddb overlaps between pipes, with the exception that we don't need the
vblank waits. So if a vblank does sneak at any point during the
sequence we can be assured that the partially latched state does not
have any overlapping ddb allocations.

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Matt Roper]

On Tue, Sep 25, 2018 at 09:34:29PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote:
> > On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote:
> > > On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
> > > > Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> > > > > On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> > > > >> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> > > > >>> 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().
> > > > >>> Changes since v2:
> > > > >>> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> > > > >>> - Move visibility changes to preparation patch.
> > > > >>> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> > > > >>>
> > > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > >>>
> > > > >>> fixup Y/UV Linkage
> > > > >>>
> > > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > >>> ---
> > > > >>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> > > > >>>  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(+), 18 deletions(-)
> > > > >>>
> > > > >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > >>> index 984bc1f26625..522699085a59 100644
> > > > >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > > >>> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> > > > >>>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> > > > >>>  	intel_state->base.visible = false;
> > > > >>>  
> > > > >>> -	/* If this is a cursor plane, no further checks are needed. */
> > > > >>> +	/* Destroy the link */
> > > > >>> +	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;
> > > > >>>  
> > > > >>> @@ -142,27 +146,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;
> > > > >>> +	}
> > > > >> This is all rather confusing. Can't we just do a preprocessing step
> > > > >> before check_planes() to add the linked planes as needed, and then
> > > > >> let the normal check_planes() do its thing?
> > > > > Also one thing that slightly worries me is what happens if someone adds
> > > > > more planes to the state after this has all been done. I guess
> > > > > currently those cases would either come from the tail end of
> > > > > intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
> > > > > would appear that wm/ddb is the only piece of code that can do this
> > > > > sort of thing.
> > > > I think this shouldn't happen, and WM is special. The only time where you want to add more planes is before check_planes().
> > > > Otherwise you can't rerun any validation as required.
> > > 
> > > You shouldn't need validation for eg. dpms on/off. I guess we currently
> > > do that although we shouldn't have to.
> > > 
> > > > 
> > > > I've now added a function icl_add_linked_planes helper that iterates over all planes in
> > > > the state, and adds any linked planes to the transaction.
> > > > 
> > > > This is run right before drm_atomic_helper_check_planes(), so we're sure that all linked
> > > > planes are added, without doing it from intel_plane_atomic_check()
> > > > 
> > > > WM will continue to do its own thing, since it's a design error IMO that it even adds
> > > > planes to the state to begin with. :)
> > > 
> > > It pretty much has to. The design error we have at the moment is not
> > > programming the watermarks from the update_plane()/disable_plane().
> > > That one I've attempted to fix in:
> > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
> > > 
> > > And supposedly that one does fix bugs related to watermarks vs.
> > > plane updates.
> > 
> > Where did the bugs arise?  Were we unsuccessful at actually evading the
> > vblank (leading to the planes and watermarks taking effect on different
> > vblanks) or is it something else?
> 
> There are a few issues here:
> - write to any plane registers apart from SURF will cancel an already
>   pending plane update. Well, it's not 100% cancelled as some registers
>   aren't part of the SURF based arming mechanism but IIRC they still
>   cause a cancellation. This means doing a watermark update before a
>   pending plane update was latched cancels most of the plane update.
>   This at least caused the cursor to remain on in when it should have
>   been turned off.

Wow, I think I've run into this problem before, but never figured out
what the exact root cause was.  I tried to write an IGT to capture it a
while back, but the behavior went away with my simpler tests, probably
because I wasn't changing enough settings to trigger the necessary
watermark updates.

The behavior was puzzling since some of the plane updates actually would
take effect (e.g., PLANE_POS being zeroed would move the plane back to
0,0), but others wouldn't (most notable the enable/disable bit in
PLANE_CTL).  So the result might be a garbage rectangle in the upper
left corner of the screen or a storm of GTT page faults that would bring
make the system unusable.

Is this actually expected behavior that's documented somewhere in the
bspec, or is it just something you've discovered through
experimentation?  I couldn't find any explanation for updates getting
partially unarmed when I went through the bspec a while back, but I may
have overlooked something.

> - overlapping DDB allocations can hard hang the box. So any vblank that
>   sneaks in while we've partiially reprogrammed the ddb could be a
>   death sentence. A suggested solution was to turn off the planes
>   before ddb reprogramming but that didn't work out so well when I
>   tried it due to the whole cancellation thing.
> 
> So I pulled in the ddb/wm programming into the normal plane update.
> That means no more accidental cancellations from elsewhere.
> 
> And to avoid any ddb overlaps we simply sequence the plane updates
> carefully. It's pretty much the same algorithm that we use to avoid
> ddb overlaps between pipes, with the exception that we don't need the
> vblank waits. So if a vblank does sneak at any point during the
> sequence we can be assured that the partially latched state does not
> have any overlapping ddb allocations.

Sounds reasonable.  Do you think we should try to land your work before
Maarten's gen11 NV12 patches?


Matt

> 
> -- 
> Ville Syrjälä
> Intel

-- 
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 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 01:18:43PM -0700, Matt Roper wrote:
> On Tue, Sep 25, 2018 at 09:34:29PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote:
> > > On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote:
<snip>
> > > > It pretty much has to. The design error we have at the moment is not
> > > > programming the watermarks from the update_plane()/disable_plane().
> > > > That one I've attempted to fix in:
> > > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
> > > > 
> > > > And supposedly that one does fix bugs related to watermarks vs.
> > > > plane updates.
> > > 
> > > Where did the bugs arise?  Were we unsuccessful at actually evading the
> > > vblank (leading to the planes and watermarks taking effect on different
> > > vblanks) or is it something else?
> > 
> > There are a few issues here:
> > - write to any plane registers apart from SURF will cancel an already
> >   pending plane update. Well, it's not 100% cancelled as some registers
> >   aren't part of the SURF based arming mechanism but IIRC they still
> >   cause a cancellation. This means doing a watermark update before a
> >   pending plane update was latched cancels most of the plane update.
> >   This at least caused the cursor to remain on in when it should have
> >   been turned off.
> 
> Wow, I think I've run into this problem before, but never figured out
> what the exact root cause was.  I tried to write an IGT to capture it a
> while back, but the behavior went away with my simpler tests, probably
> because I wasn't changing enough settings to trigger the necessary
> watermark updates.
> 
> The behavior was puzzling since some of the plane updates actually would
> take effect (e.g., PLANE_POS being zeroed would move the plane back to
> 0,0), but others wouldn't (most notable the enable/disable bit in
> PLANE_CTL).  So the result might be a garbage rectangle in the upper
> left corner of the screen or a storm of GTT page faults that would bring
> make the system unusable.
> 
> Is this actually expected behavior that's documented somewhere in the
> bspec, or is it just something you've discovered through
> experimentation?  I couldn't find any explanation for updates getting
> partially unarmed when I went through the bspec a while back, but I may
> have overlooked something.

I don't see the cancellation behaviour mentioned in the current spec.
It was actually documented for gen3 cursors (see eg. [1]) but apart
from that there is no mention of it in any spec AFAICS. I believe
only the cursors had this behaviour on gen3, and I think even that
was changed again to not cancel around the gen4-5 timeframe. SKL
seems to have re-introduced it for all planes for whatever reason.
But I've never performed an exhaustive test on all the
platforms/planes to confirm which way each one works.

[1] commit d34cfebbf9cc ("drm/i915: Fix cursor updates on some platforms")

> 
> > - overlapping DDB allocations can hard hang the box. So any vblank that
> >   sneaks in while we've partiially reprogrammed the ddb could be a
> >   death sentence. A suggested solution was to turn off the planes
> >   before ddb reprogramming but that didn't work out so well when I
> >   tried it due to the whole cancellation thing.
> > 
> > So I pulled in the ddb/wm programming into the normal plane update.
> > That means no more accidental cancellations from elsewhere.
> > 
> > And to avoid any ddb overlaps we simply sequence the plane updates
> > carefully. It's pretty much the same algorithm that we use to avoid
> > ddb overlaps between pipes, with the exception that we don't need the
> > vblank waits. So if a vblank does sneak at any point during the
> > sequence we can be assured that the partially latched state does not
> > have any overlapping ddb allocations.
> 
> Sounds reasonable.  Do you think we should try to land your work before
> Maarten's gen11 NV12 patches?

I wouldn't expect significant rebase hurdles from the nv12 work.
So I'm ok with landing the nv12 stuff first.

I'm not 100% sure I managed to handle the cursor ddb correctly in
that branch. We always allocate a ddb chunk for the cursor even
if it's disabled so it behaves slightly differently compared to
the other planes when it comes to updating the ddb from
.disable_plane(). In fact I don't even remember anymore how I
handled that case. So there might still be some work left to
polish that branch. I'm a bit busy with other things atm, so
it'll probably be a while. Unless someone else wants to pick
up where I left off, which is totally fine by me.

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

Re: [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Matt Roper]

On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> 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.

Is this paragraph left over from an earlier version of the patch?  You
don't actually modify skl_ddb_add_affected_planes() here, and you seem
to be describing what you actually do in icl_check_nv12_planes().

> 
> 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().
> Changes since v2:
> - Fix intel_atomic_get_old_plane_state() to actually return old state.
> - Move visibility changes to preparation patch.
> - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> fixup Y/UV Linkage
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Squash commit message artifact.

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
>  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(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 984bc1f26625..522699085a59 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	crtc_state->nv12_planes &= ~BIT(intel_plane->id);
>  	intel_state->base.visible = false;
>  
> -	/* If this is a cursor plane, no further checks are needed. */
> +	/* Destroy the link */
> +	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;
>  
> @@ -142,27 +146,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 +240,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 390907d77ecd..19cd6bbb43c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10726,6 +10726,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 (INTEL_GEN(dev_priv) < 11 || !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)
>  {
> @@ -10797,6 +10851,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..29c7a4bb484d 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;

While these work fine, I keep expecting 'slave' to be a plane pointer
rather than a bool.  Would it be easier to just drop the bool and have
two plane pointers?

        struct intel_plane *slave;
        struct intel_plane *master;

That would also make checkpatch quieter, since it doesn't like you using
bool's inside of structures.


Matt

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

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

[PATCH 3/7] 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

Re: [PATCH 3/7] drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.

[Matt Roper]

On Fri, Sep 21, 2018 at 07:39:41PM +0200, Maarten Lankhorst wrote:
> 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>

General comment up front:  although we're able to re-use a lot of our
gen9/gen10 functions here with relatively minimal code deltas, I feel
like it's costing us a bit in terms of clarity.  We're doing a lot of
switching up of plane_id indices based on whether we're dealing with a
non-planar format, a gen9-style planar format, a gen11-style Y plane, or
a gen11-style UV plane, and if feels unusually complicated to keep track
of whether the unified code really is doing the right thing with the
right indices for each of those cases.  I think a lot of the complexity
(at least when I run through the logic in my mind) arises because the
hardware design sort of flipped which part of the Y/UV pair is the
"main" content vs the "elsewhere" content from gen9 to gen11 (gen9
treated Y as main and UV as aux, whereas gen11 treats UV as main and Y
as slave).

I'm wondering whether this might be a case where it's simpler and more
maintainable to accept more duplication of code if it allows a more
intuitive split between the gen9-style handling and gen11-style
handling?


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

Like I mentioned above, the changes in this hunk are correct, but
they're deceptively simple for how many different cases we're actually
trying to handle here.  If we keep the fully-unified gen9/gen11 code, it
might be worth adding a quick comment before the loop to clarify that we
really have covered all bases (and prevent future code changes from
overlooking and breaking one of these):

        /*
         * We need to handle four cases here:
         *  (1) Non-planar pixel format, any platform:
         *      Data rate stored in plane_data_rate[current plane ID]
         *      uv_plane_data_rate is filled with 0
         *
         *  (2) NV12 on gen9/gen10 (current plane handles both Y and UV):
         *      Y data rate stored in plane_data_rate[current plane ID]
         *      UV data rate stored in uv_plane_data_rate[current plane ID]
         *
         *  (3) NV12 on gen11+ (Y plane content; slave to the UV plane):
         *      Do nothing since we'll calculate both data rates when
         *      handling master plane
         *
         *  (4) NV12 on gen11+ (UV plane content; master to the Y plane):
         *      Y data rate stored in plane_data_rate[slave plane ID]
         *      UV data rate stored in plane_data_rate[current plane ID]
         *      uv_plane_data_rate remains unchanged as 0 for both
         */

Similarly, it might be worth adding an assertion to
skl_allocate_pipe_ddb() at the point where we start dealing with
gen9-style uv_plane_blocks, just in case we ever screw this up:

        ...
        uv_data_rate = uv_plane_data_rate[plane_id];
        WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_data_rate > 0);
        ...

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

"aux" might not be the best term here since I believe even on gen11 we
still have aux surfaces, which are something different (e.g., for
CCS_E).  Maybe just call this "slave_id" or something?


Matt

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

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

[PATCH 4/7] 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 20bfc89c652c..3c240ad0a8d3 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -235,9 +235,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 19cd6bbb43c4..cea91235d498 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4833,8 +4833,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;
@@ -4843,18 +4842,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;
@@ -4895,7 +4890,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;
@@ -4943,7 +4938,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);
 }
 
 /**
@@ -4958,13 +4953,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),
@@ -4973,7 +4977,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 29c7a4bb484d..25be23414913 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 46c6336cb858..111d72a5d5a0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -310,7 +310,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

Re: [PATCH 4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

[Ville Syrjälä]

On Fri, Sep 21, 2018 at 07:39:42PM +0200, Maarten Lankhorst wrote:
> 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 20bfc89c652c..3c240ad0a8d3 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -235,9 +235,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 19cd6bbb43c4..cea91235d498 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4833,8 +4833,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;
> @@ -4843,18 +4842,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;
> @@ -4895,7 +4890,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;
> @@ -4943,7 +4938,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);
>  }
>  
>  /**
> @@ -4958,13 +4953,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.

This part of the comment is rather more confusing than helpful I think.
The rest seems clear enough so maybe just drop this sentence?

> +		 * 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),
> @@ -4973,7 +4977,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 29c7a4bb484d..25be23414913 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 46c6336cb858..111d72a5d5a0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -310,7 +310,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

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

Re: [PATCH 4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

[Maarten Lankhorst]

Op 21-09-18 om 20:45 schreef Ville Syrjälä:
> On Fri, Sep 21, 2018 at 07:39:42PM +0200, Maarten Lankhorst wrote:
>> 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 20bfc89c652c..3c240ad0a8d3 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -235,9 +235,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 19cd6bbb43c4..cea91235d498 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4833,8 +4833,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;
>> @@ -4843,18 +4842,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;
>> @@ -4895,7 +4890,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;
>> @@ -4943,7 +4938,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);
>>  }
>>  
>>  /**
>> @@ -4958,13 +4953,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.
> This part of the comment is rather more confusing than helpful I think.
> The rest seems clear enough so maybe just drop this sentence?
r-b if changed to:
gen10- and non-hdr planes 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),
>> @@ -4973,7 +4977,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 29c7a4bb484d..25be23414913 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 46c6336cb858..111d72a5d5a0 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -310,7 +310,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


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

Re: [PATCH 4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

[Ville Syrjälä]

On Mon, Sep 24, 2018 at 10:39:54AM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 20:45 schreef Ville Syrjälä:
> > On Fri, Sep 21, 2018 at 07:39:42PM +0200, Maarten Lankhorst wrote:
> >> 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 20bfc89c652c..3c240ad0a8d3 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -235,9 +235,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 19cd6bbb43c4..cea91235d498 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4833,8 +4833,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;
> >> @@ -4843,18 +4842,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;
> >> @@ -4895,7 +4890,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;
> >> @@ -4943,7 +4938,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);
> >>  }
> >>  
> >>  /**
> >> @@ -4958,13 +4953,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.
> > This part of the comment is rather more confusing than helpful I think.
> > The rest seems clear enough so maybe just drop this sentence?
> r-b if changed to:
> gen10- and non-hdr planes always need the scaler?

I don't know what gen10- even means. The usual notation is pre-foo.

> >> +		 * 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),
> >> @@ -4973,7 +4977,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 29c7a4bb484d..25be23414913 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 46c6336cb858..111d72a5d5a0 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -310,7 +310,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
> 

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

Re: [PATCH 4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

[Matt Roper]

On Fri, Sep 21, 2018 at 07:39:42PM +0200, Maarten Lankhorst wrote:
> 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 20bfc89c652c..3c240ad0a8d3 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -235,9 +235,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))) {

Minor kernel coding standard violation here; we need to make the entire
if/else block use braces once we add them to any branch.  Especially
here where we've got nested if/else already.

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

While this is correct, it looks really strange to have the code do
"if nv12...set mode=packed" -- maybe we should change this to definition
to _NORMAL rather than _PACKED; that's actually what the bspec calls
this bit on gen11 anyway.

It might also be worth adding a comment around here explaining how the
hardware is supposed to work since it's somewhat non-obvious:

  If NV12 on Planes 0-2: Uses chroma upsampler; scaler only used (in
  normal mode) if actual scaling is necessary
  NV12 on Planes 3-4: Scaler required (in planar mode) regardless of
  whether real scaling is necessary

>  	} 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 19cd6bbb43c4..cea91235d498 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4833,8 +4833,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;
> @@ -4843,18 +4842,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;
> @@ -4895,7 +4890,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;
> @@ -4943,7 +4938,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);

Make the final parameter here 'false' to make it clear that we're
passing a bool?

>  }
>  
>  /**
> @@ -4958,13 +4953,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;

Minor nitpick, but I'd call this (and the earlier parameter to
skl_update_scaler) either need_scaler or force_scaler to clarify that it
represents the need to reserve the hardware unit rather than making a
statement about src/dst size differences.  Even if we're on an
HDR-capable plane that doesn't automatically mandate scaler usage for
NV12, we do a separate check later to see whether we're actually doing
scaling and need to grab a scaler anyway.


Matt


> +
> +	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),
> @@ -4973,7 +4977,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 29c7a4bb484d..25be23414913 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 46c6336cb858..111d72a5d5a0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -310,7 +310,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

-- 
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 4/7] drm/i915/gen11: Program the scalers correctly for planar formats.

[Ville Syrjälä]

On Wed, Sep 26, 2018 at 05:16:40PM -0700, Matt Roper wrote:
> On Fri, Sep 21, 2018 at 07:39:42PM +0200, Maarten Lankhorst wrote:
> > 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 20bfc89c652c..3c240ad0a8d3 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -235,9 +235,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))) {
> 
> Minor kernel coding standard violation here; we need to make the entire
> if/else block use braces once we add them to any branch.  Especially
> here where we've got nested if/else already.
> 
> >  			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;
> 
> While this is correct, it looks really strange to have the code do
> "if nv12...set mode=packed" -- maybe we should change this to definition
> to _NORMAL rather than _PACKED; that's actually what the bspec calls
> this bit on gen11 anyway.
> 
> It might also be worth adding a comment around here explaining how the
> hardware is supposed to work since it's somewhat non-obvious:
> 
>   If NV12 on Planes 0-2: Uses chroma upsampler; scaler only used (in
>   normal mode) if actual scaling is necessary
>   NV12 on Planes 3-4: Scaler required (in planar mode) regardless of
>   whether real scaling is necessary

Rather that talking about specific plane numbers I suggest we stick to
the HDR vs. SDR plane terminology. Makes things more future proof at
the very least.

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

[PATCH 5/7] 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 111d72a5d5a0..c4e05b0b60bf 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -358,6 +358,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 */
@@ -385,6 +386,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

Re: [PATCH 5/7] drm/i915/gen11: Program the chroma upsampler for HDR planes.

[Ville Syrjälä]

On Fri, Sep 21, 2018 at 07:39:43PM +0200, Maarten Lankhorst wrote:
> 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 111d72a5d5a0..c4e05b0b60bf 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -358,6 +358,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 */
> @@ -385,6 +386,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;

Or vphase=-0.5 and hphase=-0.25 maybe? That would be consistent
with the phase we program into the normal scaler. But maybe the
cus considers works differently?

> +
> +			if (linked->id == PLANE_SPRITE5)
> +				cus_ctl |= PLANE_CUS_PLANE_7;
> +			else if (linked->id == PLANE_SPRITE4)
> +				cus_ctl |= PLANE_CUS_PLANE_6;

PLANE_CUS_SOMETHING(plane) ?

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

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

Re: [PATCH 5/7] drm/i915/gen11: Program the chroma upsampler for HDR planes.

[Maarten Lankhorst]

Op 21-09-18 om 20:53 schreef Ville Syrjälä:
> On Fri, Sep 21, 2018 at 07:39:43PM +0200, Maarten Lankhorst wrote:
>> 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 111d72a5d5a0..c4e05b0b60bf 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -358,6 +358,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 */
>> @@ -385,6 +386,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;
> Or vphase=-0.5 and hphase=-0.25 maybe? That would be consistent
> with the phase we program into the normal scaler. But maybe the
> cus considers works differently?

Just following what the documentation says in PLANE_CUS_CTL..

YUV 420 Chroma Siting
Left (MPEG-2): Hphase: 0, 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;
> PLANE_CUS_SOMETHING(plane) ?
Considered it, but the field is a bool. So it doesn't work like that.

I rather explicitly add the MISSING_CASE() and not hide the fact you can't use
an arbitrary plane in there.
>> +			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


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

Re: [PATCH 5/7] drm/i915/gen11: Program the chroma upsampler for HDR planes.

[Ville Syrjälä]

On Mon, Sep 24, 2018 at 10:38:17AM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 20:53 schreef Ville Syrjälä:
> > On Fri, Sep 21, 2018 at 07:39:43PM +0200, Maarten Lankhorst wrote:
> >> 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 111d72a5d5a0..c4e05b0b60bf 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -358,6 +358,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 */
> >> @@ -385,6 +386,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;
> > Or vphase=-0.5 and hphase=-0.25 maybe? That would be consistent
> > with the phase we program into the normal scaler. But maybe the
> > cus considers works differently?
> 
> Just following what the documentation says in PLANE_CUS_CTL..

I wouldn't trust the docs. They were nonsese for the normal
plane scaler too. Better verify visually.

IIRC I used a hacked version of this for the skl+ stuff:
git://github.com/vsyrjala/intel-gpu-tools.git plane_scaling_basic

> 
> YUV 420 Chroma Siting
> Left (MPEG-2): Hphase: 0, 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;
> > PLANE_CUS_SOMETHING(plane) ?
> Considered it, but the field is a bool. So it doesn't work like that.

Oh. I guess the plane_6/7 thing is the best we can do with that then.

> 
> I rather explicitly add the MISSING_CASE() and not hide the fact you can't use
> an arbitrary plane in there.
> >> +			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
> 

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

[PATCH 6/7] 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 c4e05b0b60bf..708d2dfd59d7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -339,23 +339,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;
@@ -409,7 +409,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 {
@@ -424,6 +426,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)
 {
@@ -1775,6 +1796,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);
@@ -1794,16 +1816,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 {
@@ -1877,7 +1901,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

Re: [PATCH 6/7] drm/i915/gen11: Program the Y and UV plane for planar mode correctly.

[Ville Syrjälä]

On Fri, Sep 21, 2018 at 07:39:44PM +0200, Maarten Lankhorst wrote:
> 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 c4e05b0b60bf..708d2dfd59d7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -339,23 +339,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;
> @@ -409,7 +409,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 {
> @@ -424,6 +426,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,

The !!linked thing is a bit magicy. I'd probably forget what it means
every single time. So IMO deserves to be written out in a bit more
verbose way and/or accompanied by a comment.

> +			  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);

So that one bit is the only difference (apart from the obvious
PLANE_SURF/STRIDE/OFFSET? No need to deal with the chroma vs. luma 
size difference etc?

> +}
> +
>  void
>  skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
>  {
> @@ -1775,6 +1796,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);
> @@ -1794,16 +1816,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 {
> @@ -1877,7 +1901,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

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

Re: [PATCH 6/7] drm/i915/gen11: Program the Y and UV plane for planar mode correctly.

[Maarten Lankhorst]

Op 21-09-18 om 21:01 schreef Ville Syrjälä:
> On Fri, Sep 21, 2018 at 07:39:44PM +0200, Maarten Lankhorst wrote:
>> 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 c4e05b0b60bf..708d2dfd59d7 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -339,23 +339,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;
>> @@ -409,7 +409,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 {
>> @@ -424,6 +426,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,
> The !!linked thing is a bit magicy. I'd probably forget what it means
> every single time. So IMO deserves to be written out in a bit more
> verbose way and/or accompanied by a comment.
I'll just make a variable int color_plane = plane_state->linked_plane ? 1 : 0?
>> +			  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);
> So that one bit is the only difference (apart from the obvious
> PLANE_SURF/STRIDE/OFFSET? No need to deal with the chroma vs. luma 
> size difference etc?
Yes, and all color management is ignored on the Y plane. Still there doesn't seem
to be any harm in programming it, so *shrug*.

Only not so nice thing is I found a CRC mismatch when using chroma upsampling + scaling
vs just using the chroma upsampling. Seems to be a small ~1 or 2 pixel difference in 10 bit.
Need to investigate it a bit more, but with 8 bits you don't notice it. Just an annoying crc mismatch. :)

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

[PATCH 7/7] 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 cea91235d498..1a069b90bbc2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13739,12 +13739,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;
@@ -14654,7 +14656,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: Enable planar format support on gen11.

[Patchwork]

== Series Details ==

Series: drm/i915/gen11: Enable planar format support on gen11.
URL   : https://patchwork.freedesktop.org/series/50031/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
0f2019e00594 drm/i915/gen11: Enable 6 sprites on gen11
06f6be3d4c6f drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.
-: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().

-:29: WARNING:BAD_SIGN_OFF: Duplicate signature
#29: 
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

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

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

-:289: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#289: 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, 3 warnings, 3 checks, 303 lines checked
ef7832b117a3 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
0ef08e8bfaa7 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:243:
+		} else

total: 0 errors, 0 warnings, 1 checks, 122 lines checked
4c0ba802d583 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
f4123b47f50c drm/i915/gen11: Program the Y and UV plane for planar mode correctly.
133d16ffd6ab 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.BAT: success for drm/i915/gen11: Enable planar format support on gen11.

[Patchwork]

== Series Details ==

Series: drm/i915/gen11: Enable planar format support on gen11.
URL   : https://patchwork.freedesktop.org/series/50031/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4862 -> Patchwork_10254 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-WARN (fdo#102614) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-icl-u:           INCOMPLETE (fdo#107713) -> PASS

    igt@kms_psr@primary_page_flip:
      fi-cnl-u:           FAIL (fdo#107336) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


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

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


== Build changes ==

    * Linux: CI_DRM_4862 -> Patchwork_10254

  CI_DRM_4862: 51644a4ee1a2a3b98f290fbc95738953c611fcc4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10254: 133d16ffd6abd7db4cb927c6b40f8c2d5a3c66d6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

133d16ffd6ab drm/i915/gen11: Expose planar format support on gen11.
f4123b47f50c drm/i915/gen11: Program the Y and UV plane for planar mode correctly.
4c0ba802d583 drm/i915/gen11: Program the chroma upsampler for HDR planes.
0ef08e8bfaa7 drm/i915/gen11: Program the scalers correctly for planar formats.
ef7832b117a3 drm/i915/gen11: Handle watermarks correctly for separate Y/UV planes.
06f6be3d4c6f drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.
0f2019e00594 drm/i915/gen11: Enable 6 sprites on gen11

== Logs ==

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

✓ Fi.CI.IGT: success for drm/i915/gen11: Enable planar format support on gen11.

[Patchwork]

== Series Details ==

Series: drm/i915/gen11: Enable planar format support on gen11.
URL   : https://patchwork.freedesktop.org/series/50031/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4862_full -> Patchwork_10254_full =

== Summary - WARNING ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS +1
      shard-snb:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-snb:          PASS -> INCOMPLETE (fdo#106886, fdo#105411)

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-glk:          NOTRUN -> FAIL (fdo#103158) +1

    igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-c:
      shard-glk:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          NOTRUN -> FAIL (fdo#105454, fdo#106509)

    
    ==== Possible fixes ====

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
      shard-glk:          DMESG-WARN (fdo#107956) -> PASS

    
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4862 -> Patchwork_10254

  CI_DRM_4862: 51644a4ee1a2a3b98f290fbc95738953c611fcc4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10254: 133d16ffd6abd7db4cb927c6b40f8c2d5a3c66d6 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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