[Intel-gfx] [PATCH 11/16] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state()
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Oct 11 21:12:39 CEST 2013
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.
>
>
> >
> >>
> >> 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.
>
>
> >
> >>
> >>
> >> > + }
> >> > +}
> >> > +
> >> > +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
More information about the Intel-gfx
mailing list