| Message ID | 1432315042-31857-2-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 22, 2015 at 08:17:22PM +0300, Mika Kuoppala wrote: > We initialize the internal read pointer to zero on init/reset, > but only the reset will actually zero the write pointer. > This means that on module reload we might re-read context > status buffers that were written prior reload. > > It is safest just to let the hardware keep track of the read pointer, > so that we get valid value as there is no driver state involved. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- > drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > 3 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index fece922..b079dd3 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void *data) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_engine_cs *ring; > u32 status_pointer; > - u8 read_pointer; > - u8 write_pointer; > + u32 read_pointer; > + u32 write_pointer; I don't follow this change. It seems like this is to avoid having to explicitly widen the type when converting the 32bit register value later on? Otherwise I agree very much with the sentiment of the patch! -Chris
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > Chris Wilson > Sent: Saturday, May 23, 2015 5:11 PM > To: Mika Kuoppala > Cc: intel-gfx@lists.freedesktop.org; miku@iki.fi > Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Let hardware keep track of ctx > buf read pointer > > On Fri, May 22, 2015 at 08:17:22PM +0300, Mika Kuoppala wrote: > > We initialize the internal read pointer to zero on init/reset, > > but only the reset will actually zero the write pointer. > > This means that on module reload we might re-read context > > status buffers that were written prior reload. > > > > It is safest just to let the hardware keep track of the read pointer, > > so that we get valid value as there is no driver state involved. > > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- > > drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > > 3 files changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > > index fece922..b079dd3 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void > *data) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_engine_cs *ring; > > u32 status_pointer; > > - u8 read_pointer; > > - u8 write_pointer; > > + u32 read_pointer; > > + u32 write_pointer; > > I don't follow this change. It seems like this is to avoid having to > explicitly widen the type when converting the 32bit register value later > on? > > Otherwise I agree very much with the sentiment of the patch! > -Chris Do we still need to keep the sw pointer as a WA for this register field not being restored during power context restore on certain BDW GT3? Thomas.
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Fri, May 22, 2015 at 08:17:22PM +0300, Mika Kuoppala wrote: >> We initialize the internal read pointer to zero on init/reset, >> but only the reset will actually zero the write pointer. >> This means that on module reload we might re-read context >> status buffers that were written prior reload. >> >> It is safest just to let the hardware keep track of the read pointer, >> so that we get valid value as there is no driver state involved. >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- >> drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- >> 3 files changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index fece922..b079dd3 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void *data) >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_engine_cs *ring; >> u32 status_pointer; >> - u8 read_pointer; >> - u8 write_pointer; >> + u32 read_pointer; >> + u32 write_pointer; > > I don't follow this change. It seems like this is to avoid having to > explicitly widen the type when converting the 32bit register value later > on? > Yes that is the sole purpose. I don't see any drawback. -Mika > Otherwise I agree very much with the sentiment of the patch! > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Jun 08, 2015 at 12:12:41PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Fri, May 22, 2015 at 08:17:22PM +0300, Mika Kuoppala wrote: > >> We initialize the internal read pointer to zero on init/reset, > >> but only the reset will actually zero the write pointer. > >> This means that on module reload we might re-read context > >> status buffers that were written prior reload. > >> > >> It is safest just to let the hardware keep track of the read pointer, > >> so that we get valid value as there is no driver state involved. > >> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- > >> drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > >> 3 files changed, 8 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index fece922..b079dd3 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void *data) > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct intel_engine_cs *ring; > >> u32 status_pointer; > >> - u8 read_pointer; > >> - u8 write_pointer; > >> + u32 read_pointer; > >> + u32 write_pointer; > > > > I don't follow this change. It seems like this is to avoid having to > > explicitly widen the type when converting the 32bit register value later > > on? > > > > Yes that is the sole purpose. I don't see any drawback. You could also complain that it is an index and not a pointer. :) -Chris
On Mon, Jun 08, 2015 at 12:12:41PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Fri, May 22, 2015 at 08:17:22PM +0300, Mika Kuoppala wrote: > >> We initialize the internal read pointer to zero on init/reset, > >> but only the reset will actually zero the write pointer. > >> This means that on module reload we might re-read context > >> status buffers that were written prior reload. > >> > >> It is safest just to let the hardware keep track of the read pointer, > >> so that we get valid value as there is no driver state involved. > >> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- > >> drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > >> 3 files changed, 8 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index fece922..b079dd3 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void *data) > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct intel_engine_cs *ring; > >> u32 status_pointer; > >> - u8 read_pointer; > >> - u8 write_pointer; > >> + u32 read_pointer; > >> + u32 write_pointer; > > > > I don't follow this change. It seems like this is to avoid having to > > explicitly widen the type when converting the 32bit register value later > > on? > > > > Yes that is the sole purpose. I don't see any drawback. C likes to promote most things to int anyway, so I'm not sure what this widening is all about.
On Mon, Jun 08, 2015 at 01:15:39PM +0300, Ville Syrjälä wrote: > On Mon, Jun 08, 2015 at 12:12:41PM +0300, Mika Kuoppala wrote: > > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > > > On Fri, May 22, 2015 at 08:17:22PM +0300, Mika Kuoppala wrote: > > >> We initialize the internal read pointer to zero on init/reset, > > >> but only the reset will actually zero the write pointer. > > >> This means that on module reload we might re-read context > > >> status buffers that were written prior reload. > > >> > > >> It is safest just to let the hardware keep track of the read pointer, > > >> so that we get valid value as there is no driver state involved. > > >> > > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > >> --- > > >> drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- > > >> drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- > > >> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- > > >> 3 files changed, 8 insertions(+), 11 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > >> index fece922..b079dd3 100644 > > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > >> @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void *data) > > >> struct drm_i915_private *dev_priv = dev->dev_private; > > >> struct intel_engine_cs *ring; > > >> u32 status_pointer; > > >> - u8 read_pointer; > > >> - u8 write_pointer; > > >> + u32 read_pointer; > > >> + u32 write_pointer; > > > > > > I don't follow this change. It seems like this is to avoid having to > > > explicitly widen the type when converting the 32bit register value later > > > on? > > > > > > > Yes that is the sole purpose. I don't see any drawback. > > C likes to promote most things to int anyway, so I'm not sure what this > widening is all about. Hmm, I anticapted a warning about doing a shift wider than the available type, but it does do the int promotion implicitly. The warning you get if you try hard enough is tmp.c:7:5: warning: conversion to ‘uint8_t {aka unsigned char}’ from ‘int’ may alter its value [-Wconversion] u8 <<= 8; So here it definitely should be cleanest code wins. -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fece922..b079dd3 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2026,8 +2026,8 @@ static int i915_execlists(struct seq_file *m, void *data) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_engine_cs *ring; u32 status_pointer; - u8 read_pointer; - u8 write_pointer; + u32 read_pointer; + u32 write_pointer; u32 status; u32 ctx_id; struct list_head *cursor; @@ -2060,7 +2060,7 @@ static int i915_execlists(struct seq_file *m, void *data) status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring)); seq_printf(m, "\tStatus pointer: 0x%08X\n", status_pointer); - read_pointer = ring->next_context_status_buffer; + read_pointer = (status_pointer >> 8) & 0x07; write_pointer = status_pointer & 0x07; if (read_pointer > write_pointer) write_pointer += 6; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 14dec0d..f03e81f 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -494,15 +494,15 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) { struct drm_i915_private *dev_priv = ring->dev->dev_private; u32 status_pointer; - u8 read_pointer; - u8 write_pointer; + u32 read_pointer; + u32 write_pointer; u32 status; u32 status_id; u32 submit_contexts = 0; status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring)); - read_pointer = ring->next_context_status_buffer; + read_pointer = (status_pointer >> 8) & 0x07; write_pointer = status_pointer & 0x07; if (read_pointer > write_pointer) write_pointer += 6; @@ -537,11 +537,9 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) spin_unlock(&ring->execlist_lock); WARN(submit_contexts > 2, "More than two context complete events?\n"); - ring->next_context_status_buffer = write_pointer % 6; I915_WRITE(RING_CONTEXT_STATUS_PTR(ring), - _MASKED_FIELD(0x07 << 8, - ((u32)ring->next_context_status_buffer & 0x07) << 8)); + _MASKED_FIELD(0x07 << 8, (write_pointer % 6) << 8)); } static int execlists_context_queue(struct intel_engine_cs *ring, @@ -1090,7 +1088,6 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring) _MASKED_BIT_DISABLE(GFX_REPLAY_MODE) | _MASKED_BIT_ENABLE(GFX_RUN_LIST_ENABLE)); POSTING_READ(RING_MODE_GEN7(ring)); - ring->next_context_status_buffer = 0; DRM_DEBUG_DRIVER("Execlists enabled for %s\n", ring->name); memset(&ring->hangcheck, 0, sizeof(ring->hangcheck)); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 39f6dfc..400d69f 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -240,7 +240,7 @@ struct intel_engine_cs { spinlock_t execlist_lock; struct list_head execlist_queue; struct list_head execlist_retired_req_list; - u8 next_context_status_buffer; + u32 irq_keep_mask; /* bitmask for interrupts that should not be masked */ int (*emit_request)(struct intel_ringbuffer *ringbuf, struct drm_i915_gem_request *request);
We initialize the internal read pointer to zero on init/reset, but only the reset will actually zero the write pointer. This means that on module reload we might re-read context status buffers that were written prior reload. It is safest just to let the hardware keep track of the read pointer, so that we get valid value as there is no driver state involved. Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- drivers/gpu/drm/i915/intel_lrc.c | 11 ++++------- drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-)