From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 11/16] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state() Date: Mon, 14 Oct 2013 14:21:35 +0300 Message-ID: <20131014112135.GD13047@intel.com> References: <1381335490-4906-1-git-send-email-ville.syrjala@linux.intel.com> <1381335490-4906-12-git-send-email-ville.syrjala@linux.intel.com> <20131011171514.GX13047@intel.com> <20131011191239.GY13047@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id C5443E6C7D for ; Mon, 14 Oct 2013 04:21:46 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Oct 11, 2013 at 04:46:58PM -0300, Paulo Zanoni wrote: > 2013/10/11 Ville Syrj=E4l=E4 : > > On Fri, Oct 11, 2013 at 03:15:48PM -0300, Paulo Zanoni wrote: > >> 2013/10/11 Ville Syrj=E4l=E4 : > >> > On Fri, Oct 11, 2013 at 01:45:27PM -0300, Paulo Zanoni wrote: > >> >> 2013/10/9 : > >> >> > From: Ville Syrj=E4l=E4 > >> >> > > >> >> > Fill out the HSW watermark s/w tracking structures with the curre= nt > >> >> > hardware state in intel_modeset_setup_hw_state(). This allows us = to skip > >> >> > the HW state readback during watermark programming and just use t= he values > >> >> > we keep around in dev_priv->wm. Reduces the overhead of the water= mark > >> >> > programming quite a bit. > >> >> > > >> >> > Signed-off-by: Ville Syrj=E4l=E4 > >> >> > --- > >> >> > 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/d= rm/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 =3D 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/i= 915/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 *d= ev_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/i9= 15/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_pri= v, > >> >> > struct hsw_wm_values *results) > >> >> > { > >> >> > - struct hsw_wm_values previous; > >> >> > + struct hsw_wm_values *previous =3D &dev_priv->wm.hw; > >> >> > unsigned int dirty; > >> >> > uint32_t val; > >> >> > > >> >> > - previous.wm_pipe[0] =3D I915_READ(WM0_PIPEA_ILK); > >> >> > - previous.wm_pipe[1] =3D I915_READ(WM0_PIPEB_ILK); > >> >> > - previous.wm_pipe[2] =3D I915_READ(WM0_PIPEC_IVB); > >> >> > - previous.wm_lp[0] =3D I915_READ(WM1_LP_ILK); > >> >> > - previous.wm_lp[1] =3D I915_READ(WM2_LP_ILK); > >> >> > - previous.wm_lp[2] =3D I915_READ(WM3_LP_ILK); > >> >> > - previous.wm_lp_spr[0] =3D I915_READ(WM1S_LP_ILK); > >> >> > - previous.wm_lp_spr[1] =3D I915_READ(WM2S_LP_IVB); > >> >> > - previous.wm_lp_spr[2] =3D I915_READ(WM3S_LP_IVB); > >> >> > - previous.wm_linetime[0] =3D I915_READ(PIPE_WM_LINETIME(PI= PE_A)); > >> >> > - previous.wm_linetime[1] =3D I915_READ(PIPE_WM_LINETIME(PI= PE_B)); > >> >> > - previous.wm_linetime[2] =3D I915_READ(PIPE_WM_LINETIME(PI= PE_C)); > >> >> > - > >> >> > - previous.partitioning =3D (I915_READ(WM_MISC) & WM_MISC_D= ATA_PARTITION_5_6) ? > >> >> > - INTEL_DDB_PART_5_6 : INTEL_DDB_PA= RT_1_2; > >> >> > - > >> >> > - previous.enable_fbc_wm =3D !(I915_READ(DISP_ARB_CTL) & DI= SP_FBC_WM_DIS); > >> >> > - > >> >> > - dirty =3D ilk_compute_wm_dirty(dev_priv->dev, &previous, = results); > >> >> > + dirty =3D ilk_compute_wm_dirty(dev_priv->dev, previous, r= esults); > >> >> > if (!dirty) > >> >> > return; > >> >> > > >> >> > - if (dirty & WM_DIRTY_LP(3) && previous.wm_lp[2] !=3D 0) > >> >> > + if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] !=3D 0) > >> >> > I915_WRITE(WM3_LP_ILK, 0); > >> >> > - if (dirty & WM_DIRTY_LP(2) && previous.wm_lp[1] !=3D 0) > >> >> > + if (dirty & WM_DIRTY_LP(2) && previous->wm_lp[1] !=3D 0) > >> >> > I915_WRITE(WM2_LP_ILK, 0); > >> >> > - if (dirty & WM_DIRTY_LP(1) && previous.wm_lp[0] !=3D 0) > >> >> > + if (dirty & WM_DIRTY_LP(1) && previous->wm_lp[0] !=3D 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 dr= m_i915_private *dev_priv, > >> >> > I915_WRITE(DISP_ARB_CTL, val); > >> >> > } > >> >> > > >> >> > - if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] !=3D = results->wm_lp_spr[0]) > >> >> > + if (dirty & WM_DIRTY_LP(1) && previous->wm_lp_spr[0] !=3D= 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] !=3D = results->wm_lp_spr[1]) > >> >> > + if (dirty & WM_DIRTY_LP(2) && previous->wm_lp_spr[1] !=3D= 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] !=3D = results->wm_lp_spr[2]) > >> >> > + if (dirty & WM_DIRTY_LP(3) && previous->wm_lp_spr[2] !=3D= 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] !=3D 0) > >> >> > @@ -3142,6 +3124,76 @@ static void sandybridge_update_sprite_wm(s= truct drm_plane *plane, > >> >> > I915_WRITE(WM3S_LP_IVB, sprite_wm); > >> >> > } > >> >> > > >> >> > +static void ilk_init_pipe_wm(struct drm_crtc *crtc) > >> >> > +{ > >> >> > + struct drm_device *dev =3D crtc->dev; > >> >> > + struct drm_i915_private *dev_priv =3D dev->dev_private; > >> >> > + struct hsw_wm_values *hw =3D &dev_priv->wm.hw; > >> >> > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > >> >> > + struct intel_pipe_wm *active =3D &intel_crtc->wm.active; > >> >> > + enum pipe pipe =3D intel_crtc->pipe; > >> >> > + static const unsigned int wm0_pipe_reg[] =3D { > >> >> > + [PIPE_A] =3D WM0_PIPEA_ILK, > >> >> > + [PIPE_B] =3D WM0_PIPEB_ILK, > >> >> > + [PIPE_C] =3D WM0_PIPEC_IVB, > >> >> > + }; > >> >> > + > >> >> > + hw->wm_pipe[pipe] =3D I915_READ(wm0_pipe_reg[pipe]); > >> >> > + hw->wm_linetime[pipe] =3D 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 patche= s. > >> > 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 enabl= ed > >> > 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 =3D 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 =3D true; > >> >> > + active->wm[0].pri_val =3D (tmp & WM0_PIPE_PLANE_M= ASK) >> WM0_PIPE_PLANE_SHIFT; > >> >> > + active->wm[0].spr_val =3D (tmp & WM0_PIPE_SPRITE_= MASK) >> WM0_PIPE_SPRITE_SHIFT; > >> >> > + active->wm[0].cur_val =3D tmp & WM0_PIPE_CURSOR_M= ASK; > >> >> > + active->linetime =3D hw->wm_linetime[pipe]; > >> >> > + } else { > >> >> > + int level, max_level =3D 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 =3D 0; level <=3D max_level; level++) > >> >> > + active->wm[level].enable =3D 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 !=3D 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/disa= ble > > 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=3D= 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 thi= s... > = > > > >> > >> > >> > > >> >> > >> >> > >> >> > + } > >> >> > +} > >> >> > + > >> >> > +void ilk_init_wm(struct drm_device *dev) > >> >> > >> >> IMHO, maintaining the _get_hw_state nomenclature would be an improv= ement. > >> > > >> > OK. > >> > > >> >> > >> >> > >> >> > +{ > >> >> > + struct drm_i915_private *dev_priv =3D dev->dev_private; > >> >> > + struct hsw_wm_values *hw =3D &dev_priv->wm.hw; > >> >> > + struct drm_crtc *crtc; > >> >> > + > >> >> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, he= ad) > >> >> > + ilk_init_pipe_wm(crtc); > >> >> > + > >> >> > + hw->wm_lp[0] =3D I915_READ(WM1_LP_ILK); > >> >> > + hw->wm_lp[1] =3D I915_READ(WM2_LP_ILK); > >> >> > + hw->wm_lp[2] =3D I915_READ(WM3_LP_ILK); > >> >> > + > >> >> > + hw->wm_lp_spr[0] =3D I915_READ(WM1S_LP_ILK); > >> >> > + hw->wm_lp_spr[1] =3D I915_READ(WM2S_LP_IVB); > >> >> > + hw->wm_lp_spr[2] =3D I915_READ(WM3S_LP_IVB); > >> >> > + > >> >> > + hw->partitioning =3D > >> >> > + !!(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 =3D (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 =3D > >> >> > + !(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 t= hink > >> > 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 be= come > >> > 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 stat= e. > >> > >> 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@lists.freedesktop.org > >> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> >> > >> >> > >> >> > >> >> -- > >> >> Paulo Zanoni > >> > > >> > -- > >> > Ville Syrj=E4l=E4 > >> > Intel OTC > >> > >> > >> > >> -- > >> Paulo Zanoni > > > > -- > > Ville Syrj=E4l=E4 > > Intel OTC > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC