[Intel-gfx] [PATCH 1/4] drm/i915: Pass plane id to watermark calculation functions

[Intel-gfx] [PATCH 1/4] drm/i915: Pass plane id to watermark calculation functions

[Stanislav Lisovskiy]

Sometimes we might need to change the way we calculate
watermarks, based on which particular plane it is calculated
for. Thus it would be convenient to pass plane_id to those
functions.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2ff260117476..c5c1b6bef9a2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4274,6 +4274,7 @@ static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
 				 u32 plane_pixel_rate, struct skl_wm_params *wp,
 				 int color_plane);
 static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
+				 enum plane_id plane_id,
 				 int level,
 				 unsigned int latency,
 				 const struct skl_wm_params *wp,
@@ -4300,7 +4301,7 @@ skl_cursor_allocation(const struct intel_crtc_state *crtc_state,
 	for (level = 0; level <= max_level; level++) {
 		unsigned int latency = dev_priv->wm.skl_latency[level];
 
-		skl_compute_plane_wm(crtc_state, level, latency, &wp, &wm, &wm);
+		skl_compute_plane_wm(crtc_state, PLANE_CURSOR, level, latency, &wp, &wm, &wm);
 		if (wm.min_ddb_alloc == U16_MAX)
 			break;
 
@@ -5530,6 +5531,7 @@ static int skl_wm_max_lines(struct drm_i915_private *dev_priv)
 }
 
 static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
+				 enum plane_id plane_id,
 				 int level,
 				 unsigned int latency,
 				 const struct skl_wm_params *wp,
@@ -5657,6 +5659,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 
 static void
 skl_compute_wm_levels(const struct intel_crtc_state *crtc_state,
+		      enum plane_id plane_id,
 		      const struct skl_wm_params *wm_params,
 		      struct skl_wm_level *levels)
 {
@@ -5668,7 +5671,7 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state,
 		struct skl_wm_level *result = &levels[level];
 		unsigned int latency = dev_priv->wm.skl_latency[level];
 
-		skl_compute_plane_wm(crtc_state, level, latency,
+		skl_compute_plane_wm(crtc_state, plane_id, level, latency,
 				     wm_params, result_prev, result);
 
 		result_prev = result;
@@ -5676,6 +5679,7 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state,
 }
 
 static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state,
+				enum plane_id plane_id,
 				const struct skl_wm_params *wm_params,
 				struct skl_plane_wm *plane_wm)
 {
@@ -5684,7 +5688,7 @@ static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state,
 	struct skl_wm_level *levels = plane_wm->wm;
 	unsigned int latency = dev_priv->wm.skl_latency[0] + dev_priv->sagv_block_time_us;
 
-	skl_compute_plane_wm(crtc_state, 0, latency,
+	skl_compute_plane_wm(crtc_state, plane_id, 0, latency,
 			     wm_params, &levels[0],
 			     sagv_wm);
 }
@@ -5767,13 +5771,13 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
 	if (ret)
 		return ret;
 
-	skl_compute_wm_levels(crtc_state, &wm_params, wm->wm);
+	skl_compute_wm_levels(crtc_state, plane_id, &wm_params, wm->wm);
 
 	skl_compute_transition_wm(dev_priv, &wm->trans_wm,
 				  &wm->wm[0], &wm_params);
 
 	if (DISPLAY_VER(dev_priv) >= 12) {
-		tgl_compute_sagv_wm(crtc_state, &wm_params, wm);
+		tgl_compute_sagv_wm(crtc_state, plane_id, &wm_params, wm);
 
 		skl_compute_transition_wm(dev_priv, &wm->sagv.trans_wm,
 					  &wm->sagv.wm0, &wm_params);
@@ -5798,7 +5802,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
 	if (ret)
 		return ret;
 
-	skl_compute_wm_levels(crtc_state, &wm_params, wm->uv_wm);
+	skl_compute_wm_levels(crtc_state, plane_id, &wm_params, wm->uv_wm);
 
 	return 0;
 }
-- 
2.24.1.485.gad05a3d8e5

[Intel-gfx] [PATCH 2/4] drm/i915: Introduce do_async_flip flag to intel_plane_state

[Stanislav Lisovskiy]

There might be various logical contructs when we might want
to enable async flip, so lets calculate those and set this
flag, so that there is no need in long conditions in other
places.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic_plane.c  | 2 +-
 drivers/gpu/drm/i915/display/intel_display.c       | 3 +++
 drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index a5bfc047162b..9dceb952009c 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -502,7 +502,7 @@ void intel_update_plane(struct intel_plane *plane,
 
 	trace_intel_update_plane(&plane->base, crtc);
 
-	if (crtc_state->uapi.async_flip && plane->async_flip)
+	if (plane_state->do_async_flip)
 		plane->async_flip(plane, crtc_state, plane_state, true);
 	else
 		plane->update_plane(plane, crtc_state, plane_state);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a610087e6885..b8fe40050463 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5558,6 +5558,9 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
 			 needs_scaling(new_plane_state))))
 		new_crtc_state->disable_lp_wm = true;
 
+	if (new_crtc_state->uapi.async_flip && plane->async_flip)
+		new_plane_state->do_async_flip = true;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 26744f8713ab..d60adcd75757 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -632,6 +632,9 @@ struct intel_plane_state {
 
 	struct intel_fb_view view;
 
+	/* Indicates if async flip is required */
+	bool do_async_flip;
+
 	/* Plane pxp decryption state */
 	bool decrypt;
 
-- 
2.24.1.485.gad05a3d8e5

[Intel-gfx] [PATCH 3/4] drm/i915: Use wm0 only during async flips for DG2

[Stanislav Lisovskiy]

This optimization allows to achieve higher perfomance
during async flips.
For the first async flip we have to still temporarily
switch to sync flip, in order to reprogram plane
watermarks, so this requires taking into account
old plane state's do_async_flip flag.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_pm.c              |  5 ++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b8fe40050463..872bbb965992 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5439,6 +5439,16 @@ static bool needs_scaling(const struct intel_plane_state *state)
 	return (src_w != dst_w || src_h != dst_h);
 }
 
+static bool needs_async_flip_wm_override(struct intel_plane *plane,
+					 const struct intel_plane_state *new_plane_state,
+					 const struct intel_plane_state *old_plane_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	return (DISPLAY_VER(dev_priv) >= 13) && !old_plane_state->do_async_flip &&
+	       new_plane_state->do_async_flip;
+}
+
 int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
 				    struct intel_crtc_state *new_crtc_state,
 				    const struct intel_plane_state *old_plane_state,
@@ -5558,7 +5568,8 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
 			 needs_scaling(new_plane_state))))
 		new_crtc_state->disable_lp_wm = true;
 
-	if (new_crtc_state->uapi.async_flip && plane->async_flip)
+	if (new_crtc_state->uapi.async_flip && new_plane_state->do_async_flip &&
+	    !needs_async_flip_wm_override(plane, new_plane_state, old_plane_state))
 		new_plane_state->do_async_flip = true;
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c5c1b6bef9a2..0b45d1d61d0f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5542,8 +5542,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 	uint_fixed_16_16_t method1, method2;
 	uint_fixed_16_16_t selected_result;
 	u32 blocks, lines, min_ddb_alloc = 0;
+	bool dg2_async_flip_optimization = (DISPLAY_VER(dev_priv) >= 13) &&
+					   crtc_state->uapi.async_flip &&
+					   plane_id != PLANE_CURSOR;
 
