[Intel-gfx] [PATCH 11/16] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state()

[Ville Syrjälä]

On Fri, Oct 11, 2013 at 04:46:58PM -0300, Paulo Zanoni wrote:
> 2013/10/11 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> > On Fri, Oct 11, 2013 at 03:15:48PM -0300, Paulo Zanoni wrote:
> >> 2013/10/11 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> >> > On Fri, Oct 11, 2013 at 01:45:27PM -0300, Paulo Zanoni wrote:
> >> >> 2013/10/9  <ville.syrjala at linux.intel.com>:
> >> >> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> >> >
> >> >> > Fill out the HSW watermark s/w tracking structures with the current
> >> >> > hardware state in intel_modeset_setup_hw_state(). This allows us to skip
> >> >> > the HW state readback during watermark programming and just use the values
> >> >> > we keep around in dev_priv->wm. Reduces the overhead of the watermark
> >> >> > programming quite a bit.
> >> >> >
> >> >> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/intel_display.c |   3 +
> >> >> >  drivers/gpu/drm/i915/intel_drv.h     |   1 +
> >> >> >  drivers/gpu/drm/i915/intel_pm.c      | 104 ++++++++++++++++++++++++++---------
> >> >> >  3 files changed, 82 insertions(+), 26 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> >> > index 27f98bc..194f933 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> >> > @@ -10820,6 +10820,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >> >> >                 pll->on = false;
> >> >> >         }
> >> >> >
> >> >> > +       if (IS_HASWELL(dev))
> >> >> > +               ilk_init_wm(dev);
> >> >>
> >> >> If is_HSW, then ILK_something is quite confusing :) Not everybody is
> >> >> aware of your greater plans for total watermarks domination.
> >> >>
> >> >>
> >> >> > +
> >> >> >         if (force_restore) {
> >> >> >                 i915_redisable_vga(dev);
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> >> > index 3325b0b..bdb1708 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> >> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> >> > @@ -818,6 +818,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv);
> >> >> >  void gen6_rps_boost(struct drm_i915_private *dev_priv);
> >> >> >  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> >> >> >  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> >> >> > +void ilk_init_wm(struct drm_device *dev);
> >> >> >
> >> >> >
> >> >> >  /* intel_sdvo.c */
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> >> > index 5bd8c73..cebd9b4 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_pm.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> >> > @@ -2840,37 +2840,19 @@ static unsigned int ilk_compute_wm_dirty(struct drm_device *dev,
> >> >> >  static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
> >> >> >                                 struct hsw_wm_values *results)
> >> >> >  {
> >> >> > -       struct hsw_wm_values previous;
> >> >> > +       struct hsw_wm_values *previous = &dev_priv->wm.hw;
> >> >> >         unsigned int dirty;
> >> >> >         uint32_t val;
> >> >> >
> >> >> > -       previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK);
> >> >> > -       previous.wm_pipe[1] = I915_READ(WM0_PIPEB_ILK);
> >> >> > -       previous.wm_pipe[2] = I915_READ(WM0_PIPEC_IVB);
> >> >> > -       previous.wm_lp[0] = I915_READ(WM1_LP_ILK);
> >> >> > -       previous.wm_lp[1] = I915_READ(WM2_LP_ILK);
> >> >> > -       previous.wm_lp[2] = I915_READ(WM3_LP_ILK);
> >> >> > -       previous.wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
> >> >> > -       previous.wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
> >> >> > -       previous.wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
> >> >> > -       previous.wm_linetime[0] = I915_READ(PIPE_WM_LINETIME(PIPE_A));
> >> >> > -       previous.wm_linetime[1] = I915_READ(PIPE_WM_LINETIME(PIPE_B));
> >> >> > -       previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C));
> >> >> > -
> >> >> > -       previous.partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
> >> >> > -                               INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> >> >> > -
> >> >> > -       previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> >> >> > -
> >> >> > -       dirty = ilk_compute_wm_dirty(dev_priv->dev, &previous, results);
> >> >> > +       dirty = ilk_compute_wm_dirty(dev_priv->dev, previous, results);
> >> >> >         if (!dirty)
> >> >> >                 return;
> >> >> >
> >> >> > -       if (dirty & WM_DIRTY_LP(3) && previous.wm_lp[2] != 0)
> >> >> > +       if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] != 0)
> >> >> >                 I915_WRITE(WM3_LP_ILK, 0);
> >> >> > -       if (dirty & WM_DIRTY_LP(2) && previous.wm_lp[1] != 0)
> >> >> > +       if (dirty & WM_DIRTY_LP(2) && previous->wm_lp[1] != 0)
> >> >> >                 I915_WRITE(WM2_LP_ILK, 0);
> >> >> > -       if (dirty & WM_DIRTY_LP(1) && previous.wm_lp[0] != 0)
> >> >> > +       if (dirty & WM_DIRTY_LP(1) && previous->wm_lp[0] != 0)
> >> >> >                 I915_WRITE(WM1_LP_ILK, 0);
> >> >> >
> >> >> >         if (dirty & WM_DIRTY_PIPE(PIPE_A))
> >> >> > @@ -2905,11 +2887,11 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
> >> >> >                 I915_WRITE(DISP_ARB_CTL, val);
> >> >> >         }
> >> >> >
> >> >> > -       if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] != results->wm_lp_spr[0])
> >> >> > +       if (dirty & WM_DIRTY_LP(1) && previous->wm_lp_spr[0] != results->wm_lp_spr[0])
> >> >> >                 I915_WRITE(WM1S_LP_ILK, results->wm_lp_spr[0]);
> >> >> > -       if (dirty & WM_DIRTY_LP(2) && previous.wm_lp_spr[1] != results->wm_lp_spr[1])
> >> >> > +       if (dirty & WM_DIRTY_LP(2) && previous->wm_lp_spr[1] != results->wm_lp_spr[1])
> >> >> >                 I915_WRITE(WM2S_LP_IVB, results->wm_lp_spr[1]);
> >> >> > -       if (dirty & WM_DIRTY_LP(3) && previous.wm_lp_spr[2] != results->wm_lp_spr[2])
> >> >> > +       if (dirty & WM_DIRTY_LP(3) && previous->wm_lp_spr[2] != results->wm_lp_spr[2])
> >> >> >                 I915_WRITE(WM3S_LP_IVB, results->wm_lp_spr[2]);
> >> >> >
> >> >> >         if (dirty & WM_DIRTY_LP(1) && results->wm_lp[0] != 0)
> >> >> > @@ -3142,6 +3124,76 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane,
> >> >> >         I915_WRITE(WM3S_LP_IVB, sprite_wm);
> >> >> >  }
> >> >> >
> >> >> > +static void ilk_init_pipe_wm(struct drm_crtc *crtc)
> >> >> > +{
> >> >> > +       struct drm_device *dev = crtc->dev;
> >> >> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> >> >> > +       struct hsw_wm_values *hw = &dev_priv->wm.hw;
> >> >> > +       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> >> > +       struct intel_pipe_wm *active = &intel_crtc->wm.active;
> >> >> > +       enum pipe pipe = intel_crtc->pipe;
> >> >> > +       static const unsigned int wm0_pipe_reg[] = {
> >> >> > +               [PIPE_A] = WM0_PIPEA_ILK,
> >> >> > +               [PIPE_B] = WM0_PIPEB_ILK,
> >> >> > +               [PIPE_C] = WM0_PIPEC_IVB,
> >> >> > +       };
> >> >> > +
> >> >> > +       hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]);
> >> >> > +       hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> >> >> > +
> >> >> > +       /* Assume sprites are disabled */
> >> >>
> >> >> Why? Please write in the comment.
> >> >
> >> > Actually that's a leftover from before I reordered some of my patches.
> >> > In a later stage I want to track sprite status in intel_pipe_wm, so the
> >> > comment was meant to tell the reader why we don't populate that
> >> > information here. And the real reason for not populating that
> >> > information is that I'm lazy and figured sprites will never be enabled
> >> > when we load the driver.
> >> >
> >> > But for now, I'll just kill the comment since it's utter nonsense at the
> >> > moment.
> >> >
> >> >>
> >> >>
> >> >> > +
> >> >> > +       if (intel_crtc_active(crtc)) {
> >> >> > +               u32 tmp = hw->wm_pipe[pipe];
> >> >> > +
> >> >> > +               /*
> >> >> > +                * For active pipes LP0 watermark is marked as
> >> >> > +                * enabled, and LP1+ watermaks as disabled since
> >> >> > +                * we can't really reverse compute them in case
> >> >> > +                * multiple pipes are active.
> >> >> > +                */
> >> >> > +               active->wm[0].enable = true;
> >> >> > +               active->wm[0].pri_val = (tmp & WM0_PIPE_PLANE_MASK) >> WM0_PIPE_PLANE_SHIFT;
> >> >> > +               active->wm[0].spr_val = (tmp & WM0_PIPE_SPRITE_MASK) >> WM0_PIPE_SPRITE_SHIFT;
> >> >> > +               active->wm[0].cur_val = tmp & WM0_PIPE_CURSOR_MASK;
> >> >> > +               active->linetime = hw->wm_linetime[pipe];
> >> >> > +       } else {
> >> >> > +               int level, max_level = ilk_wm_max_level(dev);
> >> >> > +
> >> >> > +               /*
> >> >> > +                * For inactive pipes, all watermark levels
> >> >> > +                * should be marked as enabled but zeroed,
> >> >> > +                * which is what we'd comoute them to.
> >> >> > +                */
> >> >> > +               for (level = 0; level <= max_level; level++)
> >> >> > +                       active->wm[level].enable = true;
> >> >>
> >> >> Why exactly do we compute them like this?
> >> >
> >> > The assumption is that for a disabled pipe all watermarks are zero,
> >> > which means all the levels are valid.
> >>
> >> But valid != enabled. This is the confusing part IMHO.
> >
> > I blame you for that ;) I called this sucker 'valid' in my original
> > monster RFC patch, but your HSW watermark rework had pretty much the
> > same thing but called 'enable' and that's what we have now.
> 
> I have to be honest, I thought it would be waaaay easier for you to
> reuse my watermarks code.

It's quite possible I've made it harder on myself by sticking a bit too
closely to my original vision :)

> 
> >
> >>
> >>
> >> >
> >> >>
> >> >> One thing that I noticed is that, both on current -nightly (without
> >> >> your series) and with your series, when we disable all the screens we
> >> >> zero all the watermarks, but leave the "enable" bits of the LP
> >> >> watermarks enabled. IMHO we should treat this as a bug and fix it. I
> >> >> wonder if the comment above is related with this problem.
> >> >
> >> > Why is that a problem?
> >>
> >> Because it relies on something that's only implied by the
> >> specification and probably not really thoroughly validated (or
> >> validated at all). I really don't like abusing the HW like that. Just
> >> take a look at the amount of HW workarounds we already have for the
> >> "happy cases"... I really prefer to be on the safer side.
> >
> > Well, we can't really "fix" it unless we reorganize the rtc_enable/disable
> > code. The plane enable/disable should get moved for everyrhing like we
> > did for HSW, so that it's clear when we no longer depend on the
> > watermarks. And after that we'd need to either move the crtc->active=false
> > assignment earlier in the .crtc_disable, or we need to add some other
> > knob for the watermark code to check.
> 
> I thought it was a simple regression since my original code didn't do this...


> 
> >
> >>
> >>
> >> >
> >> >>
> >> >>
> >> >> > +       }
> >> >> > +}
> >> >> > +
> >> >> > +void ilk_init_wm(struct drm_device *dev)
> >> >>
> >> >> IMHO, maintaining the _get_hw_state nomenclature would be an improvement.
> >> >
> >> > OK.
> >> >
> >> >>
> >> >>
> >> >> > +{
> >> >> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> >> >> > +       struct hsw_wm_values *hw = &dev_priv->wm.hw;
> >> >> > +       struct drm_crtc *crtc;
> >> >> > +
> >> >> > +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> >> >> > +               ilk_init_pipe_wm(crtc);
> >> >> > +
> >> >> > +       hw->wm_lp[0] = I915_READ(WM1_LP_ILK);
> >> >> > +       hw->wm_lp[1] = I915_READ(WM2_LP_ILK);
> >> >> > +       hw->wm_lp[2] = I915_READ(WM3_LP_ILK);
> >> >> > +
> >> >> > +       hw->wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
> >> >> > +       hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
> >> >> > +       hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
> >> >> > +
> >> >> > +       hw->partitioning =
> >> >> > +               !!(I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6);
> >> >>
> >> >> This is a little bit dangerous... We never know if we're not going to
> >> >> add a 4_6 partition type in the middle of the enum for Gen 17. And if
> >> >> we add it, I'm 100% sure we'll forget to patch this line. I usually
> >> >> try to avoid these things.
> >> >
> >> > I happen to know that we won't get such a thing ;) Well, unless the
> >> > hardware designers start backpedaling after a few gens.
> >> >
> >> > So how would you write this?
> >>
> >> With the simpler form:
> >>
> >> hw->partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
> >> INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> >>
> >> Even if the enum value changes, the code will remain correct on
> >> Haswell, I hope :)
> >
> > OK.
> >
> >>
> >>
> >> >
> >> >>
> >> >>
> >> >> > +
> >> >> > +       hw->enable_fbc_wm =
> >> >> > +               !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> >> >> > +}
> >> >> > +
> >> >> >  /**
> >> >> >   * intel_update_watermarks - update FIFO watermark values based on current modes
> >> >>
> >> >> Also, this patch makes me wonder about those places where we change
> >> >> the HW state directly (init_clock_gating, for example) without
> >> >> touching the struct you just added. That specific init_clock_gating is
> >> >> run before we call ilk_init_wm, so it shouldn't be a problem on
> >> >> boot/resume, but it still leaves me worried...
> >> >
> >> > One option would be to kill the WM stuff from init_clock_gating. I think
> >> > it's just a safety measure to have it there, but I don't really see much
> >> > reason to keep it.
> >>
> >> Yeah, I always wonder if that's really needed or not.
> >>
> >> Advantages of keeping it:
> >> - it reduces our possibility of triggering an underrun when we do the
> >> other clock_gating stuff
> >> - if the BIOS does it wrong, we can minimize its failure
> >>
> >> Disadvantages:
> >> - we may want the possible underruns when we do the other clock_gating
> >> stuff, since they're probably wrong anyway
> >> - if we ever get to a point where we do zero modesets when loading the
> >> driver, we'll just mess the watermarks.
> >>
> >> To counter the second advantage of keeping it, we could even write
> >> intel_sanitize_watermarks :)
> >>
> >> But that's all material for future patches.
> >>
> >> >
> >> >>
> >> >> Also, perhaps we could have one of those functions that try to check
> >> >> if the tracked state is really the HW state...
> >> >
> >> > Yeah, that should be doable. But the watermark update is going to become
> >> > a staged process when I'm through with it, so we need to be careful
> >> > where we do the check to make sure we compare with the right sw state.
> >>
> >> Let's focus on merging your current plans first, then we think about
> >> these things then.
> >>
> >>
> >> >
> >> >>
> >> >> >   *
> >> >> > --
> >> >> > 1.8.1.5
> >> >> >
> >> >> > _______________________________________________
> >> >> > Intel-gfx mailing list
> >> >> > Intel-gfx at lists.freedesktop.org
> >> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >> Paulo Zanoni
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel OTC
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC