From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH 11/16] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state() Date: Fri, 11 Oct 2013 16:46:58 -0300 Message-ID: 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 mail-oa0-f52.google.com (mail-oa0-f52.google.com [209.85.219.52]) by gabe.freedesktop.org (Postfix) with ESMTP id E09B2E69F4 for ; Fri, 11 Oct 2013 12:46:59 -0700 (PDT) Received: by mail-oa0-f52.google.com with SMTP id n2so2800500oag.11 for ; Fri, 11 Oct 2013 12:46:58 -0700 (PDT) In-Reply-To: <20131011191239.GY13047@intel.com> 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: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org 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 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 waterma= rk >> >> > 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/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 dr= m_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/i91= 5/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_pr= iv); >> >> > void intel_aux_display_runtime_put(struct drm_i915_private *dev_pr= iv); >> >> > +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(st= ruct 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 =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(PIPE= _A)); >> >> > - previous.wm_linetime[1] =3D I915_READ(PIPE_WM_LINETIME(PIPE= _B)); >> >> > - previous.wm_linetime[2] =3D I915_READ(PIPE_WM_LINETIME(PIPE= _C)); >> >> > - >> >> > - previous.partitioning =3D (I915_READ(WM_MISC) & WM_MISC_DAT= A_PARTITION_5_6) ? >> >> > - INTEL_DDB_PART_5_6 : INTEL_DDB_PART= _1_2; >> >> > - >> >> > - previous.enable_fbc_wm =3D !(I915_READ(DISP_ARB_CTL) & DISP= _FBC_WM_DIS); >> >> > - >> >> > - dirty =3D ilk_compute_wm_dirty(dev_priv->dev, &previous, re= sults); >> >> > + dirty =3D ilk_compute_wm_dirty(dev_priv->dev, previous, res= ults); >> >> > 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 drm_= i915_private *dev_priv, >> >> > I915_WRITE(DISP_ARB_CTL, val); >> >> > } >> >> > >> >> > - if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] !=3D re= sults->wm_lp_spr[0]) >> >> > + if (dirty & WM_DIRTY_LP(1) && previous->wm_lp_spr[0] !=3D r= esults->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 re= sults->wm_lp_spr[1]) >> >> > + if (dirty & WM_DIRTY_LP(2) && previous->wm_lp_spr[1] !=3D r= esults->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 re= sults->wm_lp_spr[2]) >> >> > + if (dirty & WM_DIRTY_LP(3) && previous->wm_lp_spr[2] !=3D r= esults->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(str= uct 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 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 t= he >> > 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_MAS= K) >> WM0_PIPE_PLANE_SHIFT; >> >> > + active->wm[0].spr_val =3D (tmp & WM0_PIPE_SPRITE_MA= SK) >> WM0_PIPE_SPRITE_SHIFT; >> >> > + active->wm[0].cur_val =3D tmp & WM0_PIPE_CURSOR_MAS= K; >> >> > + 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. > >> >> >> > >> >> >> >> 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=3Dfa= lse > 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 improvem= ent. >> > >> > 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, head) >> >> > + 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 thi= nk >> > it's just a safety measure to have it there, but I don't really see mu= ch >> > 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 beco= me >> > 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@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