From: Ben Widawsky <ben@bwidawsk.net>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/i915: Extract reading INSTDONE
Date: Thu, 16 Aug 2012 08:52:55 -0700 [thread overview]
Message-ID: <02cf9b08fbb0ca2a964b46af2efbfac2@bwidawsk.net> (raw)
In-Reply-To: <87y5lfcpzp.fsf@intel.com>
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
next prev parent reply other threads:[~2012-08-16 15:52 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 [this message]
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
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=02cf9b08fbb0ca2a964b46af2efbfac2@bwidawsk.net \
--to=ben@bwidawsk.net \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
/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.