From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH v4 3/6] drm/i915/skl: Only flush pipes when we change the ddb allocation Date: Thu, 28 Jul 2016 06:14:30 -0700 Message-ID: <20160728131430.GJ32025@intel.com> References: <1469554483-24999-1-git-send-email-cpaul@redhat.com> <1469554483-24999-4-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: <1469554483-24999-4-git-send-email-cpaul@redhat.com> Sender: stable-owner@vger.kernel.org To: Lyude Cc: intel-gfx@lists.freedesktop.org, Maarten Lankhorst , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , stable@vger.kernel.org, Daniel Vetter , Radhakrishna Sripada , Hans de Goede , Jani Nikula , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Jul 26, 2016 at 01:34:39PM -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 vbla= nk > every time we have to update the watermark values because the cursor = was > moving between screens will introduce a rather noticable lag for user= s. >=20 > Signed-off-by: Lyude > 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/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++-- > 2 files changed, 30 insertions(+), 2 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i= 915_drv.h > index 1f6fe8c..d65e897 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 { > =20 > 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/i= ntel_pm.c > index 8ae3f2b..cda4196 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3892,6 +3892,12 @@ static void skl_flush_wm_values(struct drm_i91= 5_private *dev_priv, > new_ddb =3D &new_values->ddb; > cur_ddb =3D &dev_priv->wm.skl_hw.ddb; > =20 > + /* We only ever need to flush when the ddb allocations change */ > + if (!new_values->ddb_changed) > + return; > + > + new_values->ddb_changed =3D false; > + > /* > * First pass: flush the pipes with the new allocation contained in= to > * the old space. > @@ -3996,6 +4002,22 @@ pipes_modified(struct drm_atomic_state *state) > return ret; > } > =20 > +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])) !=3D 0 || > + memcmp(&old->plane[pipe], &new->plane[pipe], > + sizeof(old->plane[pipe])) !=3D 0 || > + memcmp(&old->y_plane[pipe], &new->y_plane[pipe], > + sizeof(old->y_plane[pipe])) !=3D 0) > + return true; > + > + return false; > +} > + > static int > skl_compute_ddb(struct drm_atomic_state *state) > { > @@ -4003,7 +4025,8 @@ skl_compute_ddb(struct drm_atomic_state *state) > struct drm_i915_private *dev_priv =3D to_i915(dev); > struct intel_atomic_state *intel_state =3D to_intel_atomic_state(st= ate); > struct intel_crtc *intel_crtc; > - struct skl_ddb_allocation *ddb =3D &intel_state->wm_results.ddb; > + struct skl_ddb_allocation *new_ddb =3D &intel_state->wm_results.ddb= ; > + struct skl_ddb_allocation *old_ddb =3D &dev_priv->wm.skl_hw.ddb; > uint32_t realloc_pipes =3D pipes_modified(state); > int ret; > =20 > @@ -4041,9 +4064,13 @@ skl_compute_ddb(struct drm_atomic_state *state= ) > if (IS_ERR(cstate)) > return PTR_ERR(cstate); > =20 > - ret =3D skl_allocate_pipe_ddb(cstate, ddb); > + ret =3D skl_allocate_pipe_ddb(cstate, new_ddb); > if (ret) > return ret; > + > + if (!intel_state->wm_results.ddb_changed && I think you can simplify and drop the first part of this condition here= , but in general, Reviewed-by: Matt Roper > + skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe)) > + intel_state->wm_results.ddb_changed =3D true; > } > =20 > return 0; > --=20 > 2.7.4 >=20 --=20 Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795