| Message ID | 1469129020-2680-3-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 21, 2016 at 03:23:38PM -0400, Lyude wrote: > Manual pipe flushes are only necessary in order to make sure that we prevent > pipes with changed ddb allocations from overlapping from one another at > any point in time. Additionally, forcing us to wait for the next vblank > every time we have to update the watermark values because the cursor was > moving between screens will introduce a rather noticable lag for users. > > Signed-off-by: Lyude <cpaul@redhat.com> > Cc: stable@vger.kernel.org > 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> > Cc: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++-- > 2 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c97724d..9e1e045 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1597,6 +1597,7 @@ struct skl_ddb_allocation { > > struct skl_wm_values { > unsigned dirty_pipes; > + bool ddb_changed; > struct skl_ddb_allocation ddb; > uint32_t wm_linetime[I915_MAX_PIPES]; > uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 56ddd71..55237ea 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3789,6 +3789,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, > new_ddb = &new_values->ddb; > cur_ddb = &dev_priv->wm.skl_hw.ddb; > > + /* We only ever need to flush when the ddb allocations change */ I'd expand the comment a bit here since the logic is non-obvious. In general, we could still have WM changes (which need a flush) even if the DDB doesn't change. However in such cases, we'll be doing a normal flush as part of update_plane, update_primary_plane, or update_cursor so there's no need to do an extra, early flush here. The whole three-step flush dance is only needed to ensure proper inter-pipe ordering when the inter-pipe DDB allocation changes; the three-step handling we have here still isn't happening at the right time during the atomic commit, but fixing that up is something that one of your later patches will presumably address. > + if (!new_values->ddb_changed) > + return; > + > + new_values->ddb_changed = false; > + > /* > * First pass: flush the pipes with the new allocation contained into > * the old space. > @@ -3893,6 +3899,22 @@ pipes_modified(struct drm_atomic_state *state) > return ret; > } > > +static bool > +skl_pipe_ddb_changed(struct skl_ddb_allocation *old, > + struct skl_ddb_allocation *new, > + enum pipe pipe) > +{ > + if (memcmp(&old->pipe[pipe], &new->pipe[pipe], > + sizeof(old->pipe[pipe])) != 0 || > + memcmp(&old->plane[pipe], &new->plane[pipe], > + sizeof(old->plane[pipe])) != 0 || > + memcmp(&old->y_plane[pipe], &new->y_plane[pipe], > + sizeof(old->y_plane[pipe])) != 0) > + return true; I think I may have misled you when we talked on IRC the other day. Given the assumption that proper flushing happens during vblank evasion (via the update_{primary,plane,cursor} functions) that should cover all of our flushing needs except for the intra-pipe DDB changes where we need special ordering. So given that this patch isn't yet tackling the problems with inter-pipe DDB and is only concerned with cases where the list of active CRTC's isn't changing, I believe you only need to test the pipe allocation here and the {plane,y_plane} allocations shouldn't really matter. Even if the plane allocations change, I think we should still have a sequence of: update_wm() { write WM's write DDB } flush_wm_values() { skip flushing because DDB pipe[] alloc didn't change } vblank evasion { update_*() { write plane registers flush (plane, WM, and DDB) } } Actually, I feel we could almost drop the skl_flush_wm_values() function completely in this patch. Even in the cross-pipe allocation change case all pipes would get flushed they'd just be flushed in the wrong order, possibly causing underruns. However the way we're handling flushing right now is already wrong and can/will cause underruns, so we wouldn't really be making the situation worse than it already is, just a different kind of wrong. Then it would be simpler to follow up with another patch that handles all of the pipe updates in the proper order to solve that final problem. Matt > + > + return false; > +} > + > static int > skl_compute_ddb(struct drm_atomic_state *state) > { > @@ -3900,7 +3922,8 @@ skl_compute_ddb(struct drm_atomic_state *state) > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct intel_crtc *intel_crtc; > - struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb; > + struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; > + struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; > uint32_t realloc_pipes = pipes_modified(state); > int ret; > > @@ -3938,9 +3961,13 @@ skl_compute_ddb(struct drm_atomic_state *state) > if (IS_ERR(cstate)) > return PTR_ERR(cstate); > > - ret = skl_allocate_pipe_ddb(cstate, ddb); > + ret = skl_allocate_pipe_ddb(cstate, new_ddb); > if (ret) > return ret; > + > + if (!intel_state->wm_results.ddb_changed && > + skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe)) > + intel_state->wm_results.ddb_changed = true; > } > > return 0; > -- > 2.7.4 >
Op 21-07-16 om 21:23 schreef Lyude: > Manual pipe flushes are only necessary in order to make sure that we prevent > pipes with changed ddb allocations from overlapping from one another at > any point in time. Additionally, forcing us to wait for the next vblank > every time we have to update the watermark values because the cursor was > moving between screens will introduce a rather noticable lag for users. This screams for a testcase in kms_cursor_legacy, when does this happen exactly? I'm guessing it needs 2 screens, page flip on both and cursor update on both, one hiding the cursor other screen making it reappear.. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c97724d..9e1e045 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1597,6 +1597,7 @@ struct skl_ddb_allocation { struct skl_wm_values { unsigned dirty_pipes; + bool ddb_changed; struct skl_ddb_allocation ddb; uint32_t wm_linetime[I915_MAX_PIPES]; uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 56ddd71..55237ea 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3789,6 +3789,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, new_ddb = &new_values->ddb; cur_ddb = &dev_priv->wm.skl_hw.ddb; + /* We only ever need to flush when the ddb allocations change */ + if (!new_values->ddb_changed) + return; + + new_values->ddb_changed = false; + /* * First pass: flush the pipes with the new allocation contained into * the old space. @@ -3893,6 +3899,22 @@ pipes_modified(struct drm_atomic_state *state) return ret; } +static bool +skl_pipe_ddb_changed(struct skl_ddb_allocation *old, + struct skl_ddb_allocation *new, + enum pipe pipe) +{ + if (memcmp(&old->pipe[pipe], &new->pipe[pipe], + sizeof(old->pipe[pipe])) != 0 || + memcmp(&old->plane[pipe], &new->plane[pipe], + sizeof(old->plane[pipe])) != 0 || + memcmp(&old->y_plane[pipe], &new->y_plane[pipe], + sizeof(old->y_plane[pipe])) != 0) + return true; + + return false; +} + static int skl_compute_ddb(struct drm_atomic_state *state) { @@ -3900,7 +3922,8 @@ skl_compute_ddb(struct drm_atomic_state *state) struct drm_i915_private *dev_priv = to_i915(dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct intel_crtc *intel_crtc; - struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb; + struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; + struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; uint32_t realloc_pipes = pipes_modified(state); int ret; @@ -3938,9 +3961,13 @@ skl_compute_ddb(struct drm_atomic_state *state) if (IS_ERR(cstate)) return PTR_ERR(cstate); - ret = skl_allocate_pipe_ddb(cstate, ddb); + ret = skl_allocate_pipe_ddb(cstate, new_ddb); if (ret) return ret; + + if (!intel_state->wm_results.ddb_changed && + skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe)) + intel_state->wm_results.ddb_changed = true; } return 0;
Manual pipe flushes are only necessary in order to make sure that we prevent pipes with changed ddb allocations from overlapping from one another at any point in time. Additionally, forcing us to wait for the next vblank every time we have to update the watermark values because the cursor was moving between screens will introduce a rather noticable lag for users. Signed-off-by: Lyude <cpaul@redhat.com> Cc: stable@vger.kernel.org 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> Cc: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-)