| Message ID | 1468945856-23126-3-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 19, 2016 at 12:30:56PM -0400, Lyude wrote: > Now that we update the watermark values atomically, we still need to fix > the case of how we update watermarks when we haven't added or removed > pipes. > > When we haven't added or removed any pipes, we don't need to worry about > the ddb allocation changing. As such there's no order of flushing pipes > we have to guarantee; e.g. each pipe can be flushed at the earliest > given oppurtunity. This means all we have to do is: > > - Calculate the new wm values > - Update each plane's settings and wm values > - Arm the plane's registers to update at the next vblank > > Right now the watermark code takes an additional few steps: > - Rewrite the ddb allocations > - Arm all of the planes for another update at the start of the next > vblank > - *Potentially underrun later if we update the wm registers before the > start of the next vblank* > > This patch prevents us from marking pipes as dirty unless the number of > pipes, and with that the ddb allocation, has actually changed. This > results in us skipping the additional steps, preventing the hardware > from using any intermediate values during each wm update. > > This also fixes cursors causing monitors to flicker on Skylake. Finally. > This doesn't look right to me. It's true that we don't need to worry about *inter*-pipe DDB allocation changing, but *intra*-pipe DDB allocations can still change. We may be resizing planes, changing scalers, enabling/disabling new planes, etc. Those operations change the DDB allocations for the planes within the fixed pipe allocation. The watermark values will also change accordingly in that case. Matt > Signed-off-by: Lyude Paul <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/intel_pm.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 3a7709c..92158e3 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state) > if (ret) > return ret; > > - if (changed) > - results->dirty_pipes |= drm_crtc_mask(crtc); > + /* > + * We don't need to do any additional setup for the pipes if we > + * haven't changed the number of active crtcs > + */ > + if (intel_state->active_pipe_changes) { > + if (changed) > + results->dirty_pipes |= drm_crtc_mask(crtc); > > - if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) > - /* This pipe's WM's did not change */ > - continue; > + if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) > + /* This pipe's WM's did not change */ > + continue; > + } > > intel_cstate->update_wm_pre = true; > skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc); > -- > 2.7.4 >
On Tue, Jul 19, 2016 at 1:19 PM, Matt Roper <matthew.d.roper@intel.com> wrote: > On Tue, Jul 19, 2016 at 12:30:56PM -0400, Lyude wrote: >> Now that we update the watermark values atomically, we still need to fix >> the case of how we update watermarks when we haven't added or removed >> pipes. >> >> When we haven't added or removed any pipes, we don't need to worry about >> the ddb allocation changing. As such there's no order of flushing pipes >> we have to guarantee; e.g. each pipe can be flushed at the earliest >> given oppurtunity. This means all we have to do is: >> >> - Calculate the new wm values >> - Update each plane's settings and wm values >> - Arm the plane's registers to update at the next vblank >> >> Right now the watermark code takes an additional few steps: >> - Rewrite the ddb allocations >> - Arm all of the planes for another update at the start of the next >> vblank >> - *Potentially underrun later if we update the wm registers before the >> start of the next vblank* >> >> This patch prevents us from marking pipes as dirty unless the number of >> pipes, and with that the ddb allocation, has actually changed. This >> results in us skipping the additional steps, preventing the hardware >> from using any intermediate values during each wm update. >> >> This also fixes cursors causing monitors to flicker on Skylake. Finally. >> > > This doesn't look right to me. It's true that we don't need to worry > about *inter*-pipe DDB allocation changing, but *intra*-pipe DDB > allocations can still change. We may be resizing planes, changing > scalers, enabling/disabling new planes, etc. Those operations change > the DDB allocations for the planes within the fixed pipe allocation. > The watermark values will also change accordingly in that case. > fwiw, msm/mdp5 has potentially similar constraints w/ SMP block allocation (which seems like basically the same thing as DDB).. we just disallow changing width/bpp/etc on an active plane, so userspace would pick a different plane (or skip the plane for one frame of composition).. not sure if that would help.. maybe we are more limited in not actually being able to safely change SMP allocation on an active plane/pipe. But treating it as a disable of current pipe and switch to new pipe is convenient, rather than dealing with all the cases that could effect SMP block assignment. BR, -R > > Matt > >> Signed-off-by: Lyude Paul <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/intel_pm.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 3a7709c..92158e3 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state) >> if (ret) >> return ret; >> >> - if (changed) >> - results->dirty_pipes |= drm_crtc_mask(crtc); >> + /* >> + * We don't need to do any additional setup for the pipes if we >> + * haven't changed the number of active crtcs >> + */ >> + if (intel_state->active_pipe_changes) { >> + if (changed) >> + results->dirty_pipes |= drm_crtc_mask(crtc); >> >> - if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) >> - /* This pipe's WM's did not change */ >> - continue; >> + if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) >> + /* This pipe's WM's did not change */ >> + continue; >> + } >> >> intel_cstate->update_wm_pre = true; >> skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc); >> -- >> 2.7.4 >> > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3a7709c..92158e3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4086,12 +4086,18 @@ skl_compute_wm(struct drm_atomic_state *state) if (ret) return ret; - if (changed) - results->dirty_pipes |= drm_crtc_mask(crtc); + /* + * We don't need to do any additional setup for the pipes if we + * haven't changed the number of active crtcs + */ + if (intel_state->active_pipe_changes) { + if (changed) + results->dirty_pipes |= drm_crtc_mask(crtc); - if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) - /* This pipe's WM's did not change */ - continue; + if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) + /* This pipe's WM's did not change */ + continue; + } intel_cstate->update_wm_pre = true; skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
Now that we update the watermark values atomically, we still need to fix the case of how we update watermarks when we haven't added or removed pipes. When we haven't added or removed any pipes, we don't need to worry about the ddb allocation changing. As such there's no order of flushing pipes we have to guarantee; e.g. each pipe can be flushed at the earliest given oppurtunity. This means all we have to do is: - Calculate the new wm values - Update each plane's settings and wm values - Arm the plane's registers to update at the next vblank Right now the watermark code takes an additional few steps: - Rewrite the ddb allocations - Arm all of the planes for another update at the start of the next vblank - *Potentially underrun later if we update the wm registers before the start of the next vblank* This patch prevents us from marking pipes as dirty unless the number of pipes, and with that the ddb allocation, has actually changed. This results in us skipping the additional steps, preventing the hardware from using any intermediate values during each wm update. This also fixes cursors causing monitors to flicker on Skylake. Finally. Signed-off-by: Lyude Paul <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/intel_pm.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)