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.
>
> 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?
>
>
> > + }
> > +}
> > +
> > +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?
>
>
> > +
> > + 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.
>
> 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.
>
> > *
> > --
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
