From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH 2/2] drm/i915/skl: Don't mark pipes as dirty unless we've added/removed pipes Date: Tue, 19 Jul 2016 10:19:42 -0700 Message-ID: <20160719171942.GC21816@intel.com> References: <1468945856-23126-1-git-send-email-cpaul@redhat.com> <1468945856-23126-3-git-send-email-cpaul@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <1468945856-23126-3-git-send-email-cpaul@redhat.com> Sender: stable-owner@vger.kernel.org To: Lyude Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Daniel Vetter , Radhakrishna Sripada , Hans de Goede , Jani Nikula , David Airlie , "open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list)" List-Id: intel-gfx@lists.freedesktop.org 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. >=20 > When we haven't added or removed any pipes, we don't need to worry ab= out > the ddb allocation changing. As such there's no order of flushing pip= es > we have to guarantee; e.g. each pipe can be flushed at the earliest > given oppurtunity. This means all we have to do is: >=20 > - Calculate the new wm values > - Update each plane's settings and wm values > - Arm the plane's registers to update at the next vblank >=20 > 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 t= he > start of the next vblank* >=20 > 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. >=20 > This also fixes cursors causing monitors to flicker on Skylake. Final= ly. >=20 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 > Cc: stable@vger.kernel.org > Cc: Ville Syrj=E4l=E4 > Cc: Daniel Vetter > Cc: Radhakrishna Sripada > Cc: Hans de Goede > Cc: Matt Roper > --- > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/i= ntel_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; > =20 > - if (changed) > - results->dirty_pipes |=3D 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 |=3D drm_crtc_mask(crtc); > =20 > - if ((results->dirty_pipes & drm_crtc_mask(crtc)) =3D=3D 0) > - /* This pipe's WM's did not change */ > - continue; > + if ((results->dirty_pipes & drm_crtc_mask(crtc)) =3D=3D 0) > + /* This pipe's WM's did not change */ > + continue; > + } > =20 > intel_cstate->update_wm_pre =3D true; > skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc); > --=20 > 2.7.4 >=20 --=20 Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795