[PATCH 0/3] Some INSTDONE updates

[PATCH 0/3] Some INSTDONE updates

[Ben Widawsky]

Looking at some IVB error states this was very confusing. Upon looking
at updated docs, it appears we've been doing the wrong thing all along.

They're not terribly well tested yet, but hopefully they're trivial
enough that I didn't mess them up (unlikely!).

Ben Widawsky (3):
  drm/i915: Extract reading INSTDONE
  drm/i915: Add new INSTDONE registers
  drm/i915: Use new INSTDONE registers

 drivers/gpu/drm/i915/i915_debugfs.c |  8 +++--
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++-
 drivers/gpu/drm/i915/i915_irq.c     | 72 ++++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/i915_reg.h     |  5 +++
 4 files changed, 60 insertions(+), 30 deletions(-)

-- 
1.7.11.5

[PATCH 1/3] drm/i915: Extract reading INSTDONE

[Ben Widawsky]

INSTDONE is used in many places, and it varies from generation to
generation. This provides a good reason for us to extract the logic to
read the relevant information.

The patch has no functional change. It's prep for some new stuff.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++++++++----------------
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0c37101..0bf2f92 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1071,6 +1071,20 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
 	return NULL;
 }
 
+static void i915_get_extra_instdone(struct drm_device *dev,
+				    uint32_t instdone[I915_NUM_INSTDONE_REG])
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	if (INTEL_INFO(dev)->gen < 4) {
+		instdone[0] = I915_READ(INSTDONE);
+		instdone[1] = 0;
+	} else {
+		instdone[0] = I915_READ(INSTDONE_I965);
+		instdone[1] = I915_READ(INSTDONE1);
+	}
+}
+
+
 static void i915_record_ring_state(struct drm_device *dev,
 				   struct drm_i915_error_state *error,
 				   struct intel_ring_buffer *ring)
@@ -1286,6 +1300,7 @@ void i915_destroy_error_state(struct drm_device *dev)
 static void i915_report_and_clear_eir(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t instdone[I915_NUM_INSTDONE_REG];
 	u32 eir = I915_READ(EIR);
 	int pipe;
 
@@ -1294,16 +1309,18 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 	pr_err("render error detected, EIR: 0x%08x\n", eir);
 
+	memset(instdone, 0, sizeof(instdone));
+	i915_get_extra_instdone(dev, instdone);
+
 	if (IS_G4X(dev)) {
 		if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
 			u32 ipeir = I915_READ(IPEIR_I965);
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n",
-			       I915_READ(INSTDONE_I965));
+			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
+			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1342,7 +1359,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
-			pr_err("  INSTDONE: 0x%08x\n", I915_READ(INSTDONE));
+			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
 			I915_WRITE(IPEIR, ipeir);
 			POSTING_READ(IPEIR);
@@ -1351,10 +1368,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n",
-			       I915_READ(INSTDONE_I965));
+			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
+			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1669,7 +1685,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 {
 	struct drm_device *dev = (struct drm_device *)data;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
+	uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
 	struct intel_ring_buffer *ring;
 	bool err = false, idle;
 	int i;
@@ -1697,25 +1713,20 @@ void i915_hangcheck_elapsed(unsigned long data)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen < 4) {
-		instdone = I915_READ(INSTDONE);
-		instdone1 = 0;
-	} else {
-		instdone = I915_READ(INSTDONE_I965);
-		instdone1 = I915_READ(INSTDONE1);
-	}
+	memset(instdone, 0, sizeof(instdone));
+	i915_get_extra_instdone(dev, instdone);
 
 	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
-	    dev_priv->last_instdone == instdone &&
-	    dev_priv->last_instdone1 == instdone1) {
+	    dev_priv->last_instdone == instdone[0] &&
+	    dev_priv->last_instdone1 == instdone[1]) {
 		if (i915_hangcheck_hung(dev))
 			return;
 	} else {
 		dev_priv->hangcheck_count = 0;
 
 		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
-		dev_priv->last_instdone = instdone;
-		dev_priv->last_instdone1 = instdone1;
+		dev_priv->last_instdone = instdone[0];
+		dev_priv->last_instdone1 = instdone[1];
 	}
 
 repeat:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2f7b688..948ede9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -485,6 +485,7 @@
 #define IPEIR_I965	0x02064
 #define IPEHR_I965	0x02068
 #define INSTDONE_I965	0x0206c
+#define I915_NUM_INSTDONE_REG	2
 #define RING_IPEIR(base)	((base)+0x64)
 #define RING_IPEHR(base)	((base)+0x68)
 #define RING_INSTDONE(base)	((base)+0x6c)
-- 
1.7.11.5

Re: [PATCH 1/3] drm/i915: Extract reading INSTDONE

[Jani Nikula]

On Thu, 16 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
> INSTDONE is used in many places, and it varies from generation to
> generation. This provides a good reason for us to extract the logic to
> read the relevant information.
>
> The patch has no functional change. It's prep for some new stuff.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 0c37101..0bf2f92 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1071,6 +1071,20 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
>  	return NULL;
>  }
>  
> +static void i915_get_extra_instdone(struct drm_device *dev,
> +				    uint32_t instdone[I915_NUM_INSTDONE_REG])
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	if (INTEL_INFO(dev)->gen < 4) {
> +		instdone[0] = I915_READ(INSTDONE);
> +		instdone[1] = 0;
> +	} else {
> +		instdone[0] = I915_READ(INSTDONE_I965);
> +		instdone[1] = I915_READ(INSTDONE1);
> +	}
> +}
> +
> +
>  static void i915_record_ring_state(struct drm_device *dev,
>  				   struct drm_i915_error_state *error,
>  				   struct intel_ring_buffer *ring)
> @@ -1286,6 +1300,7 @@ void i915_destroy_error_state(struct drm_device *dev)
>  static void i915_report_and_clear_eir(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t instdone[I915_NUM_INSTDONE_REG];
>  	u32 eir = I915_READ(EIR);
>  	int pipe;
>  
> @@ -1294,16 +1309,18 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  	pr_err("render error detected, EIR: 0x%08x\n", eir);
>  
> +	memset(instdone, 0, sizeof(instdone));
> +	i915_get_extra_instdone(dev, instdone);

You always memset before i915_get_extra_instdone. What if you moved the
memset inside i915_get_extra_instdone, and added a size_t argument to
it?

BR,
Jani.

> +
>  	if (IS_G4X(dev)) {
>  		if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
>  			u32 ipeir = I915_READ(IPEIR_I965);
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> -			pr_err("  INSTDONE: 0x%08x\n",
> -			       I915_READ(INSTDONE_I965));
> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>  			I915_WRITE(IPEIR_I965, ipeir);
>  			POSTING_READ(IPEIR_I965);
> @@ -1342,7 +1359,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
> -			pr_err("  INSTDONE: 0x%08x\n", I915_READ(INSTDONE));
> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
>  			I915_WRITE(IPEIR, ipeir);
>  			POSTING_READ(IPEIR);
> @@ -1351,10 +1368,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> -			pr_err("  INSTDONE: 0x%08x\n",
> -			       I915_READ(INSTDONE_I965));
> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>  			I915_WRITE(IPEIR_I965, ipeir);
>  			POSTING_READ(IPEIR_I965);
> @@ -1669,7 +1685,7 @@ void i915_hangcheck_elapsed(unsigned long data)
>  {
>  	struct drm_device *dev = (struct drm_device *)data;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
> +	uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
>  	struct intel_ring_buffer *ring;
>  	bool err = false, idle;
>  	int i;
> @@ -1697,25 +1713,20 @@ void i915_hangcheck_elapsed(unsigned long data)
>  		return;
>  	}
>  
> -	if (INTEL_INFO(dev)->gen < 4) {
> -		instdone = I915_READ(INSTDONE);
> -		instdone1 = 0;
> -	} else {
> -		instdone = I915_READ(INSTDONE_I965);
> -		instdone1 = I915_READ(INSTDONE1);
> -	}
> +	memset(instdone, 0, sizeof(instdone));
> +	i915_get_extra_instdone(dev, instdone);
>  
>  	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
> -	    dev_priv->last_instdone == instdone &&
> -	    dev_priv->last_instdone1 == instdone1) {
> +	    dev_priv->last_instdone == instdone[0] &&
> +	    dev_priv->last_instdone1 == instdone[1]) {
>  		if (i915_hangcheck_hung(dev))
>  			return;
>  	} else {
>  		dev_priv->hangcheck_count = 0;
>  
>  		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
> -		dev_priv->last_instdone = instdone;
> -		dev_priv->last_instdone1 = instdone1;
> +		dev_priv->last_instdone = instdone[0];
> +		dev_priv->last_instdone1 = instdone[1];
>  	}
>  
>  repeat:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2f7b688..948ede9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -485,6 +485,7 @@
>  #define IPEIR_I965	0x02064
>  #define IPEHR_I965	0x02068
>  #define INSTDONE_I965	0x0206c
> +#define I915_NUM_INSTDONE_REG	2
>  #define RING_IPEIR(base)	((base)+0x64)
>  #define RING_IPEHR(base)	((base)+0x68)
>  #define RING_INSTDONE(base)	((base)+0x6c)
> -- 
> 1.7.11.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Extract reading INSTDONE

[Ben Widawsky]

On 2012-08-15 23:25, Jani Nikula wrote:
> On Thu, 16 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
>> INSTDONE is used in many places, and it varies from generation to
>> generation. This provides a good reason for us to extract the logic 
>> to
>> read the relevant information.
>>
>> The patch has no functional change. It's prep for some new stuff.
>>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 49 
>> +++++++++++++++++++++++++----------------
>>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>>  2 files changed, 31 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 0c37101..0bf2f92 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1071,6 +1071,20 @@ i915_error_first_batchbuffer(struct 
>> drm_i915_private *dev_priv,
>>  	return NULL;
>>  }
>>
>> +static void i915_get_extra_instdone(struct drm_device *dev,
>> +				    uint32_t instdone[I915_NUM_INSTDONE_REG])
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	if (INTEL_INFO(dev)->gen < 4) {
>> +		instdone[0] = I915_READ(INSTDONE);
>> +		instdone[1] = 0;
>> +	} else {
>> +		instdone[0] = I915_READ(INSTDONE_I965);
>> +		instdone[1] = I915_READ(INSTDONE1);
>> +	}
>> +}
>> +
>> +
>>  static void i915_record_ring_state(struct drm_device *dev,
>>  				   struct drm_i915_error_state *error,
>>  				   struct intel_ring_buffer *ring)
>> @@ -1286,6 +1300,7 @@ void i915_destroy_error_state(struct 
>> drm_device *dev)
>>  static void i915_report_and_clear_eir(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	uint32_t instdone[I915_NUM_INSTDONE_REG];
>>  	u32 eir = I915_READ(EIR);
>>  	int pipe;
>>
>> @@ -1294,16 +1309,18 @@ static void i915_report_and_clear_eir(struct 
>> drm_device *dev)
>>
>>  	pr_err("render error detected, EIR: 0x%08x\n", eir);
>>
>> +	memset(instdone, 0, sizeof(instdone));
>> +	i915_get_extra_instdone(dev, instdone);
>
> You always memset before i915_get_extra_instdone. What if you moved 
> the
> memset inside i915_get_extra_instdone, and added a size_t argument to
> it?
>
> BR,
> Jani.

Only reason I like the way it is is so that the caller could 
potentially figure out whether or not certain registers exist based on 
whether or not they change. I think it's somewhat uncommon to clear out 
a data structure in the way you mention unless the callee is also 
allocating the memory. However, in hindsight, it would probably be 
easier to just do a GEN check instead, and it's more lines deleted. If 
Daniel suggests he's interested in the patches, I'll happily cave in to 
whatever he wants.


>
>> +
>>  	if (IS_G4X(dev)) {
>>  		if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
>>  			u32 ipeir = I915_READ(IPEIR_I965);
>>
>>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
>> -			pr_err("  INSTDONE: 0x%08x\n",
>> -			       I915_READ(INSTDONE_I965));
>> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
>> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
>> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>>  			I915_WRITE(IPEIR_I965, ipeir);
>>  			POSTING_READ(IPEIR_I965);
>> @@ -1342,7 +1359,7 @@ static void i915_report_and_clear_eir(struct 
>> drm_device *dev)
>>
>>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
>>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
>> -			pr_err("  INSTDONE: 0x%08x\n", I915_READ(INSTDONE));
>> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
>>  			I915_WRITE(IPEIR, ipeir);
>>  			POSTING_READ(IPEIR);
>> @@ -1351,10 +1368,9 @@ static void i915_report_and_clear_eir(struct 
>> drm_device *dev)
>>
>>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
>> -			pr_err("  INSTDONE: 0x%08x\n",
>> -			       I915_READ(INSTDONE_I965));
>> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
>> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
>> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>>  			I915_WRITE(IPEIR_I965, ipeir);
>>  			POSTING_READ(IPEIR_I965);
>> @@ -1669,7 +1685,7 @@ void i915_hangcheck_elapsed(unsigned long 
>> data)
>>  {
>>  	struct drm_device *dev = (struct drm_device *)data;
>>  	drm_i915_private_t *dev_priv = dev->dev_private;
>> -	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
>> +	uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
>>  	struct intel_ring_buffer *ring;
>>  	bool err = false, idle;
>>  	int i;
>> @@ -1697,25 +1713,20 @@ void i915_hangcheck_elapsed(unsigned long 
>> data)
>>  		return;
>>  	}
>>
>> -	if (INTEL_INFO(dev)->gen < 4) {
>> -		instdone = I915_READ(INSTDONE);
>> -		instdone1 = 0;
>> -	} else {
>> -		instdone = I915_READ(INSTDONE_I965);
>> -		instdone1 = I915_READ(INSTDONE1);
>> -	}
>> +	memset(instdone, 0, sizeof(instdone));
>> +	i915_get_extra_instdone(dev, instdone);
>>
>>  	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
>> -	    dev_priv->last_instdone == instdone &&
>> -	    dev_priv->last_instdone1 == instdone1) {
>> +	    dev_priv->last_instdone == instdone[0] &&
>> +	    dev_priv->last_instdone1 == instdone[1]) {
>>  		if (i915_hangcheck_hung(dev))
>>  			return;
>>  	} else {
>>  		dev_priv->hangcheck_count = 0;
>>
>>  		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
>> -		dev_priv->last_instdone = instdone;
>> -		dev_priv->last_instdone1 = instdone1;
>> +		dev_priv->last_instdone = instdone[0];
>> +		dev_priv->last_instdone1 = instdone[1];
>>  	}
>>
>>  repeat:
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 2f7b688..948ede9 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -485,6 +485,7 @@
>>  #define IPEIR_I965	0x02064
>>  #define IPEHR_I965	0x02068
>>  #define INSTDONE_I965	0x0206c
>> +#define I915_NUM_INSTDONE_REG	2
>>  #define RING_IPEIR(base)	((base)+0x64)
>>  #define RING_IPEHR(base)	((base)+0x68)
>>  #define RING_INSTDONE(base)	((base)+0x6c)
>> --
>> 1.7.11.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 2/3] drm/i915: Add new INSTDONE registers

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 948ede9..b290221 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -485,7 +485,11 @@
 #define IPEIR_I965	0x02064
 #define IPEHR_I965	0x02068
 #define INSTDONE_I965	0x0206c
-#define I915_NUM_INSTDONE_REG	2
+#define GEN7_INSTDONE_1		0x0206c
+#define GEN7_SC_INSTDONE	0x07100
+#define GEN7_SAMPLER_INSTDONE	0x0e160
+#define GEN7_ROW_INSTDONE	0x0e164
+#define I915_NUM_INSTDONE_REG	4
 #define RING_IPEIR(base)	((base)+0x64)
 #define RING_IPEHR(base)	((base)+0x68)
 #define RING_INSTDONE(base)	((base)+0x6c)
-- 
1.7.11.5

[PATCH 3/3] drm/i915: Use new INSTDONE registers

[Ben Widawsky]

Using the extracted INSTDONE reading, and our new register definitions,
update our hangcheck and error collection to use it.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++---
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
 drivers/gpu/drm/i915/i915_irq.c     | 47 +++++++++++++++++++++++--------------
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0e8f14d..c1474fb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -645,10 +645,9 @@ static void i915_ring_error_state(struct seq_file *m,
 	seq_printf(m, "  IPEIR: 0x%08x\n", error->ipeir[ring]);
 	seq_printf(m, "  IPEHR: 0x%08x\n", error->ipehr[ring]);
 	seq_printf(m, "  INSTDONE: 0x%08x\n", error->instdone[ring]);
-	if (ring == RCS && INTEL_INFO(dev)->gen >= 4) {
-		seq_printf(m, "  INSTDONE1: 0x%08x\n", error->instdone1);
+	if (ring == RCS && INTEL_INFO(dev)->gen >= 4)
 		seq_printf(m, "  BBADDR: 0x%08llx\n", error->bbaddr);
-	}
+
 	if (INTEL_INFO(dev)->gen >= 4)
 		seq_printf(m, "  INSTPS: 0x%08x\n", error->instps[ring]);
 	seq_printf(m, "  INSTPM: 0x%08x\n", error->instpm[ring]);
@@ -697,6 +696,9 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	for (i = 0; i < dev_priv->num_fence_regs; i++)
 		seq_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
 
+	for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
+		seq_printf(m, "  INSTDONE_%d: 0x%08x\n", i, error->extra_instdone[i]);
+
 	if (INTEL_INFO(dev)->gen >= 6) {
 		seq_printf(m, "ERROR: 0x%08x\n", error->error);
 		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b69be6..cd2ee29 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -198,7 +198,7 @@ struct drm_i915_error_state {
 	u32 error; /* gen6+ */
 	u32 instpm[I915_NUM_RINGS];
 	u32 instps[I915_NUM_RINGS];
-	u32 instdone1;
+	u32 extra_instdone[I915_NUM_INSTDONE_REG];
 	u32 seqno[I915_NUM_RINGS];
 	u64 bbaddr;
 	u32 fault_reg[I915_NUM_RINGS];
@@ -460,8 +460,7 @@ typedef struct drm_i915_private {
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
 	uint32_t last_acthd[I915_NUM_RINGS];
-	uint32_t last_instdone;
-	uint32_t last_instdone1;
+	uint32_t prev_instdone[I915_NUM_INSTDONE_REG];
 
 	unsigned int stop_rings;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0bf2f92..668bc70 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1075,12 +1075,26 @@ static void i915_get_extra_instdone(struct drm_device *dev,
 				    uint32_t instdone[I915_NUM_INSTDONE_REG])
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (INTEL_INFO(dev)->gen < 4) {
+
+	switch(INTEL_INFO(dev)->gen) {
+	case 2:
+	case 3:
 		instdone[0] = I915_READ(INSTDONE);
-		instdone[1] = 0;
-	} else {
+		break;
+	case 4:
+	case 5:
+	case 6:
 		instdone[0] = I915_READ(INSTDONE_I965);
 		instdone[1] = I915_READ(INSTDONE1);
+		break;
+	default:
+		WARN_ONCE(1, "Unsupported platform\n");
+	case 7:
+		instdone[0] = I915_READ(GEN7_INSTDONE_1);
+		instdone[1] = I915_READ(GEN7_SC_INSTDONE);
+		instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
+		instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
+		break;
 	}
 }
 
@@ -1106,10 +1120,8 @@ static void i915_record_ring_state(struct drm_device *dev,
 		error->ipehr[ring->id] = I915_READ(RING_IPEHR(ring->mmio_base));
 		error->instdone[ring->id] = I915_READ(RING_INSTDONE(ring->mmio_base));
 		error->instps[ring->id] = I915_READ(RING_INSTPS(ring->mmio_base));
-		if (ring->id == RCS) {
-			error->instdone1 = I915_READ(INSTDONE1);
+		if (ring->id == RCS)
 			error->bbaddr = I915_READ64(BB_ADDR);
-		}
 	} else {
 		error->faddr[ring->id] = I915_READ(DMA_FADD_I8XX);
 		error->ipeir[ring->id] = I915_READ(IPEIR);
@@ -1184,6 +1196,7 @@ static void i915_capture_error_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct drm_i915_error_state *error;
+	uint32_t instdone[I915_NUM_INSTDONE_REG];
 	unsigned long flags;
 	int i, pipe;
 
@@ -1225,6 +1238,10 @@ static void i915_capture_error_state(struct drm_device *dev)
 		error->done_reg = I915_READ(DONE_REG);
 	}
 
+	memset(instdone, 0, sizeof(instdone));
+	i915_get_extra_instdone(dev, instdone);
+	memcpy(error->extra_instdone, instdone, sizeof(instdone));
+
 	i915_gem_record_fences(dev, error);
 	i915_gem_record_rings(dev, error);
 
@@ -1302,7 +1319,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t instdone[I915_NUM_INSTDONE_REG];
 	u32 eir = I915_READ(EIR);
-	int pipe;
+	int pipe, i;
 
 	if (!eir)
 		return;
@@ -1318,9 +1335,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
+			for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
+				pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1354,12 +1371,13 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 	if (eir & I915_ERROR_INSTRUCTION) {
 		pr_err("instruction error\n");
 		pr_err("  INSTPM: 0x%08x\n", I915_READ(INSTPM));
+		for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
+			pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
 		if (INTEL_INFO(dev)->gen < 4) {
 			u32 ipeir = I915_READ(IPEIR);
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
-			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
 			I915_WRITE(IPEIR, ipeir);
 			POSTING_READ(IPEIR);
@@ -1368,9 +1386,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1715,18 +1731,15 @@ void i915_hangcheck_elapsed(unsigned long data)
 
 	memset(instdone, 0, sizeof(instdone));
 	i915_get_extra_instdone(dev, instdone);
-
 	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
-	    dev_priv->last_instdone == instdone[0] &&
-	    dev_priv->last_instdone1 == instdone[1]) {
+	    memcmp(dev_priv->prev_instdone, instdone, sizeof(instdone)) == 0) {
 		if (i915_hangcheck_hung(dev))
 			return;
 	} else {
 		dev_priv->hangcheck_count = 0;
 
 		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
-		dev_priv->last_instdone = instdone[0];
-		dev_priv->last_instdone1 = instdone[1];
+		memcpy(dev_priv->prev_instdone, instdone, sizeof(instdone));
 	}
 
 repeat:
-- 
1.7.11.5

Re: [PATCH 3/3] drm/i915: Use new INSTDONE registers

[Jani Nikula]

On Thu, 16 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
> Using the extracted INSTDONE reading, and our new register definitions,
> update our hangcheck and error collection to use it.

Hi Ben, please find some nitpicks below...

BR,
Jani.

>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++---
>  drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
>  drivers/gpu/drm/i915/i915_irq.c     | 47 +++++++++++++++++++++++--------------
>  3 files changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0e8f14d..c1474fb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -645,10 +645,9 @@ static void i915_ring_error_state(struct seq_file *m,
>  	seq_printf(m, "  IPEIR: 0x%08x\n", error->ipeir[ring]);
>  	seq_printf(m, "  IPEHR: 0x%08x\n", error->ipehr[ring]);
>  	seq_printf(m, "  INSTDONE: 0x%08x\n", error->instdone[ring]);
> -	if (ring == RCS && INTEL_INFO(dev)->gen >= 4) {
> -		seq_printf(m, "  INSTDONE1: 0x%08x\n", error->instdone1);
> +	if (ring == RCS && INTEL_INFO(dev)->gen >= 4)
>  		seq_printf(m, "  BBADDR: 0x%08llx\n", error->bbaddr);
> -	}
> +
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		seq_printf(m, "  INSTPS: 0x%08x\n", error->instps[ring]);
>  	seq_printf(m, "  INSTPM: 0x%08x\n", error->instpm[ring]);
> @@ -697,6 +696,9 @@ static int i915_error_state(struct seq_file *m, void *unused)
>  	for (i = 0; i < dev_priv->num_fence_regs; i++)
>  		seq_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
>  
> +	for (i = 0; i < I915_NUM_INSTDONE_REG; i++)

ARRAY_SIZE(error->extra_instdone)?

> +		seq_printf(m, "  INSTDONE_%d: 0x%08x\n", i, error->extra_instdone[i]);
> +
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		seq_printf(m, "ERROR: 0x%08x\n", error->error);
>  		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9b69be6..cd2ee29 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -198,7 +198,7 @@ struct drm_i915_error_state {
>  	u32 error; /* gen6+ */
>  	u32 instpm[I915_NUM_RINGS];
>  	u32 instps[I915_NUM_RINGS];
> -	u32 instdone1;
> +	u32 extra_instdone[I915_NUM_INSTDONE_REG];
>  	u32 seqno[I915_NUM_RINGS];
>  	u64 bbaddr;
>  	u32 fault_reg[I915_NUM_RINGS];
> @@ -460,8 +460,7 @@ typedef struct drm_i915_private {
>  	struct timer_list hangcheck_timer;
>  	int hangcheck_count;
>  	uint32_t last_acthd[I915_NUM_RINGS];
> -	uint32_t last_instdone;
> -	uint32_t last_instdone1;
> +	uint32_t prev_instdone[I915_NUM_INSTDONE_REG];
>  
>  	unsigned int stop_rings;
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 0bf2f92..668bc70 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1075,12 +1075,26 @@ static void i915_get_extra_instdone(struct drm_device *dev,
>  				    uint32_t instdone[I915_NUM_INSTDONE_REG])
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	if (INTEL_INFO(dev)->gen < 4) {
> +
> +	switch(INTEL_INFO(dev)->gen) {
> +	case 2:
> +	case 3:
>  		instdone[0] = I915_READ(INSTDONE);
> -		instdone[1] = 0;
> -	} else {
> +		break;
> +	case 4:
> +	case 5:
> +	case 6:
>  		instdone[0] = I915_READ(INSTDONE_I965);
>  		instdone[1] = I915_READ(INSTDONE1);
> +		break;
> +	default:
> +		WARN_ONCE(1, "Unsupported platform\n");
> +	case 7:
> +		instdone[0] = I915_READ(GEN7_INSTDONE_1);
> +		instdone[1] = I915_READ(GEN7_SC_INSTDONE);
> +		instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
> +		instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
> +		break;
>  	}
>  }
>  
> @@ -1106,10 +1120,8 @@ static void i915_record_ring_state(struct drm_device *dev,
>  		error->ipehr[ring->id] = I915_READ(RING_IPEHR(ring->mmio_base));
>  		error->instdone[ring->id] = I915_READ(RING_INSTDONE(ring->mmio_base));
>  		error->instps[ring->id] = I915_READ(RING_INSTPS(ring->mmio_base));
> -		if (ring->id == RCS) {
> -			error->instdone1 = I915_READ(INSTDONE1);
> +		if (ring->id == RCS)
>  			error->bbaddr = I915_READ64(BB_ADDR);
> -		}
>  	} else {
>  		error->faddr[ring->id] = I915_READ(DMA_FADD_I8XX);
>  		error->ipeir[ring->id] = I915_READ(IPEIR);
> @@ -1184,6 +1196,7 @@ static void i915_capture_error_state(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_i915_error_state *error;
> +	uint32_t instdone[I915_NUM_INSTDONE_REG];
>  	unsigned long flags;
>  	int i, pipe;
>  
> @@ -1225,6 +1238,10 @@ static void i915_capture_error_state(struct drm_device *dev)
>  		error->done_reg = I915_READ(DONE_REG);
>  	}
>  
> +	memset(instdone, 0, sizeof(instdone));
> +	i915_get_extra_instdone(dev, instdone);
> +	memcpy(error->extra_instdone, instdone, sizeof(instdone));

If i915_get_extra_instdone did the memset as suggested, you could scrap
the temp instdone array, and get the data directly to
error->extra_instdone. (And use sizeof the target, not source in
memcpy!)

> +
>  	i915_gem_record_fences(dev, error);
>  	i915_gem_record_rings(dev, error);
>  
> @@ -1302,7 +1319,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t instdone[I915_NUM_INSTDONE_REG];
>  	u32 eir = I915_READ(EIR);
> -	int pipe;
> +	int pipe, i;
>  
>  	if (!eir)
>  		return;
> @@ -1318,9 +1335,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> -			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
> +			for (i = 0; i < I915_NUM_INSTDONE_REG; i++)

ARRAY_SIZE(instdone)?

> +				pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> -			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>  			I915_WRITE(IPEIR_I965, ipeir);
>  			POSTING_READ(IPEIR_I965);
> @@ -1354,12 +1371,13 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  	if (eir & I915_ERROR_INSTRUCTION) {
>  		pr_err("instruction error\n");
>  		pr_err("  INSTPM: 0x%08x\n", I915_READ(INSTPM));
> +		for (i = 0; i < I915_NUM_INSTDONE_REG; i++)

ARRAY_SIZE(instdone)?

> +			pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
>  		if (INTEL_INFO(dev)->gen < 4) {
>  			u32 ipeir = I915_READ(IPEIR);
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
> -			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
>  			I915_WRITE(IPEIR, ipeir);
>  			POSTING_READ(IPEIR);
> @@ -1368,9 +1386,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> -			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> -			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>  			I915_WRITE(IPEIR_I965, ipeir);
>  			POSTING_READ(IPEIR_I965);
> @@ -1715,18 +1731,15 @@ void i915_hangcheck_elapsed(unsigned long data)
>  
>  	memset(instdone, 0, sizeof(instdone));
>  	i915_get_extra_instdone(dev, instdone);
> -
>  	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
> -	    dev_priv->last_instdone == instdone[0] &&
> -	    dev_priv->last_instdone1 == instdone[1]) {
> +	    memcmp(dev_priv->prev_instdone, instdone, sizeof(instdone)) == 0) {
>  		if (i915_hangcheck_hung(dev))
>  			return;
>  	} else {
>  		dev_priv->hangcheck_count = 0;
>  
>  		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
> -		dev_priv->last_instdone = instdone[0];
> -		dev_priv->last_instdone1 = instdone[1];
> +		memcpy(dev_priv->prev_instdone, instdone, sizeof(instdone));

Somehow it always feels better to use sizeof the target rather than
source even if they're supposed to be the same size.

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

Re: [PATCH 3/3] drm/i915: Use new INSTDONE registers

[Ben Widawsky]

On Thu, 16 Aug 2012 09:31:51 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Thu, 16 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Using the extracted INSTDONE reading, and our new register definitions,
> > update our hangcheck and error collection to use it.
> 
> Hi Ben, please find some nitpicks below...
> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++---
> >  drivers/gpu/drm/i915/i915_drv.h     |  5 ++--
> >  drivers/gpu/drm/i915/i915_irq.c     | 47 +++++++++++++++++++++++--------------
> >  3 files changed, 37 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 0e8f14d..c1474fb 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -645,10 +645,9 @@ static void i915_ring_error_state(struct seq_file *m,
> >  	seq_printf(m, "  IPEIR: 0x%08x\n", error->ipeir[ring]);
> >  	seq_printf(m, "  IPEHR: 0x%08x\n", error->ipehr[ring]);
> >  	seq_printf(m, "  INSTDONE: 0x%08x\n", error->instdone[ring]);
> > -	if (ring == RCS && INTEL_INFO(dev)->gen >= 4) {
> > -		seq_printf(m, "  INSTDONE1: 0x%08x\n", error->instdone1);
> > +	if (ring == RCS && INTEL_INFO(dev)->gen >= 4)
> >  		seq_printf(m, "  BBADDR: 0x%08llx\n", error->bbaddr);
> > -	}
> > +
> >  	if (INTEL_INFO(dev)->gen >= 4)
> >  		seq_printf(m, "  INSTPS: 0x%08x\n", error->instps[ring]);
> >  	seq_printf(m, "  INSTPM: 0x%08x\n", error->instpm[ring]);
> > @@ -697,6 +696,9 @@ static int i915_error_state(struct seq_file *m, void *unused)
> >  	for (i = 0; i < dev_priv->num_fence_regs; i++)
> >  		seq_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
> >  
> > +	for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
> 
> ARRAY_SIZE(error->extra_instdone)?
> 

Agree.

> > +		seq_printf(m, "  INSTDONE_%d: 0x%08x\n", i, error->extra_instdone[i]);
> > +
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		seq_printf(m, "ERROR: 0x%08x\n", error->error);
> >  		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9b69be6..cd2ee29 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -198,7 +198,7 @@ struct drm_i915_error_state {
> >  	u32 error; /* gen6+ */
> >  	u32 instpm[I915_NUM_RINGS];
> >  	u32 instps[I915_NUM_RINGS];
> > -	u32 instdone1;
> > +	u32 extra_instdone[I915_NUM_INSTDONE_REG];
> >  	u32 seqno[I915_NUM_RINGS];
> >  	u64 bbaddr;
> >  	u32 fault_reg[I915_NUM_RINGS];
> > @@ -460,8 +460,7 @@ typedef struct drm_i915_private {
> >  	struct timer_list hangcheck_timer;
> >  	int hangcheck_count;
> >  	uint32_t last_acthd[I915_NUM_RINGS];
> > -	uint32_t last_instdone;
> > -	uint32_t last_instdone1;
> > +	uint32_t prev_instdone[I915_NUM_INSTDONE_REG];
> >  
> >  	unsigned int stop_rings;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 0bf2f92..668bc70 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1075,12 +1075,26 @@ static void i915_get_extra_instdone(struct drm_device *dev,
> >  				    uint32_t instdone[I915_NUM_INSTDONE_REG])
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	if (INTEL_INFO(dev)->gen < 4) {
> > +
> > +	switch(INTEL_INFO(dev)->gen) {
> > +	case 2:
> > +	case 3:
> >  		instdone[0] = I915_READ(INSTDONE);
> > -		instdone[1] = 0;
> > -	} else {
> > +		break;
> > +	case 4:
> > +	case 5:
> > +	case 6:
> >  		instdone[0] = I915_READ(INSTDONE_I965);
> >  		instdone[1] = I915_READ(INSTDONE1);
> > +		break;
> > +	default:
> > +		WARN_ONCE(1, "Unsupported platform\n");
> > +	case 7:
> > +		instdone[0] = I915_READ(GEN7_INSTDONE_1);
> > +		instdone[1] = I915_READ(GEN7_SC_INSTDONE);
> > +		instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
> > +		instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
> > +		break;
> >  	}
> >  }
> >  
> > @@ -1106,10 +1120,8 @@ static void i915_record_ring_state(struct drm_device *dev,
> >  		error->ipehr[ring->id] = I915_READ(RING_IPEHR(ring->mmio_base));
> >  		error->instdone[ring->id] = I915_READ(RING_INSTDONE(ring->mmio_base));
> >  		error->instps[ring->id] = I915_READ(RING_INSTPS(ring->mmio_base));
> > -		if (ring->id == RCS) {
> > -			error->instdone1 = I915_READ(INSTDONE1);
> > +		if (ring->id == RCS)
> >  			error->bbaddr = I915_READ64(BB_ADDR);
> > -		}
> >  	} else {
> >  		error->faddr[ring->id] = I915_READ(DMA_FADD_I8XX);
> >  		error->ipeir[ring->id] = I915_READ(IPEIR);
> > @@ -1184,6 +1196,7 @@ static void i915_capture_error_state(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj;
> >  	struct drm_i915_error_state *error;
> > +	uint32_t instdone[I915_NUM_INSTDONE_REG];
> >  	unsigned long flags;
> >  	int i, pipe;
> >  
> > @@ -1225,6 +1238,10 @@ static void i915_capture_error_state(struct drm_device *dev)
> >  		error->done_reg = I915_READ(DONE_REG);
> >  	}
> >  
> > +	memset(instdone, 0, sizeof(instdone));
> > +	i915_get_extra_instdone(dev, instdone);
> > +	memcpy(error->extra_instdone, instdone, sizeof(instdone));
> 
> If i915_get_extra_instdone did the memset as suggested, you could scrap
> the temp instdone array, and get the data directly to
> error->extra_instdone. (And use sizeof the target, not source in
> memcpy!)

Agree (pending whatever Daniel says).

> 
> > +
> >  	i915_gem_record_fences(dev, error);
> >  	i915_gem_record_rings(dev, error);
> >  
> > @@ -1302,7 +1319,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t instdone[I915_NUM_INSTDONE_REG];
> >  	u32 eir = I915_READ(EIR);
> > -	int pipe;
> > +	int pipe, i;
> >  
> >  	if (!eir)
> >  		return;
> > @@ -1318,9 +1335,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
> >  
> >  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
> >  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> > -			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
> > +			for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
> 
> ARRAY_SIZE(instdone)?
> 
> > +				pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
> >  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> > -			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
> >  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
> >  			I915_WRITE(IPEIR_I965, ipeir);
> >  			POSTING_READ(IPEIR_I965);
> > @@ -1354,12 +1371,13 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
> >  	if (eir & I915_ERROR_INSTRUCTION) {
> >  		pr_err("instruction error\n");
> >  		pr_err("  INSTPM: 0x%08x\n", I915_READ(INSTPM));
> > +		for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
> 
> ARRAY_SIZE(instdone)?
> 
> > +			pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
> >  		if (INTEL_INFO(dev)->gen < 4) {
> >  			u32 ipeir = I915_READ(IPEIR);
> >  
> >  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
> >  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
> > -			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
> >  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
> >  			I915_WRITE(IPEIR, ipeir);
> >  			POSTING_READ(IPEIR);
> > @@ -1368,9 +1386,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
> >  
> >  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
> >  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> > -			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
> >  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> > -			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
> >  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
> >  			I915_WRITE(IPEIR_I965, ipeir);
> >  			POSTING_READ(IPEIR_I965);
> > @@ -1715,18 +1731,15 @@ void i915_hangcheck_elapsed(unsigned long data)
> >  
> >  	memset(instdone, 0, sizeof(instdone));
> >  	i915_get_extra_instdone(dev, instdone);
> > -
> >  	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
> > -	    dev_priv->last_instdone == instdone[0] &&
> > -	    dev_priv->last_instdone1 == instdone[1]) {
> > +	    memcmp(dev_priv->prev_instdone, instdone, sizeof(instdone)) == 0) {
> >  		if (i915_hangcheck_hung(dev))
> >  			return;
> >  	} else {
> >  		dev_priv->hangcheck_count = 0;
> >  
> >  		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
> > -		dev_priv->last_instdone = instdone[0];
> > -		dev_priv->last_instdone1 = instdone[1];
> > +		memcpy(dev_priv->prev_instdone, instdone, sizeof(instdone));
> 
> Somehow it always feels better to use sizeof the target rather than
> source even if they're supposed to be the same size.

Disagree.

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



-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 1/3 v2] drm/i915: Extract reading INSTDONE

[Ben Widawsky]

INSTDONE is used in many places, and it varies from generation to
generation. This provides a good reason for us to extract the logic to
read the relevant information.

The patch has no functional change. It's prep for some new stuff.

v2: move the memset inside of i915_get_extra_instdone (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_irq.c | 50 +++++++++++++++++++++++++----------------
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0c37101..46eb6e9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1071,6 +1071,23 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
 	return NULL;
 }
 
+/* NB: please notice the memset */
+static void i915_get_extra_instdone(struct drm_device *dev,
+				    uint32_t instdone[I915_NUM_INSTDONE_REG])
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	memset(instdone, 0, sizeof(instdone));
+
+	if (INTEL_INFO(dev)->gen < 4) {
+		instdone[0] = I915_READ(INSTDONE);
+		instdone[1] = 0;
+	} else {
+		instdone[0] = I915_READ(INSTDONE_I965);
+		instdone[1] = I915_READ(INSTDONE1);
+	}
+}
+
+
 static void i915_record_ring_state(struct drm_device *dev,
 				   struct drm_i915_error_state *error,
 				   struct intel_ring_buffer *ring)
@@ -1286,6 +1303,7 @@ void i915_destroy_error_state(struct drm_device *dev)
 static void i915_report_and_clear_eir(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t instdone[I915_NUM_INSTDONE_REG];
 	u32 eir = I915_READ(EIR);
 	int pipe;
 
@@ -1294,16 +1312,17 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 	pr_err("render error detected, EIR: 0x%08x\n", eir);
 
+	i915_get_extra_instdone(dev, instdone);
+
 	if (IS_G4X(dev)) {
 		if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
 			u32 ipeir = I915_READ(IPEIR_I965);
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n",
-			       I915_READ(INSTDONE_I965));
+			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
+			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1342,7 +1361,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
-			pr_err("  INSTDONE: 0x%08x\n", I915_READ(INSTDONE));
+			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
 			I915_WRITE(IPEIR, ipeir);
 			POSTING_READ(IPEIR);
@@ -1351,10 +1370,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n",
-			       I915_READ(INSTDONE_I965));
+			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
+			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1669,7 +1687,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 {
 	struct drm_device *dev = (struct drm_device *)data;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
+	uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
 	struct intel_ring_buffer *ring;
 	bool err = false, idle;
 	int i;
@@ -1697,25 +1715,19 @@ void i915_hangcheck_elapsed(unsigned long data)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen < 4) {
-		instdone = I915_READ(INSTDONE);
-		instdone1 = 0;
-	} else {
-		instdone = I915_READ(INSTDONE_I965);
-		instdone1 = I915_READ(INSTDONE1);
-	}
+	i915_get_extra_instdone(dev, instdone);
 
 	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
-	    dev_priv->last_instdone == instdone &&
-	    dev_priv->last_instdone1 == instdone1) {
+	    dev_priv->last_instdone == instdone[0] &&
+	    dev_priv->last_instdone1 == instdone[1]) {
 		if (i915_hangcheck_hung(dev))
 			return;
 	} else {
 		dev_priv->hangcheck_count = 0;
 
 		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
-		dev_priv->last_instdone = instdone;
-		dev_priv->last_instdone1 = instdone1;
+		dev_priv->last_instdone = instdone[0];
+		dev_priv->last_instdone1 = instdone[1];
 	}
 
 repeat:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 321ae72..7ebe6e5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -478,6 +478,7 @@
 #define IPEIR_I965	0x02064
 #define IPEHR_I965	0x02068
 #define INSTDONE_I965	0x0206c
+#define I915_NUM_INSTDONE_REG	2
 #define RING_IPEIR(base)	((base)+0x64)
 #define RING_IPEHR(base)	((base)+0x68)
 #define RING_INSTDONE(base)	((base)+0x6c)
-- 
1.7.11.5

Re: [PATCH 1/3 v2] drm/i915: Extract reading INSTDONE

[Jani Nikula]

On Fri, 17 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
> INSTDONE is used in many places, and it varies from generation to
> generation. This provides a good reason for us to extract the logic to
> read the relevant information.
>
> The patch has no functional change. It's prep for some new stuff.
>
> v2: move the memset inside of i915_get_extra_instdone (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 50 +++++++++++++++++++++++++----------------
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  2 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 0c37101..46eb6e9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1071,6 +1071,23 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
>  	return NULL;
>  }
>  
> +/* NB: please notice the memset */
> +static void i915_get_extra_instdone(struct drm_device *dev,
> +				    uint32_t instdone[I915_NUM_INSTDONE_REG])
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	memset(instdone, 0, sizeof(instdone));

This won't do what you want. sizeof(instdone) == sizeof(uint32_t *)
here. You either have to use I915_NUM_INSTDONE_REG or pass the size.

I don't know what that foo[SIZE] style really is good for in function
parameters. I guess it does have a documentation aspect, and some clever
compilers or static analyzers could flag out of bounds accesses. But it
also gives a kind of false sense of security, and makes one susceptible
to errors like this. So perhaps make that uint32_t *instdone while at
it?

BR,
Jani.

> +
> +	if (INTEL_INFO(dev)->gen < 4) {
> +		instdone[0] = I915_READ(INSTDONE);
> +		instdone[1] = 0;
> +	} else {
> +		instdone[0] = I915_READ(INSTDONE_I965);
> +		instdone[1] = I915_READ(INSTDONE1);
> +	}
> +}
> +
> +
>  static void i915_record_ring_state(struct drm_device *dev,
>  				   struct drm_i915_error_state *error,
>  				   struct intel_ring_buffer *ring)
> @@ -1286,6 +1303,7 @@ void i915_destroy_error_state(struct drm_device *dev)
>  static void i915_report_and_clear_eir(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t instdone[I915_NUM_INSTDONE_REG];
>  	u32 eir = I915_READ(EIR);
>  	int pipe;
>  
> @@ -1294,16 +1312,17 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  	pr_err("render error detected, EIR: 0x%08x\n", eir);
>  
> +	i915_get_extra_instdone(dev, instdone);
> +
>  	if (IS_G4X(dev)) {
>  		if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
>  			u32 ipeir = I915_READ(IPEIR_I965);
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> -			pr_err("  INSTDONE: 0x%08x\n",
> -			       I915_READ(INSTDONE_I965));
> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>  			I915_WRITE(IPEIR_I965, ipeir);
>  			POSTING_READ(IPEIR_I965);
> @@ -1342,7 +1361,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
> -			pr_err("  INSTDONE: 0x%08x\n", I915_READ(INSTDONE));
> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
>  			I915_WRITE(IPEIR, ipeir);
>  			POSTING_READ(IPEIR);
> @@ -1351,10 +1370,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  
>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
> -			pr_err("  INSTDONE: 0x%08x\n",
> -			       I915_READ(INSTDONE_I965));
> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>  			I915_WRITE(IPEIR_I965, ipeir);
>  			POSTING_READ(IPEIR_I965);
> @@ -1669,7 +1687,7 @@ void i915_hangcheck_elapsed(unsigned long data)
>  {
>  	struct drm_device *dev = (struct drm_device *)data;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
> +	uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
>  	struct intel_ring_buffer *ring;
>  	bool err = false, idle;
>  	int i;
> @@ -1697,25 +1715,19 @@ void i915_hangcheck_elapsed(unsigned long data)
>  		return;
>  	}
>  
> -	if (INTEL_INFO(dev)->gen < 4) {
> -		instdone = I915_READ(INSTDONE);
> -		instdone1 = 0;
> -	} else {
> -		instdone = I915_READ(INSTDONE_I965);
> -		instdone1 = I915_READ(INSTDONE1);
> -	}
> +	i915_get_extra_instdone(dev, instdone);
>  
>  	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
> -	    dev_priv->last_instdone == instdone &&
> -	    dev_priv->last_instdone1 == instdone1) {
> +	    dev_priv->last_instdone == instdone[0] &&
> +	    dev_priv->last_instdone1 == instdone[1]) {
>  		if (i915_hangcheck_hung(dev))
>  			return;
>  	} else {
>  		dev_priv->hangcheck_count = 0;
>  
>  		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
> -		dev_priv->last_instdone = instdone;
> -		dev_priv->last_instdone1 = instdone1;
> +		dev_priv->last_instdone = instdone[0];
> +		dev_priv->last_instdone1 = instdone[1];
>  	}
>  
>  repeat:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 321ae72..7ebe6e5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -478,6 +478,7 @@
>  #define IPEIR_I965	0x02064
>  #define IPEHR_I965	0x02068
>  #define INSTDONE_I965	0x0206c
> +#define I915_NUM_INSTDONE_REG	2
>  #define RING_IPEIR(base)	((base)+0x64)
>  #define RING_IPEHR(base)	((base)+0x68)
>  #define RING_INSTDONE(base)	((base)+0x6c)
> -- 
> 1.7.11.5

Re: [PATCH 1/3 v2] drm/i915: Extract reading INSTDONE

[Ben Widawsky]

On 2012-08-17 01:46, Jani Nikula wrote:
> On Fri, 17 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
>> INSTDONE is used in many places, and it varies from generation to
>> generation. This provides a good reason for us to extract the logic 
>> to
>> read the relevant information.
>>
>> The patch has no functional change. It's prep for some new stuff.
>>
>> v2: move the memset inside of i915_get_extra_instdone (Jani)
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 50 
>> +++++++++++++++++++++++++----------------
>>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>>  2 files changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 0c37101..46eb6e9 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1071,6 +1071,23 @@ i915_error_first_batchbuffer(struct 
>> drm_i915_private *dev_priv,
>>  	return NULL;
>>  }
>>
>> +/* NB: please notice the memset */
>> +static void i915_get_extra_instdone(struct drm_device *dev,
>> +				    uint32_t instdone[I915_NUM_INSTDONE_REG])
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	memset(instdone, 0, sizeof(instdone));
>
> This won't do what you want. sizeof(instdone) == sizeof(uint32_t *)
> here. You either have to use I915_NUM_INSTDONE_REG or pass the size.

Ooops; now that I've made myself look properly bad... Or I could call 
memset outside of the function :-)

>
> I don't know what that foo[SIZE] style really is good for in function
> parameters. I guess it does have a documentation aspect, and some 
> clever
> compilers or static analyzers could flag out of bounds accesses. But 
> it
> also gives a kind of false sense of security, and makes one 
> susceptible
> to errors like this. So perhaps make that uint32_t *instdone while at
> it?
>
> BR,
> Jani.

Well, I liked it for documentation and because I thought gcc could do 
the right thing with it, ie. make the sizeof stuff work correctly. Since 
it doesn't, I'm left not caring; and I just want to get an r-b. So I'll 
do it how you asked.

>
>> +
>> +	if (INTEL_INFO(dev)->gen < 4) {
>> +		instdone[0] = I915_READ(INSTDONE);
>> +		instdone[1] = 0;
>> +	} else {
>> +		instdone[0] = I915_READ(INSTDONE_I965);
>> +		instdone[1] = I915_READ(INSTDONE1);
>> +	}
>> +}
>> +
>> +
>>  static void i915_record_ring_state(struct drm_device *dev,
>>  				   struct drm_i915_error_state *error,
>>  				   struct intel_ring_buffer *ring)
>> @@ -1286,6 +1303,7 @@ void i915_destroy_error_state(struct 
>> drm_device *dev)
>>  static void i915_report_and_clear_eir(struct drm_device *dev)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	uint32_t instdone[I915_NUM_INSTDONE_REG];
>>  	u32 eir = I915_READ(EIR);
>>  	int pipe;
>>
>> @@ -1294,16 +1312,17 @@ static void i915_report_and_clear_eir(struct 
>> drm_device *dev)
>>
>>  	pr_err("render error detected, EIR: 0x%08x\n", eir);
>>
>> +	i915_get_extra_instdone(dev, instdone);
>> +
>>  	if (IS_G4X(dev)) {
>>  		if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
>>  			u32 ipeir = I915_READ(IPEIR_I965);
>>
>>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
>> -			pr_err("  INSTDONE: 0x%08x\n",
>> -			       I915_READ(INSTDONE_I965));
>> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
>> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
>> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>>  			I915_WRITE(IPEIR_I965, ipeir);
>>  			POSTING_READ(IPEIR_I965);
>> @@ -1342,7 +1361,7 @@ static void i915_report_and_clear_eir(struct 
>> drm_device *dev)
>>
>>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
>>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
>> -			pr_err("  INSTDONE: 0x%08x\n", I915_READ(INSTDONE));
>> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
>>  			I915_WRITE(IPEIR, ipeir);
>>  			POSTING_READ(IPEIR);
>> @@ -1351,10 +1370,9 @@ static void i915_report_and_clear_eir(struct 
>> drm_device *dev)
>>
>>  			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
>>  			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
>> -			pr_err("  INSTDONE: 0x%08x\n",
>> -			       I915_READ(INSTDONE_I965));
>> +			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
>>  			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
>> -			pr_err("  INSTDONE1: 0x%08x\n", I915_READ(INSTDONE1));
>> +			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
>>  			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>>  			I915_WRITE(IPEIR_I965, ipeir);
>>  			POSTING_READ(IPEIR_I965);
>> @@ -1669,7 +1687,7 @@ void i915_hangcheck_elapsed(unsigned long 
>> data)
>>  {
>>  	struct drm_device *dev = (struct drm_device *)data;
>>  	drm_i915_private_t *dev_priv = dev->dev_private;
>> -	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
>> +	uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
>>  	struct intel_ring_buffer *ring;
>>  	bool err = false, idle;
>>  	int i;
>> @@ -1697,25 +1715,19 @@ void i915_hangcheck_elapsed(unsigned long 
>> data)
>>  		return;
>>  	}
>>
>> -	if (INTEL_INFO(dev)->gen < 4) {
>> -		instdone = I915_READ(INSTDONE);
>> -		instdone1 = 0;
>> -	} else {
>> -		instdone = I915_READ(INSTDONE_I965);
>> -		instdone1 = I915_READ(INSTDONE1);
>> -	}
>> +	i915_get_extra_instdone(dev, instdone);
>>
>>  	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
>> -	    dev_priv->last_instdone == instdone &&
>> -	    dev_priv->last_instdone1 == instdone1) {
>> +	    dev_priv->last_instdone == instdone[0] &&
>> +	    dev_priv->last_instdone1 == instdone[1]) {
>>  		if (i915_hangcheck_hung(dev))
>>  			return;
>>  	} else {
>>  		dev_priv->hangcheck_count = 0;
>>
>>  		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
>> -		dev_priv->last_instdone = instdone;
>> -		dev_priv->last_instdone1 = instdone1;
>> +		dev_priv->last_instdone = instdone[0];
>> +		dev_priv->last_instdone1 = instdone[1];
>>  	}
>>
>>  repeat:
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 321ae72..7ebe6e5 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -478,6 +478,7 @@
>>  #define IPEIR_I965	0x02064
>>  #define IPEHR_I965	0x02068
>>  #define INSTDONE_I965	0x0206c
>> +#define I915_NUM_INSTDONE_REG	2
>>  #define RING_IPEIR(base)	((base)+0x64)
>>  #define RING_IPEHR(base)	((base)+0x68)
>>  #define RING_INSTDONE(base)	((base)+0x6c)
>> --
>> 1.7.11.5

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 3/3 v2] drm/i915: Use new INSTDONE registers

[Ben Widawsky]

Using the extracted INSTDONE reading, and our new register definitions,
update our hangcheck and error collection to use it.

v2: Now assuming i915_get_extra_instdone does the memset we can clean up the
code a bit (Jani)
I ignored some of Jani's bikesheds as I didn't see any value to the modification.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++---
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++---
 drivers/gpu/drm/i915/i915_irq.c     | 43 ++++++++++++++++++++++---------------
 3 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a18e936..ee9fd16 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -645,10 +645,9 @@ static void i915_ring_error_state(struct seq_file *m,
 	seq_printf(m, "  IPEIR: 0x%08x\n", error->ipeir[ring]);
 	seq_printf(m, "  IPEHR: 0x%08x\n", error->ipehr[ring]);
 	seq_printf(m, "  INSTDONE: 0x%08x\n", error->instdone[ring]);
-	if (ring == RCS && INTEL_INFO(dev)->gen >= 4) {
-		seq_printf(m, "  INSTDONE1: 0x%08x\n", error->instdone1);
+	if (ring == RCS && INTEL_INFO(dev)->gen >= 4)
 		seq_printf(m, "  BBADDR: 0x%08llx\n", error->bbaddr);
-	}
+
 	if (INTEL_INFO(dev)->gen >= 4)
 		seq_printf(m, "  INSTPS: 0x%08x\n", error->instps[ring]);
 	seq_printf(m, "  INSTPM: 0x%08x\n", error->instpm[ring]);
@@ -697,6 +696,9 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	for (i = 0; i < dev_priv->num_fence_regs; i++)
 		seq_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
 
+	for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
+		seq_printf(m, "  INSTDONE_%d: 0x%08x\n", i, error->extra_instdone[i]);
+
 	if (INTEL_INFO(dev)->gen >= 6) {
 		seq_printf(m, "ERROR: 0x%08x\n", error->error);
 		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ed3ba70..17dd520 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -198,7 +198,7 @@ struct drm_i915_error_state {
 	u32 error; /* gen6+ */
 	u32 instpm[I915_NUM_RINGS];
 	u32 instps[I915_NUM_RINGS];
-	u32 instdone1;
+	u32 extra_instdone[I915_NUM_INSTDONE_REG];
 	u32 seqno[I915_NUM_RINGS];
 	u64 bbaddr;
 	u32 fault_reg[I915_NUM_RINGS];
@@ -453,8 +453,7 @@ typedef struct drm_i915_private {
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
 	uint32_t last_acthd[I915_NUM_RINGS];
-	uint32_t last_instdone;
-	uint32_t last_instdone1;
+	uint32_t prev_instdone[I915_NUM_INSTDONE_REG];
 
 	unsigned int stop_rings;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 46eb6e9..f595d01 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1078,12 +1078,25 @@ static void i915_get_extra_instdone(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	memset(instdone, 0, sizeof(instdone));
 
-	if (INTEL_INFO(dev)->gen < 4) {
+	switch(INTEL_INFO(dev)->gen) {
+	case 2:
+	case 3:
 		instdone[0] = I915_READ(INSTDONE);
-		instdone[1] = 0;
-	} else {
+		break;
+	case 4:
+	case 5:
+	case 6:
 		instdone[0] = I915_READ(INSTDONE_I965);
 		instdone[1] = I915_READ(INSTDONE1);
+		break;
+	default:
+		WARN_ONCE(1, "Unsupported platform\n");
+	case 7:
+		instdone[0] = I915_READ(GEN7_INSTDONE_1);
+		instdone[1] = I915_READ(GEN7_SC_INSTDONE);
+		instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
+		instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
+		break;
 	}
 }
 
@@ -1109,10 +1122,8 @@ static void i915_record_ring_state(struct drm_device *dev,
 		error->ipehr[ring->id] = I915_READ(RING_IPEHR(ring->mmio_base));
 		error->instdone[ring->id] = I915_READ(RING_INSTDONE(ring->mmio_base));
 		error->instps[ring->id] = I915_READ(RING_INSTPS(ring->mmio_base));
-		if (ring->id == RCS) {
-			error->instdone1 = I915_READ(INSTDONE1);
+		if (ring->id == RCS)
 			error->bbaddr = I915_READ64(BB_ADDR);
-		}
 	} else {
 		error->faddr[ring->id] = I915_READ(DMA_FADD_I8XX);
 		error->ipeir[ring->id] = I915_READ(IPEIR);
@@ -1228,6 +1239,8 @@ static void i915_capture_error_state(struct drm_device *dev)
 		error->done_reg = I915_READ(DONE_REG);
 	}
 
+	i915_get_extra_instdone(dev, error->extra_instdone);
+
 	i915_gem_record_fences(dev, error);
 	i915_gem_record_rings(dev, error);
 
@@ -1305,7 +1318,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t instdone[I915_NUM_INSTDONE_REG];
 	u32 eir = I915_READ(EIR);
-	int pipe;
+	int pipe, i;
 
 	if (!eir)
 		return;
@@ -1320,9 +1333,9 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
+			for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
+				pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1356,12 +1369,13 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 	if (eir & I915_ERROR_INSTRUCTION) {
 		pr_err("instruction error\n");
 		pr_err("  INSTPM: 0x%08x\n", I915_READ(INSTPM));
+		for (i = 0; i < I915_NUM_INSTDONE_REG; i++)
+			pr_err("  INSTDONE_%d: 0x%08x\n", i, instdone[i]);
 		if (INTEL_INFO(dev)->gen < 4) {
 			u32 ipeir = I915_READ(IPEIR);
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR));
-			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD));
 			I915_WRITE(IPEIR, ipeir);
 			POSTING_READ(IPEIR);
@@ -1370,9 +1384,7 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 
 			pr_err("  IPEIR: 0x%08x\n", I915_READ(IPEIR_I965));
 			pr_err("  IPEHR: 0x%08x\n", I915_READ(IPEHR_I965));
-			pr_err("  INSTDONE: 0x%08x\n", instdone[0]);
 			pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
-			pr_err("  INSTDONE1: 0x%08x\n", instdone[1]);
 			pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
 			I915_WRITE(IPEIR_I965, ipeir);
 			POSTING_READ(IPEIR_I965);
@@ -1716,18 +1728,15 @@ void i915_hangcheck_elapsed(unsigned long data)
 	}
 
 	i915_get_extra_instdone(dev, instdone);
-
 	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
-	    dev_priv->last_instdone == instdone[0] &&
-	    dev_priv->last_instdone1 == instdone[1]) {
+	    memcmp(dev_priv->prev_instdone, instdone, sizeof(instdone)) == 0) {
 		if (i915_hangcheck_hung(dev))
 			return;
 	} else {
 		dev_priv->hangcheck_count = 0;
 
 		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
-		dev_priv->last_instdone = instdone[0];
-		dev_priv->last_instdone1 = instdone[1];
+		memcpy(dev_priv->prev_instdone, instdone, sizeof(instdone));
 	}
 
 repeat:
-- 
1.7.11.5