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