From mboxrd@z Thu Jan 1 00:00:00 1970 From: Deepak S Subject: Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write Date: Tue, 09 Sep 2014 21:45:08 +0530 Message-ID: <540F278C.7070407@intel.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 6049C6E295 for ; Mon, 8 Sep 2014 09:20:01 -0700 (PDT) In-Reply-To: <20140908144032.GC15520@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: Daniel Vetter , =?windows-1252?Q?Ville_Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 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/i= ntel_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_eng= ine_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 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 o= ut waiting for forcewake old ack to clear" "and Hang.