[PATCH] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[PATCH] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Michel Thierry]

A previous commit resets the Context Status Buffer (CSB) read pointer in
ring init
    commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")

This is generally correct, but this pointer is not reset after
suspend/resume in some platforms (cht). In this case, the driver should
read the register value instead of resetting the sw read counter to 0.
Otherwise we process old events, leading to unwanted pre-emptions or
something worse.

But in other platforms (bdw) and also during GPU reset or power up, the
CSBWP is reset to 0x7 (an invalid number), and in this case the read
pointer should be set to 0.

Signed-off-by: Lei Shen <lei.shen@intel.com>
Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff9a481..dd87812 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1479,6 +1479,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 next_context_status_buffer_hw;
 
 	lrc_setup_hardware_status_page(ring,
 				ring->default_context->engine[ring->id].state);
@@ -1496,7 +1497,28 @@ 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;
+
+	/*
+	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
+	 * zero, we need to read the write pointer from hardware and use its
+	 * value because "this register is power context save restored".
+	 * Effectively, these states have been observed:
+	 *
+	 *      | 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   |
+	 */
+	next_context_status_buffer_hw = I915_READ(RING_CONTEXT_STATUS_PTR(ring)) & 0x07;
+
+	/*
+	 * When the CSB registers are reset (also after power-up / gpu reset),
+	 * CSB write pointer is set to all 1's, which is not valid, use 0 in
+	 * this special case.
+	 */
+	if (next_context_status_buffer_hw == 0x7)
+		next_context_status_buffer_hw = 0;
+
+	ring->next_context_status_buffer = next_context_status_buffer_hw;
 	DRM_DEBUG_DRIVER("Execlists enabled for %s\n", ring->name);
 
 	memset(&ring->hangcheck, 0, sizeof(ring->hangcheck));
-- 
2.5.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Mika Kuoppala]

Michel Thierry <michel.thierry@intel.com> writes:

> A previous commit resets the Context Status Buffer (CSB) read pointer in
> ring init
>     commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")
>
> This is generally correct, but this pointer is not reset after
> suspend/resume in some platforms (cht). In this case, the driver should
> read the register value instead of resetting the sw read counter to 0.
> Otherwise we process old events, leading to unwanted pre-emptions or
> something worse.
>
> But in other platforms (bdw) and also during GPU reset or power up, the
> CSBWP is reset to 0x7 (an invalid number), and in this case the read
> pointer should be set to 0.
>
> Signed-off-by: Lei Shen <lei.shen@intel.com>
> Signed-off-by: Deepak S <deepak.s@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ff9a481..dd87812 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1479,6 +1479,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u8 next_context_status_buffer_hw;
>  
>  	lrc_setup_hardware_status_page(ring,
>  				ring->default_context->engine[ring->id].state);
> @@ -1496,7 +1497,28 @@ 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;
> +
> +	/*
> +	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
> +	 * zero, we need to read the write pointer from hardware and use its
> +	 * value because "this register is power context save restored".
> +	 * Effectively, these states have been observed:
> +	 *
> +	 *      | 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   |
> +	 */
> +	next_context_status_buffer_hw = I915_READ(RING_CONTEXT_STATUS_PTR(ring)) & 0x07;
> +
> +	/*
> +	 * When the CSB registers are reset (also after power-up / gpu reset),
> +	 * CSB write pointer is set to all 1's, which is not valid, use 0 in
> +	 * this special case.
> +	 */
> +	if (next_context_status_buffer_hw == 0x7)
> +		next_context_status_buffer_hw = 0;


If hardware has been reset and we have b111 here, I assume
the first write will be at index zero.

If we look at the interrupt code there is while (read_pointer <
write_pointer). Initializing next status buffer to zero
would mean that you miss the first write to csb[0].

If b111 is found, I think the correct value is 5.

-Mika


> +
> +	ring->next_context_status_buffer = next_context_status_buffer_hw;
>  	DRM_DEBUG_DRIVER("Execlists enabled for %s\n", ring->name);
>  
>  	memset(&ring->hangcheck, 0, sizeof(ring->hangcheck));
> -- 
> 2.5.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Michel Thierry]

On 9/25/2015 4:44 PM, Mika Kuoppala wrote:
> Michel Thierry <michel.thierry@intel.com> writes:
>> -	ring->next_context_status_buffer = 0;
>> +
>> +	/*
>> +	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
>> +	 * zero, we need to read the write pointer from hardware and use its
>> +	 * value because "this register is power context save restored".
>> +	 * Effectively, these states have been observed:
>> +	 *
>> +	 *      | 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   |
>> +	 */
>> +	next_context_status_buffer_hw = I915_READ(RING_CONTEXT_STATUS_PTR(ring)) & 0x07;
>> +
>> +	/*
>> +	 * When the CSB registers are reset (also after power-up / gpu reset),
>> +	 * CSB write pointer is set to all 1's, which is not valid, use 0 in
>> +	 * this special case.
>> +	 */
>> +	if (next_context_status_buffer_hw == 0x7)
>> +		next_context_status_buffer_hw = 0;
>
>
> If hardware has been reset and we have b111 here, I assume
> the first write will be at index zero.
>
> If we look at the interrupt code there is while (read_pointer <
> write_pointer). Initializing next status buffer to zero
> would mean that you miss the first write to csb[0].
>
> If b111 is found, I think the correct value is 5.

Correct, it needs to be set to 5. Luckily, csb[0] would always be 
Idle-to-Active, which we ignore.

>
> -Mika
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Michel Thierry]

A previous commit resets the Context Status Buffer (CSB) read pointer in
ring init
    commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")

This is generally correct, but this pointer is not reset after
suspend/resume in some platforms (cht). In this case, the driver should
read the register value instead of resetting the sw read counter to 0.
Otherwise we process old events, leading to unwanted pre-emptions or
something worse.

But in other platforms (bdw) and also during GPU reset or power up, the
CSBWP is reset to 0x7 (an invalid number), and in this case the read
pointer should be set to 5 (the interrupt code will increment this
counter one more time, and will start reading from CSB[0]).

v2: When the CSB registers are reset, the read pointer needs to be set
to 5, otherwise the first write (CSB[0]) won't be read (Mika).
Replace magic numbers with GEN8_CSB_ENTRIES (6) and GEN8_CSB_PTR_MASK
(0x07).

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Lei Shen <lei.shen@intel.com>
Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_lrc.h |  2 ++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff9a481..ee25731 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -511,18 +511,18 @@ 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 & 0x07;
+	write_pointer = status_pointer & GEN8_CSB_PTR_MASK;
 	if (read_pointer > write_pointer)
-		write_pointer += 6;
+		write_pointer += GEN8_CSB_ENTRIES;
 
 	spin_lock(&ring->execlist_lock);
 
 	while (read_pointer < write_pointer) {
 		read_pointer++;
 		status = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
-				(read_pointer % 6) * 8);
+				(read_pointer % GEN8_CSB_ENTRIES) * 8);
 		status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
-				(read_pointer % 6) * 8 + 4);
+				(read_pointer % GEN8_CSB_ENTRIES) * 8 + 4);
 
 		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
 			continue;
@@ -554,10 +554,12 @@ 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;
+	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
 
 	I915_WRITE(RING_CONTEXT_STATUS_PTR(ring),
-		   _MASKED_FIELD(0x07 << 8, ((u32)ring->next_context_status_buffer & 0x07) << 8));
+		   _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8,
+				 ((u32)ring->next_context_status_buffer &
+				  GEN8_CSB_PTR_MASK) << 8));
 }
 
 static int execlists_context_queue(struct drm_i915_gem_request *request)
@@ -1479,6 +1481,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 next_context_status_buffer_hw;
 
 	lrc_setup_hardware_status_page(ring,
 				ring->default_context->engine[ring->id].state);
