| Message ID | 1452018609-10142-2-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/5/2016 6:30 PM, Ben Widawsky wrote: > I think this patch is a worthwhile cleanup even if it might look only marginally > useful. It gets more useful in upcoming patches and for handling of future GEN > platforms. > > The only non-mechanical part of this is the removal of the extra & operation on > the ring->next_context_status_buffer. This is safe because right above this, we > already did a modulus operation. > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- > drivers/gpu/drm/i915/intel_lrc.c | 15 +++++++++------ > drivers/gpu/drm/i915/intel_lrc.h | 18 ++++++++++++++++-- > 3 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 0fc38bb..3b05bd1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2092,13 +2092,13 @@ static int i915_execlists(struct seq_file *m, void *data) > seq_printf(m, "\tStatus pointer: 0x%08X\n", status_pointer); > > read_pointer = ring->next_context_status_buffer; > - write_pointer = status_pointer & 0x07; > + write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); > if (read_pointer > write_pointer) > - write_pointer += 6; > + write_pointer += GEN8_CSB_ENTRIES; > seq_printf(m, "\tRead pointer: 0x%08X, write pointer 0x%08X\n", > read_pointer, write_pointer); > > - for (i = 0; i < 6; i++) { > + for (i = 0; i < GEN8_CSB_ENTRIES; i++) { > status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, i)); > ctx_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, i)); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 8096c6a..7fb2035 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -516,7 +516,7 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) > status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring)); > > read_pointer = ring->next_context_status_buffer; > - write_pointer = status_pointer & GEN8_CSB_PTR_MASK; > + write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); > if (read_pointer > write_pointer) > write_pointer += GEN8_CSB_ENTRIES; > > @@ -559,10 +559,11 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) > WARN(submit_contexts > 2, "More than two context complete events?\n"); > ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES; > > + /* Update the read pointer to the old write pointer. Manual ringbuffer > + * management ftw </sarcasm> */ > I915_WRITE(RING_CONTEXT_STATUS_PTR(ring), > - _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8, > - ((u32)ring->next_context_status_buffer & > - GEN8_CSB_PTR_MASK) << 8)); > + _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, > + ring->next_context_status_buffer << 8)); > } > > static int execlists_context_queue(struct drm_i915_gem_request *request) > @@ -1506,9 +1507,11 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring) > * | Suspend-to-idle (freeze) | Suspend-to-RAM (mem) | > * BDW | CSB regs not reset | CSB regs reset | > * CHT | CSB regs not reset | CSB regs not reset | > + * SKL | ? | ? | > + * BXT | ? | ? | > */ > - next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring)) > - & GEN8_CSB_PTR_MASK); > + next_context_status_buffer_hw = > + GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(ring))); > > /* > * When the CSB registers are reset (also after power-up / gpu reset), > diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h > index ae90f86..de41ad6 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.h > +++ b/drivers/gpu/drm/i915/intel_lrc.h > @@ -25,8 +25,6 @@ > #define _INTEL_LRC_H_ > > #define GEN8_LR_CONTEXT_ALIGN 4096 > -#define GEN8_CSB_ENTRIES 6 > -#define GEN8_CSB_PTR_MASK 0x07 > > /* Execlists regs */ > #define RING_ELSP(ring) _MMIO((ring)->mmio_base + 0x230) > @@ -40,6 +38,22 @@ > #define RING_CONTEXT_STATUS_BUF_HI(ring, i) _MMIO((ring)->mmio_base + 0x370 + (i) * 8 + 4) > #define RING_CONTEXT_STATUS_PTR(ring) _MMIO((ring)->mmio_base + 0x3a0) > > +/* The docs specify that the write pointer wraps around after 5h, "After status > + * is written out to the last available status QW at offset 5h, this pointer > + * wraps to 0." > + * > + * Therefore, one must infer than even though there are 3 bits available, 6 and > + * 7 appear to be * reserved. I always see 7 (b111) after boot/reset, but before any execlist has been submitted. > + */ > +#define GEN8_CSB_ENTRIES 6 > +#define GEN8_CSB_PTR_MASK 0x7 > +#define GEN8_CSB_READ_PTR_MASK (GEN8_CSB_PTR_MASK << 8) > +#define GEN8_CSB_WRITE_PTR_MASK (GEN8_CSB_PTR_MASK << 0) > +#define GEN8_CSB_WRITE_PTR(csb_status) \ > + (((csb_status) & GEN8_CSB_WRITE_PTR_MASK) >> 0) > +#define GEN8_CSB_READ_PTR(csb_status) \ > + (((csb_status) & GEN8_CSB_READ_PTR_MASK) >> 8) > + > /* Logical Rings */ > int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request); > int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request); > -- > 2.6.4 Reviewed-by: Michel Thierry <michel.thierry@intel.com> > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 0fc38bb..3b05bd1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2092,13 +2092,13 @@ static int i915_execlists(struct seq_file *m, void *data) seq_printf(m, "\tStatus pointer: 0x%08X\n", status_pointer); read_pointer = ring->next_context_status_buffer; - write_pointer = status_pointer & 0x07; + write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); if (read_pointer > write_pointer) - write_pointer += 6; + write_pointer += GEN8_CSB_ENTRIES; seq_printf(m, "\tRead pointer: 0x%08X, write pointer 0x%08X\n", read_pointer, write_pointer); - for (i = 0; i < 6; i++) { + for (i = 0; i < GEN8_CSB_ENTRIES; i++) { status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, i)); ctx_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, i)); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 8096c6a..7fb2035 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -516,7 +516,7 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring)); read_pointer = ring->next_context_status_buffer; - write_pointer = status_pointer & GEN8_CSB_PTR_MASK; + write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); if (read_pointer > write_pointer) write_pointer += GEN8_CSB_ENTRIES; @@ -559,10 +559,11 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) WARN(submit_contexts > 2, "More than two context complete events?\n"); ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES; + /* Update the read pointer to the old write pointer. Manual ringbuffer + * management ftw </sarcasm> */ I915_WRITE(RING_CONTEXT_STATUS_PTR(ring), - _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8, - ((u32)ring->next_context_status_buffer & - GEN8_CSB_PTR_MASK) << 8)); + _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, + ring->next_context_status_buffer << 8)); } static int execlists_context_queue(struct drm_i915_gem_request *request) @@ -1506,9 +1507,11 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring) * | Suspend-to-idle (freeze) | Suspend-to-RAM (mem) | * BDW | CSB regs not reset | CSB regs reset | * CHT | CSB regs not reset | CSB regs not reset | + * SKL | ? | ? | + * BXT | ? | ? | */ - next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring)) - & GEN8_CSB_PTR_MASK); + next_context_status_buffer_hw = + GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(ring))); /* * When the CSB registers are reset (also after power-up / gpu reset), diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index ae90f86..de41ad6 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -25,8 +25,6 @@ #define _INTEL_LRC_H_ #define GEN8_LR_CONTEXT_ALIGN 4096 -#define GEN8_CSB_ENTRIES 6 -#define GEN8_CSB_PTR_MASK 0x07 /* Execlists regs */ #define RING_ELSP(ring) _MMIO((ring)->mmio_base + 0x230) @@ -40,6 +38,22 @@ #define RING_CONTEXT_STATUS_BUF_HI(ring, i) _MMIO((ring)->mmio_base + 0x370 + (i) * 8 + 4) #define RING_CONTEXT_STATUS_PTR(ring) _MMIO((ring)->mmio_base + 0x3a0) +/* The docs specify that the write pointer wraps around after 5h, "After status + * is written out to the last available status QW at offset 5h, this pointer + * wraps to 0." + * + * Therefore, one must infer than even though there are 3 bits available, 6 and + * 7 appear to be * reserved. + */ +#define GEN8_CSB_ENTRIES 6 +#define GEN8_CSB_PTR_MASK 0x7 +#define GEN8_CSB_READ_PTR_MASK (GEN8_CSB_PTR_MASK << 8) +#define GEN8_CSB_WRITE_PTR_MASK (GEN8_CSB_PTR_MASK << 0) +#define GEN8_CSB_WRITE_PTR(csb_status) \ + (((csb_status) & GEN8_CSB_WRITE_PTR_MASK) >> 0) +#define GEN8_CSB_READ_PTR(csb_status) \ + (((csb_status) & GEN8_CSB_READ_PTR_MASK) >> 8) + /* Logical Rings */ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request); int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
I think this patch is a worthwhile cleanup even if it might look only marginally useful. It gets more useful in upcoming patches and for handling of future GEN platforms. The only non-mechanical part of this is the removal of the extra & operation on the ring->next_context_status_buffer. This is safe because right above this, we already did a modulus operation. Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- drivers/gpu/drm/i915/intel_lrc.c | 15 +++++++++------ drivers/gpu/drm/i915/intel_lrc.h | 18 ++++++++++++++++-- 3 files changed, 28 insertions(+), 11 deletions(-)