| Message ID | 1469048403-32016-3-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 20, 2016 at 04:59:58PM -0400, Lyude wrote: > From: Matt Roper <matthew.d.roper@intel.com> > > When we write watermark values to the hardware, those values are stored > in dev_priv->wm.skl_hw. However with recent watermark changes, the > results structure we're copying from only contains valid watermark and > DDB values for the pipes that are actually changing; the values for > other pipes remain 0. Thus a blind copy of the entire skl_wm_values > structure will clobber the values for unchanged pipes...we need to be > more selective and only copy over the values for the changing pipes. > > This mistake was hidden until recently due to another bug that caused us > to erroneously re-calculate watermarks for all active pipes rather than > changing pipes. Only when that bug was fixed was the impact of this bug > discovered (e.g., modesets failing with "Requested display configuration > exceeds system watermark limitations" messages and leaving watermarks > non-functional, even ones initiated by intel_fbdev_restore_mode). > > Changes since v1: > - Add a function for copying a pipe's wm values > (skl_copy_wm_for_pipe()) so we can reuse this later Your changes look good to me. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)") > Fixes: 9b6130227495 ("drm/i915/gen9: Re-allocate DDB only for changed pipes") > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Lyude <cpaul@redhat.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index fa86bea..b7d4af1 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3966,6 +3966,24 @@ skl_compute_ddb(struct drm_atomic_state *state) > return 0; > } > > +static void > +skl_copy_wm_for_pipe(struct skl_wm_values *dst, > + struct skl_wm_values *src, > + enum pipe pipe) > +{ > + dst->wm_linetime[pipe] = src->wm_linetime[pipe]; > + memcpy(dst->plane[pipe], src->plane[pipe], > + sizeof(dst->plane[pipe])); > + memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], > + sizeof(dst->plane_trans[pipe])); > + > + dst->ddb.pipe[pipe] = src->ddb.pipe[pipe]; > + memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe], > + sizeof(dst->ddb.y_plane[pipe])); > + memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe], > + sizeof(dst->ddb.plane[pipe])); > +} > + > static int > skl_compute_wm(struct drm_atomic_state *state) > { > @@ -4038,8 +4056,10 @@ static void skl_update_wm(struct drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct skl_wm_values *results = &dev_priv->wm.skl_results; > + struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw; > struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); > struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; > + int pipe; > > if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) > return; > @@ -4051,8 +4071,12 @@ static void skl_update_wm(struct drm_crtc *crtc) > skl_write_wm_values(dev_priv, results); > skl_flush_wm_values(dev_priv, results); > > - /* store the new configuration */ > - dev_priv->wm.skl_hw = *results; > + /* > + * Store the new configuration (but only for the pipes that have > + * changed; the other values weren't recomputed). > + */ > + for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes) > + skl_copy_wm_for_pipe(hw_vals, results, pipe); > > mutex_unlock(&dev_priv->wm.wm_mutex); > } > -- > 2.7.4 >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fa86bea..b7d4af1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3966,6 +3966,24 @@ skl_compute_ddb(struct drm_atomic_state *state) return 0; } +static void +skl_copy_wm_for_pipe(struct skl_wm_values *dst, + struct skl_wm_values *src, + enum pipe pipe) +{ + dst->wm_linetime[pipe] = src->wm_linetime[pipe]; + memcpy(dst->plane[pipe], src->plane[pipe], + sizeof(dst->plane[pipe])); + memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], + sizeof(dst->plane_trans[pipe])); + + dst->ddb.pipe[pipe] = src->ddb.pipe[pipe]; + memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe], + sizeof(dst->ddb.y_plane[pipe])); + memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe], + sizeof(dst->ddb.plane[pipe])); +} + static int skl_compute_wm(struct drm_atomic_state *state) { @@ -4038,8 +4056,10 @@ static void skl_update_wm(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = to_i915(dev); struct skl_wm_values *results = &dev_priv->wm.skl_results; + struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw; struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; + int pipe; if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) return; @@ -4051,8 +4071,12 @@ static void skl_update_wm(struct drm_crtc *crtc) skl_write_wm_values(dev_priv, results); skl_flush_wm_values(dev_priv, results); - /* store the new configuration */ - dev_priv->wm.skl_hw = *results; + /* + * Store the new configuration (but only for the pipes that have + * changed; the other values weren't recomputed). + */ + for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes) + skl_copy_wm_for_pipe(hw_vals, results, pipe); mutex_unlock(&dev_priv->wm.wm_mutex); }