Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Cleanup instdone collection

[Imre Deak]

On pe, 2016-09-16 at 12:56 +0300, Jani Nikula wrote:
> On Tue, 06 Sep 2016, Mika Kuoppala 
> wrote:
> > Imre Deak  writes:
> > 
> > > From: Ben Widawsky 
> > > 
> > > 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 
> > > Signed-off-by: Imre Deak 
> > 
> > Reviewed-by: Mika Kuoppala 
> 
> I didn't receive the original messages, and I can't find them on the
> list either.

Can't find it in the list archive either, not sure where it got stuck.
I'll resend it.

--Imre

> 
> 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, );
> > >  
> > >   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,
> > > );
> > >  
> > > - 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,
> > > +    
> > > >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;
> > >   

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Cleanup instdone collection

[Jani Nikula]

On Tue, 06 Sep 2016, Mika Kuoppala  wrote:
> Imre Deak  writes:
>
>> From: Ben Widawsky 
>>
>> 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 
>> Signed-off-by: Imre Deak 
>
> Reviewed-by: Mika Kuoppala 

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, );
>>  
>>  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, );
>>  
>> -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,
>> +   >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 

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Cleanup instdone collection

[Mika Kuoppala]

Imre Deak  writes:

> From: Ben Widawsky 
>
> 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 
> Signed-off-by: Imre Deak 

Reviewed-by: Mika Kuoppala 

> ---
>  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, );
>  
>   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, );
>  
> - 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,
> +>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