diff mbox

[1/5] drm/i915: Cleanup some of the CSB handling

Message ID 1452018609-10142-2-git-send-email-benjamin.widawsky@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky Jan. 5, 2016, 6:30 p.m. UTC
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(-)

Comments

Michel Thierry Jan. 6, 2016, 3:08 p.m. UTC | #1
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 mbox

Patch

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);