| Message ID | 20190925203352.9827-4-james.ausmus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Extract SAGV block time function | expand |
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@linux.intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: James Ausmus <james.ausmus@intel.com> > --- > 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. > > - /* 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
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@linux.intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > --- > > 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
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]; - /* 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;
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@linux.intel.com> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: James Ausmus <james.ausmus@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 63 +++++++++++++++++---------------- 1 file changed, 32 insertions(+), 31 deletions(-)