On Tue, 06 Sep 2016, Mika Kuoppala <mika.kuopp...@linux.intel.com> wrote:
> Imre Deak <imre.d...@intel.com> writes:
>
>> From: Ben Widawsky <benjamin.widaw...@intel.com>
>>
>> Consolidate the instdone logic so we can get a bit fancier. This patch also
>> removes the duplicated print of INSTDONE[0].
>>
>> v2: (Imre)
>> - Rebased on top of hangcheck INSTDONE changes.
>> - Move all INSTDONE registers into a single struct, store it within the
>>   engine error struct during error capturing.
>>
>> Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
>> Signed-off-by: Imre Deak <imre.d...@intel.com>
>
> Reviewed-by: Mika Kuoppala <mika.kuopp...@intel.com>

I didn't receive the original messages, and I can't find them on the
list either.

BR,
Jani.


>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c     | 41 +++++++++++-----
>>  drivers/gpu/drm/i915/i915_drv.h         |  8 ++--
>>  drivers/gpu/drm/i915/i915_gpu_error.c   | 84 
>> +++++++++++++++++++++++----------
>>  drivers/gpu/drm/i915/i915_irq.c         | 69 ++++++++++++++++++---------
>>  drivers/gpu/drm/i915/i915_reg.h         |  1 -
>>  drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++-
>>  6 files changed, 151 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 3fde507..45244f9 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1277,15 +1277,36 @@ out:
>>      return ret;
>>  }
>>  
>> +static void i915_instdone_info(struct drm_i915_private *dev_priv,
>> +                           struct seq_file *m,
>> +                           struct intel_instdone *instdone)
>> +{
>> +    seq_printf(m, "\t\tINSTDONE: 0x%08x\n",
>> +               instdone->instdone);
>> +
>> +    if (INTEL_GEN(dev_priv) <= 3)
>> +            return;
>> +
>> +    seq_printf(m, "\t\tSC_INSTDONE: 0x%08x\n",
>> +               instdone->slice_common);
>> +
>> +    if (INTEL_GEN(dev_priv) <= 6)
>> +            return;
>> +
>> +    seq_printf(m, "\t\tSAMPLER_INSTDONE: 0x%08x\n",
>> +               instdone->sampler);
>> +    seq_printf(m, "\t\tROW_INSTDONE: 0x%08x\n",
>> +               instdone->row);
>> +}
>> +
>>  static int i915_hangcheck_info(struct seq_file *m, void *unused)
>>  {
>>      struct drm_i915_private *dev_priv = node_to_i915(m->private);
>>      struct intel_engine_cs *engine;
>>      u64 acthd[I915_NUM_ENGINES];
>>      u32 seqno[I915_NUM_ENGINES];
>> -    u32 instdone[I915_NUM_INSTDONE_REG];
>> +    struct intel_instdone instdone;
>>      enum intel_engine_id id;
>> -    int j;
>>  
>>      if (!i915.enable_hangcheck) {
>>              seq_printf(m, "Hangcheck disabled\n");
>> @@ -1299,7 +1320,7 @@ static int i915_hangcheck_info(struct seq_file *m, 
>> void *unused)
>>              seqno[id] = intel_engine_get_seqno(engine);
>>      }
>>  
>> -    i915_get_extra_instdone(dev_priv, instdone);
>> +    i915_get_engine_instdone(dev_priv, RCS, &instdone);
>>  
>>      intel_runtime_pm_put(dev_priv);
>>  
>> @@ -1327,18 +1348,14 @@ static int i915_hangcheck_info(struct seq_file *m, 
>> void *unused)
>>              seq_printf(m, "\taction = %d\n", engine->hangcheck.action);
>>  
>>              if (engine->id == RCS) {
>> -                    seq_puts(m, "\tinstdone read =");
>> +                    seq_puts(m, "\tinstdone read =\n");
>>  
>> -                    for (j = 0; j < I915_NUM_INSTDONE_REG; j++)
>> -                            seq_printf(m, " 0x%08x", instdone[j]);
>> +                    i915_instdone_info(dev_priv, m, &instdone);
>>  
>> -                    seq_puts(m, "\n\tinstdone accu =");
>> +                    seq_puts(m, "\tinstdone accu =\n");
>>  
>> -                    for (j = 0; j < I915_NUM_INSTDONE_REG; j++)
>> -                            seq_printf(m, " 0x%08x",
>> -                                       engine->hangcheck.instdone[j]);
>> -
>> -                    seq_puts(m, "\n");
>> +                    i915_instdone_info(dev_priv, m,
>> +                                       &engine->hangcheck.instdone);
>>              }
>>      }
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 757b1d1..20bf72e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -747,7 +747,7 @@ struct drm_i915_error_state {
>>      u32 gam_ecochk;
>>      u32 gab_ctl;
>>      u32 gfx_mode;
>> -    u32 extra_instdone[I915_NUM_INSTDONE_REG];
>> +
>>      u64 fence[I915_MAX_NUM_FENCES];
>>      struct intel_overlay_error_state *overlay;
>>      struct intel_display_error_state *display;
>> @@ -779,7 +779,6 @@ struct drm_i915_error_state {
>>              u32 hws;
>>              u32 ipeir;
>>              u32 ipehr;
>> -            u32 instdone;
>>              u32 bbstate;
>>              u32 instpm;
>>              u32 instps;
>> @@ -790,6 +789,7 @@ struct drm_i915_error_state {
>>              u64 faddr;
>>              u32 rc_psmi; /* sleep state */
>>              u32 semaphore_mboxes[I915_NUM_ENGINES - 1];
>> +            struct intel_instdone instdone;
>>  
>>              struct drm_i915_error_object {
>>                      int page_count;
>> @@ -3560,7 +3560,9 @@ 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_i915_private *dev_priv, uint32_t 
>> *instdone);
>> +void i915_get_engine_instdone(struct drm_i915_private *dev_priv,
>> +                          enum intel_engine_id engine_id,
>> +                          struct intel_instdone *instdone);
>>  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 aed55e4..80fe101 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -228,6 +228,27 @@ static const char *hangcheck_action_to_str(enum 
>> intel_engine_hangcheck_action a)
>>      return "unknown";
>>  }
>>  
>> +static void error_print_instdone(struct drm_i915_error_state_buf *m,
>> +                             struct drm_i915_error_engine *ee)
>> +{
>> +    err_printf(m, "  INSTDONE: 0x%08x\n",
>> +               ee->instdone.instdone);
>> +
>> +    if (ee->engine_id != RCS || INTEL_GEN(m->i915) <= 3)
>> +            return;
>> +
>> +    err_printf(m, "  SC_INSTDONE: 0x%08x\n",
>> +               ee->instdone.slice_common);
>> +
>> +    if (INTEL_GEN(m->i915) <= 6)
>> +            return;
>> +
>> +    err_printf(m, "  SAMPLER_INSTDONE: 0x%08x\n",
>> +               ee->instdone.sampler);
>> +    err_printf(m, "  ROW_INSTDONE: 0x%08x\n",
>> +               ee->instdone.row);
>> +}
>> +
>>  static void error_print_engine(struct drm_i915_error_state_buf *m,
>>                             struct drm_i915_error_engine *ee)
>>  {
>> @@ -242,7 +263,9 @@ static void error_print_engine(struct 
>> drm_i915_error_state_buf *m,
>>                 (u32)(ee->acthd>>32), (u32)ee->acthd);
>>      err_printf(m, "  IPEIR: 0x%08x\n", ee->ipeir);
>>      err_printf(m, "  IPEHR: 0x%08x\n", ee->ipehr);
>> -    err_printf(m, "  INSTDONE: 0x%08x\n", ee->instdone);
>> +
>> +    error_print_instdone(m, ee);
>> +
>>      if (ee->batchbuffer) {
>>              u64 start = ee->batchbuffer->gtt_offset;
>>              u64 end = start + ee->batchbuffer->gtt_size;
>> @@ -402,10 +425,6 @@ 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]);
>> -
>>      if (INTEL_INFO(dev)->gen >= 6) {
>>              err_printf(m, "ERROR: 0x%08x\n", error->error);
>>  
>> @@ -851,7 +870,8 @@ static uint32_t i915_error_generate_code(struct 
>> drm_i915_private *dev_priv,
>>                      if (engine_id)
>>                              *engine_id = i;
>>  
>> -                    return error->engine[i].ipehr ^ 
>> error->engine[i].instdone;
>> +                    return error->engine[i].ipehr ^
>> +                           error->engine[i].instdone.instdone;
>>              }
>>      }
>>  
>> @@ -983,7 +1003,6 @@ static void error_record_engine_registers(struct 
>> drm_i915_error_state *error,
>>              ee->faddr = I915_READ(RING_DMA_FADD(engine->mmio_base));
>>              ee->ipeir = I915_READ(RING_IPEIR(engine->mmio_base));
>>              ee->ipehr = I915_READ(RING_IPEHR(engine->mmio_base));
>> -            ee->instdone = I915_READ(RING_INSTDONE(engine->mmio_base));
>>              ee->instps = I915_READ(RING_INSTPS(engine->mmio_base));
>>              ee->bbaddr = I915_READ(RING_BBADDR(engine->mmio_base));
>>              if (INTEL_GEN(dev_priv) >= 8) {
>> @@ -995,9 +1014,10 @@ static void error_record_engine_registers(struct 
>> drm_i915_error_state *error,
>>              ee->faddr = I915_READ(DMA_FADD_I8XX);
>>              ee->ipeir = I915_READ(IPEIR);
>>              ee->ipehr = I915_READ(IPEHR);
>> -            ee->instdone = I915_READ(GEN2_INSTDONE);
>>      }
>>  
>> +    i915_get_engine_instdone(dev_priv, engine->id, &ee->instdone);
>> +
>>      ee->waiting = intel_engine_has_waiter(engine);
>>      ee->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
>>      ee->acthd = intel_engine_get_active_head(engine);
>> @@ -1357,8 +1377,6 @@ 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_priv, error->extra_instdone);
>>  }
>>  
>>  static void i915_error_capture_msg(struct drm_i915_private *dev_priv,
>> @@ -1517,20 +1535,38 @@ 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_i915_private *dev_priv,
>> -                         uint32_t *instdone)
>> +void i915_get_engine_instdone(struct drm_i915_private *dev_priv,
>> +                          enum intel_engine_id engine_id,
>> +                          struct intel_instdone *instdone)
>>  {
>> -    memset(instdone, 0, sizeof(*instdone) * I915_NUM_INSTDONE_REG);
>> -
>> -    if (IS_GEN2(dev_priv) || IS_GEN3(dev_priv))
>> -            instdone[0] = I915_READ(GEN2_INSTDONE);
>> -    else if (IS_GEN4(dev_priv) || IS_GEN5(dev_priv) || IS_GEN6(dev_priv)) {
>> -            instdone[0] = I915_READ(RING_INSTDONE(RENDER_RING_BASE));
>> -            instdone[1] = I915_READ(GEN4_INSTDONE1);
>> -    } else if (INTEL_GEN(dev_priv) >= 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);
>> +    u32 mmio_base = dev_priv->engine[engine_id].mmio_base;
>> +
>> +    memset(instdone, 0, sizeof(*instdone));
>> +
>> +    switch (INTEL_GEN(dev_priv)) {
>> +    default:
>> +            instdone->instdone = I915_READ(RING_INSTDONE(mmio_base));
>> +
>> +            if (engine_id != RCS)
>> +                    break;
>> +
>> +            instdone->slice_common = I915_READ(GEN7_SC_INSTDONE);
>> +            instdone->sampler = I915_READ(GEN7_SAMPLER_INSTDONE);
>> +            instdone->row = I915_READ(GEN7_ROW_INSTDONE);
>> +
>> +            break;
>> +    case 6:
>> +    case 5:
>> +    case 4:
>> +            instdone->instdone = I915_READ(RING_INSTDONE(mmio_base));
>> +
>> +            if (engine_id == RCS)
>> +                    /* HACK: Using the wrong struct member */
>> +                    instdone->slice_common = I915_READ(GEN4_INSTDONE1);
>> +            break;
>> +    case 3:
>> +    case 2:
>> +            instdone->instdone = I915_READ(GEN2_INSTDONE);
>> +            break;
>>      }
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 82358d4..82cf823 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2550,18 +2550,36 @@ static void i915_reset_and_wakeup(struct 
>> drm_i915_private *dev_priv)
>>      }
>>  }
>>  
>> +static inline void
>> +i915_err_print_instdone(struct drm_i915_private *dev_priv,
>> +                    struct intel_instdone *instdone)
>> +{
>> +    pr_err("  INSTDONE: 0x%08x\n", instdone->instdone);
>> +
>> +    if (INTEL_GEN(dev_priv) <= 3)
>> +            return;
>> +
>> +    pr_err("  SC_INSTDONE: 0x%08x\n", instdone->slice_common);
>> +
>> +    if (INTEL_GEN(dev_priv) <= 6)
>> +            return;
>> +
>> +    pr_err("  SAMPLER_INSTDONE: 0x%08x\n", instdone->sampler);
>> +    pr_err("  ROW_INSTDONE: 0x%08x\n", instdone->row);
>> +}
>> +
>>  static void i915_report_and_clear_eir(struct drm_i915_private *dev_priv)
>>  {
>> -    uint32_t instdone[I915_NUM_INSTDONE_REG];
>> +    struct intel_instdone instdone;
>>      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_priv, instdone);
>> +    i915_get_engine_instdone(dev_priv, RCS, &instdone);
>>  
>>      if (IS_G4X(dev_priv)) {
>>              if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
>> @@ -2569,8 +2587,7 @@ static void i915_report_and_clear_eir(struct 
>> drm_i915_private *dev_priv)
>>  
>>                      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(dev_priv, &instdone);
>>                      pr_err("  INSTPS: 0x%08x\n", I915_READ(INSTPS));
>>                      pr_err("  ACTHD: 0x%08x\n", I915_READ(ACTHD_I965));
>>                      I915_WRITE(IPEIR_I965, ipeir);
>> @@ -2605,8 +2622,7 @@ static void i915_report_and_clear_eir(struct 
>> drm_i915_private *dev_priv)
>>      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(dev_priv, &instdone);
>>              if (INTEL_GEN(dev_priv) < 4) {
>>                      u32 ipeir = I915_READ(IPEIR);
>>  
>> @@ -2949,31 +2965,42 @@ static void semaphore_clear_deadlocks(struct 
>> drm_i915_private *dev_priv)
>>              engine->hangcheck.deadlock = 0;
>>  }
>>  
>> +static bool instdone_unchanged(u32 current_instdone, u32 *old_instdone)
>> +{
>> +    u32 tmp = current_instdone | *old_instdone;
>> +    bool unchanged;
>> +
>> +    unchanged = tmp == *old_instdone;
>> +    *old_instdone |= tmp;
>> +
>> +    return unchanged;
>> +}
>> +
>>  static bool subunits_stuck(struct intel_engine_cs *engine)
>>  {
>> -    u32 instdone[I915_NUM_INSTDONE_REG];
>> +    struct drm_i915_private *dev_priv = engine->i915;
>> +    struct intel_instdone instdone;
>> +    struct intel_instdone *accu_instdone = &engine->hangcheck.instdone;
>>      bool stuck;
>> -    int i;
>>  
>>      if (engine->id != RCS)
>>              return true;
>>  
>> -    i915_get_extra_instdone(engine->i915, instdone);
>> +    i915_get_engine_instdone(dev_priv, RCS, &instdone);
>>  
>>      /* There might be unstable subunit states even when
>>       * actual head is not moving. Filter out the unstable ones by
>>       * accumulating the undone -> done transitions and only
>>       * consider those as progress.
>>       */
>> -    stuck = true;
>> -    for (i = 0; i < I915_NUM_INSTDONE_REG; i++) {
>> -            const u32 tmp = instdone[i] | engine->hangcheck.instdone[i];
>> -
>> -            if (tmp != engine->hangcheck.instdone[i])
>> -                    stuck = false;
>> -
>> -            engine->hangcheck.instdone[i] |= tmp;
>> -    }
>> +    stuck = instdone_unchanged(instdone.instdone,
>> +                               &accu_instdone->instdone);
>> +    stuck &= instdone_unchanged(instdone.slice_common,
>> +                                &accu_instdone->slice_common);
>> +    stuck &= instdone_unchanged(instdone.sampler,
>> +                                &accu_instdone->sampler);
>> +    stuck &= instdone_unchanged(instdone.row,
>> +                                &accu_instdone->row);
>>  
>>      return stuck;
>>  }
>> @@ -2984,7 +3011,7 @@ head_stuck(struct intel_engine_cs *engine, u64 acthd)
>>      if (acthd != engine->hangcheck.acthd) {
>>  
>>              /* Clear subunit states on head movement */
>> -            memset(engine->hangcheck.instdone, 0,
>> +            memset(&engine->hangcheck.instdone, 0,
>>                     sizeof(engine->hangcheck.instdone));
>>  
>>              return HANGCHECK_ACTIVE;
>> @@ -3158,7 +3185,7 @@ static void i915_hangcheck_elapsed(struct work_struct 
>> *work)
>>                      /* Clear head and subunit states on seqno movement */
>>                      acthd = 0;
>>  
>> -                    memset(engine->hangcheck.instdone, 0,
>> +                    memset(&engine->hangcheck.instdone, 0,
>>                             sizeof(engine->hangcheck.instdone));
>>              }
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index a29d707..e84bc2e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1708,7 +1708,6 @@ enum skl_disp_power_wells {
>>  #define GEN7_SC_INSTDONE    _MMIO(0x7100)
>>  #define GEN7_SAMPLER_INSTDONE       _MMIO(0xe160)
>>  #define GEN7_ROW_INSTDONE   _MMIO(0xe164)
>> -#define I915_NUM_INSTDONE_REG       4
>>  #define RING_IPEIR(base)    _MMIO((base)+0x64)
>>  #define RING_IPEHR(base)    _MMIO((base)+0x68)
>>  /*
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 84aea54..afc3f1e 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -73,13 +73,21 @@ enum intel_engine_hangcheck_action {
>>  
>>  #define HANGCHECK_SCORE_RING_HUNG 31
>>  
>> +struct intel_instdone {
>> +    u32 instdone;
>> +    /* The following exist only in the RCS engine */
>> +    u32 slice_common;
>> +    u32 sampler;
>> +    u32 row;
>> +};
>> +
>>  struct intel_engine_hangcheck {
>>      u64 acthd;
>>      u32 seqno;
>>      int score;
>>      enum intel_engine_hangcheck_action action;
>>      int deadlock;
>> -    u32 instdone[I915_NUM_INSTDONE_REG];
>> +    struct intel_instdone instdone;
>>  };
>>  
>>  struct intel_ring {
>> -- 
>> 2.5.0
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to