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: Wed, 10 Sep 2014 17:47:39 +0200 Message-ID: <20140910154739.GU15520@phenom.ffwll.local> References: <1410270256-26413-1-git-send-email-deepak.s@linux.intel.com> <20140908140243.GZ4193@intel.com> <20140908141423.GA4193@intel.com> <20140908144032.GC15520@phenom.ffwll.local> <540F278C.7070407@intel.com> <20140909142550.124035ff@jbarnes-t420> <20140910071645.GP15520@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-we0-f181.google.com (mail-we0-f181.google.com [74.125.82.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 534E76E342 for ; Wed, 10 Sep 2014 08:47:15 -0700 (PDT) Received: by mail-we0-f181.google.com with SMTP id w62so2429578wes.40 for ; Wed, 10 Sep 2014 08:47:14 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140910071645.GP15520@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: Deepak S , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 10, 2014 at 09:16:45AM +0200, Daniel Vetter wrote: > On Tue, Sep 09, 2014 at 02:25:50PM -0700, Jesse Barnes wrote: > > On Tue, 09 Sep 2014 21:45:08 +0530 > > Deepak S wrote: > > = > > > = > > > On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote: > > > > 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 wrote: > > > >>>> 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_engine_cs *ring, > > > >>>> * Instead, we do the runtime_pm_get/put when > > > >>>> creating/destroying requests. */ > > > >>>> 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 > > > = > > > hmm , agreed do we have a reworked execlist? The reason why added > > > this, on chv when i enable execlist, due to incorrect forcewake count > > > is causing multiple entries to forcewake_get resulting in "*ERROR*: > > > Timed out waiting for forcewake old ack to clear" "and Hang. > > = > > I'm hoping we can get execlists reworked on top of the request/seqno > > stuff shortly after it lands, but I don't think that's a reason to > > block this fix, since Chris is still busy fixing up the request > > changes. > = > Queued for -next, thanks for the patch. Aside if someone gets bored and wants to apply some polish: And __ variant of force_wake_get/put which only asserts that the device isn't runtime suspended instead of doing the runtime_pm_get/put would be cute - we have one other user in the hsw/bdw rpm code already. The current force_wake_get/put functions would then just wrap those raw versions. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch