[Intel-gfx] [PATCH] drm/i915: Take runtime pm reference on hangcheck_info

[Mika Kuoppala]

Daniel Vetter <daniel at ffwll.ch> writes:

> On Thu, Feb 05, 2015 at 12:16:32PM +0200, Mika Kuoppala wrote:
>> We read the coherent current seqno and actual head from ring.
>> For hardware access we need to take runtime_pm reference, which brings in
>> locking. As this debugfs entry is for debugging hangcheck behaviour,
>> including locking problems, we need to be flexible on taking them.
>> 
>> Try to see if we get a lock and if so, get seqno and actual head
>> from hardware. If we don't have exclusive access, get lazy coherent
>> seqno and print token acthd for which the user can see that the
>> seqno is of different nature.
>> 
>> Testcase: igt/pm_rpm/debugfs-read
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88910
>> Tested-by: Ding Heng <hengx.ding at intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 26 +++++++++++++++++++++++---
>>  1 file changed, 23 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 9af17fb..5a6b0e2 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1223,8 +1223,11 @@ out:
>>  static int i915_hangcheck_info(struct seq_file *m, void *unused)
>>  {
>>  	struct drm_info_node *node = m->private;
>> -	struct drm_i915_private *dev_priv = to_i915(node->minor->dev);
>> +	struct drm_device *dev = node->minor->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_engine_cs *ring;
>> +	u64 acthd[I915_NUM_RINGS];
>> +	u32 seqno[I915_NUM_RINGS];
>>  	int i;
>>  
>>  	if (!i915.enable_hangcheck) {
>> @@ -1232,6 +1235,23 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>>  		return 0;
>>  	}
>>  
>> +	if (mutex_trylock(&dev->struct_mutex)) {
>
> Why do we need dev->struct_mutex here?

Apparently intel_runtime_pm_get is fine without it. Cargoculted
from the neighbouring debugfs entry.

Only thing I can think of is that it reveals if we had
exclusive access depending of achtd token value printed. But is
that information then of any use I dont know.

I'll respin

-Mika

> -Daniel
>
>> +		intel_runtime_pm_get(dev_priv);
>> +
>> +		for_each_ring(ring, dev_priv, i) {
>> +			seqno[i] = ring->get_seqno(ring, false);
>> +			acthd[i] = intel_ring_get_active_head(ring);
>> +		}
>> +
>> +		intel_runtime_pm_put(dev_priv);
>> +		mutex_unlock(&dev->struct_mutex);
>> +	} else {
>> +		for_each_ring(ring, dev_priv, i) {
>> +			seqno[i] = ring->get_seqno(ring, true);
>> +			acthd[i] = -1;
>> +		}
>> +	}
>> +
>>  	if (delayed_work_pending(&dev_priv->gpu_error.hangcheck_work)) {
>>  		seq_printf(m, "Hangcheck active, fires in %dms\n",
>>  			   jiffies_to_msecs(dev_priv->gpu_error.hangcheck_work.timer.expires -
>> @@ -1242,12 +1262,12 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>>  	for_each_ring(ring, dev_priv, i) {
>>  		seq_printf(m, "%s:\n", ring->name);
>>  		seq_printf(m, "\tseqno = %x [current %x]\n",
>> -			   ring->hangcheck.seqno, ring->get_seqno(ring, false));
>> +			   ring->hangcheck.seqno, seqno[i]);
>>  		seq_printf(m, "\taction = %d\n", ring->hangcheck.action);
>>  		seq_printf(m, "\tscore = %d\n", ring->hangcheck.score);
>>  		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
>>  			   (long long)ring->hangcheck.acthd,
>> -			   (long long)intel_ring_get_active_head(ring));
>> +			   (long long)acthd[i]);
>>  		seq_printf(m, "\tmax ACTHD = 0x%08llx\n",
>>  			   (long long)ring->hangcheck.max_acthd);
>>  	}
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch