Re: [PATCH 13/35] drm/i915: Store the watermark latency values in dev_priv

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 30, 2013 at 06:42:45PM -0300, Paulo Zanoni wrote:
> I forgot to add intel-gfx in the review email...
> 
> 2013/7/30 Paulo Zanoni <przanoni@gmail.com>:
> > 2013/7/5  <ville.syrjala@linux.intel.com>:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Rather than having to read the latency values out every time, just
> >> store them in dev_priv.
> >>
> >> On ILK and IVB there is a difference between some of the latency
> >> values for different planes, so store the latency values for each
> >> plane type separately, and apply the necesary fixups during init.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h |  9 ++++++
> >>  drivers/gpu/drm/i915/intel_pm.c | 62 +++++++++++++++++++++++++++++++++++------
> >>  2 files changed, 62 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 99eb980..60f9437 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1184,6 +1184,15 @@ typedef struct drm_i915_private {
> >>
> >>         struct i915_suspend_saved_registers regfile;
> >>
> >> +       struct {
> >> +               /* watermark latency values for primary */
> >> +               uint16_t pri_latency[5];
> >> +               /* watermark latency values for sprite */
> >> +               uint16_t spr_latency[5];
> >> +               /* watermark latency values for cursor */
> >> +               uint16_t cur_latency[5];
> >> +       } wm;
> >> +
> >>         /* Old dri1 support infrastructure, beware the dragons ya fools entering
> >>          * here! */
> >>         struct i915_dri1_state dri1;
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index e4d2477..68a1de4 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -2367,6 +2367,39 @@ static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[5])
> >>         }
> >>  }
> >>
> >> +static void intel_fixup_spr_wm_latency(struct drm_device *dev, uint16_t wm[5])
> >> +{
> >> +       /* ILK sprite LP0 latency is 1300 ns */
> >> +       if (INTEL_INFO(dev)->gen == 5)
> >> +               wm[0] = 13;
> >> +}
> >> +
> >> +static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5])
> >> +{
> >> +       /* ILK cursor LP0 latency is 1300 ns */
> >> +       if (INTEL_INFO(dev)->gen == 5)
> >> +               wm[0] = 13;
> >> +
> >> +       /* WaDoubleCursorLP3Latency:ivb */
> >> +       if (IS_IVYBRIDGE(dev))
> >> +               wm[3] *= 2;
> >
> > Doesn't this WA apply only to pre-production steppings?

Hmm. Possibly. We have it in the current code, but both W/A database
and BSpec do indicate that it's perhaps not needed.

> >
> >
> >> +}
> >> +
> >> +static void intel_setup_wm_latency(struct drm_device *dev)
> >> +{
> >> +       struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +       intel_read_wm_latency(dev, dev_priv->wm.pri_latency);
> >> +
> >> +       memcpy(dev_priv->wm.spr_latency, dev_priv->wm.pri_latency,
> >> +              sizeof dev_priv->wm.pri_latency);
> >> +       memcpy(dev_priv->wm.cur_latency, dev_priv->wm.pri_latency,
> >> +              sizeof dev_priv->wm.pri_latency);
> >
> > Checkpatch.pl complains about these sizeof without parens.

I hate checkpatch for that. Complaining about perfectly valid C. But I
can "fix" it up to reduce checkpatch warnings.

> >
> >
> >> +
> >> +       intel_fixup_spr_wm_latency(dev, dev_priv->wm.spr_latency);
> >> +       intel_fixup_cur_wm_latency(dev, dev_priv->wm.cur_latency);
> >> +}
> >> +
> >>  static void hsw_compute_wm_parameters(struct drm_device *dev,
> >>                                       struct hsw_pipe_wm_parameters *params,
> >>                                       struct hsw_wm_maximums *lp_max_1_2,
> >> @@ -2612,16 +2645,17 @@ static void haswell_update_wm(struct drm_device *dev)
> >>         struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
> >>         struct hsw_pipe_wm_parameters params[3];
> >>         struct hsw_wm_values results_1_2, results_5_6, *best_results;
> >> -       uint16_t wm[5] = {};
> >>         enum hsw_data_buf_partitioning partitioning;
> >>
> >> -       intel_read_wm_latency(dev, wm);
> >>         hsw_compute_wm_parameters(dev, params, &lp_max_1_2, &lp_max_5_6);
> >>
> >> -       hsw_compute_wm_results(dev, params, wm, &lp_max_1_2, &results_1_2);
> >> +       hsw_compute_wm_results(dev, params,
> >> +                              dev_priv->wm.pri_latency,
> >> +                              &lp_max_1_2, &results_1_2);
> >>         if (lp_max_1_2.pri != lp_max_5_6.pri) {
> >> -               hsw_compute_wm_results(dev, params, wm, &lp_max_5_6,
> >> -                                      &results_5_6);
> >> +               hsw_compute_wm_results(dev, params,
> >> +                                      dev_priv->wm.pri_latency,
> >> +                                      &lp_max_5_6, &results_5_6);
> >
> > My vote would be to just not pass dev_priv->wm.pri_latency here, it's
> > useless since we're already passing dev. And in the place where we
> > actually use dev_priv->wm.pri_latency we should add a comment saying
> > that on Haswell pri_latency should be the same as the others,
> > otherwise people will start wondering why you're using pri_latency and
> > not the others.

I think I changed it in a later patch to grab the latency values from
the correct place for each plane. I could see about squashing those
changes into this patch if you think it's better.

> >
> >
> >>                 best_results = hsw_find_best_result(&results_1_2, &results_5_6);
> >>         } else {
> >>                 best_results = &results_1_2;
> >> @@ -5211,8 +5245,12 @@ void intel_init_pm(struct drm_device *dev)
> >>
> >>         /* For FIFO watermark updates */
> >>         if (HAS_PCH_SPLIT(dev)) {
> >> +               intel_setup_wm_latency(dev);
> >> +
> >>                 if (IS_GEN5(dev)) {
> >> -                       if (I915_READ(MLTR_ILK) & ILK_SRLT_MASK)
> >> +                       if (dev_priv->wm.pri_latency[1] &&
> >> +                           dev_priv->wm.spr_latency[1] &&
> >> +                           dev_priv->wm.cur_latency[1])
> >
> > I always wondered why do we have these checks. Do we ever see real
> > machines that have zero values in the latency fields? On some gens we
> > only check wm[0] and not the others, but on ILK we check wm[1] and not
> > the others... Anyway, this is not a problem that should be solved by
> > this patch, but I'd love answers :)

Yeah, I have no idea if these checks make any sense. The reason I'm
checking just wm[1] on ILK is that wm[0] is hardcoded, but wm[1] comes
from the MLTR register, so the new code matches the old code a bit
better. Although since the old code just checks that there is something
in the register, it might plug in the watermark funcs even if not some
of the pri, spr and cur latency values are zero.

> >
> >
> >>                                 dev_priv->display.update_wm = ironlake_update_wm;
> >>                         else {
> >>                                 DRM_DEBUG_KMS("Failed to get proper latency. "
> >> @@ -5221,7 +5259,9 @@ void intel_init_pm(struct drm_device *dev)
> >>                         }
> >>                         dev_priv->display.init_clock_gating = ironlake_init_clock_gating;
> >>                 } else if (IS_GEN6(dev)) {
> >> -                       if (SNB_READ_WM0_LATENCY()) {
> >> +                       if (dev_priv->wm.pri_latency[0] &&
> >> +                           dev_priv->wm.spr_latency[0] &&
> >> +                           dev_priv->wm.cur_latency[0]) {
> >>                                 dev_priv->display.update_wm = sandybridge_update_wm;
> >>                                 dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm;
> >>                         } else {
> >> @@ -5231,7 +5271,9 @@ void intel_init_pm(struct drm_device *dev)
> >>                         }
> >>                         dev_priv->display.init_clock_gating = gen6_init_clock_gating;
> >>                 } else if (IS_IVYBRIDGE(dev)) {
> >> -                       if (SNB_READ_WM0_LATENCY()) {
> >> +                       if (dev_priv->wm.pri_latency[0] &&
> >> +                           dev_priv->wm.spr_latency[0] &&
> >> +                           dev_priv->wm.cur_latency[0]) {
> >>                                 dev_priv->display.update_wm = ivybridge_update_wm;
> >>                                 dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm;
> >>                         } else {
> >> @@ -5241,7 +5283,9 @@ void intel_init_pm(struct drm_device *dev)
> >>                         }
> >>                         dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
> >>                 } else if (IS_HASWELL(dev)) {
> >> -                       if (I915_READ64(MCH_SSKPD)) {
> >> +                       if (dev_priv->wm.pri_latency[0] &&
> >> +                           dev_priv->wm.spr_latency[0] &&
> >> +                           dev_priv->wm.cur_latency[0]) {
> >>                                 dev_priv->display.update_wm = haswell_update_wm;
> >>                                 dev_priv->display.update_sprite_wm =
> >>                                         haswell_update_sprite_wm;
> >> --
> >> 1.8.1.5
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC