| Message ID | 1469129020-2680-2-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 21-07-16 om 21:23 schreef Lyude: > 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 Looks like I can hit this when I wrote some tests for patch 2 in this series. testcase will be kms_cursor_legacy.2x-flip-vs-cursor-legacy, but I haven't committed the changes yet.
Op 21-07-16 om 21:23 schreef Lyude: > 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 > > 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> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > Cc: stable@vger.kernel.org > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > Cc: Hans de Goede <hdegoede@redhat.com> Testcase: kms_cursor_legacy.2x-flip-vs-cursor-legacy For patch 2/4 too probably.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 64d628c..56ddd71 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3946,6 +3946,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) { @@ -4018,8 +4036,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; @@ -4031,8 +4051,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); }