From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 18/24] drm/i915: unify ring irq refcounts (again) Date: Thu, 4 Jul 2013 22:52:36 +0200 Message-ID: <20130704205236.GN18285@phenom.ffwll.local> References: <1371037046-3732-1-git-send-email-daniel.vetter@ffwll.ch> <1371037046-3732-19-git-send-email-daniel.vetter@ffwll.ch> <20130628172432.GB20241@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f47.google.com (mail-ee0-f47.google.com [74.125.83.47]) by gabe.freedesktop.org (Postfix) with ESMTP id 5818BE5FA7 for ; Thu, 4 Jul 2013 13:52:38 -0700 (PDT) Received: by mail-ee0-f47.google.com with SMTP id e49so960730eek.20 for ; Thu, 04 Jul 2013 13:52:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130628172432.GB20241@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: Ben Widawsky Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 28, 2013 at 10:24:32AM -0700, Ben Widawsky wrote: > On Wed, Jun 12, 2013 at 01:37:20PM +0200, Daniel Vetter wrote: > > With the simplified locking there's no reason any more to keep the > > refcounts seperate. > > I guess my nitpick from previous patch is echoed here. The reason still > exists, the benefit just doesn't outweigh the complexity, and I think > we'd want separate enable_masks for pm/gt to make it worthwhile to use > separate refcounts. Imo it's clear enough that ring->irq_refcount is a refcount for the ring interrupt. I've added the lost comment how it is protected by the device irq_lock again as compensation. -Daniel > > Anyway, can you add a comment in the code about what irq_refcount does. > with that, it's > Reviewed-by: Ben Widawsky > > > > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++++++---------- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +---- > > 2 files changed, 11 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index a7c9934..b75e9d0 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -819,7 +819,7 @@ gen5_ring_get_irq(struct intel_ring_buffer *ring) > > return false; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (ring->irq_refcount.gt++ == 0) { > > + if (ring->irq_refcount++ == 0) { > > dev_priv->gt_irq_mask &= ~ring->irq_enable_mask; > > I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > POSTING_READ(GTIMR); > > @@ -837,7 +837,7 @@ gen5_ring_put_irq(struct intel_ring_buffer *ring) > > unsigned long flags; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (--ring->irq_refcount.gt == 0) { > > + if (--ring->irq_refcount == 0) { > > dev_priv->gt_irq_mask |= ring->irq_enable_mask; > > I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > > POSTING_READ(GTIMR); > > @@ -856,7 +856,7 @@ i9xx_ring_get_irq(struct intel_ring_buffer *ring) > > return false; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (ring->irq_refcount.gt++ == 0) { > > + if (ring->irq_refcount++ == 0) { > > dev_priv->irq_mask &= ~ring->irq_enable_mask; > > I915_WRITE(IMR, dev_priv->irq_mask); > > POSTING_READ(IMR); > > @@ -874,7 +874,7 @@ i9xx_ring_put_irq(struct intel_ring_buffer *ring) > > unsigned long flags; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (--ring->irq_refcount.gt == 0) { > > + if (--ring->irq_refcount == 0) { > > dev_priv->irq_mask |= ring->irq_enable_mask; > > I915_WRITE(IMR, dev_priv->irq_mask); > > POSTING_READ(IMR); > > @@ -893,7 +893,7 @@ i8xx_ring_get_irq(struct intel_ring_buffer *ring) > > return false; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (ring->irq_refcount.gt++ == 0) { > > + if (ring->irq_refcount++ == 0) { > > dev_priv->irq_mask &= ~ring->irq_enable_mask; > > I915_WRITE16(IMR, dev_priv->irq_mask); > > POSTING_READ16(IMR); > > @@ -911,7 +911,7 @@ i8xx_ring_put_irq(struct intel_ring_buffer *ring) > > unsigned long flags; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (--ring->irq_refcount.gt == 0) { > > + if (--ring->irq_refcount == 0) { > > dev_priv->irq_mask |= ring->irq_enable_mask; > > I915_WRITE16(IMR, dev_priv->irq_mask); > > POSTING_READ16(IMR); > > @@ -1004,7 +1004,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring) > > gen6_gt_force_wake_get(dev_priv); > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (ring->irq_refcount.gt++ == 0) { > > + if (ring->irq_refcount++ == 0) { > > if (HAS_L3_GPU_CACHE(dev) && ring->id == RCS) > > I915_WRITE_IMR(ring, > > ~(ring->irq_enable_mask | > > @@ -1028,7 +1028,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring) > > unsigned long flags; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (--ring->irq_refcount.gt == 0) { > > + if (--ring->irq_refcount == 0) { > > if (HAS_L3_GPU_CACHE(dev) && ring->id == RCS) > > I915_WRITE_IMR(ring, > > ~GT_RENDER_L3_PARITY_ERROR_INTERRUPT); > > @@ -1054,7 +1054,7 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring) > > return false; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (ring->irq_refcount.pm++ == 0) { > > + if (ring->irq_refcount++ == 0) { > > u32 pm_imr = I915_READ(GEN6_PMIMR); > > I915_WRITE_IMR(ring, ~ring->irq_enable_mask); > > I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask); > > @@ -1076,7 +1076,7 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring) > > return; > > > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > > - if (--ring->irq_refcount.pm == 0) { > > + if (--ring->irq_refcount == 0) { > > u32 pm_imr = I915_READ(GEN6_PMIMR); > > I915_WRITE_IMR(ring, ~0); > > I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask); > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > > index f960805..26e304c 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -74,10 +74,7 @@ struct intel_ring_buffer { > > */ > > u32 last_retired_head; > > > > - struct { > > - u32 gt; /* protected by dev_priv->irq_lock */ > > - u32 pm; /* protected by dev_priv->irq_lock */ > > - } irq_refcount; > > + unsigned irq_refcount; > > u32 irq_enable_mask; /* bitmask to enable ring interrupt */ > > u32 trace_irq_seqno; > > u32 sync_seqno[I915_NUM_RINGS-1]; > > -- > > 1.8.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ben Widawsky, Intel Open Source Technology Center -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch