On Fri, 2019-10-25 at 13:24 +0300, Ville Syrjälä wrote:
> On Fri, Oct 25, 2019 at 12:53:51PM +0300, Stanislav Lisovskiy wrote:
> > Currently intel_can_enable_sagv function contains
> > a mix of workarounds for different platforms
> > some of them are not valid for gens >= 11 already,
> > so lets split it into separate functions.
> >
> > v2:
> > - Rework watermark calculation algorithm to
> > attempt to calculate Level 0 watermark
> > with added sagv block time latency and
> > check if it fits in DBuf in order to
> > determine if SAGV can be enabled already
> > at this stage, just as BSpec 49325 states.
> > if that fails rollback to usual Level 0
> > latency and disable SAGV.
> > - Remove unneeded tabs(James Ausmus)
> >
> > v3: Rebased the patch
> >
> > v4: - Added back interlaced check for Gen12 and
> > added separate function for TGL SAGV check
> > (thanks to James Ausmus for spotting)
> > - Removed unneeded gen check
> > - Extracted Gen12 SAGV decision making code
> > to a separate function from skl_compute_wm
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxx>
> > Cc: James Ausmus <james.ausmus@xxxxxxxxx>
> > ---
> > .../drm/i915/display/intel_display_types.h | 8 +
> > drivers/gpu/drm/i915/intel_pm.c | 254
> > +++++++++++++++++-
> > 2 files changed, 254 insertions(+), 8 deletions(-)
> >
> >
> Do we use active or enable elsewhere to decide whether to compute wms
> for a pipe? Should be consistent here so we don't get into some wonky
> state where we didn't compute normal wms but are computing the sagv
> wm.
Good question, I have seen it either this or that everywhere, so we
probably need to discuss which one I should use.
>
> > +
> > + for_each_plane_id_on_crtc(crtc, plane_id) {
> > + struct skl_plane_wm *wm =
> > + &new_crtc_state-
> > >wm.skl.optimal.planes[plane_id];
> > +
> > + /* Skip this plane if it's not enabled */
> > + if (!wm->wm[0].plane_en)
> > + continue;
> > +
> > + /* Find the highest enabled wm level for this
> > plane */
> > + for (level = ilk_wm_max_level(dev_priv);
> > + !wm->wm[level].plane_en; --level) {
> > + }
> > +
> > + latency = dev_priv->wm.skl_latency[level];
> > +
> > + /*
> > + * If any of the planes on this pipe don't
> > enable
> > + * wm levels that incur memory latencies higher
> > than
> > + * sagv_block_time_us we can't enable SAGV.
> > + */
> > + if (latency < dev_priv->sagv_block_time_us)
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> >
> > +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > +{
> > + struct drm_device *dev = state->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > + if (INTEL_GEN(dev_priv) >= 12)
> > + return tgl_can_enable_sagv(state);
> > + else if (INTEL_GEN(dev_priv) == 11)
> > + return icl_can_enable_sagv(state);
> > +
> > + return skl_can_enable_sagv(state);
>
> Why do we have three separate code paths now? I believe there should
> be
> just two.
>
> Also if you go to the trouble of adding dev_priv->..can_sagv just
> make
> it work for all platforms.
..can_sagv is not in dev_priv - it is part of intel_atomic_state,
so at least here I avoided using dev_priv.
>
> >
> >
> > + /*
> > + * Lets assume we can tolerate SAGV for now,
> > + * until watermark calculations prove the opposite
> > + * if any of the pipe planes in the state will
> > + * fail the requirements it will be assigned to false
> > + * in skl_compute_ddb.
> > + */
> > + state->gen12_can_sagv = true;
> > +
> > + for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > + new_crtc_state, i) {
> > + ret = tgl_check_pipe_fits_sagv_wm(new_crtc_state, ddb);
> > + if (ret) {
> > + state->gen12_can_sagv = false;
> > + break;
> + }
>
> This is not going to work. We need the infromation from _all_ pipes,
> not
> just the ones in the state. We probably want to make that can_sagv
> thing
> a bitmask of pipes so that we don't have to have all pipes in the
> state.
But isn't it so that even if at least one plane/pipe can't tolerate
SAGV, we can't enable it already? So what is the point of checking
other planes/pipes then?
Or may be I'm missing something here.
>
> > + }
> > +
> > + if (state->gen12_can_sagv) {
> > + /*
> > + * If we determined that we can actually enable SAGV,
> > then
> > + * actually use those levels
> > tgl_check_pipe_fits_sagv_wm
> > + * has already taken care of checking if L0 + sagv
> > block time
> > + * fits into ddb.
> > + */
> > + for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > + new_crtc_state, i) {
> > + struct intel_plane *plane;
> > + for_each_intel_plane_on_crtc(&dev_priv->drm,
> > crtc, plane) {
> > + enum plane_id plane_id = plane->id;
> > + struct skl_plane_wm *plane_wm = \
> > + &new_crtc_state-
> > >wm.skl.optimal.planes[plane_id];
> > + struct skl_wm_level *sagv_wm0 =
> > &plane_wm->sagv_wm_l0;
> > + struct skl_wm_level *l0_wm0 =
> > &plane_wm->wm[0];
> > +
> > + memcpy(l0_wm0, sagv_wm0, sizeof(struct
> > skl_wm_level));
> > + }
> > + }
> > + }
> > +}
> > +
> > static int
> > skl_compute_wm(struct intel_atomic_state *state)
> > {
> > + struct drm_device *dev = state->base.dev;
> > + const struct drm_i915_private *dev_priv = to_i915(dev);
> > struct intel_crtc *crtc;
> > struct intel_crtc_state *new_crtc_state;
> > struct intel_crtc_state *old_crtc_state;
> > @@ -5553,6 +5785,9 @@ skl_compute_wm(struct intel_atomic_state
> > *state)
> > /* Clear all dirty flags */
> > results->dirty_pipes = 0;
> >
> > + /* If we exit before check is done */
> > + state->gen12_can_sagv = false;
> > +
> > ret = skl_ddb_add_affected_pipes(state);
> > if (ret)
> > return ret;
> > @@ -5579,6 +5814,9 @@ skl_compute_wm(struct intel_atomic_state
> > *state)
> > results->dirty_pipes |= BIT(crtc->pipe);
> > }
> >
> > + if (INTEL_GEN(dev_priv) >= 12)
> > + tgl_check_and_set_sagv(state);
> > +
> > ret = skl_compute_ddb(state);
> > if (ret)
> > return ret;
> > --
> > 2.17.1
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
