2013/10/11 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > On Fri, Oct 11, 2013 at 01:45:27PM -0300, Paulo Zanoni wrote: >> 2013/10/9 <ville.syrjala@xxxxxxxxxxxxxxx>: >> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> > >> > 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@xxxxxxxxxxxxxxx> >> > --- >> > 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. > >> >> 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. > >> >> >> > + } >> > +} >> > + >> > +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 :) > >> >> >> > + >> > + 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@xxxxxxxxxxxxxxxxxxxxx >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx