From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 1/7] drm/i915: create macros for the "unclaimed register" checks Date: Mon, 28 Jan 2013 12:17:35 -0800 Message-ID: <20130128201734.GA1923@lundgren.jf.intel.com> References: <1359147462-3902-1-git-send-email-przanoni@gmail.com> <1359147462-3902-2-git-send-email-przanoni@gmail.com> <20130125170201.2ccb6af9@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.chad-versace.us (209-20-75-48.static.cloud-ips.com [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id F2531E5C63 for ; Mon, 28 Jan 2013 12:16:14 -0800 (PST) Content-Disposition: inline In-Reply-To: <20130125170201.2ccb6af9@bwidawsk.net> 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: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, Jan 25, 2013 at 05:02:01PM -0800, Ben Widawsky wrote: > On Fri, 25 Jan 2013 18:57:36 -0200 > Paulo Zanoni wrote: > > > From: Paulo Zanoni > > > > This avoids polluting i915_write##x and also allows us to reuse code > > on i915_read##x. > > > > Signed-off-by: Paulo Zanoni Reviewed-by: Ben Widawsky > > --- > > drivers/gpu/drm/i915/i915_drv.c | 24 ++++++++++++++++-------- > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 9cc8f87..ae0a55a 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1224,6 +1224,20 @@ ilk_dummy_write(struct drm_i915_private *dev_priv) > > I915_WRITE_NOTRACE(MI_MODE, 0); > > } > > > > +#define UNCLAIMED_REG_CLEAR(dev_priv, reg) \ > > + if (IS_HASWELL(dev_priv->dev) && \ > > + (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \ > > + DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \ > > + I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \ > > + } > > + > > You don't really need the if here, do you? I think the way it worked > originally was fine. I misread this the first time around. Please ignore this comment. Apologies. > > > +#define UNCLAIMED_REG_CHECK(dev_priv, reg) \ > > + if (IS_HASWELL(dev_priv->dev) && \ > > + (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \ > > + DRM_ERROR("Unclaimed write to %x\n", reg); \ > > + writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT); \ > > + } > > + > > #define __i915_read(x, y) \ > > u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \ > > u##x val = 0; \ > > I suppose you could add some 'unlikely' predictions in another patch if > you wanted. I really don't know what the consensus is on using those > these day. > > > @@ -1262,10 +1276,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \ > > } \ > > if (IS_GEN5(dev_priv->dev)) \ > > ilk_dummy_write(dev_priv); \ > > - if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \ > > - DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \ > > - I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \ > > - } \ > > + UNCLAIMED_REG_CLEAR(dev_priv, reg); \ > > if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \ > > write##y(val, dev_priv->regs + reg + 0x180000); \ > > } else { \ > > Here's what I meant above, you replace 2 lines of code with 4 from the > macro. (Including an extra MMIO read). > I misread this the first time around. Please ignore this comment. Apologies. > > @@ -1274,10 +1285,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \ > > if (unlikely(__fifo_ret)) { \ > > gen6_gt_check_fifodbg(dev_priv); \ > > } \ > > - if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \ > > - DRM_ERROR("Unclaimed write to %x\n", reg); \ > > - writel(ERR_INT_MMIO_UNCLAIMED, dev_priv->regs + GEN7_ERR_INT); \ > > - } \ > > + UNCLAIMED_REG_CHECK(dev_priv, reg); \ > > } > > __i915_write(8, b) > > __i915_write(16, w) > > > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center