-	if (latency == 0) {
+	if (latency == 0 || (dg2_async_flip_optimization && (level > 0))) {
 		/* reject it */
 		result->min_ddb_alloc = U16_MAX;
 		return;
-- 
2.24.1.485.gad05a3d8e5

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0b45d1d61d0f..e1594f43bb1b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5245,9 +5245,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->plane_data_rate[plane_id];
-		extra = min_t(u16, alloc_size,
-			      DIV64_U64_ROUND_UP(alloc_size * rate,
-						 total_data_rate));
+
+		if (IS_DG2(dev_priv) && crtc_state->uapi.async_flip) {
+			extra = 0;
+		} else {
+			extra = min_t(u16, alloc_size,
+				      DIV64_U64_ROUND_UP(alloc_size * rate,
+							 total_data_rate));
+		}
+
 		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
@@ -5256,9 +5262,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->uv_plane_data_rate[plane_id];
-		extra = min_t(u16, alloc_size,
-			      DIV64_U64_ROUND_UP(alloc_size * rate,
-						 total_data_rate));
+
+		if (IS_DG2(dev_priv) && crtc_state->uapi.async_flip) {
+			extra = 0;
+		} else {
+			extra = min_t(u16, alloc_size,
+				      DIV64_U64_ROUND_UP(alloc_size * rate,
+							 total_data_rate));
+		}
+
 		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
-- 
2.24.1.485.gad05a3d8e5

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use wm0 only during async flips for DG2

[Ville Syrjälä]

On Fri, Dec 03, 2021 at 11:40:40AM +0200, Stanislav Lisovskiy wrote:
> This optimization allows to achieve higher perfomance
> during async flips.
> For the first async flip we have to still temporarily
> switch to sync flip, in order to reprogram plane
> watermarks, so this requires taking into account
> old plane state's do_async_flip flag.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 13 ++++++++++++-
>  drivers/gpu/drm/i915/intel_pm.c              |  5 ++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index b8fe40050463..872bbb965992 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5439,6 +5439,16 @@ static bool needs_scaling(const struct intel_plane_state *state)
>  	return (src_w != dst_w || src_h != dst_h);
>  }
>  
> +static bool needs_async_flip_wm_override(struct intel_plane *plane,
> +					 const struct intel_plane_state *new_plane_state,
> +					 const struct intel_plane_state *old_plane_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	return (DISPLAY_VER(dev_priv) >= 13) && !old_plane_state->do_async_flip &&
> +	       new_plane_state->do_async_flip;
> +}
> +
>  int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
>  				    struct intel_crtc_state *new_crtc_state,
>  				    const struct intel_plane_state *old_plane_state,
> @@ -5558,7 +5568,8 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
>  			 needs_scaling(new_plane_state))))
>  		new_crtc_state->disable_lp_wm = true;
>  
> -	if (new_crtc_state->uapi.async_flip && plane->async_flip)
> +	if (new_crtc_state->uapi.async_flip && new_plane_state->do_async_flip &&
> +	    !needs_async_flip_wm_override(plane, new_plane_state, old_plane_state))
>  		new_plane_state->do_async_flip = true;

That code looks super confused.

>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c5c1b6bef9a2..0b45d1d61d0f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5542,8 +5542,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  	uint_fixed_16_16_t method1, method2;
>  	uint_fixed_16_16_t selected_result;
>  	u32 blocks, lines, min_ddb_alloc = 0;
> +	bool dg2_async_flip_optimization = (DISPLAY_VER(dev_priv) >= 13) &&
> +					   crtc_state->uapi.async_flip &&
> +					   plane_id != PLANE_CURSOR;

Function please. Hmm. And rather than checking the plane_id we should
probably just check plane->async_flip since we don't want to minimize
the watermarks for any other plane than the one doing the async flips.

>  
> -	if (latency == 0) {
> +	if (latency == 0 || (dg2_async_flip_optimization && (level > 0))) {
>  		/* reject it */
>  		result->min_ddb_alloc = U16_MAX;
>  		return;
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Fri, Dec 03, 2021 at 11:40:41AM +0200, Stanislav Lisovskiy wrote:
> In terms of async flip optimization we don't to allocate
> extra ddb space, so lets skip it.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0b45d1d61d0f..e1594f43bb1b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5245,9 +5245,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->plane_data_rate[plane_id];
> -		extra = min_t(u16, alloc_size,
> -			      DIV64_U64_ROUND_UP(alloc_size * rate,
> -						 total_data_rate));
> +
> +		if (IS_DG2(dev_priv) && crtc_state->uapi.async_flip) {

We should have a sensible function for this.

> +			extra = 0;

Aren't we going to get the WARN(alloc_size != 0) if this
is the only enabled plane?

> +		} else {
> +			extra = min_t(u16, alloc_size,
> +				      DIV64_U64_ROUND_UP(alloc_size * rate,
> +							 total_data_rate));
> +		}
> +
>  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
> @@ -5256,9 +5262,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->uv_plane_data_rate[plane_id];
> -		extra = min_t(u16, alloc_size,
> -			      DIV64_U64_ROUND_UP(alloc_size * rate,
> -						 total_data_rate));
> +
> +		if (IS_DG2(dev_priv) && crtc_state->uapi.async_flip) {
> +			extra = 0;
> +		} else {
> +			extra = min_t(u16, alloc_size,
> +				      DIV64_U64_ROUND_UP(alloc_size * rate,
> +							 total_data_rate));
> +		}
> +
>  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 3/4] drm/i915: Use wm0 only during async flips for DG2

[Lisovskiy, Stanislav]

On Fri, Dec 03, 2021 at 12:00:42PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 03, 2021 at 11:40:40AM +0200, Stanislav Lisovskiy wrote:
> > This optimization allows to achieve higher perfomance
> > during async flips.
> > For the first async flip we have to still temporarily
> > switch to sync flip, in order to reprogram plane
> > watermarks, so this requires taking into account
> > old plane state's do_async_flip flag.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 13 ++++++++++++-
> >  drivers/gpu/drm/i915/intel_pm.c              |  5 ++++-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index b8fe40050463..872bbb965992 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5439,6 +5439,16 @@ static bool needs_scaling(const struct intel_plane_state *state)
> >  	return (src_w != dst_w || src_h != dst_h);
> >  }
> >  
> > +static bool needs_async_flip_wm_override(struct intel_plane *plane,
> > +					 const struct intel_plane_state *new_plane_state,
> > +					 const struct intel_plane_state *old_plane_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +
> > +	return (DISPLAY_VER(dev_priv) >= 13) && !old_plane_state->do_async_flip &&
> > +	       new_plane_state->do_async_flip;
> > +}
> > +
> >  int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
> >  				    struct intel_crtc_state *new_crtc_state,
> >  				    const struct intel_plane_state *old_plane_state,
> > @@ -5558,7 +5568,8 @@ int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_stat
> >  			 needs_scaling(new_plane_state))))
> >  		new_crtc_state->disable_lp_wm = true;
> >  
> > -	if (new_crtc_state->uapi.async_flip && plane->async_flip)
> > +	if (new_crtc_state->uapi.async_flip && new_plane_state->do_async_flip &&
> > +	    !needs_async_flip_wm_override(plane, new_plane_state, old_plane_state))
> >  		new_plane_state->do_async_flip = true;
> 
> That code looks super confused.

Oh.. thanks for spotting new_plane_state->do_async_flip is redundant here.
The logic is such that if old_plane_state didn't have do_async_flip flag set,
but the new_plane_state has, it means we do first async flip here, thus
it requires temporary switching to sync flip as we discussed.

So I actually meant something like this here:

> > +   if (new_crtc_state->uapi.async_flip && !needs_async_flip_wm_override(plane, 
						new_plane_state, old_plane_state))


> 
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index c5c1b6bef9a2..0b45d1d61d0f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5542,8 +5542,11 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> >  	uint_fixed_16_16_t method1, method2;
> >  	uint_fixed_16_16_t selected_result;
> >  	u32 blocks, lines, min_ddb_alloc = 0;
> > +	bool dg2_async_flip_optimization = (DISPLAY_VER(dev_priv) >= 13) &&
> > +					   crtc_state->uapi.async_flip &&
> > +					   plane_id != PLANE_CURSOR;
> 
> Function please. Hmm. And rather than checking the plane_id we should
> probably just check plane->async_flip since we don't want to minimize
> the watermarks for any other plane than the one doing the async flips.

Function sounds like good idea.

Stan

> 
> >  
> > -	if (latency == 0) {
> > +	if (latency == 0 || (dg2_async_flip_optimization && (level > 0))) {
> >  		/* reject it */
> >  		result->min_ddb_alloc = U16_MAX;
> >  		return;
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Lisovskiy, Stanislav]

On Fri, Dec 03, 2021 at 12:03:41PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 03, 2021 at 11:40:41AM +0200, Stanislav Lisovskiy wrote:
> > In terms of async flip optimization we don't to allocate
> > extra ddb space, so lets skip it.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0b45d1d61d0f..e1594f43bb1b 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5245,9 +5245,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->plane_data_rate[plane_id];
> > -		extra = min_t(u16, alloc_size,
> > -			      DIV64_U64_ROUND_UP(alloc_size * rate,
> > -						 total_data_rate));
> > +
> > +		if (IS_DG2(dev_priv) && crtc_state->uapi.async_flip) {
> 
> We should have a sensible function for this.

Agree, tbh was thinking about this as well.

> 
> > +			extra = 0;
> 
> Aren't we going to get the WARN(alloc_size != 0) if this
> is the only enabled plane?

Actually true as well, I remember I did get this warn when
I was testing it. 
I guess we shouldn't trigger WARN for that case then?

> 
> > +		} else {
> > +			extra = min_t(u16, alloc_size,
> > +				      DIV64_U64_ROUND_UP(alloc_size * rate,
> > +							 total_data_rate));
> > +		}
> > +
> >  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> > @@ -5256,9 +5262,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->uv_plane_data_rate[plane_id];
> > -		extra = min_t(u16, alloc_size,
> > -			      DIV64_U64_ROUND_UP(alloc_size * rate,
> > -						 total_data_rate));
> > +
> > +		if (IS_DG2(dev_priv) && crtc_state->uapi.async_flip) {
> > +			extra = 0;
> > +		} else {
> > +			extra = min_t(u16, alloc_size,
> > +				      DIV64_U64_ROUND_UP(alloc_size * rate,
> > +							 total_data_rate));
> > +		}
> > +
> >  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/4] drm/i915: Pass plane id to watermark calculation functions

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Pass plane id to watermark calculation functions
URL   : https://patchwork.freedesktop.org/series/97536/
State : failure

== Summary ==

Applying: drm/i915: Pass plane id to watermark calculation functions
Applying: drm/i915: Introduce do_async_flip flag to intel_plane_state
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/display/intel_atomic_plane.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 drm/i915: Introduce do_async_flip flag to intel_plane_state
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Lisovskiy, Stanislav]

