From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers Date: Tue, 24 Sep 2013 13:14:19 +0200 Message-ID: <20130924111419.GP13668@phenom.ffwll.local> References: <1377521156-10045-1-git-send-email-chris@chris-wilson.co.uk> <20130826131516.GQ11428@intel.com> <20130924104721.GN13668@phenom.ffwll.local> <20130924111254.GE10644@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ea0-f181.google.com (mail-ea0-f181.google.com [209.85.215.181]) by gabe.freedesktop.org (Postfix) with ESMTP id 41622E7349 for ; Tue, 24 Sep 2013 04:14:03 -0700 (PDT) Received: by mail-ea0-f181.google.com with SMTP id d10so2380932eaj.40 for ; Tue, 24 Sep 2013 04:14:02 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130924111254.GE10644@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , Daniel Vetter , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Sep 24, 2013 at 12:12:54PM +0100, Chris Wilson wrote: > On Tue, Sep 24, 2013 at 12:47:21PM +0200, Daniel Vetter wrote: > > On Mon, Aug 26, 2013 at 04:15:16PM +0300, Ville Syrj=E4l=E4 wrote: > > > On Mon, Aug 26, 2013 at 01:45:56PM +0100, Chris Wilson wrote: > > > > The RPS register writing routines use the current value of min/max = to > > > > set certain limits and interrupt gating. If we set those afterwards= , we > > > > risk setting up the hw incorrectly and losing power management even= ts, > > > > and worse, trigger some internal assertions. > > > > = > > > > Reorder the calling sequences to be correct, and remove the then > > > > unrequired clamping from inside set_rps(). And for a bonus, fix the= bug > > > > of calling gen6_set_rps() from Valleyview. > > > > = > > > > Signed-off-by: Chris Wilson > > > > --- > > > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > > > > drivers/gpu/drm/i915/i915_sysfs.c | 16 ++++++++-------- > > > > drivers/gpu/drm/i915/intel_pm.c | 19 +++++-------------- > > > > 3 files changed, 14 insertions(+), 23 deletions(-) > > > > = > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/= i915/i915_debugfs.c > > > > index 2a276c8..b2b1730 100644 > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > > @@ -2113,7 +2113,7 @@ i915_max_freq_set(void *data, u64 val) > > > > if (IS_VALLEYVIEW(dev)) { > > > > val =3D vlv_freq_opcode(dev_priv->mem_freq, val); > > > > dev_priv->rps.max_delay =3D val; > > > > - gen6_set_rps(dev, val); > > > > + valleyview_set_rps(dev, val); > > > = > > > Not caused by your patch, but why on earth are we telling the GPU > > > to switch to the new max_freq here? > > > = > > > In the old way of doing things I presume this should have been > > > set_rps(cur_delay). And in the new way we should add the = > > > same 'cur_delay > val' check here that we have in i915_sysfs. > > > = > > > Maybe we should just have some kind of > > > rps_set_minmax(new_min, new_max) func that takes care of > > > this stuff in a single location. > > = > > We might as well just rip out the debugfs interfaces now that we have a= ll > > this stuff in sysfs. > = > Actually, the debugfs can serve a purpose for giving us hw values > instead of our bookkeeping values (which is what sysfs should provide). > So lose the ability to write values, but keep the low level reads > intact. Yeah, that sounds useful. Maybe shovel it all into the aggregate info file we already have in debugfs. -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch