* [PATCH 1/5] drm/i915: remove duplicate names for the render ring INSTDONE register
2015-09-30 20:00 [PATCH 0/5] drm/i915: per slice/subslice INSTDONE capturing Imre Deak
@ 2015-09-30 20:00 ` Imre Deak
2015-09-30 20:00 ` [PATCH 2/5] drm/i915: rename INSTDONE to GEN2_INSTDONE Imre Deak
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2015-09-30 20:00 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
We use 3 different names to refer to the same render ring INSTDONE
register. This can be confusing when comparing two parts of the code
accessing the register via different names. Although the GEN4 version's
layout is different, we treat it the same way as the GEN7+ version, in
that we simply read it out during error capture. So remove the
duplicates and leave a comment about the GEN4 difference.
Note that there is also a GEN2 version of this register, but that's on a
different address so not handled in this patch.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
drivers/gpu/drm/i915/i915_reg.h | 6 ++++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index d979dca..27423ed 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1390,10 +1390,10 @@ void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
if (IS_GEN2(dev) || IS_GEN3(dev))
instdone[0] = I915_READ(INSTDONE);
else if (IS_GEN4(dev) || IS_GEN5(dev) || IS_GEN6(dev)) {
- instdone[0] = I915_READ(INSTDONE_I965);
+ instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
instdone[1] = I915_READ(INSTDONE1);
} else if (INTEL_INFO(dev)->gen >= 7) {
- instdone[0] = I915_READ(GEN7_INSTDONE_1);
+ instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
instdone[1] = I915_READ(GEN7_SC_INSTDONE);
instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 05c8621..d27894c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1593,14 +1593,16 @@ enum skl_disp_power_wells {
#endif
#define IPEIR_I965 0x02064
#define IPEHR_I965 0x02068
-#define INSTDONE_I965 0x0206c
-#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)
+/*
+ * On GEN4, only the render ring INSTDONE exists and has a different
+ * layout than the GEN7+ version.
+ */
#define RING_INSTDONE(base) ((base)+0x6c)
#define RING_INSTPS(base) ((base)+0x70)
#define RING_DMA_FADD(base) ((base)+0x78)
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/5] drm/i915: rename INSTDONE to GEN2_INSTDONE
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 ` Imre Deak
2015-09-30 20:00 ` [PATCH 3/5] drm/i915: rename INSTDONE1 to GEN4_INSTDONE1 Imre Deak
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2015-09-30 20:00 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
We have a bunch of INSTDONE registers for different platforms and
purposes and it's not immediately clear which instance they are just by
looking at the register name. This one was added on GEN2, where it was
the only INSTDONE register, so mark it as such.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
drivers/gpu/drm/i915/i915_reg.h | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 27423ed..85d9a39 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -886,7 +886,7 @@ static void i915_record_ring_state(struct drm_device *dev,
ering->faddr = I915_READ(DMA_FADD_I8XX);
ering->ipeir = I915_READ(IPEIR);
ering->ipehr = I915_READ(IPEHR);
- ering->instdone = I915_READ(INSTDONE);
+ ering->instdone = I915_READ(GEN2_INSTDONE);
}
ering->waiting = waitqueue_active(&ring->irq_queue);
@@ -1388,7 +1388,7 @@ void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
memset(instdone, 0, sizeof(*instdone) * I915_NUM_INSTDONE_REG);
if (IS_GEN2(dev) || IS_GEN3(dev))
- instdone[0] = I915_READ(INSTDONE);
+ instdone[0] = I915_READ(GEN2_INSTDONE);
else if (IS_GEN4(dev) || IS_GEN5(dev) || IS_GEN6(dev)) {
instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
instdone[1] = I915_READ(INSTDONE1);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d27894c..6e3d816 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1602,6 +1602,7 @@ enum skl_disp_power_wells {
/*
* On GEN4, only the render ring INSTDONE exists and has a different
* layout than the GEN7+ version.
+ * The GEN2 counterpart of this register is GEN2_INSTDONE.
*/
#define RING_INSTDONE(base) ((base)+0x6c)
#define RING_INSTPS(base) ((base)+0x70)
@@ -1619,7 +1620,7 @@ enum skl_disp_power_wells {
#define PWRCTX_EN (1<<0)
#define IPEIR 0x02088
#define IPEHR 0x0208c
-#define INSTDONE 0x02090
+#define GEN2_INSTDONE 0x02090
#define NOPID 0x02094
#define HWSTAM 0x02098
#define DMA_FADD_I8XX 0x020d0
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/5] drm/i915: rename INSTDONE1 to GEN4_INSTDONE1
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 ` 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
4 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2015-09-30 20:00 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
This register was added on GEN4, by the name INSTDONE_1 whereas the GEN6
specification calls it INSTDONE_2. Keep the original name with a
platform prefix to make it clearer which INSTDONE register instance this
is. Also add a comment about the SNB alternative name.
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
drivers/gpu/drm/i915/i915_reg.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 85d9a39..2f04e4f 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1391,7 +1391,7 @@ void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
instdone[0] = I915_READ(GEN2_INSTDONE);
else if (IS_GEN4(dev) || IS_GEN5(dev) || IS_GEN6(dev)) {
instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
- instdone[1] = I915_READ(INSTDONE1);
+ instdone[1] = I915_READ(GEN4_INSTDONE1);
} else if (INTEL_INFO(dev)->gen >= 7) {
instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
instdone[1] = I915_READ(GEN7_SC_INSTDONE);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6e3d816..c12f3b8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1611,7 +1611,7 @@ enum skl_disp_power_wells {
#define RING_INSTPM(base) ((base)+0xc0)
#define RING_MI_MODE(base) ((base)+0x9c)
#define INSTPS 0x02070 /* 965+ only */
-#define INSTDONE1 0x0207c /* 965+ only */
+#define GEN4_INSTDONE1 0x0207c /* 965+ only, aka INSTDONE_2 on SNB */
#define ACTHD_I965 0x02074
#define HWS_PGA 0x02080
#define HWS_ADDRESS_MASK 0xfffff000
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/5] drm/i915: rename INSTDONE1 to GEN4_INSTDONE1
2015-09-30 20:00 ` [PATCH 3/5] drm/i915: rename INSTDONE1 to GEN4_INSTDONE1 Imre Deak
@ 2015-10-01 22:58 ` Ben Widawsky
0 siblings, 0 replies; 13+ messages in thread
From: Ben Widawsky @ 2015-10-01 22:58 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Wed, Sep 30, 2015 at 11:00:44PM +0300, Imre Deak wrote:
> This register was added on GEN4, by the name INSTDONE_1 whereas the GEN6
> specification calls it INSTDONE_2. Keep the original name with a
> platform prefix to make it clearer which INSTDONE register instance this
> is. Also add a comment about the SNB alternative name.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> drivers/gpu/drm/i915/i915_reg.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 85d9a39..2f04e4f 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1391,7 +1391,7 @@ void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
> instdone[0] = I915_READ(GEN2_INSTDONE);
> else if (IS_GEN4(dev) || IS_GEN5(dev) || IS_GEN6(dev)) {
> instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
> - instdone[1] = I915_READ(INSTDONE1);
> + instdone[1] = I915_READ(GEN4_INSTDONE1);
> } else if (INTEL_INFO(dev)->gen >= 7) {
> instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
> instdone[1] = I915_READ(GEN7_SC_INSTDONE);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6e3d816..c12f3b8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1611,7 +1611,7 @@ enum skl_disp_power_wells {
> #define RING_INSTPM(base) ((base)+0xc0)
> #define RING_MI_MODE(base) ((base)+0x9c)
> #define INSTPS 0x02070 /* 965+ only */
> -#define INSTDONE1 0x0207c /* 965+ only */
> +#define GEN4_INSTDONE1 0x0207c /* 965+ only, aka INSTDONE_2 on SNB */
> #define ACTHD_I965 0x02074
> #define HWS_PGA 0x02080
> #define HWS_ADDRESS_MASK 0xfffff000
1-3 are:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
I'll look over 4, and 5, but since I originally authored them, I probably won't
make a good reviewer.
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] drm/i915: Cleanup instdone collection
2015-09-30 20:00 [PATCH 0/5] drm/i915: per slice/subslice INSTDONE capturing Imre Deak
` (2 preceding siblings ...)
2015-09-30 20:00 ` [PATCH 3/5] drm/i915: rename INSTDONE1 to GEN4_INSTDONE1 Imre Deak
@ 2015-09-30 20:00 ` Imre Deak
2015-09-30 20:00 ` [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice Imre Deak
4 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2015-09-30 20:00 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
From: Ben Widawsky <benjamin.widawsky@intel.com>
Consolidate the instdone logic so we can get a bit fancier. This patch also
removes the duplicated print of INSTDONE[0].
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
Changes: (Imre)
- keep capturing GEN4_INSTDONE1 too
- don't save GEN7 registers on GEN4-6
---
drivers/gpu/drm/i915/i915_drv.h | 10 ++++++--
drivers/gpu/drm/i915/i915_gpu_error.c | 43 ++++++++++++++++++++++-------------
drivers/gpu/drm/i915/i915_irq.c | 25 ++++++++++++++------
drivers/gpu/drm/i915/i915_reg.h | 1 -
4 files changed, 53 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9c10270..621acf1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -513,7 +513,12 @@ struct drm_i915_error_state {
u32 gam_ecochk;
u32 gab_ctl;
u32 gfx_mode;
- u32 extra_instdone[I915_NUM_INSTDONE_REG];
+ struct extra_instdone {
+ u32 slice_common;
+ u32 sampler;
+ u32 row;
+ } extra_instdone;
+
u64 fence[I915_MAX_NUM_FENCES];
struct intel_overlay_error_state *overlay;
struct intel_display_error_state *display;
@@ -3259,7 +3264,8 @@ void i915_error_state_get(struct drm_device *dev,
void i915_error_state_put(struct i915_error_state_file_priv *error_priv);
void i915_destroy_error_state(struct drm_device *dev);
-void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone);
+void i915_get_extra_instdone(struct drm_device *dev,
+ struct extra_instdone *extra);
const char *i915_cache_level_str(struct drm_i915_private *i915, int type);
/* i915_cmd_parser.c */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 2f04e4f..e78e512 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -383,9 +383,11 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
for (i = 0; i < dev_priv->num_fence_regs; i++)
err_printf(m, " fence[%d] = %08llx\n", i, error->fence[i]);
- for (i = 0; i < ARRAY_SIZE(error->extra_instdone); i++)
- err_printf(m, " INSTDONE_%d: 0x%08x\n", i,
- error->extra_instdone[i]);
+ 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);
if (INTEL_INFO(dev)->gen >= 6) {
err_printf(m, "ERROR: 0x%08x\n", error->error);
@@ -1233,7 +1235,7 @@ static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
error->eir = I915_READ(EIR);
error->pgtbl_er = I915_READ(PGTBL_ER);
- i915_get_extra_instdone(dev, error->extra_instdone);
+ i915_get_extra_instdone(dev, &error->extra_instdone);
}
static void i915_error_capture_msg(struct drm_device *dev,
@@ -1382,20 +1384,29 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
}
/* NB: please notice the memset */
-void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
+void i915_get_extra_instdone(struct drm_device *dev,
+ struct extra_instdone *extra)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- memset(instdone, 0, sizeof(*instdone) * I915_NUM_INSTDONE_REG);
- if (IS_GEN2(dev) || IS_GEN3(dev))
- instdone[0] = I915_READ(GEN2_INSTDONE);
- else if (IS_GEN4(dev) || IS_GEN5(dev) || IS_GEN6(dev)) {
- instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
- instdone[1] = I915_READ(GEN4_INSTDONE1);
- } else if (INTEL_INFO(dev)->gen >= 7) {
- instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
- instdone[1] = I915_READ(GEN7_SC_INSTDONE);
- instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
- instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
+ /*
+ * The render INSTDONE register (GEN2_INSTDONE, RING_INSTDONE) is read
+ * by the ring collection.
+ */
+ 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);
+ break;
+ case 6:
+ case 5:
+ case 4:
+ /* HACK: Using the wrong struct member */
+ extra->slice_common = I915_READ(GEN4_INSTDONE1);
+ break;
+ case 3:
+ case 2:
+ break;
}
}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 76bd40e..8a3dc73 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2473,19 +2473,32 @@ static void i915_reset_and_wakeup(struct drm_device *dev)
}
}
+static inline void
+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);
+}
+
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 render_instdone;
+ struct extra_instdone extra = {0};
u32 eir = I915_READ(EIR);
- int pipe, i;
+ int pipe;
if (!eir)
return;
pr_err("render error detected, EIR: 0x%08x\n", eir);
- i915_get_extra_instdone(dev, instdone);
+ render_instdone = I915_READ(INTEL_INFO(dev)->gen < 4 ?
+ GEN2_INSTDONE :
+ RING_INSTDONE(RENDER_RING_BASE));
+ i915_get_extra_instdone(dev, &extra);
if (IS_G4X(dev)) {
if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
@@ -2493,8 +2506,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));
- for (i = 0; i < ARRAY_SIZE(instdone); i++)
- pr_err(" INSTDONE_%d: 0x%08x\n", i, instdone[i]);
+ i915_err_print_instdone(render_instdone, &extra);
pr_err(" INSTPS: 0x%08x\n", I915_READ(INSTPS));
pr_err(" ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
I915_WRITE(IPEIR_I965, ipeir);
@@ -2529,8 +2541,7 @@ 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 < ARRAY_SIZE(instdone); i++)
- pr_err(" INSTDONE_%d: 0x%08x\n", i, instdone[i]);
+ i915_err_print_instdone(render_instdone, &extra);
if (INTEL_INFO(dev)->gen < 4) {
u32 ipeir = I915_READ(IPEIR);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c12f3b8..ac5f49a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1596,7 +1596,6 @@ enum skl_disp_power_wells {
#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)
/*
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
2015-09-30 20:00 [PATCH 0/5] drm/i915: per slice/subslice INSTDONE capturing Imre Deak
` (3 preceding siblings ...)
2015-09-30 20:00 ` [PATCH 4/5] drm/i915: Cleanup instdone collection Imre Deak
@ 2015-09-30 20:00 ` Imre Deak
2015-10-01 23:00 ` Ben Widawsky
2015-10-01 23:56 ` Ben Widawsky
4 siblings, 2 replies; 13+ messages in thread
From: Imre Deak @ 2015-09-30 20:00 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
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;
+}
+
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
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-01 23:56 ` Ben Widawsky
1 sibling, 1 reply; 13+ messages in thread
From: Ben Widawsky @ 2015-10-01 23:00 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
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++) {
Ideally we should only be doing this for the active slices. I was hoping you'd
have a chance to look into that. Do we know if this works on SKUs with fewer
slices?
> + 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;
> +}
> +
> 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
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
2015-10-01 23:00 ` Ben Widawsky
@ 2015-10-02 10:58 ` Imre Deak
2015-10-02 15:52 ` Ben Widawsky
0 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2015-10-02 10:58 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Thu, 2015-10-01 at 16:00 -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++) {
>
> Ideally we should only be doing this for the active slices.
Looking at it now the spec is not too clear to me:
"""
When slice 0 is disabled (when fuse reflection MMADR 0x9120[25] = 0),
this field must be set to a valid slice (slice 1 or slice 2) before
issuing a read to a register in a slice unit.
"""
Why is slice 0 special? I would understand the above if it just said
"the field must be set to a valid slice", without mentioning specific
slices.
But yes, I think it would be safer to access only active slices. I would
also limit the readout to active subslices, although there is no similar
note about it in bspec.
> I was hoping you'd have a chance to look into that.
Ok, I'll resend this patch with the above changed.
> Do we know if this works on SKUs with fewer slices?
It worked on CHV and BXT.
> > + 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;
> > +}
> > +
> > 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
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
2015-10-02 10:58 ` Imre Deak
@ 2015-10-02 15:52 ` Ben Widawsky
0 siblings, 0 replies; 13+ messages in thread
From: Ben Widawsky @ 2015-10-02 15:52 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Fri, Oct 02, 2015 at 01:58:13PM +0300, Imre Deak wrote:
> On Thu, 2015-10-01 at 16:00 -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++) {
> >
> > Ideally we should only be doing this for the active slices.
>
> Looking at it now the spec is not too clear to me:
> """
> When slice 0 is disabled (when fuse reflection MMADR 0x9120[25] = 0),
> this field must be set to a valid slice (slice 1 or slice 2) before
> issuing a read to a register in a slice unit.
> """
>
The default is to go to slice 0. So if you have slice 0 disabled (which I
believe only exists in special fused configs) you will end up reading garbage on
the MMIO. This is true of all MMIO, not just INSTDONE. In other words, if we
actually have such things shipping, they surely aren't working today.
> Why is slice 0 special? I would understand the above if it just said
> "the field must be set to a valid slice", without mentioning specific
> slices.
>
> But yes, I think it would be safer to access only active slices. I would
> also limit the readout to active subslices, although there is no similar
> note about it in bspec.
>
> > I was hoping you'd have a chance to look into that.
>
> Ok, I'll resend this patch with the above changed.
>
> > Do we know if this works on SKUs with fewer slices?
>
> It worked on CHV and BXT.
>
> > > + 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;
> > > +}
> > > +
> > > 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
> > >
> >
>
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
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-01 23:56 ` Ben Widawsky
2015-10-02 9:44 ` Imre Deak
1 sibling, 1 reply; 13+ messages in thread
From: Ben Widawsky @ 2015-10-01 23:56 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
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.
> +
> 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
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
2015-10-01 23:56 ` Ben Widawsky
@ 2015-10-02 9:44 ` Imre Deak
2015-10-02 13:03 ` Daniel Vetter
0 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2015-10-02 9:44 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
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?
> > +
> > 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
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice
2015-10-02 9:44 ` Imre Deak
@ 2015-10-02 13:03 ` Daniel Vetter
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2015-10-02 13:03 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, Ben Widawsky
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
^ permalink raw reply [flat|nested] 13+ messages in thread