From: Daniel Vetter <daniel@ffwll.ch>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
Date: Fri, 2 Oct 2015 15:03:57 +0200 [thread overview]
Message-ID: <20151002130357.GM3383@phenom.ffwll.local> (raw)
In-Reply-To: <1443779085.4038.6.camel@intel.com>
On Fri, Oct 02, 2015 at 12:44:45PM +0300, Imre Deak wrote:
> On Thu, 2015-10-01 at 16:56 -0700, Ben Widawsky wrote:
> > On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > > From: Ben Widawsky <benjamin.widawsky@intel.com>
> > >
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > >
> > > ---
> > > Changes (Imre):
> > > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > > - keep printing the render ring INSTDONE to dmesg
> > > - don't hard code the extra_instdone array sizes
> > > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > > - fix typo when capturing to extra->row
> > > - warn if the MCR selectors are non-zero
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 6 ++--
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 62 +++++++++++++++++++++++++++++++----
> > > drivers/gpu/drm/i915/i915_irq.c | 4 +--
> > > drivers/gpu/drm/i915/i915_reg.h | 5 +++
> > > 4 files changed, 66 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 621acf1..d1b4011 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> > > u32 gam_ecochk;
> > > u32 gab_ctl;
> > > u32 gfx_mode;
> > > +#define INSTDONE_SLICE_NUM 3
> > > +#define INSTDONE_SUBSLICE_NUM 3
> > > struct extra_instdone {
> > > u32 slice_common;
> > > - u32 sampler;
> > > - u32 row;
> > > + u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > > + u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > > } extra_instdone;
> > >
> > > u64 fence[I915_MAX_NUM_FENCES];
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index e78e512..c6d1cbc 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > struct drm_i915_error_state *error = error_priv->error;
> > > struct drm_i915_error_object *obj;
> > > - int i, j, offset, elt;
> > > + int i, j, slice, subslice, offset, elt;
> > > int max_hangcheck_score;
> > >
> > > if (!error) {
> > > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> > >
> > > err_printf(m, " SC_INSTDONE (slice common): 0x%08x\n",
> > > error->extra_instdone.slice_common);
> > > - err_printf(m, " SAMPLER_INTSDONE: 0x%08x\n",
> > > - error->extra_instdone.sampler);
> > > - err_printf(m, " ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > > + for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > > + for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > > + subslice++) {
> > > + struct extra_instdone *extra = &error->extra_instdone;
> > > +
> > > + err_printf(m, " SAMPLER_INTSDONE: 0x%08x\n",
> > > + extra->sampler[slice][subslice]);
> > > + err_printf(m, " ROW_INSTDONE: 0x%08x\n",
> > > + extra->row[slice][subslice]);
> > > + }
> > > + }
> > >
> > > if (INTEL_INFO(dev)->gen >= 6) {
> > > err_printf(m, "ERROR: 0x%08x\n", error->error);
> > > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
> > > }
> > > }
> > >
> > > -/* NB: please notice the memset */
> > > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, int
> > > + slice, int subslice, uint32_t offset) {
> > > + /*
> > > + * XXX: The MCR register should be locked, but since we are only using
> > > + * it for debug/error state, it's not terribly important to
> > > + * synchronize it properly.
> > > + */
> > > + uint32_t tmp = I915_READ(GEN8_MCR_SELECTOR);
> > > + uint32_t ret;
> > > +
> > > + /*
> > > + * The HW expects the slice and sublice selectors to be reset to 0
> > > + * after reading out the registers.
> > > + */
> > > + WARN_ON_ONCE(tmp & (GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK));
> > > +
> > > + I915_WRITE(GEN8_MCR_SELECTOR, tmp | GEN8_MCR_SLICE(slice) |
> > > + GEN8_MCR_SUBSLICE(subslice));
> > > + ret = I915_READ(offset);
> > > + I915_WRITE(GEN8_MCR_SELECTOR, tmp);
> > > +
> > > + return ret;
> > > +}
> >
> > Hmm. I have second thoughts on this. We should take struct_mutex to prevent any
> > reads/writes while we're doing this.
>
> Right. I guess taking mutex_lock on the error path would be frowned
> upon. So how about extending uncore.lock from I915_WRITE to the whole
> sequence instead?
Don't grab locks from the error handling code. And it looks like we're
doing funky stuff here anyway, so I'd just leave this as-is.
-Daniel
>
> > > +
> > > void i915_get_extra_instdone(struct drm_device *dev,
> > > struct extra_instdone *extra)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > + int slice, subslice;
> > >
> > > /*
> > > * The render INSTDONE register (GEN2_INSTDONE, RING_INSTDONE) is read
> > > @@ -1396,8 +1428,24 @@ void i915_get_extra_instdone(struct drm_device *dev,
> > > switch (INTEL_INFO(dev)->gen) {
> > > default:
> > > extra->slice_common = I915_READ(GEN7_SC_INSTDONE);
> > > - extra->sampler = I915_READ(GEN7_SAMPLER_INSTDONE);
> > > - extra->row = I915_READ(GEN7_ROW_INSTDONE);
> > > + for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > > + for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > > + subslice++) {
> > > + extra->sampler[slice][subslice] =
> > > + instdone_read(dev_priv,
> > > + slice, subslice,
> > > + GEN7_SAMPLER_INSTDONE);
> > > + extra->row[slice][subslice] =
> > > + instdone_read(dev_priv,
> > > + slice, subslice,
> > > + GEN7_ROW_INSTDONE);
> > > + }
> > > + }
> > > + break;
> > > + case 7:
> > > + extra->slice_common = I915_READ(GEN7_SC_INSTDONE);
> > > + extra->sampler[0][0] = I915_READ(GEN7_SAMPLER_INSTDONE);
> > > + extra->row[0][0] = I915_READ(GEN7_ROW_INSTDONE);
> > > break;
> > > case 6:
> > > case 5:
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 8a3dc73..3cfcd1f 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2478,8 +2478,8 @@ i915_err_print_instdone(uint32_t render, struct extra_instdone *extra)
> > > {
> > > pr_err(" INSTDONE (render): 0x%08x\n", render);
> > > pr_err(" INSTDONE (common): 0x%08x\n", extra->slice_common);
> > > - pr_err(" INSTDONE (sampler): 0x%08x\n", extra->sampler);
> > > - pr_err(" INSTDONE (row): 0x%08x\n", extra->row);
> > > + pr_err(" INSTDONE (sampler): 0x%08x\n", extra->sampler[0][0]);
> > > + pr_err(" INSTDONE (row): 0x%08x\n", extra->row[0][0]);
> > > }
> > >
> > > static void i915_report_and_clear_eir(struct drm_device *dev)
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index ac5f49a..2412ec7 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1596,6 +1596,11 @@ enum skl_disp_power_wells {
> > > #define GEN7_SC_INSTDONE 0x07100
> > > #define GEN7_SAMPLER_INSTDONE 0x0e160
> > > #define GEN7_ROW_INSTDONE 0x0e164
> > > +#define GEN8_MCR_SELECTOR 0xfdc
> > > +#define GEN8_MCR_SLICE(slice) (((slice) & 3) << 26)
> > > +#define GEN8_MCR_SLICE_MASK (GEN8_MCR_SLICE(3))
> > > +#define GEN8_MCR_SUBSLICE(slice) (((slice) & 3) << 24)
> > > +#define GEN8_MCR_SUBSLICE_MASK (GEN8_MCR_SUBSLICE(3))
> > > #define RING_IPEIR(base) ((base)+0x64)
> > > #define RING_IPEHR(base) ((base)+0x68)
> > > /*
> > > --
> > > 2.1.4
> > >
> >
>
>
--
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
prev parent reply other threads:[~2015-10-02 13:01 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-30 20:00 [PATCH 0/5] drm/i915: per slice/subslice INSTDONE capturing Imre Deak
2015-09-30 20:00 ` [PATCH 1/5] drm/i915: remove duplicate names for the render ring INSTDONE register Imre Deak
2015-09-30 20:00 ` [PATCH 2/5] drm/i915: rename INSTDONE to GEN2_INSTDONE Imre Deak
2015-09-30 20:00 ` [PATCH 3/5] drm/i915: rename INSTDONE1 to GEN4_INSTDONE1 Imre Deak
2015-10-01 22:58 ` Ben Widawsky
2015-09-30 20:00 ` [PATCH 4/5] drm/i915: Cleanup instdone collection Imre Deak
2015-09-30 20:00 ` [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice Imre Deak
2015-10-01 23:00 ` Ben Widawsky
2015-10-02 10:58 ` Imre Deak
2015-10-02 15:52 ` Ben Widawsky
2015-10-01 23:56 ` Ben Widawsky
2015-10-02 9:44 ` Imre Deak
2015-10-02 13:03 ` Daniel Vetter [this message]
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=20151002130357.GM3383@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=benjamin.widawsky@intel.com \
--cc=imre.deak@intel.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox