From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup Date: Tue, 7 Jan 2014 21:53:02 +0200 Message-ID: <20140107195301.GG4800@intel.com> References: <1389104050-6426-1-git-send-email-ville.syrjala@linux.intel.com> <1389104050-6426-4-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 mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E18F7FA958 for ; Tue, 7 Jan 2014 11:53:05 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Tue, Jan 07, 2014 at 05:34:40PM -0200, Paulo Zanoni wrote: > 2014/1/7 : > > From: Ville Syrj=E4l=E4 > > > > Avoid duplicating the same piece of code several times by separating > > the watemark vfunc setup from the init_clock_gating vfunc setup on PCH > > platforms. > > > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_pm.c | 78 +++++++++------------------------= -------- > > 1 file changed, 16 insertions(+), 62 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index 04e1e29..a177a93 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5575,73 +5575,27 @@ void intel_init_pm(struct drm_device *dev) > > if (HAS_PCH_SPLIT(dev)) { > > intel_setup_wm_latency(dev); > > > > - if (IS_GEN5(dev)) { > > - if (dev_priv->wm.pri_latency[1] && > > - dev_priv->wm.spr_latency[1] && > > - dev_priv->wm.cur_latency[1]) { > > - dev_priv->display.update_wm =3D ilk_upd= ate_wm; > > - dev_priv->display.update_sprite_wm =3D > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to get proper lat= ency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm =3D NULL; > > - } > > + if ((IS_GEN5(dev) && dev_priv->wm.pri_latency[1] && > > + dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_la= tency[1]) || > > + (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] && > > + dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_la= tency[0])) { > > + dev_priv->display.update_wm =3D ilk_update_wm; > > + dev_priv->display.update_sprite_wm =3D ilk_upda= te_sprite_wm; > = > My tiny little brain doesn't remember why on gen5 we check for > latency[1] instead of latency[0]. I know this is not the goal if your > patch, but maybe a follow-up patch adding a little comment would be > nice :) I did it that way since the latency[0] values are hardcoded on ILK. So we can't tell from latency[0] values whether the BIOS programmed the latency values or not. I had an idea to kill these checks, and possibly just dynamically calculate the max level for which we have latency data. But that's for another patch. > = > For the 3 patches on the series: > Reviewed-by: Paulo Zanoni > = > = > > + } else { > > + DRM_DEBUG_KMS("Failed to read display plane lat= ency. " > > + "Disable CxSR\n"); > > + } > > + > > + if (IS_GEN5(dev)) > > dev_priv->display.init_clock_gating =3D ironlak= e_init_clock_gating; > > - } else if (IS_GEN6(dev)) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm =3D ilk_upd= ate_wm; > > - dev_priv->display.update_sprite_wm =3D > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display p= lane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm =3D NULL; > > - } > > + else if (IS_GEN6(dev)) > > dev_priv->display.init_clock_gating =3D gen6_in= it_clock_gating; > > - } else if (IS_IVYBRIDGE(dev)) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm =3D ilk_upd= ate_wm; > > - dev_priv->display.update_sprite_wm =3D > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display p= lane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm =3D NULL; > > - } > > + else if (IS_IVYBRIDGE(dev)) > > dev_priv->display.init_clock_gating =3D ivybrid= ge_init_clock_gating; > > - } else if (IS_HASWELL(dev)) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm =3D ilk_upd= ate_wm; > > - dev_priv->display.update_sprite_wm =3D > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display p= lane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm =3D NULL; > > - } > > + else if (IS_HASWELL(dev)) > > dev_priv->display.init_clock_gating =3D haswell= _init_clock_gating; > > - } else if (INTEL_INFO(dev)->gen =3D=3D 8) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm =3D ilk_upd= ate_wm; > > - dev_priv->display.update_sprite_wm =3D > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display p= lane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm =3D NULL; > > - } > > + else if (INTEL_INFO(dev)->gen =3D=3D 8) > > dev_priv->display.init_clock_gating =3D gen8_in= it_clock_gating; > > - } else > > - dev_priv->display.update_wm =3D NULL; > > } else if (IS_VALLEYVIEW(dev)) { > > dev_priv->display.update_wm =3D valleyview_update_wm; > > dev_priv->display.init_clock_gating =3D > > -- > > 1.8.3.2 > > > > _______________________________________________ > > 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