On Mon, Jan 24, 2022 at 08:55:57PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 24, 2022 at 03:52:34PM +0200, Stanislav Lisovskiy wrote:
> > In terms of async flip optimization we don't to allocate
> > extra ddb space, so lets skip it.
> > 
> > v2: - Extracted min ddb async flip check to separate function
> >       (Ville Syrjälä)
> >     - Used this function to prevent false positive WARN
> >       to be triggered(Ville Syrjälä)
> > 
> > v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
> >       it more universal.
> >     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
> >     - Use rate = 0 instead of just setting extra = 0, thus
> >       letting other planes to use extra ddb and avoiding WARN
> >       (Ville Syrjälä)
> > 
> > v4: - Renamed needs_min_ddb as s/needs/use/ to match
> >       the wm0 counterpart(Ville Syrjälä)
> >     - Added plane->async_flip check to use_min_ddb(now
> >       passing plane as a parameter to do that)(Ville Syrjälä)
> >     - Account for use_min_ddb also when calculating total data rate
> >       (Ville Syrjälä)
> > 
> > v5:
> >     - Use for_each_intel_plane_on_crtc instead of for_each_intel_plane_id
> >       to get plane->async_flip check and account for all planes(Ville Syrjälä)
> >     - Fix line wrapping(Ville Syrjälä)
> >     - Set plane data rate conditionally, avoiding on redundant assignment
> >       (Ville Syrjälä)
> >     - Removed redundant whitespace(Ville Syrjälä)
> >     - Handle use_min_ddb case in skl_plane_relative_data_rate instead of
> >       icl_get_total_relative_data_rate(Ville Syrjälä)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0bb4c941f950..bb147e5a77b6 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4906,6 +4906,16 @@ static u8 skl_compute_dbuf_slices(struct intel_crtc *crtc, u8 active_pipes)
> >  	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
> >  }
> >  
> > +static bool use_min_ddb(const struct intel_crtc_state *crtc_state,
> > +			struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> > +	return DISPLAY_VER(i915) >= 13 &&
> > +	       crtc_state->uapi.async_flip &&
> > +	       plane->async_flip;
> > +}
> > +
> >  static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
> >  				 struct intel_plane *plane)
> >  {
> > @@ -4934,6 +4944,14 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> >  	if (plane->id == PLANE_CURSOR)
> >  		return 0;
> >  
> > +	/*
> > +	 * We calculate extra ddb based on ratio plane rate/total data rate
> > +	 * in case, in some cases we should not allocate extra ddb for the plane,
> > +	 * so do not count its data rate, if this is the case.
> > +	 */
> > +	if (use_min_ddb(crtc_state, plane))
> > +		return 0;
> > +
> >  	if (color_plane == 1 &&
> >  	    !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
> >  		return 0;
> 
> Yeah this looks nice and simple. Only minor nit is that I'd probably
> have put it after this ccs vs. planar related thing which is a more
> static decision than the async flip optimization.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I guess the first patch in this series is also ok?

Just wondering, if I can push it now.

Stan

> 
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Mon, Jan 24, 2022 at 03:52:34PM +0200, Stanislav Lisovskiy wrote:
> In terms of async flip optimization we don't to allocate
> extra ddb space, so lets skip it.
> 
> v2: - Extracted min ddb async flip check to separate function
>       (Ville Syrjälä)
>     - Used this function to prevent false positive WARN
>       to be triggered(Ville Syrjälä)
> 
> v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
>       it more universal.
>     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
>     - Use rate = 0 instead of just setting extra = 0, thus
>       letting other planes to use extra ddb and avoiding WARN
>       (Ville Syrjälä)
> 
> v4: - Renamed needs_min_ddb as s/needs/use/ to match
>       the wm0 counterpart(Ville Syrjälä)
>     - Added plane->async_flip check to use_min_ddb(now
>       passing plane as a parameter to do that)(Ville Syrjälä)
>     - Account for use_min_ddb also when calculating total data rate
>       (Ville Syrjälä)
> 
> v5:
>     - Use for_each_intel_plane_on_crtc instead of for_each_intel_plane_id
>       to get plane->async_flip check and account for all planes(Ville Syrjälä)
>     - Fix line wrapping(Ville Syrjälä)
>     - Set plane data rate conditionally, avoiding on redundant assignment
>       (Ville Syrjälä)
>     - Removed redundant whitespace(Ville Syrjälä)
>     - Handle use_min_ddb case in skl_plane_relative_data_rate instead of
>       icl_get_total_relative_data_rate(Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0bb4c941f950..bb147e5a77b6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4906,6 +4906,16 @@ static u8 skl_compute_dbuf_slices(struct intel_crtc *crtc, u8 active_pipes)
>  	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
>  }
>  
> +static bool use_min_ddb(const struct intel_crtc_state *crtc_state,
> +			struct intel_plane *plane)
> +{
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
> +	return DISPLAY_VER(i915) >= 13 &&
> +	       crtc_state->uapi.async_flip &&
> +	       plane->async_flip;
> +}
> +
>  static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
>  				 struct intel_plane *plane)
>  {
> @@ -4934,6 +4944,14 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
>  	if (plane->id == PLANE_CURSOR)
>  		return 0;
>  
> +	/*
> +	 * We calculate extra ddb based on ratio plane rate/total data rate
> +	 * in case, in some cases we should not allocate extra ddb for the plane,
> +	 * so do not count its data rate, if this is the case.
> +	 */
> +	if (use_min_ddb(crtc_state, plane))
> +		return 0;
> +
>  	if (color_plane == 1 &&
>  	    !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
>  		return 0;

Yeah this looks nice and simple. Only minor nit is that I'd probably
have put it after this ccs vs. planar related thing which is a more
static decision than the async flip optimization.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

v2: - Extracted min ddb async flip check to separate function
      (Ville Syrjälä)
    - Used this function to prevent false positive WARN
      to be triggered(Ville Syrjälä)

v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
      it more universal.
    - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
    - Use rate = 0 instead of just setting extra = 0, thus
      letting other planes to use extra ddb and avoiding WARN
      (Ville Syrjälä)

v4: - Renamed needs_min_ddb as s/needs/use/ to match
      the wm0 counterpart(Ville Syrjälä)
    - Added plane->async_flip check to use_min_ddb(now
      passing plane as a parameter to do that)(Ville Syrjälä)
    - Account for use_min_ddb also when calculating total data rate
      (Ville Syrjälä)

v5:
    - Use for_each_intel_plane_on_crtc instead of for_each_intel_plane_id
      to get plane->async_flip check and account for all planes(Ville Syrjälä)
    - Fix line wrapping(Ville Syrjälä)
    - Set plane data rate conditionally, avoiding on redundant assignment
      (Ville Syrjälä)
    - Removed redundant whitespace(Ville Syrjälä)
    - Handle use_min_ddb case in skl_plane_relative_data_rate instead of
      icl_get_total_relative_data_rate(Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0bb4c941f950..bb147e5a77b6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4906,6 +4906,16 @@ static u8 skl_compute_dbuf_slices(struct intel_crtc *crtc, u8 active_pipes)
 	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
 }
 
+static bool use_min_ddb(const struct intel_crtc_state *crtc_state,
+			struct intel_plane *plane)
+{
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
+	return DISPLAY_VER(i915) >= 13 &&
+	       crtc_state->uapi.async_flip &&
+	       plane->async_flip;
+}
+
 static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
 				 struct intel_plane *plane)
 {
@@ -4934,6 +4944,14 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
 	if (plane->id == PLANE_CURSOR)
 		return 0;
 
+	/*
+	 * We calculate extra ddb based on ratio plane rate/total data rate
+	 * in case, in some cases we should not allocate extra ddb for the plane,
+	 * so do not count its data rate, if this is the case.
+	 */
+	if (use_min_ddb(crtc_state, plane))
+		return 0;
+
 	if (color_plane == 1 &&
 	    !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
 		return 0;
-- 
2.24.1.485.gad05a3d8e5

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Mon, Jan 24, 2022 at 11:51:23AM +0200, Stanislav Lisovskiy wrote:
> In terms of async flip optimization we don't to allocate
> extra ddb space, so lets skip it.
> 
> v2: - Extracted min ddb async flip check to separate function
>       (Ville Syrjälä)
>     - Used this function to prevent false positive WARN
>       to be triggered(Ville Syrjälä)
> 
> v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
>       it more universal.
>     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
>     - Use rate = 0 instead of just setting extra = 0, thus
>       letting other planes to use extra ddb and avoiding WARN
>       (Ville Syrjälä)
> 
> v4: - Renamed needs_min_ddb as s/needs/use/ to match
>       the wm0 counterpart(Ville Syrjälä)
>     - Added plane->async_flip check to use_min_ddb(now
>       passing plane as a parameter to do that)(Ville Syrjälä)
>     - Account for use_min_ddb also when calculating total data rate
>       (Ville Syrjälä)
> 
> v5:
>     - Use for_each_intel_plane_on_crtc instead of for_each_intel_plane_id
>       to get plane->async_flip check and account for all planes(Ville Syrjälä)
>     - Fix line wrapping(Ville Syrjälä)
>     - Set plane data rate conditionally, avoiding on redundant assignment
>       (Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c               | 40 ++++++++++++++++---
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index ead789709477..c238177e5563 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -23,7 +23,6 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
>  				 unsigned int rate);
>  unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
>  				    const struct intel_plane_state *plane_state);
> -

supurious whitespace changes

>  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state);
>  void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f6c742b583c1..7ce26f22e10e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4988,6 +4988,16 @@ skl_get_total_relative_data_rate(struct intel_atomic_state *state,
>  	return total_data_rate;
>  }
>  
> +static bool use_min_ddb(struct intel_crtc_state *crtc_state,
> +			struct intel_plane *plane)
> +{
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
> +	return DISPLAY_VER(i915) >= 13 &&
> +	       crtc_state->uapi.async_flip &&
> +	       plane->async_flip;
> +}
> +
>  static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
>  				 struct intel_plane *plane)
>  {
> @@ -5002,6 +5012,7 @@ static u64
>  icl_get_total_relative_data_rate(struct intel_atomic_state *state,
>  				 struct intel_crtc *crtc)
>  {
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>  	struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	const struct intel_plane_state *plane_state;
> @@ -5043,8 +5054,15 @@ icl_get_total_relative_data_rate(struct intel_atomic_state *state,
>  		}
>  	}
>  
> -	for_each_plane_id_on_crtc(crtc, plane_id)
> -		total_data_rate += crtc_state->plane_data_rate[plane_id];

Hmm. I was going to say we should remove plane_id now, but looks like
the other loop still uses it. I would move the declaration into that
loop now to make it less likely we mess up and use the wrong thing.

> +	for_each_intel_plane_on_crtc(&i915->drm, crtc, plane) {
> +		/*
> +		 * We calculate extra ddb based on ratio plane rate/total data rate
> +		 * in case, in some cases we should not allocate extra ddb for the plane,
> +		 * so do not count its data rate, if this is the case.
> +		 */
> +		if (!use_min_ddb(crtc_state, plane))
> +			total_data_rate += crtc_state->plane_data_rate[plane->id];
> +	}
>  
>  	return total_data_rate;
>  }
> @@ -5130,6 +5148,7 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  	u16 alloc_size, start = 0;
>  	u16 total[I915_MAX_PLANES] = {};
>  	u16 uv_total[I915_MAX_PLANES] = {};
> +	struct intel_plane *plane;
>  	u64 total_data_rate;
>  	enum plane_id plane_id;
>  	u32 blocks;
> @@ -5206,7 +5225,7 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  	 * watermark level, plus an extra share of the leftover blocks
>  	 * proportional to its relative data rate.
>  	 */
> -	for_each_plane_id_on_crtc(crtc, plane_id) {
> +	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>  		const struct skl_plane_wm *wm =
>  			&crtc_state->wm.skl.optimal.planes[plane_id];

And here we do use the wrong thing. Should be plane->id.

Apparently some other loops still use plane_id, so can't remove it
fully :(

It's looking a bit messy overall. Might just be much cleaner to
calculate the plane_data_rate[] stuff as zero to start with so we
wouldn't have to deal with any of this here. Looks like you could
just handle it in skl_plane_relative_data_rate() in fact.

>  		u64 rate;
> @@ -5222,10 +5241,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  		if (total_data_rate == 0)
>  			break;
>  
> -		rate = crtc_state->plane_data_rate[plane_id];
> +		if (use_min_ddb(crtc_state, plane))
> +			rate = 0;
> +		else
> +			rate = crtc_state->plane_data_rate[plane_id];
> +
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> +
>  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
> @@ -5233,14 +5257,20 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  		if (total_data_rate == 0)
>  			break;
>  
> -		rate = crtc_state->uv_plane_data_rate[plane_id];
> +		if (use_min_ddb(crtc_state, plane))
> +			rate = 0;
> +		else
> +			rate = crtc_state->uv_plane_data_rate[plane_id];
> +
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> +
>  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
>  	}
> +
>  	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
>  
>  	/* Set the actual DDB start/end points for each plane */
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

v2: - Extracted min ddb async flip check to separate function
      (Ville Syrjälä)
    - Used this function to prevent false positive WARN
      to be triggered(Ville Syrjälä)

v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
      it more universal.
    - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
    - Use rate = 0 instead of just setting extra = 0, thus
      letting other planes to use extra ddb and avoiding WARN
      (Ville Syrjälä)

v4: - Renamed needs_min_ddb as s/needs/use/ to match
      the wm0 counterpart(Ville Syrjälä)
    - Added plane->async_flip check to use_min_ddb(now
      passing plane as a parameter to do that)(Ville Syrjälä)
    - Account for use_min_ddb also when calculating total data rate
      (Ville Syrjälä)

v5:
    - Use for_each_intel_plane_on_crtc instead of for_each_intel_plane_id
      to get plane->async_flip check and account for all planes(Ville Syrjälä)
    - Fix line wrapping(Ville Syrjälä)
    - Set plane data rate conditionally, avoiding on redundant assignment
      (Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 .../gpu/drm/i915/display/intel_atomic_plane.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c               | 40 ++++++++++++++++---
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
index ead789709477..c238177e5563 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
@@ -23,7 +23,6 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
 				 unsigned int rate);
 unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
 				    const struct intel_plane_state *plane_state);
-
 unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
 				   const struct intel_plane_state *plane_state);
 void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f6c742b583c1..7ce26f22e10e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4988,6 +4988,16 @@ skl_get_total_relative_data_rate(struct intel_atomic_state *state,
 	return total_data_rate;
 }
 
+static bool use_min_ddb(struct intel_crtc_state *crtc_state,
+			struct intel_plane *plane)
+{
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
+	return DISPLAY_VER(i915) >= 13 &&
+	       crtc_state->uapi.async_flip &&
+	       plane->async_flip;
+}
+
 static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
 				 struct intel_plane *plane)
 {
@@ -5002,6 +5012,7 @@ static u64
 icl_get_total_relative_data_rate(struct intel_atomic_state *state,
 				 struct intel_crtc *crtc)
 {
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
 	struct intel_crtc_state *crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
 	const struct intel_plane_state *plane_state;
@@ -5043,8 +5054,15 @@ icl_get_total_relative_data_rate(struct intel_atomic_state *state,
 		}
 	}
 
-	for_each_plane_id_on_crtc(crtc, plane_id)
-		total_data_rate += crtc_state->plane_data_rate[plane_id];
+	for_each_intel_plane_on_crtc(&i915->drm, crtc, plane) {
+		/*
+		 * We calculate extra ddb based on ratio plane rate/total data rate
+		 * in case, in some cases we should not allocate extra ddb for the plane,
+		 * so do not count its data rate, if this is the case.
+		 */
+		if (!use_min_ddb(crtc_state, plane))
+			total_data_rate += crtc_state->plane_data_rate[plane->id];
+	}
 
 	return total_data_rate;
 }
@@ -5130,6 +5148,7 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 	u16 alloc_size, start = 0;
 	u16 total[I915_MAX_PLANES] = {};
 	u16 uv_total[I915_MAX_PLANES] = {};
+	struct intel_plane *plane;
 	u64 total_data_rate;
 	enum plane_id plane_id;
 	u32 blocks;
@@ -5206,7 +5225,7 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 	 * watermark level, plus an extra share of the leftover blocks
 	 * proportional to its relative data rate.
 	 */
-	for_each_plane_id_on_crtc(crtc, plane_id) {
+	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
 		const struct skl_plane_wm *wm =
 			&crtc_state->wm.skl.optimal.planes[plane_id];
 		u64 rate;
@@ -5222,10 +5241,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 		if (total_data_rate == 0)
 			break;
 
-		rate = crtc_state->plane_data_rate[plane_id];
+		if (use_min_ddb(crtc_state, plane))
+			rate = 0;
+		else
+			rate = crtc_state->plane_data_rate[plane_id];
+
 		extra = min_t(u16, alloc_size,
 			      DIV64_U64_ROUND_UP(alloc_size * rate,
 						 total_data_rate));
+
 		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
@@ -5233,14 +5257,20 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 		if (total_data_rate == 0)
 			break;
 
-		rate = crtc_state->uv_plane_data_rate[plane_id];
+		if (use_min_ddb(crtc_state, plane))
+			rate = 0;
+		else
+			rate = crtc_state->uv_plane_data_rate[plane_id];
+
 		extra = min_t(u16, alloc_size,
 			      DIV64_U64_ROUND_UP(alloc_size * rate,
 						 total_data_rate));
+
 		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
 	}
+
 	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
 
 	/* Set the actual DDB start/end points for each plane */
-- 
2.24.1.485.gad05a3d8e5

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Lisovskiy, Stanislav]

On Mon, Jan 24, 2022 at 11:16:36AM +0200, Ville Syrjälä wrote:
> On Mon, Jan 24, 2022 at 11:06:53AM +0200, Stanislav Lisovskiy wrote:
> > In terms of async flip optimization we don't to allocate
> > extra ddb space, so lets skip it.
> > 
> > v2: - Extracted min ddb async flip check to separate function
> >       (Ville Syrjälä)
> >     - Used this function to prevent false positive WARN
> >       to be triggered(Ville Syrjälä)
> > 
> > v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
> >       it more universal.
> >     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
> >     - Use rate = 0 instead of just setting extra = 0, thus
> >       letting other planes to use extra ddb and avoiding WARN
> >       (Ville Syrjälä)
> > 
> > v4: - Renamed needs_min_ddb as s/needs/use/ to match
> >       the wm0 counterpart(Ville Syrjälä)
> >     - Added plane->async_flip check to use_min_ddb(now
> >       passing plane as a parameter to do that)(Ville Syrjälä)
> >     - Account for use_min_ddb also when calculating total data rate
> >       (Ville Syrjälä)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c |  2 +-
> >  .../gpu/drm/i915/display/intel_atomic_plane.h |  3 +-
> >  drivers/gpu/drm/i915/intel_pm.c               | 53 ++++++++++++++-----
> >  3 files changed, 44 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index b20cf2c16691..9d79ab987b2e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -374,7 +374,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >  					       old_plane_state, new_plane_state);
> >  }
> >  
> > -static struct intel_plane *
> > +struct intel_plane *
> >  intel_crtc_get_plane(struct intel_crtc *crtc, enum plane_id plane_id)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > index ead789709477..aaddcc636f98 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > @@ -23,7 +23,8 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
> >  				 unsigned int rate);
> >  unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> >  				    const struct intel_plane_state *plane_state);
> > -
> > +struct intel_plane *intel_crtc_get_plane(struct intel_crtc *crtc,
> > +					  enum plane_id plane_id);
> >  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
> >  				   const struct intel_plane_state *plane_state);
> >  void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8269f1e9c784..fbe6a45801bc 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4988,6 +4988,25 @@ skl_get_total_relative_data_rate(struct intel_atomic_state *state,
> >  	return total_data_rate;
> >  }
> >  
> > +static bool use_min_ddb(struct intel_crtc_state *crtc_state,
> > +			struct intel_plane *plane)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> > +	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip &&
> > +	       plane->async_flip;
> > +}
> > +
> > +static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
> > +				 struct intel_plane *plane)
> 
> Please put this function into the right spot to begin with.
> Avoids polluting this patch with unrelated code movement.

It happened this way, because initially use_minimal_wm0_only which
is needed only by skl_compute_plane_wm was much lower, however as
you recommended to group it with use_min_ddb, so had to move it higher,
because use_min_ddb is required way higher in the file.

However I will then change the patch where it was introduced, so that
its already there. 

> 
> > +{
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +
> > +	return DISPLAY_VER(i915) >= 13 &&
> > +	       crtc_state->uapi.async_flip &&
> > +	       plane->async_flip;
> 
> Line wrapping is different between the two functions despite the
> identical contents, which looks a bit weird.
> 
> > +}
> > +
> >  static u64
> >  icl_get_total_relative_data_rate(struct intel_atomic_state *state,
> >  				 struct intel_crtc *crtc)
> > @@ -5033,8 +5052,15 @@ icl_get_total_relative_data_rate(struct intel_atomic_state *state,
> >  		}
> >  	}
> >  
> > -	for_each_plane_id_on_crtc(crtc, plane_id)
> > -		total_data_rate += crtc_state->plane_data_rate[plane_id];
> > +	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> 
> for_each_intel_plane_on_crtc() otherwise we miss all
> the planes not in the state.

Right, we recalculate those which changed first, then get total data rate.
For some reason I was looking at first cycle so just took same macro.

> 
> > +		/*
> > +		 * We calculate extra ddb based on ratio plane rate/total data rate
> > +		 * in case, in some cases we should not allocate extra ddb for the plane,
> > +		 * so do not count its data rate, if this is the case.
> > +		 */
> > +		if (!use_min_ddb(crtc_state, plane))
> > +			total_data_rate += crtc_state->plane_data_rate[plane->id];
> > +	}
> >  
> >  	return total_data_rate;
> >  }
> > @@ -5199,6 +5225,8 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  	for_each_plane_id_on_crtc(crtc, plane_id) {
> 
> for_each_intel_plane_on_crtc()...
> 
> >  		const struct skl_plane_wm *wm =
> >  			&crtc_state->wm.skl.optimal.planes[plane_id];
> > +		struct intel_plane *plane =
> > +			intel_crtc_get_plane(crtc, plane_id);
> 
> ... avoids needing that function.
> 
> >  		u64 rate;
> >  		u16 extra;
> >  
> > @@ -5213,9 +5241,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->plane_data_rate[plane_id];
> > +
> > +		if (use_min_ddb(crtc_state, plane))
> > +			rate = 0;
> 
> else
> 	rate = crtc_state->plane_data_rate[plane_id];
> 
> is how I'd write that probably.
> 
> > +
> >  		extra = min_t(u16, alloc_size,
> >  			      DIV64_U64_ROUND_UP(alloc_size * rate,
> >  						 total_data_rate));
> > +
> >  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> > @@ -5224,13 +5257,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->uv_plane_data_rate[plane_id];
> > +
> > +		if (use_min_ddb(crtc_state, plane))
> > +			rate = 0;
> > +
> >  		extra = min_t(u16, alloc_size,
> >  			      DIV64_U64_ROUND_UP(alloc_size * rate,
> >  						 total_data_rate));
> > +
> >  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> >  	}
> > +
> >  	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
> >  
> >  	/* Set the actual DDB start/end points for each plane */
> > @@ -5497,16 +5536,6 @@ static int skl_wm_max_lines(struct drm_i915_private *dev_priv)
> >  		return 31;
> >  }
> >  
> > -static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
> > -				 struct intel_plane *plane)
> > -{
> > -	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > -
> > -	return DISPLAY_VER(i915) >= 13 &&
> > -	       crtc_state->uapi.async_flip &&
> > -	       plane->async_flip;
> > -}
> > -
> >  static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> >  				 struct intel_plane *plane,
> >  				 int level,
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Mon, Jan 24, 2022 at 11:06:53AM +0200, Stanislav Lisovskiy wrote:
> In terms of async flip optimization we don't to allocate
> extra ddb space, so lets skip it.
> 
> v2: - Extracted min ddb async flip check to separate function
>       (Ville Syrjälä)
>     - Used this function to prevent false positive WARN
>       to be triggered(Ville Syrjälä)
> 
> v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
>       it more universal.
>     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
>     - Use rate = 0 instead of just setting extra = 0, thus
>       letting other planes to use extra ddb and avoiding WARN
>       (Ville Syrjälä)
> 
> v4: - Renamed needs_min_ddb as s/needs/use/ to match
>       the wm0 counterpart(Ville Syrjälä)
>     - Added plane->async_flip check to use_min_ddb(now
>       passing plane as a parameter to do that)(Ville Syrjälä)
>     - Account for use_min_ddb also when calculating total data rate
>       (Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c |  2 +-
>  .../gpu/drm/i915/display/intel_atomic_plane.h |  3 +-
>  drivers/gpu/drm/i915/intel_pm.c               | 53 ++++++++++++++-----
>  3 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index b20cf2c16691..9d79ab987b2e 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -374,7 +374,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  					       old_plane_state, new_plane_state);
>  }
>  
> -static struct intel_plane *
> +struct intel_plane *
>  intel_crtc_get_plane(struct intel_crtc *crtc, enum plane_id plane_id)
>  {
>  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> index ead789709477..aaddcc636f98 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> @@ -23,7 +23,8 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
>  				 unsigned int rate);
>  unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
>  				    const struct intel_plane_state *plane_state);
> -
> +struct intel_plane *intel_crtc_get_plane(struct intel_crtc *crtc,
> +					  enum plane_id plane_id);
>  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state);
>  void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8269f1e9c784..fbe6a45801bc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4988,6 +4988,25 @@ skl_get_total_relative_data_rate(struct intel_atomic_state *state,
>  	return total_data_rate;
>  }
>  
> +static bool use_min_ddb(struct intel_crtc_state *crtc_state,
> +			struct intel_plane *plane)
> +{
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
> +	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip &&
> +	       plane->async_flip;
> +}
> +
> +static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
> +				 struct intel_plane *plane)

Please put this function into the right spot to begin with.
Avoids polluting this patch with unrelated code movement.

> +{
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
> +	return DISPLAY_VER(i915) >= 13 &&
> +	       crtc_state->uapi.async_flip &&
> +	       plane->async_flip;

Line wrapping is different between the two functions despite the
identical contents, which looks a bit weird.

> +}
> +
>  static u64
>  icl_get_total_relative_data_rate(struct intel_atomic_state *state,
>  				 struct intel_crtc *crtc)
> @@ -5033,8 +5052,15 @@ icl_get_total_relative_data_rate(struct intel_atomic_state *state,
>  		}
>  	}
>  
> -	for_each_plane_id_on_crtc(crtc, plane_id)
> -		total_data_rate += crtc_state->plane_data_rate[plane_id];
> +	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {

for_each_intel_plane_on_crtc() otherwise we miss all
the planes not in the state.

> +		/*
> +		 * We calculate extra ddb based on ratio plane rate/total data rate
> +		 * in case, in some cases we should not allocate extra ddb for the plane,
> +		 * so do not count its data rate, if this is the case.
> +		 */
> +		if (!use_min_ddb(crtc_state, plane))
> +			total_data_rate += crtc_state->plane_data_rate[plane->id];
> +	}
>  
>  	return total_data_rate;
>  }
> @@ -5199,6 +5225,8 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  	for_each_plane_id_on_crtc(crtc, plane_id) {

for_each_intel_plane_on_crtc()...

>  		const struct skl_plane_wm *wm =
>  			&crtc_state->wm.skl.optimal.planes[plane_id];
> +		struct intel_plane *plane =
> +			intel_crtc_get_plane(crtc, plane_id);

... avoids needing that function.

>  		u64 rate;
>  		u16 extra;
>  
> @@ -5213,9 +5241,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->plane_data_rate[plane_id];
> +
> +		if (use_min_ddb(crtc_state, plane))
> +			rate = 0;

else
	rate = crtc_state->plane_data_rate[plane_id];

is how I'd write that probably.

> +
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> +
>  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
> @@ -5224,13 +5257,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->uv_plane_data_rate[plane_id];
> +
> +		if (use_min_ddb(crtc_state, plane))
> +			rate = 0;
> +
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> +
>  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
>  	}
> +
>  	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
>  
>  	/* Set the actual DDB start/end points for each plane */
> @@ -5497,16 +5536,6 @@ static int skl_wm_max_lines(struct drm_i915_private *dev_priv)
>  		return 31;
>  }
>  
> -static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
> -				 struct intel_plane *plane)
> -{
> -	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> -
> -	return DISPLAY_VER(i915) >= 13 &&
> -	       crtc_state->uapi.async_flip &&
> -	       plane->async_flip;
> -}
> -
>  static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  				 struct intel_plane *plane,
>  				 int level,
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

v2: - Extracted min ddb async flip check to separate function
      (Ville Syrjälä)
    - Used this function to prevent false positive WARN
      to be triggered(Ville Syrjälä)

v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
      it more universal.
    - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
    - Use rate = 0 instead of just setting extra = 0, thus
      letting other planes to use extra ddb and avoiding WARN
      (Ville Syrjälä)

v4: - Renamed needs_min_ddb as s/needs/use/ to match
      the wm0 counterpart(Ville Syrjälä)
    - Added plane->async_flip check to use_min_ddb(now
      passing plane as a parameter to do that)(Ville Syrjälä)
    - Account for use_min_ddb also when calculating total data rate
      (Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 .../gpu/drm/i915/display/intel_atomic_plane.c |  2 +-
 .../gpu/drm/i915/display/intel_atomic_plane.h |  3 +-
 drivers/gpu/drm/i915/intel_pm.c               | 53 ++++++++++++++-----
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index b20cf2c16691..9d79ab987b2e 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -374,7 +374,7 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 					       old_plane_state, new_plane_state);
 }
 
-static struct intel_plane *
+struct intel_plane *
 intel_crtc_get_plane(struct intel_crtc *crtc, enum plane_id plane_id)
 {
 	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
index ead789709477..aaddcc636f98 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
@@ -23,7 +23,8 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src,
 				 unsigned int rate);
 unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
 				    const struct intel_plane_state *plane_state);
-
+struct intel_plane *intel_crtc_get_plane(struct intel_crtc *crtc,
+					  enum plane_id plane_id);
 unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
 				   const struct intel_plane_state *plane_state);
 void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8269f1e9c784..fbe6a45801bc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4988,6 +4988,25 @@ skl_get_total_relative_data_rate(struct intel_atomic_state *state,
 	return total_data_rate;
 }
 
+static bool use_min_ddb(struct intel_crtc_state *crtc_state,
+			struct intel_plane *plane)
+{
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
+	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip &&
+	       plane->async_flip;
+}
+
+static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
+				 struct intel_plane *plane)
+{
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
+	return DISPLAY_VER(i915) >= 13 &&
+	       crtc_state->uapi.async_flip &&
+	       plane->async_flip;
+}
+
 static u64
 icl_get_total_relative_data_rate(struct intel_atomic_state *state,
 				 struct intel_crtc *crtc)
@@ -5033,8 +5052,15 @@ icl_get_total_relative_data_rate(struct intel_atomic_state *state,
 		}
 	}
 
-	for_each_plane_id_on_crtc(crtc, plane_id)
-		total_data_rate += crtc_state->plane_data_rate[plane_id];
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		/*
+		 * We calculate extra ddb based on ratio plane rate/total data rate
+		 * in case, in some cases we should not allocate extra ddb for the plane,
+		 * so do not count its data rate, if this is the case.
+		 */
+		if (!use_min_ddb(crtc_state, plane))
+			total_data_rate += crtc_state->plane_data_rate[plane->id];
+	}
 
 	return total_data_rate;
 }
@@ -5199,6 +5225,8 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 	for_each_plane_id_on_crtc(crtc, plane_id) {
 		const struct skl_plane_wm *wm =
 			&crtc_state->wm.skl.optimal.planes[plane_id];
+		struct intel_plane *plane =
+			intel_crtc_get_plane(crtc, plane_id);
 		u64 rate;
 		u16 extra;
 
@@ -5213,9 +5241,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->plane_data_rate[plane_id];
+
+		if (use_min_ddb(crtc_state, plane))
+			rate = 0;
+
 		extra = min_t(u16, alloc_size,
 			      DIV64_U64_ROUND_UP(alloc_size * rate,
 						 total_data_rate));
+
 		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
@@ -5224,13 +5257,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->uv_plane_data_rate[plane_id];
+
+		if (use_min_ddb(crtc_state, plane))
+			rate = 0;
+
 		extra = min_t(u16, alloc_size,
 			      DIV64_U64_ROUND_UP(alloc_size * rate,
 						 total_data_rate));
+
 		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
 	}
+
 	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
 
 	/* Set the actual DDB start/end points for each plane */
@@ -5497,16 +5536,6 @@ static int skl_wm_max_lines(struct drm_i915_private *dev_priv)
 		return 31;
 }
 
-static bool use_minimal_wm0_only(const struct intel_crtc_state *crtc_state,
-				 struct intel_plane *plane)
-{
-	struct drm_i915_private *i915 = to_i915(plane->base.dev);
-
-	return DISPLAY_VER(i915) >= 13 &&
-	       crtc_state->uapi.async_flip &&
-	       plane->async_flip;
-}
-
 static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
 				 struct intel_plane *plane,
 				 int level,
-- 
2.24.1.485.gad05a3d8e5

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Sun, Jan 23, 2022 at 10:34:17PM +0200, Lisovskiy, Stanislav wrote:
> On Fri, Jan 21, 2022 at 02:06:12PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 21, 2022 at 10:06:15AM +0200, Stanislav Lisovskiy wrote:
> > > In terms of async flip optimization we don't to allocate
> > > extra ddb space, so lets skip it.
> > > 
> > > v2: - Extracted min ddb async flip check to separate function
> > >       (Ville Syrjälä)
> > >     - Used this function to prevent false positive WARN
> > >       to be triggered(Ville Syrjälä)
> > > 
> > > v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
> > >       it more universal.
> > >     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
> > >     - Use rate = 0 instead of just setting extra = 0, thus
> > >       letting other planes to use extra ddb and avoiding WARN
> > >       (Ville Syrjälä)
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 5fb022a2a4d7..18fb35c480ef 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
> > >  	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
> > >  }
> > >  
> > > +static bool needs_min_ddb(struct drm_i915_private *i915,
> > > +			  struct intel_crtc_state *crtc_state)
> > 
> > s/needs/use/ to match the wm0 counterpart?
> > 
> > Could use a comment as well perhaps, or maybe just put this right
> > next to the wm0 counterpart so the reader can see both together and
> > make the connection.
> > 
> > Hmm. Actually I think this would also need the plane->async_flip
> > check here too or else we'll drop all the planes to min ddb
> > instead of just the plane doing async flips.
> > 
> > Oh, and I think we need this same thing when calculating the
> > total_data_rate or else the numbers won't match.
> 
> Yes, there seems to be a problem with that approach, we use ratio
> from data plane_data_rate/total_data_rate to determine how we split
> extra ddb blocks, however if plane data rate can be just set as 0
> here localle, total_data_rate is obtained from crtc_state->plane_data_rate,
> which is being calculated first.
> So if we trick icl_get_total_relative_data_rate function to calculate
> total_data_rate corresponding to rate = 0, we will then have 
> crtc_state->plane_data_rate[plane_id] set to 0, which is probably
> not what we want.

These are just the relative data rates so they're not actually used for
anything else. So I guess we could even set them to 0. Though I don't
even recall if the current code really works or not. I think there
might have been some problem with calculating these that I perhaps fixed
with my latest ddb series (or maybe I already fixed it with some earlier
series, can't remember anymore).

> 
> Or should I just edit icl_get_total_relative_data_rate so that it
> still calculates crtc_state->plane_data_rate properly however, the
> doesn't add those to total_data_rate, if use_min_ddb(plane) is set?

This should work too. Can't immediately think why one approach would
be strictly better than the other.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Lisovskiy, Stanislav]

On Fri, Jan 21, 2022 at 02:06:12PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 21, 2022 at 10:06:15AM +0200, Stanislav Lisovskiy wrote:
> > In terms of async flip optimization we don't to allocate
> > extra ddb space, so lets skip it.
> > 
> > v2: - Extracted min ddb async flip check to separate function
> >       (Ville Syrjälä)
> >     - Used this function to prevent false positive WARN
> >       to be triggered(Ville Syrjälä)
> > 
> > v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
> >       it more universal.
> >     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
> >     - Use rate = 0 instead of just setting extra = 0, thus
> >       letting other planes to use extra ddb and avoiding WARN
> >       (Ville Syrjälä)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5fb022a2a4d7..18fb35c480ef 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
> >  	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
> >  }
> >  
> > +static bool needs_min_ddb(struct drm_i915_private *i915,
> > +			  struct intel_crtc_state *crtc_state)
> 
> s/needs/use/ to match the wm0 counterpart?
> 
> Could use a comment as well perhaps, or maybe just put this right
> next to the wm0 counterpart so the reader can see both together and
> make the connection.
> 
> Hmm. Actually I think this would also need the plane->async_flip
> check here too or else we'll drop all the planes to min ddb
> instead of just the plane doing async flips.
> 
> Oh, and I think we need this same thing when calculating the
> total_data_rate or else the numbers won't match.

Yes, there seems to be a problem with that approach, we use ratio
from data plane_data_rate/total_data_rate to determine how we split
extra ddb blocks, however if plane data rate can be just set as 0
here localle, total_data_rate is obtained from crtc_state->plane_data_rate,
which is being calculated first.
So if we trick icl_get_total_relative_data_rate function to calculate
total_data_rate corresponding to rate = 0, we will then have 
crtc_state->plane_data_rate[plane_id] set to 0, which is probably
not what we want.

Or should I just edit icl_get_total_relative_data_rate so that it
still calculates crtc_state->plane_data_rate properly however, the
doesn't add those to total_data_rate, if use_min_ddb(plane) is set?

Stan

> 
> > +{
> > +	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip;
> > +}
> > +
> >  static int
> >  skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  		       struct intel_crtc *crtc)
> > @@ -5225,9 +5231,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->plane_data_rate[plane_id];
> > +
> > +		if (needs_min_ddb(dev_priv, crtc_state))
> > +			rate = 0;
> > +
> >  		extra = min_t(u16, alloc_size,
> >  			      DIV64_U64_ROUND_UP(alloc_size * rate,
> >  						 total_data_rate));
> > +
> >  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> > @@ -5236,13 +5247,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->uv_plane_data_rate[plane_id];
> > +
> > +		if (needs_min_ddb(dev_priv, crtc_state))
> > +			rate = 0;
> > +
> >  		extra = min_t(u16, alloc_size,
> >  			      DIV64_U64_ROUND_UP(alloc_size * rate,
> >  						 total_data_rate));
> > +
> >  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> >  	}
> > +
> >  	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
> >  
> >  	/* Set the actual DDB start/end points for each plane */
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Fri, Jan 21, 2022 at 10:06:15AM +0200, Stanislav Lisovskiy wrote:
> In terms of async flip optimization we don't to allocate
> extra ddb space, so lets skip it.
> 
> v2: - Extracted min ddb async flip check to separate function
>       (Ville Syrjälä)
>     - Used this function to prevent false positive WARN
>       to be triggered(Ville Syrjälä)
> 
> v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
>       it more universal.
>     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
>     - Use rate = 0 instead of just setting extra = 0, thus
>       letting other planes to use extra ddb and avoiding WARN
>       (Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5fb022a2a4d7..18fb35c480ef 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
>  	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
>  }
>  
> +static bool needs_min_ddb(struct drm_i915_private *i915,
> +			  struct intel_crtc_state *crtc_state)

s/needs/use/ to match the wm0 counterpart?

Could use a comment as well perhaps, or maybe just put this right
next to the wm0 counterpart so the reader can see both together and
make the connection.

Hmm. Actually I think this would also need the plane->async_flip
check here too or else we'll drop all the planes to min ddb
instead of just the plane doing async flips.

Oh, and I think we need this same thing when calculating the
total_data_rate or else the numbers won't match.

> +{
> +	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip;
> +}
> +
>  static int
>  skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  		       struct intel_crtc *crtc)
> @@ -5225,9 +5231,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->plane_data_rate[plane_id];
> +
> +		if (needs_min_ddb(dev_priv, crtc_state))
> +			rate = 0;
> +
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> +
>  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
> @@ -5236,13 +5247,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->uv_plane_data_rate[plane_id];
> +
> +		if (needs_min_ddb(dev_priv, crtc_state))
> +			rate = 0;
> +
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> +
>  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
>  	}
> +
>  	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
>  
>  	/* Set the actual DDB start/end points for each plane */
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

v2: - Extracted min ddb async flip check to separate function
      (Ville Syrjälä)
    - Used this function to prevent false positive WARN
      to be triggered(Ville Syrjälä)

v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
      it more universal.
    - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
    - Use rate = 0 instead of just setting extra = 0, thus
      letting other planes to use extra ddb and avoiding WARN
      (Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5fb022a2a4d7..18fb35c480ef 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
 	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
 }
 
+static bool needs_min_ddb(struct drm_i915_private *i915,
+			  struct intel_crtc_state *crtc_state)
+{
+	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip;
+}
+
 static int
 skl_allocate_plane_ddb(struct intel_atomic_state *state,
 		       struct intel_crtc *crtc)
@@ -5225,9 +5231,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->plane_data_rate[plane_id];
+
+		if (needs_min_ddb(dev_priv, crtc_state))
+			rate = 0;
+
 		extra = min_t(u16, alloc_size,
 			      DIV64_U64_ROUND_UP(alloc_size * rate,
 						 total_data_rate));
+
 		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
@@ -5236,13 +5247,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->uv_plane_data_rate[plane_id];
+
+		if (needs_min_ddb(dev_priv, crtc_state))
+			rate = 0;
+
 		extra = min_t(u16, alloc_size,
 			      DIV64_U64_ROUND_UP(alloc_size * rate,
 						 total_data_rate));
+
 		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
 	}
+
 	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
 
 	/* Set the actual DDB start/end points for each plane */
-- 
2.24.1.485.gad05a3d8e5

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Ville Syrjälä]

On Tue, Jan 18, 2022 at 12:48:39PM +0200, Stanislav Lisovskiy wrote:
> In terms of async flip optimization we don't to allocate
> extra ddb space, so lets skip it.
> 
> v2: - Extracted min ddb async flip check to separate function
>       (Ville Syrjälä)
>     - Used this function to prevent false positive WARN
>       to be triggered(Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5d350ddc447f..4922c9108f08 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
>  	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
>  }
>  
> +static bool dg2_need_min_ddb(struct drm_i915_private *i915,
> +			     struct intel_crtc_state *crtc_state)
> +{
> +	return IS_DG2(i915) && crtc_state->uapi.async_flip;

Why are we using IS_DG2 here vs. DISPLAY>=13 for the wm0 only decision?

> +}
> +
>  static int
>  skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  		       struct intel_crtc *crtc)
> @@ -5226,9 +5232,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->plane_data_rate[plane_id];
> -		extra = min_t(u16, alloc_size,
> -			      DIV64_U64_ROUND_UP(alloc_size * rate,
> -						 total_data_rate));
> +
> +		if (dg2_need_min_ddb(dev_priv, crtc_state)) {
> +			extra = 0;
> +		} else {
> +			extra = min_t(u16, alloc_size,
> +				      DIV64_U64_ROUND_UP(alloc_size * rate,
> +							 total_data_rate));
> +		}

Hmm. I wonder if we should just set rate=0 instead. That would
let the other planes pick up the now unused extra ddb space. Would 
also avoid having to skip the WARN since we'd still allocate the 
full ddb.

> +
>  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
> @@ -5237,14 +5249,22 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
>  			break;
>  
>  		rate = crtc_state->uv_plane_data_rate[plane_id];
> -		extra = min_t(u16, alloc_size,
> -			      DIV64_U64_ROUND_UP(alloc_size * rate,
> -						 total_data_rate));
> +
> +		if (dg2_need_min_ddb(dev_priv, crtc_state)) {
> +			extra = 0;
> +		} else {
> +			extra = min_t(u16, alloc_size,
> +				      DIV64_U64_ROUND_UP(alloc_size * rate,
> +							 total_data_rate));
> +		}
> +
>  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
>  	}
> -	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
> +
> +	if (!dg2_need_min_ddb(dev_priv, crtc_state))
> +		drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
>  
>  	/* Set the actual DDB start/end points for each plane */
>  	start = alloc->start;
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

v2: - Extracted min ddb async flip check to separate function
      (Ville Syrjälä)
    - Used this function to prevent false positive WARN
      to be triggered(Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5d350ddc447f..4922c9108f08 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
 	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
 }
 
+static bool dg2_need_min_ddb(struct drm_i915_private *i915,
+			     struct intel_crtc_state *crtc_state)
+{
+	return IS_DG2(i915) && crtc_state->uapi.async_flip;
+}
+
 static int
 skl_allocate_plane_ddb(struct intel_atomic_state *state,
 		       struct intel_crtc *crtc)
@@ -5226,9 +5232,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->plane_data_rate[plane_id];
-		extra = min_t(u16, alloc_size,
-			      DIV64_U64_ROUND_UP(alloc_size * rate,
-						 total_data_rate));
+
+		if (dg2_need_min_ddb(dev_priv, crtc_state)) {
+			extra = 0;
+		} else {
+			extra = min_t(u16, alloc_size,
+				      DIV64_U64_ROUND_UP(alloc_size * rate,
+							 total_data_rate));
+		}
+
 		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
@@ -5237,14 +5249,22 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->uv_plane_data_rate[plane_id];
-		extra = min_t(u16, alloc_size,
-			      DIV64_U64_ROUND_UP(alloc_size * rate,
-						 total_data_rate));
+
+		if (dg2_need_min_ddb(dev_priv, crtc_state)) {
+			extra = 0;
+		} else {
+			extra = min_t(u16, alloc_size,
+				      DIV64_U64_ROUND_UP(alloc_size * rate,
+							 total_data_rate));
+		}
+
 		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
 	}
-	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
+
+	if (!dg2_need_min_ddb(dev_priv, crtc_state))
+		drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
 
 	/* Set the actual DDB start/end points for each plane */
 	start = alloc->start;
-- 
2.24.1.485.gad05a3d8e5

[Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

[Stanislav Lisovskiy]

In terms of async flip optimization we don't to allocate
extra ddb space, so lets skip it.

v2: - Extracted min ddb async flip check to separate function
      (Ville Syrjälä)
    - Used this function to prevent false positive WARN
      to be triggered(Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 34 ++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a529a116164e..619131de5d93 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5105,6 +5105,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
 	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
 }
 
+static bool dg2_need_min_ddb(struct drm_i915_private *i915,
+			     struct intel_crtc_state *crtc_state)
+{
+	return IS_DG2(i915) && crtc_state->uapi.async_flip;
+}
+
 static int
 skl_allocate_plane_ddb(struct intel_atomic_state *state,
 		       struct intel_crtc *crtc)
@@ -5213,9 +5219,15 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->plane_data_rate[plane_id];
-		extra = min_t(u16, alloc_size,
-			      DIV64_U64_ROUND_UP(alloc_size * rate,
-						 total_data_rate));
+
+		if (dg2_need_min_ddb(dev_priv, crtc_state)) {
+			extra = 0;
+		} else {
+			extra = min_t(u16, alloc_size,
+				      DIV64_U64_ROUND_UP(alloc_size * rate,
+							 total_data_rate));
+		}
+
 		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
@@ -5224,14 +5236,22 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
 			break;
 
 		rate = crtc_state->uv_plane_data_rate[plane_id];
-		extra = min_t(u16, alloc_size,
-			      DIV64_U64_ROUND_UP(alloc_size * rate,
-						 total_data_rate));
+
+		if (dg2_need_min_ddb(dev_priv, crtc_state)) {
+			extra = 0;
+		} else {
+			extra = min_t(u16, alloc_size,
+				      DIV64_U64_ROUND_UP(alloc_size * rate,
+							 total_data_rate));
+		}
+
 		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
 		alloc_size -= extra;
 		total_data_rate -= rate;
 	}
-	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
+
+	if (!dg2_need_min_ddb(dev_priv, crtc_state))
+		drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
 
 	/* Set the actual DDB start/end points for each plane */
 	start = alloc->start;
-- 
2.24.1.485.gad05a3d8e5