From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write Date: Mon, 8 Sep 2014 16:40:32 +0200 Message-ID: <20140908144032.GC15520@phenom.ffwll.local> References: <1410270256-26413-1-git-send-email-deepak.s@linux.intel.com> <20140908140243.GZ4193@intel.com> <20140908141423.GA4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wg0-f49.google.com (mail-wg0-f49.google.com [74.125.82.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 95F4C89F5F for ; Mon, 8 Sep 2014 07:40:07 -0700 (PDT) Received: by mail-wg0-f49.google.com with SMTP id m15so1623114wgh.32 for ; Mon, 08 Sep 2014 07:40:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140908141423.GA4193@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrj=E4l=E4 wrote: > On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrj=E4l=E4 wrote: > > On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrot= e: > > > From: Deepak S > > > = > > > In chv, we have two power wells Render & Media. We need to use > > > corresponsing forcewake count. If we dont follow this we are getting > > > error "*ERROR*: Timed out waiting for forcewake old ack to clear" due= to > > > multiple entry into __vlv_force_wake_get. > > > = > > > Signed-off-by: Deepak S > > > --- > > > drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++---- > > > 1 file changed, 25 insertions(+), 4 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/= intel_lrc.c > > > index bd1b28d..bafd38b 100644 > > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > > @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_en= gine_cs *ring, > > > * Instead, we do the runtime_pm_get/put when creating/destroying r= equests. > > > */ > > > spin_lock_irqsave(&dev_priv->uncore.lock, flags); > > > - if (dev_priv->uncore.forcewake_count++ =3D=3D 0) > > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); > > > + if (IS_CHERRYVIEW(dev_priv->dev)) { > > > + if (dev_priv->uncore.fw_rendercount++ =3D=3D 0) > > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, > > > + FORCEWAKE_RENDER); > > > + if (dev_priv->uncore.fw_mediacount++ =3D=3D 0) > > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, > > > + FORCEWAKE_MEDIA); > > = > > This will wake both wells. Is that needed or should we just pick one > > based on the ring? > = > Also unlike the comment says runtime_pm_get() can't sleep since someone > must already be holding a reference, othwewise we surely can't go > writing any registers. So in theory we should be able to call > gen6_gt_force_wake_get() here, but maybe that would trigger a > might_sleep() warning. the current force wake code duplication (esp. > outside intel_uncore.c) is rather unfortunate and I'd like to see it > killed off. Maybe we just need to pull the rpm get/put outside > gen6_gt_force_wake_get()? I never really liked hiding it there anyway. Yeah this is just broken design. And if you look at the other wheel to track outstanding gpu work (requests) then it's not needed at all. But I'm not sure what's the priority of the "rework execlists to use requests" task is and when (if ever that will happen). Jesse is the arbiter for this stuff anyway, so adding him. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch