| Message ID | 1303812772-5370-1-git-send-email-boqun.feng@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Only, the first 2 chunks are required. The gen6+ paths are unaffected. And be careful of your whitespace. -Chris
On Tue, 26 Apr 2011 18:12:52 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote:
> Signed-off-by: Feng, Boqun <boqun.feng@intel.com>
Please add a description here of what the bug was and how this fixes it.
We need this fix to enable h264 decoding on G4x. And I will resend a patch, more clear and standard. -----Original Message----- From: Keith Packard [mailto:keithp@keithp.com] Sent: Wednesday, April 27, 2011 1:55 AM To: Feng, Boqun; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] fix interrupt request miss problem in bsd ring for g4x On Tue, 26 Apr 2011 18:12:52 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote: > Signed-off-by: Feng, Boqun <boqun.feng@intel.com> Please add a description here of what the bug was and how this fixes it.
I am very sorry for my careless about whitespace.
But my patch will not affect gen6+ paths, for gen6+, it use gen6_bsd_ring
, bsd_ring is only used by g4x and ironlake.
Besides, since bsd_ring_get_irq/bsd_ring_put_irq/ring_get_irq/ring_put_irq
are only used by bsd_ring, can we use a patch to merge them into two function?
-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
Sent: Tuesday, April 26, 2011 10:29 PM
To: Feng, Boqun; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] fix interrupt request miss problem in bsd ring for g4x
Only, the first 2 chunks are required. The gen6+ paths are unaffected.
And be careful of your whitespace.
-Chris
On Wed, 27 Apr 2011 14:08:57 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote: > I am very sorry for my careless about whitespace. > > But my patch will not affect gen6+ paths, for gen6+, it use gen6_bsd_ring > , bsd_ring is only used by g4x and ironlake. Reviewer error, sorry. Saw the gen6_* in the diff header as the function affected and believed it. > Besides, since bsd_ring_get_irq/bsd_ring_put_irq/ring_get_irq/ring_put_irq > are only used by bsd_ring, can we use a patch to merge them into two function? Yes, once upon a time they differed, now they are the same so please do merge them and give them a more useful name: g4x_ring_* so that there is a constant reminder that g4x also has a BSD ring and that the functions are not expected to be used with earlier chipsets. Daniel has done similar things for gen6 once we decided to drop the pre-production workarounds. Obviously that is a separate patch to the bug fix. Thanks, -Chris
Err...I just send another two patches before read this letter. : ) Ironlake and g4x share the same bsd_ring, so they share the same bsd_ring_put/get_irq functions of the ring. Given this, we can't just change the function name to g4x_ring_put/get_irq. If we do so, we need ironlake_ring_put/get_irq, too. So I just use a if-else in bsd_ring_*_irq to find out the version of the chipset and do different work. Is that OK? -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Wednesday, April 27, 2011 3:39 PM To: Feng, Boqun; intel-gfx@lists.freedesktop.org Subject: RE: [Intel-gfx] [PATCH] fix interrupt request miss problem in bsd ring for g4x On Wed, 27 Apr 2011 14:08:57 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote: > I am very sorry for my careless about whitespace. > > But my patch will not affect gen6+ paths, for gen6+, it use gen6_bsd_ring > , bsd_ring is only used by g4x and ironlake. Reviewer error, sorry. Saw the gen6_* in the diff header as the function affected and believed it. > Besides, since bsd_ring_get_irq/bsd_ring_put_irq/ring_get_irq/ring_put_irq > are only used by bsd_ring, can we use a patch to merge them into two function? Yes, once upon a time they differed, now they are the same so please do merge them and give them a more useful name: g4x_ring_* so that there is a constant reminder that g4x also has a BSD ring and that the functions are not expected to be used with earlier chipsets. Daniel has done similar things for gen6 once we decided to drop the pre-production workarounds. Obviously that is a separate patch to the bug fix. Thanks, -Chris
On Wed, 27 Apr 2011 16:23:00 +0800, "Feng, Boqun" <boqun.feng@intel.com> wrote: > Err...I just send another two patches before read this letter. : ) > > Ironlake and g4x share the same bsd_ring, so they share the same > bsd_ring_put/get_irq functions of the ring. Given this, we can't just > change the function name to g4x_ring_put/get_irq. If we do so, we > need ironlake_ring_put/get_irq, too. > So I just use a if-else in bsd_ring_*_irq to find out the version of the > chipset and do different work. > Is that OK? That's ok. We are slowly pushing the branches out to the initialisation so that the code paths each generation takes are a little clearer. At the moment, I'm more concerned about making sure our functions are consistently named and prefixed with the chipset they first work with. So we have: intel_ -> general functions, used by all i8xx_ -> gen2 i915_ -> gen3 (915/945) g33_, pineview_ -> gen3 (blk/pnv) # perhaps just g33 as pnv = g33 + mobile? i965_ -> gen4 (brw/crl) g4x_ -> gen4 (egl/ctg) ironlake_, sandybridge_, ivybridge_ -> etc So ironlake can call a g4x function, but never vice versa. [And I'd love to be able to bring the same level of consistency to our register names. Along with making sure that they correspond with public docs.] The current best practice for enablement is then to introduce new functions for new chipsets (copy and paste, then modify) ensure the hardware is working, then reduce until we have the minimal code with no regressions. -Chris
On Wed, Apr 27, 2011 at 11:15 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > At the moment, I'm more concerned about making sure our functions are > consistently named and prefixed with the chipset they first work with. > > So we have: > intel_ -> general functions, used by all > i8xx_ -> gen2 > i915_ -> gen3 (915/945) > g33_, pineview_ -> gen3 (blk/pnv) # perhaps just g33 as pnv = g33 + mobile? > i965_ -> gen4 (brw/crl) > g4x_ -> gen4 (egl/ctg) > ironlake_, sandybridge_, ivybridge_ -> etc > > So ironlake can call a g4x function, but never vice versa. Very-Much-Wanted-by: Daniel Vetter <daniel.vetter@ffwll.ch> Can you put that somewhere prominent in the sources (a new file naming_conventions.txt)? Perhaps with the guidelines I've snipped away ... -Daniel
On Wed, Apr 27, 2011 at 01:07:24PM +0200, Daniel Vetter wrote: > On Wed, Apr 27, 2011 at 11:15 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > At the moment, I'm more concerned about making sure our functions are > > consistently named and prefixed with the chipset they first work with. > > > > So we have: > > ?intel_ -> general functions, used by all > > ?i8xx_ -> gen2 > > ?i915_ -> gen3 (915/945) > > ?g33_, pineview_ -> gen3 (blk/pnv) # perhaps just g33 as pnv = g33 + mobile? > > ?i965_ -> gen4 (brw/crl) > > ?g4x_ -> gen4 (egl/ctg) > > ?ironlake_, sandybridge_, ivybridge_ -> etc > > > > So ironlake can call a g4x function, but never vice versa. > > Very-Much-Wanted-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Can you put that somewhere prominent in the sources (a new file > naming_conventions.txt)? > Perhaps with the guidelines I've snipped away ... > -Daniel Acked! Though I don't feel what you said is explicit enough, and therefore needs more clarity which Daniel asked for. ironlake should be able to call intel_*, and probably most g4x_*, but maybe it can't use some i965 functions, and certainly can't use many i8xx functions. Ben
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index e9e6f71..6606ca7 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -609,8 +609,12 @@ ring_get_irq(struct intel_ring_buffer *ring, u32 flag) return false; spin_lock(&ring->irq_lock); - if (ring->irq_refcount++ == 0) - ironlake_enable_irq(dev_priv, flag); + if (ring->irq_refcount++ == 0) { + if (HAS_PCH_SPLIT(dev)) + ironlake_enable_irq(dev_priv, flag); + else + i915_enable_irq(dev_priv, flag); + } spin_unlock(&ring->irq_lock); return true; @@ -623,8 +627,12 @@ ring_put_irq(struct intel_ring_buffer *ring, u32 flag) drm_i915_private_t *dev_priv = dev->dev_private; spin_lock(&ring->irq_lock); - if (--ring->irq_refcount == 0) - ironlake_disable_irq(dev_priv, flag); + if (--ring->irq_refcount == 0) { + if (HAS_PCH_SPLIT(dev)) + ironlake_disable_irq(dev_priv, flag); + else + i915_disable_irq(dev_priv, flag); + } spin_unlock(&ring->irq_lock); } @@ -666,12 +674,22 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag) static bool bsd_ring_get_irq(struct intel_ring_buffer *ring) { - return ring_get_irq(ring, GT_BSD_USER_INTERRUPT); + struct drm_device *dev = ring->dev; + + if (HAS_PCH_SPLIT(dev)) + return ring_get_irq(ring, GT_BSD_USER_INTERRUPT); + else + return ring_get_irq(ring, I915_BSD_USER_INTERRUPT); } static void bsd_ring_put_irq(struct intel_ring_buffer *ring) { - ring_put_irq(ring, GT_BSD_USER_INTERRUPT); + struct drm_device *dev = ring->dev; + + if (HAS_PCH_SPLIT(dev)) + ring_put_irq(ring, GT_BSD_USER_INTERRUPT); + else + ring_put_irq(ring, I915_BSD_USER_INTERRUPT); } static int
Signed-off-by: Feng, Boqun <boqun.feng@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 30 ++++++++++++++++++++++++------ 1 files changed, 24 insertions(+), 6 deletions(-)