drm/i915/gen11: Add support for the NV12 format.

On gen11, we can definitely smash the 32-bits barrier with just a when we enable all planes in the next patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 47 +++++++++++++++------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +22 -25

index 67a4d0735291..3b136fdfd24f 100644
@@ -3784,7 +3784,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 
 static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 			      const struct intel_crtc_state *cstate,
-			      const unsigned int total_data_rate,
+			      const u64 total_data_rate,
 			      const int num_active,
 			      struct skl_ddb_allocation *ddb)
 {

@@ -3798,12 +3798,12 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 		return ddb_size - 4; /* 4 blocks for bypass path allocation */
 
 	adjusted_mode = &cstate->base.adjusted_mode;
-	total_data_bw = (u64)total_data_rate * drm_mode_vrefresh(adjusted_mode);
+	total_data_bw = total_data_rate * drm_mode_vrefresh(adjusted_mode);
 
 	/*
 	 * 12GB/s is maximum BW supported by single DBuf slice.
 	 */
-	if (total_data_bw >= GBps(12) || num_active > 1) {
+	if (num_active > 1 || total_data_bw >= GBps(12)) {
 		ddb->enabled_slices = 2;
 	} else {
 		ddb->enabled_slices = 1;

@@ -3816,7 +3816,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-				   const unsigned int total_data_rate,
+				   const u64 total_data_rate,
 				   struct skl_ddb_allocation *ddb,
 				   struct skl_ddb_entry *alloc, /* out */
 				   int *num_active /* out */)

@@ -4139,7 +4139,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 	return 0;
 }
 
-static unsigned int
+static u64
 skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 			     const struct drm_plane_state *pstate,
 			     const int plane)

@@ -4151,6 +4151,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	struct drm_framebuffer *fb;
 	u32 format;
 	uint_fixed_16_16_t down_scale_amount;
+	u64 rate;
 
 	if (!intel_pstate->base.visible)
 		return 0;

@@ -4177,28 +4178,26 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 		height /= 2;
 	}
 
-	data_rate = width * height * fb->format->cpp[plane];
+	data_rate = width * height;
 
 	down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate);
 
-	return mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+	rate = mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+
+	rate *= fb->format->cpp[plane];
+	return rate;
 }
 
-/*
- * We don't overflow 32 bits. Worst case is 3 planes enabled, each fetching
- * a 8192x4096@32bpp framebuffer:
- *   3 * 4096 * 8192  * 4 < 2^32
- */
-static unsigned int
+static u64
 skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
-				 unsigned int *plane_data_rate,
-				 unsigned int *uv_plane_data_rate)
+				 u64 *plane_data_rate,
+				 u64 *uv_plane_data_rate)
 {
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
 	struct drm_plane *plane;
 	const struct drm_plane_state *pstate;
-	unsigned int total_data_rate = 0;
+	u64 total_data_rate = 0;
 
 	if (WARN_ON(!state))
 		return 0;

@@ -4206,7 +4205,7 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
 	/* Calculate and cache data rate for each plane */
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
 		enum plane_id plane_id = to_intel_plane(plane)->id;
-		unsigned int rate;
+		u64 rate;
 
 		/* packed/y */
 		rate = skl_plane_relative_data_rate(intel_cstate,

@@ -4325,11 +4324,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	uint16_t alloc_size, start;
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t uv_minimum[I915_MAX_PLANES] = {};
-	unsigned int total_data_rate;
+	u64 total_data_rate;
 	enum plane_id plane_id;
 	int num_active;
-	unsigned int plane_data_rate[I915_MAX_PLANES] = {};
-	unsigned int uv_plane_data_rate[I915_MAX_PLANES] = {};
+	u64 plane_data_rate[I915_MAX_PLANES] = {};
+	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
 	uint16_t total_min_blocks = 0;
 
 	/* Clear the partitioning for disabled planes. */

@@ -4388,7 +4387,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 
 	start = alloc->start;
 	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
-		unsigned int data_rate, uv_data_rate;
+		u64 data_rate, uv_data_rate;
 		uint16_t plane_blocks, uv_plane_blocks;
 
 		if (plane_id == PLANE_CURSOR)

@@ -4402,8 +4401,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
 		plane_blocks = minimum[plane_id];
-		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-					total_data_rate);
+		plane_blocks += alloc_size * data_rate / total_data_rate;
 
 		/* Leave disabled planes at (0,0) */
 		if (data_rate) {

@@ -4417,8 +4415,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		uv_data_rate = uv_plane_data_rate[plane_id];
 
 		uv_plane_blocks = uv_minimum[plane_id];
-		uv_plane_blocks += div_u64((uint64_t)alloc_size * uv_data_rate,
-					   total_data_rate);
+		uv_plane_blocks += alloc_size * uv_data_rate / total_data_rate;
 
 		if (uv_data_rate) {
 			ddb->uv_plane[pipe][plane_id].start = start;

Signature · 6 lines

2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines

On gen11, we can definitely smash the 32-bits barrier with just a when we enable all planes in the next patch.

Signed-off-by Maarten Lankhorst <[email protected]>

I guess the per-plane data rate is still <32bit (because it doesn't account for the refresh rate). But making everything 64bit seems safest.

Reviewed-by Ville Syrjälä <[email protected]>

Now we could also improve the per-plane data rate to include the refresh rate and get rid of inaccurate drm_mode_vrefresh() in there.

Quoted 152 lines

---
 drivers/gpu/drm/i915/intel_pm.c | 47 +++++++++++++++------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +22 -25

index 67a4d0735291..3b136fdfd24f 100644
@@ -3784,7 +3784,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 
 static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 			      const struct intel_crtc_state *cstate,
-			      const unsigned int total_data_rate,
+			      const u64 total_data_rate,
 			      const int num_active,
 			      struct skl_ddb_allocation *ddb)
 {

@@ -3798,12 +3798,12 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 		return ddb_size - 4; /* 4 blocks for bypass path allocation */
 
 	adjusted_mode = &cstate->base.adjusted_mode;
-	total_data_bw = (u64)total_data_rate * drm_mode_vrefresh(adjusted_mode);
+	total_data_bw = total_data_rate * drm_mode_vrefresh(adjusted_mode);
 
 	/*
 	 * 12GB/s is maximum BW supported by single DBuf slice.
 	 */
-	if (total_data_bw >= GBps(12) || num_active > 1) {
+	if (num_active > 1 || total_data_bw >= GBps(12)) {
 		ddb->enabled_slices = 2;
 	} else {
 		ddb->enabled_slices = 1;

@@ -3816,7 +3816,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-				   const unsigned int total_data_rate,
+				   const u64 total_data_rate,
 				   struct skl_ddb_allocation *ddb,
 				   struct skl_ddb_entry *alloc, /* out */
 				   int *num_active /* out */)

@@ -4139,7 +4139,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 	return 0;
 }
 
-static unsigned int
+static u64
 skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 			     const struct drm_plane_state *pstate,
 			     const int plane)

@@ -4151,6 +4151,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	struct drm_framebuffer *fb;
 	u32 format;
 	uint_fixed_16_16_t down_scale_amount;
+	u64 rate;
 
 	if (!intel_pstate->base.visible)
 		return 0;

@@ -4177,28 +4178,26 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 		height /= 2;
 	}
 
-	data_rate = width * height * fb->format->cpp[plane];
+	data_rate = width * height;
 
 	down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate);
 
-	return mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+	rate = mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+
+	rate *= fb->format->cpp[plane];
+	return rate;
 }
 
-/*
- * We don't overflow 32 bits. Worst case is 3 planes enabled, each fetching
- * a 8192x4096@32bpp framebuffer:
- *   3 * 4096 * 8192  * 4 < 2^32
- */
-static unsigned int
+static u64
 skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
-				 unsigned int *plane_data_rate,
-				 unsigned int *uv_plane_data_rate)
+				 u64 *plane_data_rate,
+				 u64 *uv_plane_data_rate)
 {
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
 	struct drm_plane *plane;
 	const struct drm_plane_state *pstate;
-	unsigned int total_data_rate = 0;
+	u64 total_data_rate = 0;
 
 	if (WARN_ON(!state))
 		return 0;

@@ -4206,7 +4205,7 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
 	/* Calculate and cache data rate for each plane */
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
 		enum plane_id plane_id = to_intel_plane(plane)->id;
-		unsigned int rate;
+		u64 rate;
 
 		/* packed/y */
 		rate = skl_plane_relative_data_rate(intel_cstate,

@@ -4325,11 +4324,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	uint16_t alloc_size, start;
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t uv_minimum[I915_MAX_PLANES] = {};
-	unsigned int total_data_rate;
+	u64 total_data_rate;
 	enum plane_id plane_id;
 	int num_active;
-	unsigned int plane_data_rate[I915_MAX_PLANES] = {};
-	unsigned int uv_plane_data_rate[I915_MAX_PLANES] = {};
+	u64 plane_data_rate[I915_MAX_PLANES] = {};
+	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
 	uint16_t total_min_blocks = 0;
 
 	/* Clear the partitioning for disabled planes. */

@@ -4388,7 +4387,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 
 	start = alloc->start;
 	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
-		unsigned int data_rate, uv_data_rate;
+		u64 data_rate, uv_data_rate;
 		uint16_t plane_blocks, uv_plane_blocks;
 
 		if (plane_id == PLANE_CURSOR)

@@ -4402,8 +4401,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
 		plane_blocks = minimum[plane_id];
-		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-					total_data_rate);
+		plane_blocks += alloc_size * data_rate / total_data_rate;
 
 		/* Leave disabled planes at (0,0) */
 		if (data_rate) {

@@ -4417,8 +4415,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		uv_data_rate = uv_plane_data_rate[plane_id];
 
 		uv_plane_blocks = uv_minimum[plane_id];
-		uv_plane_blocks += div_u64((uint64_t)alloc_size * uv_data_rate,
-					   total_data_rate);
+		uv_plane_blocks += alloc_size * uv_data_rate / total_data_rate;
 
 		if (uv_data_rate) {
 			ddb->uv_plane[pipe][plane_id].start = start;

Signature · 1 line

2.19.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

On gen11, we can definitely smash the 32-bits barrier with just a when we enable all planes in the next patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 47 +++++++++++++++------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +20 -21

index 67a4d0735291..3b136fdfd24f 100644
@@ -3784,7 +3784,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 
 static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 			      const struct intel_crtc_state *cstate,
-			      const unsigned int total_data_rate,
+			      const u64 total_data_rate,
 			      const int num_active,
 			      struct skl_ddb_allocation *ddb)
 {

@@ -3798,12 +3798,12 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 		return ddb_size - 4; /* 4 blocks for bypass path allocation */
 
 	adjusted_mode = &cstate->base.adjusted_mode;
-	total_data_bw = (u64)total_data_rate * drm_mode_vrefresh(adjusted_mode);
+	total_data_bw = total_data_rate * drm_mode_vrefresh(adjusted_mode);
 
 	/*
 	 * 12GB/s is maximum BW supported by single DBuf slice.
 	 */
-	if (total_data_bw >= GBps(12) || num_active > 1) {
+	if (num_active > 1 || total_data_bw >= GBps(12)) {
 		ddb->enabled_slices = 2;
 	} else {
 		ddb->enabled_slices = 1;

@@ -3816,7 +3816,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-				   const unsigned int total_data_rate,
+				   const u64 total_data_rate,
 				   struct skl_ddb_allocation *ddb,
 				   struct skl_ddb_entry *alloc, /* out */
 				   int *num_active /* out */)

@@ -4139,7 +4139,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 	return 0;
 }
 
-static unsigned int
+static u64
 skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 			     const struct drm_plane_state *pstate,
 			     const int plane)

@@ -4151,6 +4151,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	struct drm_framebuffer *fb;
 	u32 format;
 	uint_fixed_16_16_t down_scale_amount;
+	u64 rate;
 
 	if (!intel_pstate->base.visible)
 		return 0;

@@ -4177,28 +4178,26 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 		height /= 2;
 	}
 
-	data_rate = width * height * fb->format->cpp[plane];
+	data_rate = width * height;
 
 	down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate);
 
-	return mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+	rate = mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+
+	rate *= fb->format->cpp[plane];
+	return rate;
 }
 
-/*
- * We don't overflow 32 bits. Worst case is 3 planes enabled, each fetching
- * a 8192x4096@32bpp framebuffer:
- *   3 * 4096 * 8192  * 4 < 2^32
- */
-static unsigned int
+static u64
 skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
-				 unsigned int *plane_data_rate,
-				 unsigned int *uv_plane_data_rate)
+				 u64 *plane_data_rate,
+				 u64 *uv_plane_data_rate)
 {
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
 	struct drm_plane *plane;
 	const struct drm_plane_state *pstate;
-	unsigned int total_data_rate = 0;
+	u64 total_data_rate = 0;
 
 	if (WARN_ON(!state))
 		return 0;

@@ -4206,7 +4205,7 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
 	/* Calculate and cache data rate for each plane */
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
 		enum plane_id plane_id = to_intel_plane(plane)->id;
-		unsigned int rate;
+		u64 rate;
 
 		/* packed/y */
 		rate = skl_plane_relative_data_rate(intel_cstate,

@@ -4325,11 +4324,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	uint16_t alloc_size, start;
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t uv_minimum[I915_MAX_PLANES] = {};
-	unsigned int total_data_rate;
+	u64 total_data_rate;
 	enum plane_id plane_id;
 	int num_active;
-	unsigned int plane_data_rate[I915_MAX_PLANES] = {};
-	unsigned int uv_plane_data_rate[I915_MAX_PLANES] = {};
+	u64 plane_data_rate[I915_MAX_PLANES] = {};
+	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
 	uint16_t total_min_blocks = 0;
 
 	/* Clear the partitioning for disabled planes. */

@@ -4388,7 +4387,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 
 	start = alloc->start;
 	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
-		unsigned int data_rate, uv_data_rate;
+		u64 data_rate, uv_data_rate;
 		uint16_t plane_blocks, uv_plane_blocks;
 
 		if (plane_id == PLANE_CURSOR)

drivers/gpu/drm/i915/intel_pm.c +1 -2

 		/* Leave disabled planes at (0,0) */
 		if (data_rate) {

@@ -4417,8 +4415,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		uv_data_rate = uv_plane_data_rate[plane_id];
 
 		uv_plane_blocks = uv_minimum[plane_id];
-		uv_plane_blocks += div_u64((uint64_t)alloc_size * uv_data_rate,
-					   total_data_rate);
+		uv_plane_blocks += alloc_size * uv_data_rate / total_data_rate;
 
 		if (uv_data_rate) {
 			ddb->uv_plane[pipe][plane_id].start = start;

Signature · 6 lines

2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 132 lines

On gen11, we can definitely smash the 32-bits barrier with just a when we enable all planes in the next patch.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 47 +++++++++++++++------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +20 -21

index 67a4d0735291..3b136fdfd24f 100644
@@ -3784,7 +3784,7 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 
 static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 			      const struct intel_crtc_state *cstate,
-			      const unsigned int total_data_rate,
+			      const u64 total_data_rate,
 			      const int num_active,
 			      struct skl_ddb_allocation *ddb)
 {

@@ -3798,12 +3798,12 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 		return ddb_size - 4; /* 4 blocks for bypass path allocation */
 
 	adjusted_mode = &cstate->base.adjusted_mode;
-	total_data_bw = (u64)total_data_rate * drm_mode_vrefresh(adjusted_mode);
+	total_data_bw = total_data_rate * drm_mode_vrefresh(adjusted_mode);
 
 	/*
 	 * 12GB/s is maximum BW supported by single DBuf slice.
 	 */
-	if (total_data_bw >= GBps(12) || num_active > 1) {
+	if (num_active > 1 || total_data_bw >= GBps(12)) {
 		ddb->enabled_slices = 2;
 	} else {
 		ddb->enabled_slices = 1;

@@ -3816,7 +3816,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-				   const unsigned int total_data_rate,
+				   const u64 total_data_rate,
 				   struct skl_ddb_allocation *ddb,
 				   struct skl_ddb_entry *alloc, /* out */
 				   int *num_active /* out */)

@@ -4139,7 +4139,7 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 	return 0;
 }
 
-static unsigned int
+static u64
 skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 			     const struct drm_plane_state *pstate,
 			     const int plane)

@@ -4151,6 +4151,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	struct drm_framebuffer *fb;
 	u32 format;
 	uint_fixed_16_16_t down_scale_amount;
+	u64 rate;
 
 	if (!intel_pstate->base.visible)
 		return 0;

@@ -4177,28 +4178,26 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 		height /= 2;
 	}
 
-	data_rate = width * height * fb->format->cpp[plane];
+	data_rate = width * height;
 
 	down_scale_amount = skl_plane_downscale_amount(cstate, intel_pstate);
 
-	return mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+	rate = mul_round_up_u32_fixed16(data_rate, down_scale_amount);
+
+	rate *= fb->format->cpp[plane];
+	return rate;
 }
 
-/*
- * We don't overflow 32 bits. Worst case is 3 planes enabled, each fetching
- * a 8192x4096@32bpp framebuffer:
- *   3 * 4096 * 8192  * 4 < 2^32
- */
-static unsigned int
+static u64
 skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
-				 unsigned int *plane_data_rate,
-				 unsigned int *uv_plane_data_rate)
+				 u64 *plane_data_rate,
+				 u64 *uv_plane_data_rate)
 {
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
 	struct drm_plane *plane;
 	const struct drm_plane_state *pstate;
-	unsigned int total_data_rate = 0;
+	u64 total_data_rate = 0;
 
 	if (WARN_ON(!state))
 		return 0;

@@ -4206,7 +4205,7 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
 	/* Calculate and cache data rate for each plane */
 	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
 		enum plane_id plane_id = to_intel_plane(plane)->id;
-		unsigned int rate;
+		u64 rate;
 
 		/* packed/y */
 		rate = skl_plane_relative_data_rate(intel_cstate,

@@ -4325,11 +4324,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	uint16_t alloc_size, start;
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t uv_minimum[I915_MAX_PLANES] = {};
-	unsigned int total_data_rate;
+	u64 total_data_rate;
 	enum plane_id plane_id;
 	int num_active;
-	unsigned int plane_data_rate[I915_MAX_PLANES] = {};
-	unsigned int uv_plane_data_rate[I915_MAX_PLANES] = {};
+	u64 plane_data_rate[I915_MAX_PLANES] = {};
+	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
 	uint16_t total_min_blocks = 0;
 
 	/* Clear the partitioning for disabled planes. */

@@ -4388,7 +4387,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 
 	start = alloc->start;
 	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
-		unsigned int data_rate, uv_data_rate;
+		u64 data_rate, uv_data_rate;
 		uint16_t plane_blocks, uv_plane_blocks;
 

drivers/gpu/drm/i915/intel_pm.c skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, · line 4404

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +1 -2

 		if (plane_id == PLANE_CURSOR)

@@ -4402,8 +4401,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
 		plane_blocks = minimum[plane_id];
-		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-					total_data_rate);
+		plane_blocks += alloc_size * data_rate / total_data_rate;

I failed to spot the raw 64bit divisions here. Fortunately CI caught them (yay). Need to be fixed.

Op 18-10-18 om 17:11 schreef Ville Syrjälä:

Quoted 10 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_pm.c skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, · line 4404

Quoted 7 lines

drivers/gpu/drm/i915/intel_pm.c +1 -2

@@ -4402,8 +4401,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
              * result is < available as data_rate / total_data_rate < 1
              */
             plane_blocks = minimum[plane_id];
-            plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-                                    total_data_rate);
+            plane_blocks += alloc_size * data_rate / total_data_rate;

I failed to spot the raw 64bit divisions here. Fortunately CI caught them (yay). Need to be fixed.

No we can't, if we make the per plane data rate 64-bits, the divisor will overflow a div_u64..

Quoting Maarten Lankhorst (2018-10-19 14:03:47)

Quoted 11 lines

Op 18-10-18 om 17:11 schreef Ville Syrjälä:

Quoted 10 lines · Maarten Lankhorst

drivers/gpu/drm/i915/intel_pm.c skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, · line 4404

Quoted 7 lines

drivers/gpu/drm/i915/intel_pm.c +1 -2

@@ -4402,8 +4401,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
              * result is < available as data_rate / total_data_rate < 1
              */
             plane_blocks = minimum[plane_id];
-            plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-                                    total_data_rate);
+            plane_blocks += alloc_size * data_rate / total_data_rate;

I failed to spot the raw 64bit divisions here. Fortunately CI caught them (yay). Need to be fixed.

Quoted 1 line

No we can't, if we make the per plane data rate 64-bits, the divisor will overflow a div_u64..

div64_u64()
-Chris

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 74 ++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c      | 81 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 +++-
 4 files changed, 202 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +5 -1

index b957ad63cd87..154ea3dc344f 100644
@@ -122,7 +122,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;
 

drivers/gpu/drm/i915/intel_atomic_plane.c +7 -8

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

drivers/gpu/drm/i915/intel_atomic_plane.c +33 -6

+
+		if (WARN_ON(!old_linked_state))
+			return -EINVAL;
+
+		crtc = to_intel_crtc(old_linked_state->base.crtc);
+		if (WARN_ON(!crtc))
+			return -EINVAL;
+	}
+
+	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);
 
-	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,

@@ -188,6 +209,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);
 

drivers/gpu/drm/i915/intel_display.c +13 -0

index 8e1b3677e131..cbb3fb1d5ad4 100644
@@ -10721,6 +10721,80 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_add_linked_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state, *linked_plane_state;
+	int i;
+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		linked = plane_state->linked_plane;
+
+		if (!linked)
+			continue;
+
+		linked_plane_state = intel_atomic_get_plane_state(state, linked);

drivers/gpu/drm/i915/intel_display.c +6 -0

+
+		if (!(crtc_state->nv12_planes & BIT(plane->id)))
+			continue;
+
+		plane_state = intel_atomic_get_new_plane_state(state, plane);
+		if (!plane_state)

drivers/gpu/drm/i915/intel_display.c +32 -0

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

@@ -10792,6 +10866,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);

@@ -12458,6 +12535,10 @@ static int intel_atomic_check(struct drm_device *dev,
 		intel_state->cdclk.logical = dev_priv->cdclk.logical;
 	}
 
+	ret = icl_add_linked_planes(intel_state);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
 		return ret;

drivers/gpu/drm/i915/intel_drv.h +53 -0

index b5d6f6887c13..272de906a001 100644
@@ -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;
 };
 

@@ -983,6 +1003,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);

@@ -1351,6 +1374,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)

@@ -2158,6 +2202,15 @@ struct intel_plane *
 skl_universal_plane_create(struct drm_i915_private *dev_priv,
 			   enum pipe pipe, enum plane_id plane_id);
 
+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);
 

drivers/gpu/drm/i915/intel_pm.c +11 -1

index 3b136fdfd24f..d003c08bd9e4 100644
@@ -5153,11 +5153,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],

@@ -5169,6 +5170,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;

Signature · 6 lines

2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 49 lines

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 74 ++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c      | 81 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 +++-
 4 files changed, 202 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +5 -1

index b957ad63cd87..154ea3dc344f 100644
@@ -122,7 +122,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;
 

drivers/gpu/drm/i915/intel_atomic_plane.c int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ · line 151

Quoted 7 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +2 -2

@@ -143,27 +147,44 @@ 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)

My new cunning plane is to call these _plane, _new_plane_state etc. It should discourage people from using them and the aliasing intel_ types at the same time. And it avoids polluting the namespace for things we don't really want to use.

I already snuck in some uses of this ;)

Quoted 16 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +7 -8

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

drivers/gpu/drm/i915/intel_atomic_plane.c int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ · line 169

Quoted 10 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +10 -0

+	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;
+
+	if (linked && !crtc) {
+		const struct intel_plane_state *old_linked_state =
+			intel_atomic_get_old_plane_state(state, linked);

Plane without a crtc that happens to have a linked plane attached to it...

I guess that implies that 'plane' here is the slave and it was already active during the previous state (otherwise it would not have been linked to the other plane). So that means the master plane must have a valid crtc in its old plane state.

Did I decode that correctly?

Maybe what we want to do here is to just always clear the active_planes bit for the slave in the master plane's old crtc's new crtc state (quite the mouthful), and then run through the normal check_plane stuff for the slave with its own crtc (if it has one). In practice it doesn't really make any difference I suppose since our planes can't move between crtcs, but logically it would make more sense to me.

Quoted 77 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +33 -6

+
+		if (WARN_ON(!old_linked_state))
+			return -EINVAL;
+
+		crtc = to_intel_crtc(old_linked_state->base.crtc);
+		if (WARN_ON(!crtc))
+			return -EINVAL;
+	}
+
+	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);
 
-	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,

@@ -188,6 +209,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);
 

drivers/gpu/drm/i915/intel_display.c +16 -0

index 8e1b3677e131..cbb3fb1d5ad4 100644
@@ -10721,6 +10721,80 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_add_linked_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state, *linked_plane_state;
+	int i;
+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		linked = plane_state->linked_plane;
+
+		if (!linked)
+			continue;
+
+		linked_plane_state = intel_atomic_get_plane_state(state, linked);
+		if (IS_ERR(linked_plane_state))
+			return PTR_ERR(linked_plane_state);
+	}

drivers/gpu/drm/i915/intel_display.c static bool check_single_encoder_cloning(struct drm_atomic_state *state, · line 10746

Quoted 7 lines

drivers/gpu/drm/i915/intel_display.c +7 -0

+
+	return 0;
+}
+
+static int icl_check_nv12_planes(struct drm_i915_private *dev_priv,
+				 struct intel_crtc *crtc,
+				 struct intel_crtc_state *crtc_state)

Could pass just the crtc_state and dig the rest out here.

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +5 -0

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

drivers/gpu/drm/i915/intel_display.c static bool check_single_encoder_cloning(struct drm_atomic_state *state, · line 10756

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +5 -0

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

Still don't like the 'aux' in these names.
Just linked/linked_plane_state' to match the rest of the code?

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +9 -0

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

drivers/gpu/drm/i915/intel_display.c static bool check_single_encoder_cloning(struct drm_atomic_state *state, · line 10772

Quoted 7 lines

drivers/gpu/drm/i915/intel_display.c +7 -0

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

intel_atomic_get_plane() and get rid of the aliasing states?

Quoted 167 lines

drivers/gpu/drm/i915/intel_display.c +32 -0

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

@@ -10792,6 +10866,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);

@@ -12458,6 +12535,10 @@ static int intel_atomic_check(struct drm_device *dev,
 		intel_state->cdclk.logical = dev_priv->cdclk.logical;
 	}
 
+	ret = icl_add_linked_planes(intel_state);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
 		return ret;

drivers/gpu/drm/i915/intel_drv.h +53 -0

index b5d6f6887c13..272de906a001 100644
@@ -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;
 };
 

@@ -983,6 +1003,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);

@@ -1351,6 +1374,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)

@@ -2158,6 +2202,15 @@ struct intel_plane *
 skl_universal_plane_create(struct drm_i915_private *dev_priv,
 			   enum pipe pipe, enum plane_id plane_id);
 
+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);
 

drivers/gpu/drm/i915/intel_pm.c +11 -1

index 3b136fdfd24f..d003c08bd9e4 100644
@@ -5153,11 +5153,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],

@@ -5169,6 +5170,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);
 	}
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

 	return 0;

Signature · 6 lines

2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Op 18-10-18 om 18:00 schreef Ville Syrjälä:

Quoted 93 lines · Maarten Lankhorst

Quoted 49 lines

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 74 ++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c      | 81 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 +++-
 4 files changed, 202 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +5 -1

index b957ad63cd87..154ea3dc344f 100644
@@ -122,7 +122,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;
 

drivers/gpu/drm/i915/intel_atomic_plane.c int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ · line 151

Quoted 7 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +2 -2

@@ -143,27 +147,44 @@ 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)

My new cunning plane is to call these _plane, _new_plane_state etc. It should discourage people from using them and the aliasing intel_ types at the same time. And it avoids polluting the namespace for things we don't really want to use.

I already snuck in some uses of this ;)

Quoted 16 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +7 -8

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

drivers/gpu/drm/i915/intel_atomic_plane.c int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ · line 169

Quoted 10 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +10 -0

+	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;
+
+	if (linked && !crtc) {
+		const struct intel_plane_state *old_linked_state =
+			intel_atomic_get_old_plane_state(state, linked);

Plane without a crtc that happens to have a linked plane attached to it...

Quoted 7 lines

I guess that implies that 'plane' here is the slave and it was already active during the previous state (otherwise it would not have been linked to the other plane). So that means the master plane must have a valid crtc in its old plane state.

Did I decode that correctly?

Correct.

Quoted 8 lines

Maybe what we want to do here is to just always clear the active_planes bit for the slave in the master plane's old crtc's new crtc state (quite the mouthful), and then run through the normal check_plane stuff for the slave with its own crtc (if it has one). In practice it doesn't really make any difference I suppose since our planes can't move between crtcs, but logically it would make more sense to me.

Even if we could move planes,  we can't move planes between CRTC's in a single atomic commit. First comes the disabling, then comes the moving.

I think it's less of a mess of doing it this way, it keeps intel_plane_atomic_check_with_state() obvious.

Quoted 110 lines

Op 18-10-18 om 18:00 schreef Ville Syrjälä:

Quoted 93 lines · Maarten Lankhorst

Quoted 49 lines

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 74 ++++++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c      | 81 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 +++-
 4 files changed, 202 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +5 -1

index b957ad63cd87..154ea3dc344f 100644
@@ -122,7 +122,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;
 

drivers/gpu/drm/i915/intel_atomic_plane.c int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ · line 151

Quoted 7 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +2 -2

@@ -143,27 +147,44 @@ 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)

My new cunning plane is to call these _plane, _new_plane_state etc. It should discourage people from using them and the aliasing intel_ types at the same time. And it avoids polluting the namespace for things we don't really want to use.

I already snuck in some uses of this ;)

Quoted 16 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +7 -8

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

drivers/gpu/drm/i915/intel_atomic_plane.c int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_ · line 169

Quoted 10 lines

drivers/gpu/drm/i915/intel_atomic_plane.c +10 -0

+	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;
+
+	if (linked && !crtc) {
+		const struct intel_plane_state *old_linked_state =
+			intel_atomic_get_old_plane_state(state, linked);

Plane without a crtc that happens to have a linked plane attached to it...

Quoted 7 lines

I guess that implies that 'plane' here is the slave and it was already active during the previous state (otherwise it would not have been linked to the other plane). So that means the master plane must have a valid crtc in its old plane state.

Did I decode that correctly?

Correct.

Quoted 8 lines

Maybe what we want to do here is to just always clear the active_planes bit for the slave in the master plane's old crtc's new crtc state (quite the mouthful), and then run through the normal check_plane stuff for the slave with its own crtc (if it has one). In practice it doesn't really make any difference I suppose since our planes can't move between crtcs, but logically it would make more sense to me.

Quoted 4 lines

Even if we could move planes,  we can't move planes between CRTC's in a single atomic commit. First comes the disabling, then comes the moving.

I think it's less of a mess of doing it this way, it keeps intel_plane_atomic_check_with_state() obvious.

It's rather non-obvious. I had to think about it for a while.

I think it would be much clearer to keep it all in one self contained function instead of spreading it across several functions.

remove_plane_link()

{
	if (plane_state->linked && plane_state->slave) {
		old_linked_state = get_old_plane_state(plane_state->linked);
		new_crtc_state = get_new_crtc_state(old_linked_state->crtc);
		new_crtc_state->active_planes &= ~BIT(plane->id);
	}

	plane_state->linked = NULL;
	plane_state->slave = false;
}

or something.

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)
Changes since v4:
- Clear plane links in icl_check_nv12_planes() for clarity.
- Only pass crtc_state to icl_check_nv12_planes().
- Use for_each_new_intel_plane_in_state() in icl_check_nv12_planes.
- Rename aux to linked. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 19 +++++
 drivers/gpu/drm/i915/intel_display.c      | 95 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 ++-
 4 files changed, 178 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +19 -0

index b957ad63cd87..7d3685075201 100644
@@ -188,6 +188,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);
 

drivers/gpu/drm/i915/intel_display.c +5 -0

index 68c9aeeee05c..680895982e4f 100644
@@ -10723,6 +10723,95 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_add_linked_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state, *linked_plane_state;
+	int i;

drivers/gpu/drm/i915/intel_display.c +17 -0

+	}
+
+	return 0;
+}
+
+static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state);
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 11)
+		return 0;
+

drivers/gpu/drm/i915/intel_display.c +53 -0

+			crtc_state->active_planes &= ~BIT(plane->id);
+
+		plane_state->slave = false;
+	}
+
+	if (!crtc_state->nv12_planes)
+		return 0;
+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		struct intel_plane_state *linked_state = NULL;
+
+		if (plane->pipe != crtc->pipe ||
+		    !(crtc_state->nv12_planes & BIT(plane->id)))
+			continue;
+
+		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, linked) {
+			if (!icl_is_nv12_y_plane(linked->id))
+				continue;
+
+			if (crtc_state->active_planes & BIT(linked->id))
+				continue;
+
+			linked_state = intel_atomic_get_plane_state(state, linked);
+			if (IS_ERR(linked_state))
+				return PTR_ERR(linked_state);
+
+			break;
+		}
+
+		if (!linked_state) {
+			DRM_DEBUG_KMS("Need %d free Y planes for NV12\n",
+				      hweight8(crtc_state->nv12_planes));
+
+			return -EINVAL;
+		}
+
+		plane_state->linked_plane = linked;
+
+		linked_state->slave = true;
+		linked_state->linked_plane = plane;
+		crtc_state->active_planes |= BIT(linked->id);
+		DRM_DEBUG_KMS("Using %s as Y plane for %s\n", linked->base.name, plane->base.name);
+	}
+
+	return 0;
+}
+
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {

@@ -10794,6 +10883,8 @@ 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(pipe_config);
 		if (!ret)
 			ret = skl_check_pipe_max_pixel_rate(intel_crtc,
 							    pipe_config);

@@ -12460,6 +12551,10 @@ static int intel_atomic_check(struct drm_device *dev,
 		intel_state->cdclk.logical = dev_priv->cdclk.logical;
 	}
 
+	ret = icl_add_linked_planes(intel_state);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
 		return ret;

drivers/gpu/drm/i915/intel_drv.h +53 -0

index 0e9a926fca04..59b35293a5f6 100644
@@ -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;
 };
 

@@ -983,6 +1003,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);

@@ -1351,6 +1374,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)

@@ -2158,6 +2202,15 @@ struct intel_plane *
 skl_universal_plane_create(struct drm_i915_private *dev_priv,
 			   enum pipe pipe, enum plane_id plane_id);
 
+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);
 

drivers/gpu/drm/i915/intel_pm.c +11 -1

index f42b8c319046..9ba39a9c7d1c 100644
@@ -5153,11 +5153,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],

@@ -5169,6 +5170,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;

Signature · 1 line

2.19.1

Quoted 79 lines

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)
Changes since v4:
- Clear plane links in icl_check_nv12_planes() for clarity.
- Only pass crtc_state to icl_check_nv12_planes().
- Use for_each_new_intel_plane_in_state() in icl_check_nv12_planes.
- Rename aux to linked. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 19 +++++
 drivers/gpu/drm/i915/intel_display.c      | 95 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 ++-
 4 files changed, 178 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +19 -0

index b957ad63cd87..7d3685075201 100644
@@ -188,6 +188,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);
 

drivers/gpu/drm/i915/intel_display.c +5 -0

index 68c9aeeee05c..680895982e4f 100644
@@ -10723,6 +10723,95 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_add_linked_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state, *linked_plane_state;
+	int i;

drivers/gpu/drm/i915/intel_display.c static bool check_single_encoder_cloning(struct drm_atomic_state *state, · line 10740

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		linked = plane_state->linked_plane;
+
+		if (!linked)
+			continue;
+
+		linked_plane_state = intel_atomic_get_plane_state(state, linked);
+		if (IS_ERR(linked_plane_state))
+			return PTR_ERR(linked_plane_state);

WARN_ON(linked_plane_state->linked != plane); WARN_ON(linked_plane_state->slave == plane_state->slave);

perhaps?

Op 22-10-18 om 17:48 schreef Ville Syrjälä:

Quoted 93 lines · Maarten Lankhorst

Quoted 79 lines

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)
Changes since v4:
- Clear plane links in icl_check_nv12_planes() for clarity.
- Only pass crtc_state to icl_check_nv12_planes().
- Use for_each_new_intel_plane_in_state() in icl_check_nv12_planes.
- Rename aux to linked. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 19 +++++
 drivers/gpu/drm/i915/intel_display.c      | 95 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 ++-
 4 files changed, 178 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +19 -0

index b957ad63cd87..7d3685075201 100644
@@ -188,6 +188,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);
 

drivers/gpu/drm/i915/intel_display.c +5 -0

index 68c9aeeee05c..680895982e4f 100644
@@ -10723,6 +10723,95 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_add_linked_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state, *linked_plane_state;
+	int i;

drivers/gpu/drm/i915/intel_display.c static bool check_single_encoder_cloning(struct drm_atomic_state *state, · line 10740

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		linked = plane_state->linked_plane;
+
+		if (!linked)
+			continue;
+
+		linked_plane_state = intel_atomic_get_plane_state(state, linked);
+		if (IS_ERR(linked_plane_state))
+			return PTR_ERR(linked_plane_state);

WARN_ON(linked_plane_state->linked != plane); WARN_ON(linked_plane_state->slave == plane_state->slave);

Quoted 1 line

perhaps?

Those will trigger because linked_plane_state might already have been cleared. :)

Thanks for the reviews, tested on fi-icl-u and seems the smalll changes didn't break stuff, so will push it in an hour.

~Maarten

Quoted 94 lines

Op 22-10-18 om 17:48 schreef Ville Syrjälä:

Quoted 93 lines · Maarten Lankhorst

Quoted 79 lines

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.
Changes since v3:
- Fix checkpatch warning about to_intel_crtc() usage.
- Add affected planes from icl_add_linked_planes() before check_planes(), it's a cleaner way to do this. (Ville)
Changes since v4:
- Clear plane links in icl_check_nv12_planes() for clarity.
- Only pass crtc_state to icl_check_nv12_planes().
- Use for_each_new_intel_plane_in_state() in icl_check_nv12_planes.
- Rename aux to linked. (Ville)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 19 +++++
 drivers/gpu/drm/i915/intel_display.c      | 95 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++
 drivers/gpu/drm/i915/intel_pm.c           | 12 ++-
 4 files changed, 178 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_atomic_plane.c +19 -0

index b957ad63cd87..7d3685075201 100644
@@ -188,6 +188,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);
 

drivers/gpu/drm/i915/intel_display.c +5 -0

index 68c9aeeee05c..680895982e4f 100644
@@ -10723,6 +10723,95 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
+static int icl_add_linked_planes(struct intel_atomic_state *state)
+{
+	struct intel_plane *plane, *linked;
+	struct intel_plane_state *plane_state, *linked_plane_state;
+	int i;

drivers/gpu/drm/i915/intel_display.c static bool check_single_encoder_cloning(struct drm_atomic_state *state, · line 10740

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		linked = plane_state->linked_plane;
+
+		if (!linked)
+			continue;
+
+		linked_plane_state = intel_atomic_get_plane_state(state, linked);
+		if (IS_ERR(linked_plane_state))
+			return PTR_ERR(linked_plane_state);

WARN_ON(linked_plane_state->linked != plane); WARN_ON(linked_plane_state->slave == plane_state->slave);

Quoted 2 lines

Quoted 1 line

perhaps?

Those will trigger because linked_plane_state might already have been cleared. :)

OK, I'm totally confused now. Who cleared them? And why did they clear them only half way?

Quoted 3 lines

Thanks for the reviews, tested on fi-icl-u and seems the smalll changes didn't break stuff, so will push it in an hour.

~Maarten

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.

Changes since v1:
- Constify crtc_state where possible.
- Make separate paths for planar formats in skl_build_pipe_wm() (Matt)
- Make separate paths for calculating total data rate. (Matt)
- Make sure UV watermarks are unused on gen11+ by adding a WARN. (Matt)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 301 ++++++++++++++++++++++----------
 1 file changed, 207 insertions(+), 94 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +79 -25

index d003c08bd9e4..ef95b9d7884b 100644
@@ -3814,7 +3814,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 }
 
 static void
-skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
+skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 				   const struct intel_crtc_state *cstate,
 				   const u64 total_data_rate,
 				   struct skl_ddb_allocation *ddb,

@@ -3823,7 +3823,6 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 {
 	struct drm_atomic_state *state = cstate->base.state;
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
-	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
 	const struct drm_crtc_state *crtc_state;
 	const struct drm_crtc *crtc;

@@ -3945,14 +3944,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,

@@ -4141,11 +4135,11 @@ int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
 
 static u64
 skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
-			     const struct drm_plane_state *pstate,
+			     const struct intel_plane_state *intel_pstate,
 			     const int plane)
 {
-	struct intel_plane *intel_plane = to_intel_plane(pstate->plane);
-	struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
+	struct intel_plane *intel_plane =
+		to_intel_plane(intel_pstate->base.plane);
 	uint32_t data_rate;
 	uint32_t width = 0, height = 0;
 	struct drm_framebuffer *fb;

@@ -4156,7 +4150,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	if (!intel_pstate->base.visible)
 		return 0;
 
-	fb = pstate->fb;
+	fb = intel_pstate->base.fb;
 	format = fb->format->format;
 
 	if (intel_plane->id == PLANE_CURSOR)

@@ -4206,25 +4200,80 @@ 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;
 		u64 rate;
+		const struct intel_plane_state *intel_pstate =
+			to_intel_plane_state(pstate);
 
 		/* packed/y */
 		rate = skl_plane_relative_data_rate(intel_cstate,
-						    pstate, 0);
+						    intel_pstate, 0);
 		plane_data_rate[plane_id] = rate;
-
 		total_data_rate += rate;
 
 		/* uv-plane */
 		rate = skl_plane_relative_data_rate(intel_cstate,
-						    pstate, 1);
+						    intel_pstate, 1);
 		uv_plane_data_rate[plane_id] = rate;
-
 		total_data_rate += rate;
 	}
 
 	return total_data_rate;
 }
 
+static u64
+icl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
+				 u64 *plane_data_rate)
+{
+	struct drm_crtc_state *cstate = &intel_cstate->base;
+	struct drm_atomic_state *state = cstate->state;
+	struct drm_plane *plane;
+	const struct drm_plane_state *pstate;
+	u64 total_data_rate = 0;
+
+	if (WARN_ON(!state))
+		return 0;
+
+	/* Calculate and cache data rate for each plane */
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
+		const struct intel_plane_state *intel_pstate =
+			to_intel_plane_state(pstate);
+		enum plane_id plane_id = to_intel_plane(plane)->id;
+		u64 rate;
+
+		if (!intel_pstate->linked_plane) {
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    intel_pstate, 0);
+			plane_data_rate[plane_id] = rate;
+			total_data_rate += rate;
+		} else {
+			enum plane_id y_plane_id;
+
+			/*
+			 * The slave plane might not iterate in
+			 * drm_atomic_crtc_state_for_each_plane_state(),
+			 * and needs the master plane state which may be
+			 * NULL if we try get_new_plane_state(), so we
+			 * always calculate from the master.
+			 */
+			if (intel_pstate->slave)
+				continue;
+
+			/* Y plane rate is calculated on the slave */
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    intel_pstate, 0);
+			y_plane_id = intel_pstate->linked_plane->id;
+			plane_data_rate[y_plane_id] = rate;
+			total_data_rate += rate;
+
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    intel_pstate, 1);
+			plane_data_rate[plane_id] = rate;
+			total_data_rate += rate;
+		}
+	}
+
+	return total_data_rate;
+}
+
 static uint16_t
 skl_ddb_min_alloc(const struct drm_plane_state *pstate, const int plane)
 {

@@ -4297,15 +4346,25 @@ 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;
 
-		if (!pstate->visible)
+		/* slave plane must be invisible and calculated from master */
+		if (!pstate->visible || WARN_ON(plane_state->slave))
 			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 y_plane_id =
+				plane_state->linked_plane->id;
+
+			minimum[y_plane_id] = skl_ddb_min_alloc(pstate, 0);
+			minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
+		}
 	}
 
 	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);

@@ -4317,7 +4376,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 {
 	struct drm_atomic_state *state = cstate->base.state;
 	struct drm_crtc *crtc = cstate->base.crtc;
-	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;

@@ -4343,11 +4402,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 	}
 
-	total_data_rate = skl_get_total_relative_data_rate(cstate,
-							   plane_data_rate,
-							   uv_plane_data_rate);
-	skl_ddb_get_pipe_allocation_limits(dev, cstate, total_data_rate, ddb,
-					   alloc, &num_active);