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 <[email protected]>
> ---
> .../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