@@ -1496,7 +1499,29 @@ 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;
+
+	/*
+	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
+	 * zero, we need to read the write pointer from hardware and use its
+	 * value because "this register is power context save restored".
+	 * Effectively, these states have been observed:
+	 *
+	 *      | 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   |
+	 */
+	next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring))
+						   & GEN8_CSB_PTR_MASK);
+
+	/*
+	 * When the CSB registers are reset (also after power-up / gpu reset),
+	 * CSB write pointer is set to all 1's, which is not valid, use '5' in
+	 * this special case, so the first element read is CSB[0].
+	 */
+	if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK)
+		next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1);
+
+	ring->next_context_status_buffer = next_context_status_buffer_hw;
 	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_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 69d99f0..20fefb7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -25,6 +25,8 @@
 #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)			((ring)->mmio_base+0x230)
-- 
2.5.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Michel Thierry]

A previous commit resets the Context Status Buffer (CSB) read pointer in
ring init
    commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")

This is generally correct, but this pointer is not reset after
suspend/resume in some platforms (cht). In this case, the driver should
read the register value instead of resetting the sw read counter to 0.
Otherwise we process old events, leading to unwanted pre-emptions or
something worse.

But in other platforms (bdw) and also during GPU reset or power up, the
CSBWP is reset to 0x7 (an invalid number), and in this case the read
pointer should be set to 5 (the interrupt code will increment this
counter one more time, and will start reading from CSB[0]).

v2: When the CSB registers are reset, the read pointer needs to be set
to 5, otherwise the first write (CSB[0]) won't be read (Mika).
Replace magic numbers with GEN8_CSB_ENTRIES (6) and GEN8_CSB_PTR_MASK
(0x07).
v3: Rebased on top of "Parametrize LRC registers" patch.

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Lei Shen <lei.shen@intel.com>
Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_lrc.h |  2 ++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 256167b..825fa7a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -511,16 +511,16 @@ 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 & 0x07;
+	write_pointer = status_pointer & GEN8_CSB_PTR_MASK;
 	if (read_pointer > write_pointer)
-		write_pointer += 6;
+		write_pointer += GEN8_CSB_ENTRIES;
 
 	spin_lock(&ring->execlist_lock);
 
 	while (read_pointer < write_pointer) {
 		read_pointer++;
-		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % 6));
-		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % 6));
+		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % GEN8_CSB_ENTRIES));
+		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % GEN8_CSB_ENTRIES));
 
 		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
 			continue;
@@ -552,10 +552,12 @@ 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;
+	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
 
 	I915_WRITE(RING_CONTEXT_STATUS_PTR(ring),
-		   _MASKED_FIELD(0x07 << 8, ((u32)ring->next_context_status_buffer & 0x07) << 8));
+		   _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8,
+				 ((u32)ring->next_context_status_buffer &
+				  GEN8_CSB_PTR_MASK) << 8));
 }
 
 static int execlists_context_queue(struct drm_i915_gem_request *request)
@@ -1477,6 +1479,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 next_context_status_buffer_hw;
 
 	lrc_setup_hardware_status_page(ring,
 				ring->default_context->engine[ring->id].state);
@@ -1494,7 +1497,29 @@ 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;
+
+	/*
+	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
+	 * zero, we need to read the write pointer from hardware and use its
+	 * value because "this register is power context save restored".
+	 * Effectively, these states have been observed:
+	 *
+	 *      | 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   |
+	 */
+	next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring))
+						   & GEN8_CSB_PTR_MASK);
+
+	/*
+	 * When the CSB registers are reset (also after power-up / gpu reset),
+	 * CSB write pointer is set to all 1's, which is not valid, use '5' in
+	 * this special case, so the first element read is CSB[0].
+	 */
+	if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK)
+		next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1);
+
+	ring->next_context_status_buffer = next_context_status_buffer_hw;
 	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_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 8a08a27..4e60d54 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -25,6 +25,8 @@
 #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)			((ring)->mmio_base+0x230)
-- 
2.5.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Michel Thierry]

A previous commit resets the Context Status Buffer (CSB) read pointer in
ring init
    commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")

This is generally correct, but this pointer is not reset after
suspend/resume in some platforms (cht). In this case, the driver should
read the register value instead of resetting the sw read counter to 0.
Otherwise we process old events, leading to unwanted pre-emptions or
something worse.

But in other platforms (bdw) and also during GPU reset or power up, the
CSBWP is reset to 0x7 (an invalid number), and in this case the read
pointer should be set to 5 (the interrupt code will increment this
counter one more time, and will start reading from CSB[0]).

v2: When the CSB registers are reset, the read pointer needs to be set
to 5, otherwise the first write (CSB[0]) won't be read (Mika).
Replace magic numbers with GEN8_CSB_ENTRIES (6) and GEN8_CSB_PTR_MASK
(0x07).

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Lei Shen <lei.shen@intel.com>
Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_lrc.h |  2 ++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ff9a481..ee25731 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -511,18 +511,18 @@ 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 & 0x07;
+	write_pointer = status_pointer & GEN8_CSB_PTR_MASK;
 	if (read_pointer > write_pointer)
-		write_pointer += 6;
+		write_pointer += GEN8_CSB_ENTRIES;
 
 	spin_lock(&ring->execlist_lock);
 
 	while (read_pointer < write_pointer) {
 		read_pointer++;
 		status = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
-				(read_pointer % 6) * 8);
+				(read_pointer % GEN8_CSB_ENTRIES) * 8);
 		status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
-				(read_pointer % 6) * 8 + 4);
+				(read_pointer % GEN8_CSB_ENTRIES) * 8 + 4);
 
 		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
 			continue;
@@ -554,10 +554,12 @@ 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;
+	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
 
 	I915_WRITE(RING_CONTEXT_STATUS_PTR(ring),
-		   _MASKED_FIELD(0x07 << 8, ((u32)ring->next_context_status_buffer & 0x07) << 8));
+		   _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8,
+				 ((u32)ring->next_context_status_buffer &
+				  GEN8_CSB_PTR_MASK) << 8));
 }
 
 static int execlists_context_queue(struct drm_i915_gem_request *request)
@@ -1479,6 +1481,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u8 next_context_status_buffer_hw;
 
 	lrc_setup_hardware_status_page(ring,
 				ring->default_context->engine[ring->id].state);
@@ -1496,7 +1499,29 @@ 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;
+
+	/*
+	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
+	 * zero, we need to read the write pointer from hardware and use its
+	 * value because "this register is power context save restored".
+	 * Effectively, these states have been observed:
+	 *
+	 *      | 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   |
+	 */
+	next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring))
+						   & GEN8_CSB_PTR_MASK);
+
+	/*
+	 * When the CSB registers are reset (also after power-up / gpu reset),
+	 * CSB write pointer is set to all 1's, which is not valid, use '5' in
+	 * this special case, so the first element read is CSB[0].
+	 */
+	if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK)
+		next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1);
+
+	ring->next_context_status_buffer = next_context_status_buffer_hw;
 	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_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 69d99f0..20fefb7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -25,6 +25,8 @@
 #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)			((ring)->mmio_base+0x230)
-- 
2.5.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Mika Kuoppala]

Michel Thierry <michel.thierry@intel.com> writes:

