From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file Date: Mon, 15 Jul 2013 16:04:09 -0300 Message-ID: References: <1373648907-28774-1-git-send-email-chris@chris-wilson.co.uk> <20130714194249.GA24025@bwidawsk.net> <20130714203749.GA31828@cantiga.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ob0-f177.google.com (mail-ob0-f177.google.com [209.85.214.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 3DA23E63F2 for ; Mon, 15 Jul 2013 12:04:10 -0700 (PDT) Received: by mail-ob0-f177.google.com with SMTP id ta17so14199270obb.36 for ; Mon, 15 Jul 2013 12:04:09 -0700 (PDT) In-Reply-To: <20130714203749.GA31828@cantiga.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 , Ben Widawsky , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 2013/7/14 Chris Wilson : > On Sun, Jul 14, 2013 at 12:42:49PM -0700, Ben Widawsky wrote: >> On Fri, Jul 12, 2013 at 06:08:22PM +0100, Chris Wilson wrote: >> > Currently, the register access code is split between i915_drv.c and >> > intel_pm.c. It only bares a superficial resemblance to the reset of the >> > powermanagement code, so move it all into its own file. This is to ease >> > further patches to enforce serialised register access. >> > >> > v2: Scan for random abuse of I915_WRITE_NOTRACE >> > v3: Take the opportunity to rename the GT functions as uncore. Uncore is >> > the term used by the hardware design (and bspec) for all functions >> > outside of the GPU (and CPU) cores in what is also known as the System >> > Agent. >> > >> > Signed-off-by: Chris Wilson >> >> git am complains about a missing newline at EOF, but I guess Daniel will >> fix it on merge. >> >> Just bikesheds: >> As before, intel_uncore_(clear|check)_errors seems silly to me. > > I still don't understand what you mean. They are just the code that > existed before, so what is silly in moving them since they depend upon > bypassing the common i915_write/read code? > >> Also if >> you extracted those as a separate patch to the gt funcs, you could have >> had just a simple file move + rename. And as you made me realize, I'm >> not horribly thrilled with the name uncore, I liked gt. For me, the >> distinction between _pm, and _uncore isn't really large enough, ie. many >> things in _pm are really uncore also (anything touching the punit/rps, >> etc). Also, since gt_funcs never really expanded, maybe just call it >> forcewake_funcs and be done with that (since uncore forcewake sounds >> weird to me). > > Looks like you made the distinction pretty clear in that paragraph > between gt and pm. The name change is mostly a whim because we no longer > refer to this as being the GT. > >> Final bikeshed, I would like to see the reset code in a separate file as >> well (included in this could be any GEM functions we have for reset >> only, and display as well). > > No. Look at the reset code, it is far too incestrous with the gt/uncore > mechanics to be anywhere else. And it didn't seem right to separate it > up. While testing the Package C8+ feature I'm implementing, one of the things I tried was "for i in $(seq 20); do glxgears & done". After I ran this command, when I tried to drag the glxgears windows around I got a hard machine hang. Then I applied this patch series and now the problem is gone :) Also, "git am" complains about white space errors on patch 1 :) Thanks for the fix! Paulo > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni