| Message ID | 1476131459-23763-1-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Was there a new workaround added recently? Or was this something I just missed when writing the original code for this On Mon, 2016-10-10 at 17:30 -0300, Paulo Zanoni wrote: > Mahesh Kumar is already working on a proper implementation for the > workaround, but while we still don't have it, let's just > unconditionally apply the workaround for everybody and we hope we can > close all those numerous bugzilla tickets. Also, I'm not sure how > easy > it will be to backport the final implementation to the stable > Kernels, > and this patch here is probably easier to backport. > > At the present moment I still don't have confirmation that this patch > fixes any of the bugs listed below, but we should definitely try > testing all of them again. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94337 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94884 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95010 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97830 > Cc: stable@vger.kernel.org > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: Lyude <cpaul@redhat.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 49 > ++++++++++++++++++++++++++++++++++------- > 1 file changed, 41 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 62d730d..159831d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2879,6 +2879,21 @@ skl_wm_plane_id(const struct intel_plane > *plane) > } > } > > +/* > + * FIXME: We still don't have the proper code detect if we need to > apply the WA, > + * so assume we'll always need it in order to avoid underruns. > + */ > +static bool intel_needs_memory_bw_wa(struct intel_atomic_state > *state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state- > >base.dev); > + > + if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv) || > + IS_KABYLAKE(dev_priv)) > + return true; > + > + return false; > +} > + > static bool > intel_has_sagv(struct drm_i915_private *dev_priv) > { > @@ -2999,9 +3014,10 @@ bool intel_can_enable_sagv(struct > drm_atomic_state *state) > struct drm_device *dev = state->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_atomic_state *intel_state = > to_intel_atomic_state(state); > - struct drm_crtc *crtc; > + struct intel_crtc *crtc; > + struct intel_plane *plane; > enum pipe pipe; > - int level, plane; > + int level, id, latency; > > if (!intel_has_sagv(dev_priv)) > return false; > @@ -3019,27 +3035,36 @@ bool intel_can_enable_sagv(struct > drm_atomic_state *state) > > /* Since we're now guaranteed to only have one active > CRTC... */ > pipe = ffs(intel_state->active_crtcs) - 1; > - crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > - if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) > + if (crtc->base.state->mode.flags & DRM_MODE_FLAG_INTERLACE) > return false; > > - for_each_plane(dev_priv, pipe, plane) { > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > + id = skl_wm_plane_id(plane); > + > /* Skip this plane if it's not enabled */ > - if (intel_state->wm_results.plane[pipe][plane][0] == > 0) > + if (intel_state->wm_results.plane[pipe][id][0] == 0) > continue; > > /* Find the highest enabled wm level for this plane > */ > for (level = ilk_wm_max_level(dev); > - intel_state- > >wm_results.plane[pipe][plane][level] == 0; --level) > + intel_state->wm_results.plane[pipe][id][level] > == 0; --level) > { } > > + latency = dev_priv->wm.skl_latency[level]; > + > + if (intel_needs_memory_bw_wa(intel_state) && > + plane->base.state->fb->modifier[0] == > + 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 then 30µs we > can't enable > * the SAGV > */ > - if (dev_priv->wm.skl_latency[level] < > SKL_SAGV_BLOCK_TIME) > + if (latency < SKL_SAGV_BLOCK_TIME) > return false; > } > > @@ -3549,12 +3574,18 @@ static int skl_compute_plane_wm(const struct > drm_i915_private *dev_priv, > uint32_t width = 0, height = 0; > uint32_t plane_pixel_rate; > uint32_t y_tile_minimum, y_min_scanlines; > + struct intel_atomic_state *state = > + to_intel_atomic_state(cstate->base.state); > + bool apply_memory_bw_wa = intel_needs_memory_bw_wa(state); > > if (latency == 0 || !cstate->base.active || !intel_pstate- > >base.visible) { > *enabled = false; > return 0; > } > > + if (apply_memory_bw_wa && fb->modifier[0] == > I915_FORMAT_MOD_X_TILED) > + latency += 15; > + > width = drm_rect_width(&intel_pstate->base.src) >> 16; > height = drm_rect_height(&intel_pstate->base.src) >> 16; > > @@ -3607,6 +3638,8 @@ static int skl_compute_plane_wm(const struct > drm_i915_private *dev_priv, > plane_blocks_per_line); > > y_tile_minimum = plane_blocks_per_line * y_min_scanlines; > + if (apply_memory_bw_wa) > + y_tile_minimum *= 2; > > if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
Em Seg, 2016-10-10 às 16:34 -0400, Lyude Paul escreveu: > Was there a new workaround added recently? Or was this something I > just > missed when writing the original code for this It's listed on the public spec: https://01.org/sites/default/files/docum entation/intel-gfx-prm-osrc-skl-vol12-display.pdf Pages 210 - 211. > > On Mon, 2016-10-10 at 17:30 -0300, Paulo Zanoni wrote: > > > > Mahesh Kumar is already working on a proper implementation for the > > workaround, but while we still don't have it, let's just > > unconditionally apply the workaround for everybody and we hope we > > can > > close all those numerous bugzilla tickets. Also, I'm not sure how > > easy > > it will be to backport the final implementation to the stable > > Kernels, > > and this patch here is probably easier to backport. > > > > At the present moment I still don't have confirmation that this > > patch > > fixes any of the bugs listed below, but we should definitely try > > testing all of them again. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94337 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94884 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95010 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97830 > > Cc: stable@vger.kernel.org > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > > Cc: Lyude <cpaul@redhat.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 49 > > ++++++++++++++++++++++++++++++++++------- > > 1 file changed, 41 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 62d730d..159831d 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2879,6 +2879,21 @@ skl_wm_plane_id(const struct intel_plane > > *plane) > > } > > } > > > > +/* > > + * FIXME: We still don't have the proper code detect if we need to > > apply the WA, > > + * so assume we'll always need it in order to avoid underruns. > > + */ > > +static bool intel_needs_memory_bw_wa(struct intel_atomic_state > > *state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state- > > > > > > base.dev); > > + > > + if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv) || > > + IS_KABYLAKE(dev_priv)) > > + return true; > > + > > + return false; > > +} > > + > > static bool > > intel_has_sagv(struct drm_i915_private *dev_priv) > > { > > @@ -2999,9 +3014,10 @@ bool intel_can_enable_sagv(struct > > drm_atomic_state *state) > > struct drm_device *dev = state->dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > struct intel_atomic_state *intel_state = > > to_intel_atomic_state(state); > > - struct drm_crtc *crtc; > > + struct intel_crtc *crtc; > > + struct intel_plane *plane; > > enum pipe pipe; > > - int level, plane; > > + int level, id, latency; > > > > if (!intel_has_sagv(dev_priv)) > > return false; > > @@ -3019,27 +3035,36 @@ bool intel_can_enable_sagv(struct > > drm_atomic_state *state) > > > > /* Since we're now guaranteed to only have one active > > CRTC... */ > > pipe = ffs(intel_state->active_crtcs) - 1; > > - crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > > + crtc = to_intel_crtc(dev_priv- > > >pipe_to_crtc_mapping[pipe]); > > > > - if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) > > + if (crtc->base.state->mode.flags & > > DRM_MODE_FLAG_INTERLACE) > > return false; > > > > - for_each_plane(dev_priv, pipe, plane) { > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > + id = skl_wm_plane_id(plane); > > + > > /* Skip this plane if it's not enabled */ > > - if (intel_state->wm_results.plane[pipe][plane][0] > > == > > 0) > > + if (intel_state->wm_results.plane[pipe][id][0] == > > 0) > > continue; > > > > /* Find the highest enabled wm level for this > > plane > > */ > > for (level = ilk_wm_max_level(dev); > > - intel_state- > > > > > > wm_results.plane[pipe][plane][level] == 0; --level) > > + intel_state- > > >wm_results.plane[pipe][id][level] > > == 0; --level) > > { } > > > > + latency = dev_priv->wm.skl_latency[level]; > > + > > + if (intel_needs_memory_bw_wa(intel_state) && > > + plane->base.state->fb->modifier[0] == > > + 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 then 30µs we > > can't enable > > * the SAGV > > */ > > - if (dev_priv->wm.skl_latency[level] < > > SKL_SAGV_BLOCK_TIME) > > + if (latency < SKL_SAGV_BLOCK_TIME) > > return false; > > } > > > > @@ -3549,12 +3574,18 @@ static int skl_compute_plane_wm(const > > struct > > drm_i915_private *dev_priv, > > uint32_t width = 0, height = 0; > > uint32_t plane_pixel_rate; > > uint32_t y_tile_minimum, y_min_scanlines; > > + struct intel_atomic_state *state = > > + to_intel_atomic_state(cstate->base.state); > > + bool apply_memory_bw_wa = intel_needs_memory_bw_wa(state); > > > > if (latency == 0 || !cstate->base.active || !intel_pstate- > > > > > > base.visible) { > > *enabled = false; > > return 0; > > } > > > > + if (apply_memory_bw_wa && fb->modifier[0] == > > I915_FORMAT_MOD_X_TILED) > > + latency += 15; > > + > > width = drm_rect_width(&intel_pstate->base.src) >> 16; > > height = drm_rect_height(&intel_pstate->base.src) >> 16; > > > > @@ -3607,6 +3638,8 @@ static int skl_compute_plane_wm(const struct > > drm_i915_private *dev_priv, > > plane_blocks_per_line); > > > > y_tile_minimum = plane_blocks_per_line * y_min_scanlines; > > + if (apply_memory_bw_wa) > > + y_tile_minimum *= 2; > > > > if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
I feel like the PRMs should be a little less confusing so it doesn't take four of us reading it to actually get the full spec into code… Anyway, while this is going to be running on more then just skylake I feel like intel_needs_memory_bw_wa() is a little confusing since I would expect eventually we're going to have to apply other unrelated memory bandwidth workarounds on other platforms. How about skl_needs_memory_bw_wa() or gen9_needs_memory_bw_wa()? On Mon, 2016-10-10 at 20:46 +0000, Zanoni, Paulo R wrote: > Em Seg, 2016-10-10 às 16:34 -0400, Lyude Paul escreveu: > > > > Was there a new workaround added recently? Or was this something I > > just > > missed when writing the original code for this > > It's listed on the public spec: > https://01.org/sites/default/files/docum > entation/intel-gfx-prm-osrc-skl-vol12-display.pdf > > Pages 210 - 211. > > > > > > > On Mon, 2016-10-10 at 17:30 -0300, Paulo Zanoni wrote: > > > > > > > > > Mahesh Kumar is already working on a proper implementation for > > > the > > > workaround, but while we still don't have it, let's just > > > unconditionally apply the workaround for everybody and we hope we > > > can > > > close all those numerous bugzilla tickets. Also, I'm not sure how > > > easy > > > it will be to backport the final implementation to the stable > > > Kernels, > > > and this patch here is probably easier to backport. > > > > > > At the present moment I still don't have confirmation that this > > > patch > > > fixes any of the bugs listed below, but we should definitely try > > > testing all of them again. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94337 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94884 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95010 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97830 > > > Cc: stable@vger.kernel.org > > > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > > > Cc: Lyude <cpaul@redhat.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 49 > > > ++++++++++++++++++++++++++++++++++------- > > > 1 file changed, 41 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index 62d730d..159831d 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -2879,6 +2879,21 @@ skl_wm_plane_id(const struct intel_plane > > > *plane) > > > } > > > } > > > > > > +/* > > > + * FIXME: We still don't have the proper code detect if we need > > > to > > > apply the WA, > > > + * so assume we'll always need it in order to avoid underruns. > > > + */ > > > +static bool intel_needs_memory_bw_wa(struct intel_atomic_state > > > *state) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state- > > > > > > > > > > > > base.dev); > > > + > > > + if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv) || > > > + IS_KABYLAKE(dev_priv)) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > static bool > > > intel_has_sagv(struct drm_i915_private *dev_priv) > > > { > > > @@ -2999,9 +3014,10 @@ bool intel_can_enable_sagv(struct > > > drm_atomic_state *state) > > > struct drm_device *dev = state->dev; > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > struct intel_atomic_state *intel_state = > > > to_intel_atomic_state(state); > > > - struct drm_crtc *crtc; > > > + struct intel_crtc *crtc; > > > + struct intel_plane *plane; > > > enum pipe pipe; > > > - int level, plane; > > > + int level, id, latency; > > > > > > if (!intel_has_sagv(dev_priv)) > > > return false; > > > @@ -3019,27 +3035,36 @@ bool intel_can_enable_sagv(struct > > > drm_atomic_state *state) > > > > > > /* Since we're now guaranteed to only have one active > > > CRTC... */ > > > pipe = ffs(intel_state->active_crtcs) - 1; > > > - crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > > > + crtc = to_intel_crtc(dev_priv- > > > > > > > > pipe_to_crtc_mapping[pipe]); > > > > > > - if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) > > > + if (crtc->base.state->mode.flags & > > > DRM_MODE_FLAG_INTERLACE) > > > return false; > > > > > > - for_each_plane(dev_priv, pipe, plane) { > > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > + id = skl_wm_plane_id(plane); > > > + > > > /* Skip this plane if it's not enabled */ > > > - if (intel_state- > > > >wm_results.plane[pipe][plane][0] > > > == > > > 0) > > > + if (intel_state->wm_results.plane[pipe][id][0] > > > == > > > 0) > > > continue; > > > > > > /* Find the highest enabled wm level for this > > > plane > > > */ > > > for (level = ilk_wm_max_level(dev); > > > - intel_state- > > > > > > > > > > > > wm_results.plane[pipe][plane][level] == 0; --level) > > > + intel_state- > > > > > > > > wm_results.plane[pipe][id][level] > > > == 0; --level) > > > { } > > > > > > + latency = dev_priv->wm.skl_latency[level]; > > > + > > > + if (intel_needs_memory_bw_wa(intel_state) && > > > + plane->base.state->fb->modifier[0] == > > > + 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 then 30µs > > > we > > > can't enable > > > * the SAGV > > > */ > > > - if (dev_priv->wm.skl_latency[level] < > > > SKL_SAGV_BLOCK_TIME) > > > + if (latency < SKL_SAGV_BLOCK_TIME) > > > return false; > > > } > > > > > > @@ -3549,12 +3574,18 @@ static int skl_compute_plane_wm(const > > > struct > > > drm_i915_private *dev_priv, > > > uint32_t width = 0, height = 0; > > > uint32_t plane_pixel_rate; > > > uint32_t y_tile_minimum, y_min_scanlines; > > > + struct intel_atomic_state *state = > > > + to_intel_atomic_state(cstate->base.state); > > > + bool apply_memory_bw_wa = > > > intel_needs_memory_bw_wa(state); > > > > > > if (latency == 0 || !cstate->base.active || > > > !intel_pstate- > > > > > > > > > > > > base.visible) { > > > *enabled = false; > > > return 0; > > > } > > > > > > + if (apply_memory_bw_wa && fb->modifier[0] == > > > I915_FORMAT_MOD_X_TILED) > > > + latency += 15; > > > + > > > width = drm_rect_width(&intel_pstate->base.src) >> 16; > > > height = drm_rect_height(&intel_pstate->base.src) >> 16; > > > > > > @@ -3607,6 +3638,8 @@ static int skl_compute_plane_wm(const > > > struct > > > drm_i915_private *dev_priv, > > > plane_blocks_per_line); > > > > > > y_tile_minimum = plane_blocks_per_line * > > > y_min_scanlines; > > > + if (apply_memory_bw_wa) > > > + y_tile_minimum *= 2; > > > > > > if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > > > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
On Mon, 10 Oct 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > Mahesh Kumar is already working on a proper implementation for the > workaround, but while we still don't have it, let's just > unconditionally apply the workaround for everybody and we hope we can > close all those numerous bugzilla tickets. Also, I'm not sure how easy > it will be to backport the final implementation to the stable Kernels, > and this patch here is probably easier to backport. > > At the present moment I still don't have confirmation that this patch > fixes any of the bugs listed below, but we should definitely try > testing all of them again. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94337 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94884 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95010 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97830 > Cc: stable@vger.kernel.org The patch is a bit on the large side for stable. 100 lines with context is the rule. BR, Jani. > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: Lyude <cpaul@redhat.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 41 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 62d730d..159831d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2879,6 +2879,21 @@ skl_wm_plane_id(const struct intel_plane *plane) > } > } > > +/* > + * FIXME: We still don't have the proper code detect if we need to apply the WA, > + * so assume we'll always need it in order to avoid underruns. > + */ > +static bool intel_needs_memory_bw_wa(struct intel_atomic_state *state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + > + if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv) || > + IS_KABYLAKE(dev_priv)) > + return true; > + > + return false; > +} > + > static bool > intel_has_sagv(struct drm_i915_private *dev_priv) > { > @@ -2999,9 +3014,10 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) > struct drm_device *dev = state->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > - struct drm_crtc *crtc; > + struct intel_crtc *crtc; > + struct intel_plane *plane; > enum pipe pipe; > - int level, plane; > + int level, id, latency; > > if (!intel_has_sagv(dev_priv)) > return false; > @@ -3019,27 +3035,36 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) > > /* Since we're now guaranteed to only have one active CRTC... */ > pipe = ffs(intel_state->active_crtcs) - 1; > - crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > - if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) > + if (crtc->base.state->mode.flags & DRM_MODE_FLAG_INTERLACE) > return false; > > - for_each_plane(dev_priv, pipe, plane) { > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > + id = skl_wm_plane_id(plane); > + > /* Skip this plane if it's not enabled */ > - if (intel_state->wm_results.plane[pipe][plane][0] == 0) > + if (intel_state->wm_results.plane[pipe][id][0] == 0) > continue; > > /* Find the highest enabled wm level for this plane */ > for (level = ilk_wm_max_level(dev); > - intel_state->wm_results.plane[pipe][plane][level] == 0; --level) > + intel_state->wm_results.plane[pipe][id][level] == 0; --level) > { } > > + latency = dev_priv->wm.skl_latency[level]; > + > + if (intel_needs_memory_bw_wa(intel_state) && > + plane->base.state->fb->modifier[0] == > + 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 then 30µs we can't enable > * the SAGV > */ > - if (dev_priv->wm.skl_latency[level] < SKL_SAGV_BLOCK_TIME) > + if (latency < SKL_SAGV_BLOCK_TIME) > return false; > } > > @@ -3549,12 +3574,18 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > uint32_t width = 0, height = 0; > uint32_t plane_pixel_rate; > uint32_t y_tile_minimum, y_min_scanlines; > + struct intel_atomic_state *state = > + to_intel_atomic_state(cstate->base.state); > + bool apply_memory_bw_wa = intel_needs_memory_bw_wa(state); > > if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) { > *enabled = false; > return 0; > } > > + if (apply_memory_bw_wa && fb->modifier[0] == I915_FORMAT_MOD_X_TILED) > + latency += 15; > + > width = drm_rect_width(&intel_pstate->base.src) >> 16; > height = drm_rect_height(&intel_pstate->base.src) >> 16; > > @@ -3607,6 +3638,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > plane_blocks_per_line); > > y_tile_minimum = plane_blocks_per_line * y_min_scanlines; > + if (apply_memory_bw_wa) > + y_tile_minimum *= 2; > > if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
On Tue, Oct 11, 2016 at 10:34:14AM +0300, Jani Nikula wrote: > On Mon, 10 Oct 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > > Mahesh Kumar is already working on a proper implementation for the > > workaround, but while we still don't have it, let's just > > unconditionally apply the workaround for everybody and we hope we can > > close all those numerous bugzilla tickets. Also, I'm not sure how easy > > it will be to backport the final implementation to the stable Kernels, > > and this patch here is probably easier to backport. > > > > At the present moment I still don't have confirmation that this patch > > fixes any of the bugs listed below, but we should definitely try > > testing all of them again. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94337 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94884 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95010 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97830 > > Cc: stable@vger.kernel.org > > The patch is a bit on the large side for stable. 100 lines with context > is the rule. Huh? It's only 49 line of changes: > > drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++++------- > > 1 file changed, 41 insertions(+), 8 deletions(-) That's fine for stable, we take i915 stable patches much bigger than that all the time :) thanks, greg k-h
On Tue, 11 Oct 2016, Greg KH <greg@kroah.com> wrote: > On Tue, Oct 11, 2016 at 10:34:14AM +0300, Jani Nikula wrote: >> On Mon, 10 Oct 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: >> The patch is a bit on the large side for stable. 100 lines with context >> is the rule. > > Huh? It's only 49 line of changes: > >> > drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++++------- >> > 1 file changed, 41 insertions(+), 8 deletions(-) > > That's fine for stable, we take i915 stable patches much bigger than > that all the time :) Oh, I thought the rule was "100 lines, with context", but I certainly won't argue! Never mind! ;) BR, Jani.
On Tue, Oct 11, 2016 at 01:54:09PM +0300, Jani Nikula wrote: > On Tue, 11 Oct 2016, Greg KH <greg@kroah.com> wrote: > > On Tue, Oct 11, 2016 at 10:34:14AM +0300, Jani Nikula wrote: > >> On Mon, 10 Oct 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > >> The patch is a bit on the large side for stable. 100 lines with context > >> is the rule. > > > > Huh? It's only 49 line of changes: > > > >> > drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++++------- > >> > 1 file changed, 41 insertions(+), 8 deletions(-) > > > > That's fine for stable, we take i915 stable patches much bigger than > > that all the time :) > > Oh, I thought the rule was "100 lines, with context", but I certainly > won't argue! Never mind! ;) It's the "official" rule, yes, but really, context of the patch itself (i.e. what it does), is the key thing. thanks, greg k-h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 62d730d..159831d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2879,6 +2879,21 @@ skl_wm_plane_id(const struct intel_plane *plane) } } +/* + * FIXME: We still don't have the proper code detect if we need to apply the WA, + * so assume we'll always need it in order to avoid underruns. + */ +static bool intel_needs_memory_bw_wa(struct intel_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + + if (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv) || + IS_KABYLAKE(dev_priv)) + return true; + + return false; +} + static bool intel_has_sagv(struct drm_i915_private *dev_priv) { @@ -2999,9 +3014,10 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) struct drm_device *dev = state->dev; struct drm_i915_private *dev_priv = to_i915(dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); - struct drm_crtc *crtc; + struct intel_crtc *crtc; + struct intel_plane *plane; enum pipe pipe; - int level, plane; + int level, id, latency; if (!intel_has_sagv(dev_priv)) return false; @@ -3019,27 +3035,36 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) /* Since we're now guaranteed to only have one active CRTC... */ pipe = ffs(intel_state->active_crtcs) - 1; - crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); - if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) + if (crtc->base.state->mode.flags & DRM_MODE_FLAG_INTERLACE) return false; - for_each_plane(dev_priv, pipe, plane) { + for_each_intel_plane_on_crtc(dev, crtc, plane) { + id = skl_wm_plane_id(plane); + /* Skip this plane if it's not enabled */ - if (intel_state->wm_results.plane[pipe][plane][0] == 0) + if (intel_state->wm_results.plane[pipe][id][0] == 0) continue; /* Find the highest enabled wm level for this plane */ for (level = ilk_wm_max_level(dev); - intel_state->wm_results.plane[pipe][plane][level] == 0; --level) + intel_state->wm_results.plane[pipe][id][level] == 0; --level) { } + latency = dev_priv->wm.skl_latency[level]; + + if (intel_needs_memory_bw_wa(intel_state) && + plane->base.state->fb->modifier[0] == + 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 then 30µs we can't enable * the SAGV */ - if (dev_priv->wm.skl_latency[level] < SKL_SAGV_BLOCK_TIME) + if (latency < SKL_SAGV_BLOCK_TIME) return false; } @@ -3549,12 +3574,18 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, uint32_t width = 0, height = 0; uint32_t plane_pixel_rate; uint32_t y_tile_minimum, y_min_scanlines; + struct intel_atomic_state *state = + to_intel_atomic_state(cstate->base.state); + bool apply_memory_bw_wa = intel_needs_memory_bw_wa(state); if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) { *enabled = false; return 0; } + if (apply_memory_bw_wa && fb->modifier[0] == I915_FORMAT_MOD_X_TILED) + latency += 15; + width = drm_rect_width(&intel_pstate->base.src) >> 16; height = drm_rect_height(&intel_pstate->base.src) >> 16; @@ -3607,6 +3638,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, plane_blocks_per_line); y_tile_minimum = plane_blocks_per_line * y_min_scanlines; + if (apply_memory_bw_wa) + y_tile_minimum *= 2; if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
Mahesh Kumar is already working on a proper implementation for the workaround, but while we still don't have it, let's just unconditionally apply the workaround for everybody and we hope we can close all those numerous bugzilla tickets. Also, I'm not sure how easy it will be to backport the final implementation to the stable Kernels, and this patch here is probably easier to backport. At the present moment I still don't have confirmation that this patch fixes any of the bugs listed below, but we should definitely try testing all of them again. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94337 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94605 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94884 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95010 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97830 Cc: stable@vger.kernel.org Cc: Mahesh Kumar <mahesh1.kumar@intel.com> Cc: Lyude <cpaul@redhat.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 49 ++++++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 8 deletions(-)