From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] rpm Date: Wed, 10 Sep 2014 18:06:51 +0100 Message-ID: <20140910170651.GE31074@nuc-i3427.alporthouse.com> References: <1410367395-25093-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id DADBC89E65 for ; Wed, 10 Sep 2014 10:06:54 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote: > 2014-09-10 13:43 GMT-03:00 Chris Wilson : > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ > > drivers/gpu/drm/i915/intel_display.c | 20 ++------------ > > drivers/gpu/drm/i915/intel_lrc.c | 21 ++------------- > > drivers/gpu/drm/i915/intel_uncore.c | 52 +++++++++++++++--------------------- > > 4 files changed, 27 insertions(+), 68 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 5f35048..a72d8b8 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file) > > if (INTEL_INFO(dev)->gen < 6) > > return 0; > > > > + intel_runtime_pm_get(dev_priv); > > gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > > > return 0; > > @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file) > > return 0; > > > > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > > + intel_runtime_pm_put(dev_priv); > > > > return 0; > > } > > Just a minor comment on the chunk above. I didn't look at the rest of the patch. > > We used to have exactly the code that you rewrote, but we decided to > remove it because, without it, we can test that gen6_gt_force_wake_get > actually wakes up the HW and keeps it awake until someone calls > gen6_gt_force_wake_put. But why? gen6_gt_force_wake_get() should never be called outside of a rpm context - it is lowlevel register access. > If we add these get/put calls above, we won't really detect any > problems related with the actual gen6_gt_force_wake_get/put functions, > so we may hide problems. See above. That's how the design got broken in the first place and we have very lowlevel code being copied and pasted throughout the driver. -Chris -- Chris Wilson, Intel Open Source Technology Centre