On Thu, Sep 26, 2019 at 03:34:35PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 25, 2019 at 01:33:52PM -0700, James Ausmus wrote:
> > For Gen12, BSpec no longer tells us to disable SAGV when > 1 pipe is
> > active. Update intel_can_enable_sagv to allow this, and loop through all
> > active planes on all active crtcs to check against the interlaced and
> > latency restrictions.
> >
> > BSpec: 49325
> >
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Signed-off-by: James Ausmus <james.ausmus@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 63 +++++++++++++++++----------------
> > 1 file changed, 32 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ca2bec09edb5..cb50c697a6b8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3775,7 +3775,6 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > struct intel_crtc *crtc;
> > struct intel_plane *plane;
> > struct intel_crtc_state *crtc_state;
> > - enum pipe pipe;
> > int level, latency;
> > int sagv_block_time_us;
> >
> > @@ -3791,47 +3790,49 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > return true;
> >
> > /*
> > - * SKL+ workaround: bspec recommends we disable SAGV when we have
> > + * SKL-ICL workaround: bspec recommends we disable SAGV when we have
> > * more then one pipe enabled
> > */
> > - if (hweight8(state->active_pipes) > 1)
> > + if (INTEL_GEN(dev_priv) < 12 && hweight8(state->active_pipes) > 1)
> > return false;
> >
> > - /* Since we're now guaranteed to only have one active CRTC... */
> > - pipe = ffs(state->active_pipes) - 1;
> > - crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > - crtc_state = to_intel_crtc_state(crtc->base.state);
> > + for_each_intel_crtc(&dev_priv->drm, crtc) {
> > + crtc_state = to_intel_crtc_state(crtc->base.state);
> > + if (!crtc_state->base.active)
> > + continue;
> >
> > - if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > - return false;
> > + if (crtc->base.state->adjusted_mode.flags &
> > + DRM_MODE_FLAG_INTERLACE)
> > + return false;
> >
> > - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > - struct skl_plane_wm *wm =
> > - &crtc_state->wm.skl.optimal.planes[plane->id];
> > + for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > + struct skl_plane_wm *wm =
> > + &crtc_state->wm.skl.optimal.planes[plane->id];
>
> This whole loop is bothering me. I'd much rather we move to a scheme
> where each plane computes it's SAGV friendlyness when computing the
> watermarks. We'll anyway need that since we need to caclulate the
> watermarks differently for the SAGV on vs. off cases.
Hmm, I'll have to look in to this. In the meantime, I'd like to get
patches 1 & 2 of this series moving forward, as those should be what's
really necessary to be able to turn on SAGV for TGL once we're ready, so
I'll send those as a separate series, and leave relaxing the 1 pipe
restriction as it's own work.
-James
>
> >
> > - /* Skip this plane if it's not enabled */
> > - if (!wm->wm[0].plane_en)
> > - continue;
> > + /* 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)
> > - { }
> > + /* 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];
> > + latency = dev_priv->wm.skl_latency[level];
> >
> > - if (skl_needs_memory_bw_wa(dev_priv) &&
> > - plane->base.state->fb->modifier ==
> > - I915_FORMAT_MOD_X_TILED)
> > - latency += 15;
> > + if (skl_needs_memory_bw_wa(dev_priv) &&
> > + plane->base.state->fb->modifier ==
> > + I915_FORMAT_MOD_X_TILED)
> > + latency += 15;
> >
> > - /*
> > - * 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 < sagv_block_time_us)
> > - return false;
> > + /*
> > + * 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 < sagv_block_time_us)
> > + return false;
> > + }
> > }
> >
> > return true;
> > --
> > 2.22.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
