All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jani Nikula <jani.nikula@linux.intel.com>
To: intel-gfx@lists.freedesktop.org
Cc: Ben Widawsky <ben@bwidawsk.net>
Subject: Re: [PATCH 3/3] drm/i915: Use new INSTDONE registers
Date: Thu, 16 Aug 2012 09:31:51 +0300	[thread overview]
Message-ID: <87vcgjcppk.fsf@intel.com> (raw)
In-Reply-To: <1345095063-3775-4-git-send-email-ben@bwidawsk.net>

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

  reply	other threads:[~2012-08-16  6:28 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-16  5:31 [PATCH 0/3] Some INSTDONE updates Ben Widawsky
2012-08-16  5:31 ` [PATCH 1/3] drm/i915: Extract reading INSTDONE Ben Widawsky
2012-08-16  6:25   ` Jani Nikula
2012-08-16 15:52     ` Ben Widawsky
2012-08-16  5:31 ` [PATCH 2/3] drm/i915: Add new INSTDONE registers Ben Widawsky
2012-08-16  5:31 ` [PATCH 3/3] drm/i915: Use " Ben Widawsky
2012-08-16  6:31   ` Jani Nikula [this message]
2012-08-16 18:43     ` Ben Widawsky
2012-08-17  5:05 ` [PATCH 1/3 v2] drm/i915: Extract reading INSTDONE Ben Widawsky
2012-08-17  8:46   ` Jani Nikula
2012-08-17 17:01     ` Ben Widawsky
2012-08-17  5:06 ` [PATCH 3/3 v2] drm/i915: Use new INSTDONE registers Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87vcgjcppk.fsf@intel.com \
    --to=jani.nikula@linux.intel.com \
    --cc=ben@bwidawsk.net \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.