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: Wed, 8 Jan 2014 11:17:40 +0200 Message-ID: <20140108091740.GH4800@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> <20140107214727.GL4770@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 3619FFB055 for ; Wed, 8 Jan 2014 01:17:44 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140107214727.GL4770@phenom.ffwll.local> 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: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Tue, Jan 07, 2014 at 10:47:27PM +0100, Daniel Vetter wrote: > 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/i= ntel_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_u= pdate_wm; > > > - dev_priv->display.update_sprite_wm = =3D > > > - ilk_update_sprite_wm; > > > - } else { > > > - DRM_DEBUG_KMS("Failed to get proper l= atency. " > > > - "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_= latency[1]) || > > > + (!IS_GEN5(dev) && 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_update_wm; > > > + dev_priv->display.update_sprite_wm =3D ilk_up= date_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? Yeah, that could also work. > 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 ... I don't think it should happen unless the BIOS is broken. -- = Ville Syrj=E4l=E4 Intel OTC