From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 2/6] drm/i915: Use a private interface for register access within GT Date: Sun, 14 Jul 2013 12:48:29 -0700 Message-ID: <20130714194828.GB24025@bwidawsk.net> References: <1373648907-28774-1-git-send-email-chris@chris-wilson.co.uk> <1373648907-28774-2-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 shiva.localdomain (unknown [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id B6075E5DCD for ; Sun, 14 Jul 2013 12:45:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1373648907-28774-2-git-send-email-chris@chris-wilson.co.uk> 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 Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jul 12, 2013 at 06:08:23PM +0100, Chris Wilson wrote: > The GT functions for enabling register access also need to occasionally > write to and read from registers. To avoid the potential recursion as we > modify the public interface to be stricter, introduce a private register > access API for the GT functions. > > v2: Rebase > v3: Rebase onto uncore > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/intel_uncore.c | 107 +++++++++++++++++++++--------------- > 1 file changed, 64 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 8c2f460..9d4063f 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -26,6 +26,19 @@ > > #define FORCEWAKE_ACK_TIMEOUT_MS 2 > > +#define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs + (reg__)) > +#define __raw_i915_write8(dev_priv__, reg__, val__) writeb(val__, (dev_priv__)->regs + (reg__)) > + > +#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__)) > +#define __raw_i915_write16(dev_priv__, reg__, val__) writew(val__, (dev_priv__)->regs + (reg__)) > + > +#define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__)) > +#define __raw_i915_write32(dev_priv__, reg__, val__) writel(val__, (dev_priv__)->regs + (reg__)) > + > +#define __raw_i915_read64(dev_priv__, reg__) readq((dev_priv__)->regs + (reg__)) > +#define __raw_i915_write64(dev_priv__, reg__, val__) writeq(val__, (dev_priv__)->regs + (reg__)) > + > + Still waiting on the __raw_posting_read. And now I'm thinking a "posted_write" would be cool, so we don't need to explicitly insert posting reads. We have a special case in this file though, where we need a posting read, but I'd be fine with the ugly (void) in just the corner cases. If I am not mistaken, I915_*_NOTRACE would no longer be used after this patch. Maybe removing those here would also help clarify what you're doing, and make merge easier (in case someone adds a new one before this gets merged). And yes, I know you change it in an upcoming patch. Otherwise, looks good to me: Reviewed-by: Ben Widawsky [snip] -- Ben Widawsky, Intel Open Source Technology Center