From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup Date: Tue, 7 Jan 2014 22:47:27 +0100 Message-ID: <20140107214727.GL4770@phenom.ffwll.local> 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 mail-ea0-f169.google.com (mail-ea0-f169.google.com [209.85.215.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 2D0B8FAC94 for ; Tue, 7 Jan 2014 13:46:12 -0800 (PST) Received: by mail-ea0-f169.google.com with SMTP id l9so386363eaj.0 for ; Tue, 07 Jan 2014 13:46:11 -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 :) > = > For the 3 patches on the series: > Reviewed-by: Paulo Zanoni Merged to dinq, thanks for patches and review. And I'll use this occasion to repeat my plea to make our underrun reporting much louder - I fear that we'll silently break the watermark code soonish again if we don't ramp up automated testing. And I'd hate to see all the hard effort from you two wasted like that. btw for the funny wm.*_latency checking logic Paulo was confused about: Would it make sense for intel_setup_wm_latency to return a bool indicating whether doing the wm latency setup has succeeded or not? Also I wonder w a bit whether this is a real worl failure mode or whether we should just make the debug message a bit louder ... Cheers, Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch