Re: [PATCH 06/35] drm/i915: Pass the watermark level to primary WM compute functions

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 06/35] drm/i915: Pass the watermark level to primary WM compute functions
Date: Thu, 1 Aug 2013 11:01:58 +0300	[thread overview]
Message-ID: <20130801080158.GH5004@intel.com> (raw)
In-Reply-To: <CA+gsUGTn=NqNK+RQ_1DjOJtjGPMuG=1Dv+aAnNAR-yQahfvPLw@mail.gmail.com>

On Tue, Jul 30, 2013 at 04:49:18PM -0300, Paulo Zanoni wrote:
> 2013/7/5  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Passing the level insted of "is_lp" seems easier. The end result is the
> > same though.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 6b820c4..f178e26 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2185,7 +2185,7 @@ enum hsw_data_buf_partitioning {
> >  /* For both WM_PIPE and WM_LP. */
> >  static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
> >                                    uint32_t mem_value,
> > -                                  bool is_lp)
> > +                                  int level)
> >  {
> >         uint32_t method1, method2;
> >
> > @@ -2197,7 +2197,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
> >                                  params->pri_bytes_per_pixel,
> >                                  mem_value);
> >
> > -       if (!is_lp)
> > +       if (level == 0)
> >                 return method1;
> >
> >         method2 = ilk_wm_method2(params->pixel_rate,
> > @@ -2266,7 +2266,7 @@ static bool hsw_compute_lp_wm(uint32_t mem_value, struct hsw_wm_maximums *max,
> >         for (pipe = PIPE_A; pipe <= PIPE_C; pipe++) {
> >                 struct hsw_pipe_wm_parameters *p = &params[pipe];
> >
> > -               pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, true);
> > +               pri_val[pipe] = ilk_compute_pri_wm(p, mem_value, 1);
> 
> But now even when you're processing levels 2/3/4 you're telling the
> function that you're doing level 1, which is not true. We don't know
> what kind of code changes we'll be doing in January 2023, so we may
> use that incorrect "level" variable to do something wrong. IMHO,
> either we keep the current code or we pass the actual level.

Yeah. I guess we could keep it as bool now. The original reason for
passing the level was that I handled the IVB cursor latency W/A inside
the wm compute funcs, but now with the pre-populated latency info in
dev_priv passing the level makes less sense.

> 
> 
> >                 spr_val[pipe] = ilk_compute_spr_wm(p, mem_value);
> >                 cur_val[pipe] = ilk_compute_cur_wm(p, mem_value);
> >                 fbc_val[pipe] = ilk_compute_fbc_wm(p, pri_val[pipe], mem_value);
> > @@ -2296,7 +2296,7 @@ static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
> >  {
> >         uint32_t pri_val, cur_val, spr_val;
> >
> > -       pri_val = ilk_compute_pri_wm(params, mem_value, false);
> > +       pri_val = ilk_compute_pri_wm(params, mem_value, 0);
> >         spr_val = ilk_compute_spr_wm(params, mem_value);
> >         cur_val = ilk_compute_cur_wm(params, mem_value);
> >
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC