From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH 10/16] drm/i915: Improve watermark dirtyness checks Date: Fri, 11 Oct 2013 12:02:53 -0300 Message-ID: References: <1381335490-4906-1-git-send-email-ville.syrjala@linux.intel.com> <1381335490-4906-11-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ob0-f174.google.com (mail-ob0-f174.google.com [209.85.214.174]) by gabe.freedesktop.org (Postfix) with ESMTP id 80254E62F9 for ; Fri, 11 Oct 2013 08:02:55 -0700 (PDT) Received: by mail-ob0-f174.google.com with SMTP id wm4so2813948obc.19 for ; Fri, 11 Oct 2013 08:02:54 -0700 (PDT) In-Reply-To: <1381335490-4906-11-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org 2013/10/9 : > From: Ville Syrj=E4l=E4 > > Currently hsw_write_vm_values() may write to certain watermark > registers needlessly. For instance it will disable LP1+ watermarks > around WM_PIPE changes even though that's not needed. Also if only, > say, LP3 changes, the current code will again disable all LP1+ > watermarks even though only LP3 needs to be reconfigured. > > Add an easy to read function that will compute the dirtyness of the > watermarks, and use that information to further optimize the watermark > programming. Clever solution to the problem! Some comments below. > > Signed-off-by: Ville Syrj=E4l=E4 > --- > drivers/gpu/drm/i915/intel_pm.c | 95 +++++++++++++++++++++++++++++++++--= ------ > 1 file changed, 77 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index bed96fb..5bd8c73 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2776,6 +2776,63 @@ static struct intel_pipe_wm *hsw_find_best_result(= struct drm_device *dev, > } > } > > +/* dirty bits used to track which watermarks need changes */ > +#define WM_DIRTY_PIPE(pipe) (1 << (pipe)) > +#define WM_DIRTY_LINETIME(pipe) (1 << (8 + (pipe))) > +#define WM_DIRTY_LP(wm_lp) (1 << (15 + (wm_lp))) > +#define WM_DIRTY_LP_ALL (WM_DIRTY_LP(1) | WM_DIRTY_LP(2) | WM_DIRTY_LP(3= )) > +#define WM_DIRTY_FBC (1 << 24) > +#define WM_DIRTY_DDB (1 << 25) > + > +static unsigned int ilk_compute_wm_dirty(struct drm_device *dev, > + const struct hsw_wm_values *old, > + const struct hsw_wm_values *new) > +{ > + unsigned int dirty =3D 0; > + enum pipe pipe; > + int wm_lp; > + > + for_each_pipe(pipe) { > + if (old->wm_linetime[pipe] !=3D new->wm_linetime[pipe]) { > + dirty |=3D WM_DIRTY_LINETIME(pipe); > + /* Must disable LP1+ watermarks too */ > + dirty |=3D WM_DIRTY_LP_ALL; > + } > + > + if (old->wm_pipe[pipe] !=3D new->wm_pipe[pipe]) > + dirty |=3D WM_DIRTY_PIPE(pipe); I think this one also needs WM_DIRTY_LP_ALL. I don't think changing level zero without stopping the LP levels is safe. > + } > + > + if (old->enable_fbc_wm !=3D new->enable_fbc_wm) { > + dirty |=3D WM_DIRTY_FBC; > + /* Must disable LP1+ watermarks too */ > + dirty |=3D WM_DIRTY_LP_ALL; > + } > + > + if (old->partitioning !=3D new->partitioning) { > + dirty |=3D WM_DIRTY_DDB; > + /* Must disable LP1+ watermarks too */ > + dirty |=3D WM_DIRTY_LP_ALL; > + } > + > + /* LP1+ watermarks already deemed dirty, no need to continue */ > + if (dirty & WM_DIRTY_LP_ALL) > + return dirty; > + > + /* Find the lowest numbered LP1+ watermark in need of an update..= . */ > + for (wm_lp =3D 1; wm_lp <=3D 3; wm_lp++) { > + if (old->wm_lp[wm_lp - 1] !=3D new->wm_lp[wm_lp - 1] || > + old->wm_lp_spr[wm_lp - 1] !=3D new->wm_lp_spr[wm_lp -= 1]) > + break; > + } > + > + /* ...and mark it and all higher numbered LP1+ watermarks as dirt= y */ > + for (; wm_lp <=3D 3; wm_lp++) > + dirty |=3D WM_DIRTY_LP(wm_lp); > + > + return dirty; > +} > + > /* > * The spec says we shouldn't write when we don't need, because every wr= ite > * causes WMs to be re-evaluated, expending some power. > @@ -2784,6 +2841,7 @@ static void hsw_write_wm_values(struct drm_i915_pri= vate *dev_priv, > struct hsw_wm_values *results) > { > struct hsw_wm_values previous; > + unsigned int dirty; > uint32_t val; > > previous.wm_pipe[0] =3D I915_READ(WM0_PIPEA_ILK); > @@ -2804,31 +2862,32 @@ static void hsw_write_wm_values(struct drm_i915_p= rivate *dev_priv, > > previous.enable_fbc_wm =3D !(I915_READ(DISP_ARB_CTL) & DISP_FBC_W= M_DIS); > > - if (memcmp(results, &previous, sizeof(*results)) =3D=3D 0) Oh, you've just removed the memcmp I complained about... Still, I believe it had a bug. > + dirty =3D ilk_compute_wm_dirty(dev_priv->dev, &previous, results); > + if (!dirty) > return; > > - if (previous.wm_lp[2] !=3D 0) > + if (dirty & WM_DIRTY_LP(3) && previous.wm_lp[2] !=3D 0) > I915_WRITE(WM3_LP_ILK, 0); > - if (previous.wm_lp[1] !=3D 0) > + if (dirty & WM_DIRTY_LP(2) && previous.wm_lp[1] !=3D 0) > I915_WRITE(WM2_LP_ILK, 0); > - if (previous.wm_lp[0] !=3D 0) > + if (dirty & WM_DIRTY_LP(1) && previous.wm_lp[0] !=3D 0) > I915_WRITE(WM1_LP_ILK, 0); > > - if (previous.wm_pipe[0] !=3D results->wm_pipe[0]) > + if (dirty & WM_DIRTY_PIPE(PIPE_A)) > I915_WRITE(WM0_PIPEA_ILK, results->wm_pipe[0]); > - if (previous.wm_pipe[1] !=3D results->wm_pipe[1]) > + if (dirty & WM_DIRTY_PIPE(PIPE_B)) > I915_WRITE(WM0_PIPEB_ILK, results->wm_pipe[1]); > - if (previous.wm_pipe[2] !=3D results->wm_pipe[2]) > + if (dirty & WM_DIRTY_PIPE(PIPE_C)) > I915_WRITE(WM0_PIPEC_IVB, results->wm_pipe[2]); > > - if (previous.wm_linetime[0] !=3D results->wm_linetime[0]) > + if (dirty & WM_DIRTY_LINETIME(PIPE_A)) > I915_WRITE(PIPE_WM_LINETIME(PIPE_A), results->wm_linetime= [0]); > - if (previous.wm_linetime[1] !=3D results->wm_linetime[1]) > + if (dirty & WM_DIRTY_LINETIME(PIPE_B)) > I915_WRITE(PIPE_WM_LINETIME(PIPE_B), results->wm_linetime= [1]); > - if (previous.wm_linetime[2] !=3D results->wm_linetime[2]) > + if (dirty & WM_DIRTY_LINETIME(PIPE_C)) > I915_WRITE(PIPE_WM_LINETIME(PIPE_C), results->wm_linetime= [2]); > > - if (previous.partitioning !=3D results->partitioning) { > + if (dirty & WM_DIRTY_DDB) { > val =3D I915_READ(WM_MISC); > if (results->partitioning =3D=3D INTEL_DDB_PART_1_2) > val &=3D ~WM_MISC_DATA_PARTITION_5_6; > @@ -2837,7 +2896,7 @@ static void hsw_write_wm_values(struct drm_i915_pri= vate *dev_priv, > I915_WRITE(WM_MISC, val); > } > > - if (previous.enable_fbc_wm !=3D results->enable_fbc_wm) { > + if (dirty & WM_DIRTY_FBC) { > val =3D I915_READ(DISP_ARB_CTL); > if (results->enable_fbc_wm) > val &=3D ~DISP_FBC_WM_DIS; > @@ -2846,18 +2905,18 @@ static void hsw_write_wm_values(struct drm_i915_p= rivate *dev_priv, > I915_WRITE(DISP_ARB_CTL, val); > } > > - if (previous.wm_lp_spr[0] !=3D results->wm_lp_spr[0]) > + if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] !=3D results-= >wm_lp_spr[0]) > I915_WRITE(WM1S_LP_ILK, results->wm_lp_spr[0]); As far as I understood, if previous.wm_lp_spr[X] !=3D results->wm_lp_spr[X], then for sure "dirty & WM_DIRTY_LP(X)", right? So the "dirty" check would be redundant here. And we can't remove the second part because we can have "dirty & WM_DIRTY_LP(X) while the sprite values don't change. > - if (previous.wm_lp_spr[1] !=3D results->wm_lp_spr[1]) > + if (dirty & WM_DIRTY_LP(2) && previous.wm_lp_spr[1] !=3D results-= >wm_lp_spr[1]) > I915_WRITE(WM2S_LP_IVB, results->wm_lp_spr[1]); > - if (previous.wm_lp_spr[2] !=3D results->wm_lp_spr[2]) > + if (dirty & WM_DIRTY_LP(3) && previous.wm_lp_spr[2] !=3D results-= >wm_lp_spr[2]) > I915_WRITE(WM3S_LP_IVB, results->wm_lp_spr[2]); > > - if (results->wm_lp[0] !=3D 0) > + if (dirty & WM_DIRTY_LP(1) && results->wm_lp[0] !=3D 0) > I915_WRITE(WM1_LP_ILK, results->wm_lp[0]); > - if (results->wm_lp[1] !=3D 0) > + if (dirty & WM_DIRTY_LP(2) && results->wm_lp[1] !=3D 0) > I915_WRITE(WM2_LP_ILK, results->wm_lp[1]); > - if (results->wm_lp[2] !=3D 0) > + if (dirty & WM_DIRTY_LP(3) && results->wm_lp[2] !=3D 0) > I915_WRITE(WM3_LP_ILK, results->wm_lp[2]); > > dev_priv->wm.hw =3D *results; > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Paulo Zanoni