> A previous commit resets the Context Status Buffer (CSB) read pointer in
> ring init
>     commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")
>
> This is generally correct, but this pointer is not reset after
> suspend/resume in some platforms (cht). In this case, the driver should
> read the register value instead of resetting the sw read counter to 0.
> Otherwise we process old events, leading to unwanted pre-emptions or
> something worse.
>
> But in other platforms (bdw) and also during GPU reset or power up, the
> CSBWP is reset to 0x7 (an invalid number), and in this case the read
> pointer should be set to 5 (the interrupt code will increment this
> counter one more time, and will start reading from CSB[0]).
>
> v2: When the CSB registers are reset, the read pointer needs to be set
> to 5, otherwise the first write (CSB[0]) won't be read (Mika).
> Replace magic numbers with GEN8_CSB_ENTRIES (6) and GEN8_CSB_PTR_MASK
> (0x07).
> v3: Rebased on top of "Parametrize LRC registers" patch.
>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Signed-off-by: Lei Shen <lei.shen@intel.com>
> Signed-off-by: Deepak S <deepak.s@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_lrc.h |  2 ++
>  2 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 256167b..825fa7a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -511,16 +511,16 @@ 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 & 0x07;
> +	write_pointer = status_pointer & GEN8_CSB_PTR_MASK;
>  	if (read_pointer > write_pointer)
> -		write_pointer += 6;
> +		write_pointer += GEN8_CSB_ENTRIES;
>  
>  	spin_lock(&ring->execlist_lock);
>  
>  	while (read_pointer < write_pointer) {
>  		read_pointer++;
> -		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % 6));
> -		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % 6));
> +		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % GEN8_CSB_ENTRIES));
> +		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % GEN8_CSB_ENTRIES));
>  
>  		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
>  			continue;
> @@ -552,10 +552,12 @@ 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;
> +	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
>  
>  	I915_WRITE(RING_CONTEXT_STATUS_PTR(ring),
> -		   _MASKED_FIELD(0x07 << 8, ((u32)ring->next_context_status_buffer & 0x07) << 8));
> +		   _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8,
> +				 ((u32)ring->next_context_status_buffer &
> +				  GEN8_CSB_PTR_MASK) << 8));
>  }
>  
>  static int execlists_context_queue(struct drm_i915_gem_request *request)
> @@ -1477,6 +1479,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u8 next_context_status_buffer_hw;
>  
>  	lrc_setup_hardware_status_page(ring,
>  				ring->default_context->engine[ring->id].state);
> @@ -1494,7 +1497,29 @@ 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;
> +
> +	/*
> +	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
> +	 * zero, we need to read the write pointer from hardware and use its
> +	 * value because "this register is power context save restored".
> +	 * Effectively, these states have been observed:
> +	 *
> +	 *      | 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 acts similar to BDW.

> +	next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring))
> +						   & GEN8_CSB_PTR_MASK);
> +
> +	/*
> +	 * When the CSB registers are reset (also after power-up / gpu reset),
> +	 * CSB write pointer is set to all 1's, which is not valid, use '5' in
> +	 * this special case, so the first element read is CSB[0].
> +	 */
> +	if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK)
> +		next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1);
> +

Now that this has been unravelled, I hope we can move towards
of removing the driver state of next context buffer completely:

http://lists.freedesktop.org/archives/intel-gfx/2015-May/067313.html

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> +	ring->next_context_status_buffer = next_context_status_buffer_hw;
>  	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_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 8a08a27..4e60d54 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -25,6 +25,8 @@
>  #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)			((ring)->mmio_base+0x230)
> -- 
> 2.5.3



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Daniel Vetter]

On Mon, Sep 28, 2015 at 05:00:12PM +0300, Mika Kuoppala wrote:
> Michel Thierry <michel.thierry@intel.com> writes:
> 
> > A previous commit resets the Context Status Buffer (CSB) read pointer in
> > ring init
> >     commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")

This is in 4.0, so Cc: stable@vger.kernel.org and one for Jani.
-Daniel

> >
> > This is generally correct, but this pointer is not reset after
> > suspend/resume in some platforms (cht). In this case, the driver should
> > read the register value instead of resetting the sw read counter to 0.
> > Otherwise we process old events, leading to unwanted pre-emptions or
> > something worse.
> >
> > But in other platforms (bdw) and also during GPU reset or power up, the
> > CSBWP is reset to 0x7 (an invalid number), and in this case the read
> > pointer should be set to 5 (the interrupt code will increment this
> > counter one more time, and will start reading from CSB[0]).
> >
> > v2: When the CSB registers are reset, the read pointer needs to be set
> > to 5, otherwise the first write (CSB[0]) won't be read (Mika).
> > Replace magic numbers with GEN8_CSB_ENTRIES (6) and GEN8_CSB_PTR_MASK
> > (0x07).
> > v3: Rebased on top of "Parametrize LRC registers" patch.
> >
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Signed-off-by: Lei Shen <lei.shen@intel.com>
> > Signed-off-by: Deepak S <deepak.s@intel.com>
> > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_lrc.h |  2 ++
> >  2 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 256167b..825fa7a 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -511,16 +511,16 @@ 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 & 0x07;
> > +	write_pointer = status_pointer & GEN8_CSB_PTR_MASK;
> >  	if (read_pointer > write_pointer)
> > -		write_pointer += 6;
> > +		write_pointer += GEN8_CSB_ENTRIES;
> >  
> >  	spin_lock(&ring->execlist_lock);
> >  
> >  	while (read_pointer < write_pointer) {
> >  		read_pointer++;
> > -		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % 6));
> > -		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % 6));
> > +		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % GEN8_CSB_ENTRIES));
> > +		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % GEN8_CSB_ENTRIES));
> >  
> >  		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
> >  			continue;
> > @@ -552,10 +552,12 @@ 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;
> > +	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
> >  
> >  	I915_WRITE(RING_CONTEXT_STATUS_PTR(ring),
> > -		   _MASKED_FIELD(0x07 << 8, ((u32)ring->next_context_status_buffer & 0x07) << 8));
> > +		   _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8,
> > +				 ((u32)ring->next_context_status_buffer &
> > +				  GEN8_CSB_PTR_MASK) << 8));
> >  }
> >  
> >  static int execlists_context_queue(struct drm_i915_gem_request *request)
> > @@ -1477,6 +1479,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
> >  {
> >  	struct drm_device *dev = ring->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u8 next_context_status_buffer_hw;
> >  
> >  	lrc_setup_hardware_status_page(ring,
> >  				ring->default_context->engine[ring->id].state);
> > @@ -1494,7 +1497,29 @@ 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;
> > +
> > +	/*
> > +	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
> > +	 * zero, we need to read the write pointer from hardware and use its
> > +	 * value because "this register is power context save restored".
> > +	 * Effectively, these states have been observed:
> > +	 *
> > +	 *      | 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 acts similar to BDW.
> 
> > +	next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring))
> > +						   & GEN8_CSB_PTR_MASK);
> > +
> > +	/*
> > +	 * When the CSB registers are reset (also after power-up / gpu reset),
> > +	 * CSB write pointer is set to all 1's, which is not valid, use '5' in
> > +	 * this special case, so the first element read is CSB[0].
> > +	 */
> > +	if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK)
> > +		next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1);
> > +
> 
> Now that this has been unravelled, I hope we can move towards
> of removing the driver state of next context buffer completely:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-May/067313.html
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> > +	ring->next_context_status_buffer = next_context_status_buffer_hw;
> >  	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_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> > index 8a08a27..4e60d54 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.h
> > +++ b/drivers/gpu/drm/i915/intel_lrc.h
> > @@ -25,6 +25,8 @@
> >  #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)			((ring)->mmio_base+0x230)
> > -- 
> > 2.5.3
> 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Consider HW CSB write pointer before resetting the sw read pointer

[Jani Nikula]

On Mon, 28 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 28, 2015 at 05:00:12PM +0300, Mika Kuoppala wrote:
>> Michel Thierry <michel.thierry@intel.com> writes:
>> 
>> > A previous commit resets the Context Status Buffer (CSB) read pointer in
>> > ring init
>> >     commit c0a03a2e4c4e ("drm/i915: Reset CSB read pointer in ring init")
>
> This is in 4.0, so Cc: stable@vger.kernel.org and one for Jani.
> -Daniel
>
>> >
>> > This is generally correct, but this pointer is not reset after
>> > suspend/resume in some platforms (cht). In this case, the driver should
>> > read the register value instead of resetting the sw read counter to 0.
>> > Otherwise we process old events, leading to unwanted pre-emptions or
>> > something worse.
>> >
>> > But in other platforms (bdw) and also during GPU reset or power up, the
>> > CSBWP is reset to 0x7 (an invalid number), and in this case the read
>> > pointer should be set to 5 (the interrupt code will increment this
>> > counter one more time, and will start reading from CSB[0]).
>> >
>> > v2: When the CSB registers are reset, the read pointer needs to be set
>> > to 5, otherwise the first write (CSB[0]) won't be read (Mika).
>> > Replace magic numbers with GEN8_CSB_ENTRIES (6) and GEN8_CSB_PTR_MASK
>> > (0x07).
>> > v3: Rebased on top of "Parametrize LRC registers" patch.

Pushed v2 to drm-intel-fixes, as that branch doesn't have the
"Parametrize LRC registers" patch, thanks for the patch and review.

BR,
Jani.


>> >
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > Signed-off-by: Lei Shen <lei.shen@intel.com>
>> > Signed-off-by: Deepak S <deepak.s@intel.com>
>> > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++++++-------
>> >  drivers/gpu/drm/i915/intel_lrc.h |  2 ++
>> >  2 files changed, 34 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> > index 256167b..825fa7a 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> > @@ -511,16 +511,16 @@ 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 & 0x07;
>> > +	write_pointer = status_pointer & GEN8_CSB_PTR_MASK;
>> >  	if (read_pointer > write_pointer)
>> > -		write_pointer += 6;
>> > +		write_pointer += GEN8_CSB_ENTRIES;
>> >  
>> >  	spin_lock(&ring->execlist_lock);
>> >  
>> >  	while (read_pointer < write_pointer) {
>> >  		read_pointer++;
>> > -		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % 6));
>> > -		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % 6));
>> > +		status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer % GEN8_CSB_ENTRIES));
>> > +		status_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer % GEN8_CSB_ENTRIES));
>> >  
>> >  		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
>> >  			continue;
>> > @@ -552,10 +552,12 @@ 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;
>> > +	ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES;
>> >  
>> >  	I915_WRITE(RING_CONTEXT_STATUS_PTR(ring),
>> > -		   _MASKED_FIELD(0x07 << 8, ((u32)ring->next_context_status_buffer & 0x07) << 8));
>> > +		   _MASKED_FIELD(GEN8_CSB_PTR_MASK << 8,
>> > +				 ((u32)ring->next_context_status_buffer &
>> > +				  GEN8_CSB_PTR_MASK) << 8));
>> >  }
>> >  
>> >  static int execlists_context_queue(struct drm_i915_gem_request *request)
>> > @@ -1477,6 +1479,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>> >  {
>> >  	struct drm_device *dev = ring->dev;
>> >  	struct drm_i915_private *dev_priv = dev->dev_private;
>> > +	u8 next_context_status_buffer_hw;
>> >  
>> >  	lrc_setup_hardware_status_page(ring,
>> >  				ring->default_context->engine[ring->id].state);
>> > @@ -1494,7 +1497,29 @@ 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;
>> > +
>> > +	/*
>> > +	 * Instead of resetting the Context Status Buffer (CSB) read pointer to
>> > +	 * zero, we need to read the write pointer from hardware and use its
>> > +	 * value because "this register is power context save restored".
>> > +	 * Effectively, these states have been observed:
>> > +	 *
>> > +	 *      | 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 acts similar to BDW.
>> 
>> > +	next_context_status_buffer_hw = (I915_READ(RING_CONTEXT_STATUS_PTR(ring))
>> > +						   & GEN8_CSB_PTR_MASK);
>> > +
>> > +	/*
>> > +	 * When the CSB registers are reset (also after power-up / gpu reset),
>> > +	 * CSB write pointer is set to all 1's, which is not valid, use '5' in
>> > +	 * this special case, so the first element read is CSB[0].
>> > +	 */
>> > +	if (next_context_status_buffer_hw == GEN8_CSB_PTR_MASK)
>> > +		next_context_status_buffer_hw = (GEN8_CSB_ENTRIES - 1);
>> > +
>> 
>> Now that this has been unravelled, I hope we can move towards
>> of removing the driver state of next context buffer completely:
>> 
>> http://lists.freedesktop.org/archives/intel-gfx/2015-May/067313.html
>> 
>> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> 
>> > +	ring->next_context_status_buffer = next_context_status_buffer_hw;
>> >  	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_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
>> > index 8a08a27..4e60d54 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.h
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.h
>> > @@ -25,6 +25,8 @@
>> >  #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)			((ring)->mmio_base+0x230)
>> > -- 
>> > 2.5.3
>> 
>> 
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx