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@xxxxxxxxx>
> ---
> .../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
