Bug 97594 - [amdgpu SI] "drm/amd/amdgpu: Add GRBM lock to various SI functions" breaks amdgpu support for SI
Summary: [amdgpu SI] "drm/amd/amdgpu: Add GRBM lock to various SI functions" breaks am...
Status: RESOLVED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/AMDgpu (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-04 21:31 UTC by Arek Ruśniak
Modified: 2016-09-08 07:15 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Attachments
dmesg for first bad commit (58.41 KB, text/plain)
2016-09-04 21:31 UTC, Arek Ruśniak 		no flags Details
commit: deca1d1 reverted (62.13 KB, text/plain)
2016-09-04 21:45 UTC, Arek Ruśniak 		no flags Details
Patch to remove double lock (1.58 KB, patch)
2016-09-07 12:06 UTC, Tom St Denis 		no flags Details | Splinter Review
View All

Description Arek Ruśniak 2016-09-04 21:31:56 UTC 
Created attachment 126204 [details]
dmesg for first bad commit

With this commit displays off during loading amdgpu, sysrq don't work, boot is stopped. 
When i reverted it, amdgpu works as always.

dmesg is silent
Comment 1 Arek Ruśniak 2016-09-04 21:45:50 UTC 
Created attachment 126205 [details]
commit: deca1d1 reverted
Comment 2 Arek Ruśniak 2016-09-04 21:53:52 UTC 
sorry for lack of description

It's Alex's drm-next-4.9-wip branch.

First bad commit: 
commit	deca1d1f16eebfa0d070eed50a221e01cf716ee0
author	Tom St Denis <tom.stdenis@amd.com>
 
drm/amd/amdgpu: Add GRBM lock to various SI functions
Add missing lock around SE/SH/INSTANCE selections.
Comment 3 Konstantin A. Lepikhov 2016-09-06 18:59:06 UTC 
(In reply to Arek Ruśniak from comment #0)
> Created attachment 126204 [details]
> dmesg for first bad commit
> 
> With this commit displays off during loading amdgpu, sysrq don't work, boot
> is stopped. 
> When i reverted it, amdgpu works as always.
> 
> dmesg is silent

Same behavior but on PREEMPT kernel even without mentioned patch. Just to clarify that it could be more general problem than locking in this particular place.
Comment 4 Arek Ruśniak 2016-09-06 22:03:52 UTC 
Could you share config and patchset for your preempt kernel. If I will have some time I could build it. I'm just curious.

I know nothing about what preempt is. I think I should rather call my kernel - "generic" but "uname -a" gives me:
Linux darkfruit 4.8.0-rc1-20160904 #10 SMP PREEMPT Tue Sep 6 23:01:48 CEST 2016 x86_64 GNU/Linux
So I'm confused about it
Comment 5 Michel Dänzer 2016-09-07 03:45:22 UTC 
Tom, any ideas?
Comment 6 Tom St Denis 2016-09-07 12:05:09 UTC 
The problem is a double lock.  The grbm_idx_mutex lock is taken in gfx_v6_0_get_cu_info() which then calls gfx_v6_0_get_cu_active_bitmap() which also takes the lock.

The fix is to remove the lock and the select from the parent function.  I'll attach a patch momentarily.
Comment 7 Tom St Denis 2016-09-07 12:05:18 UTC 
The problem is a double lock.  The grbm_idx_mutex lock is taken in gfx_v6_0_get_cu_info() which then calls gfx_v6_0_get_cu_active_bitmap() which also takes the lock.

The fix is to remove the lock and the select from the parent function.  I'll attach a patch momentarily.
Comment 8 Tom St Denis 2016-09-07 12:06:10 UTC 
Created attachment 126274 [details] [review]
Patch to remove double lock

This removes the double lock.  It's actually patch #9 in my yet to be RB'ed series of SI cleanups ...
Comment 9 Arek Ruśniak 2016-09-07 18:46:36 UTC 
Thx, works good for me.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.