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 13:45:27 -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> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ob0-f170.google.com (mail-ob0-f170.google.com [209.85.214.170]) by gabe.freedesktop.org (Postfix) with ESMTP id 30049E8120 for ; Fri, 11 Oct 2013 09:45:28 -0700 (PDT) Received: by mail-ob0-f170.google.com with SMTP id gq1so2989799obb.29 for ; Fri, 11 Oct 2013 09:45:28 -0700 (PDT) In-Reply-To: <1381335490-4906-12-git-send-email-ville.syrjala@linux.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/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 watermark > 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 drm_devi= ce *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/i915/inte= l_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 d= rm_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_DATA_PART= ITION_5_6) ? > - INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2; > - > - previous.enable_fbc_wm =3D !(I915_READ(DISP_ARB_CTL) & DISP_FBC_W= M_DIS); > - > - dirty =3D ilk_compute_wm_dirty(dev_priv->dev, &previous, results); > + dirty =3D ilk_compute_wm_dirty(dev_priv->dev, previous, results); > 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_p= rivate *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(struct dr= m_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. > + > + 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_MASK) >> = 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_MASK; > + 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? 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. > + } > +} > + > +void ilk_init_wm(struct drm_device *dev) IMHO, maintaining the _get_hw_state nomenclature would be an improvement. > +{ > + 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. > + > + hw->enable_fbc_wm =3D > + !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS); > +} > + > /** > * intel_update_watermarks - update FIFO watermark values based on curre= nt 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... Also, perhaps we could have one of those functions that try to check if the tracked state is really the HW state... > * > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Paulo Zanoni