Am 14.02.19 um 20:03 schrieb Grodzovsky, Andrey:
On 2/14/19 12:54 PM, Kazlauskas, Nicholas wrote:
On 2/14/19 12:47 PM, Grodzovsky, Andrey wrote:
On 2/14/19 11:57 AM, Kazlauskas, Nicholas wrote:
On 2/14/19 11:47 AM, Grodzovsky, Andrey wrote:
On 2/14/19 11:07 AM, Michel Dänzer wrote:
On 2019-02-14 4:54 p.m., Kazlauskas, Nicholas wrote:
On 2/14/19 10:42 AM, Grodzovsky, Andrey wrote:
On 2/14/19 4:05 AM, Christian König wrote:
Am 13.02.19 um 19:58 schrieb Andrey Grodzovsky:
When ring hang happens amdgpu_dm_commit_planes during flip is holding
the BO reserved and then stack waiting for fences to signal in
reservation_object_wait_timeout_rcu (which won't signal because there
was a hnag). Then when we try to shutdown display block during reset
recovery from drm_atomic_helper_suspend we also try to reserve the BO
from dm_plane_helper_cleanup_fb ending in deadlock.
Also remove useless WARN_ON
Well it is good that you pointed this out, but there are more problems
than just waiting wile the BO is reserved here.
Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 19
+++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index acc4ff8..f8dec36 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4802,14 +4802,21 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
*/
abo = gem_to_amdgpu_bo(fb->obj[0]);
r = amdgpu_bo_reserve(abo, true);
Well why do we reserve the BO in the first place? As the name
indicates reservation_object_wait_timeout_rcu() just uses rcu to wait
for the BO to be idle, no need to actually reserve it at all.
That a good point, I honestly can't remember any more why it's here...
It does look unnecessary given that we are not validating the BO here.
I think we reserve it to grab the tiling flags to update the plane
address (and some other bits of the plane state with a patch I'm
currently working on). I'm guessing that we still need the reservation
so nothing else tries to grab it after it becomes idle, but I'm not
overly familiar with that bit.
The BO is already pinned at this point, isn't it? That's enough to
prevent it from moving, no reservation needed.
Yes, agree.
Andrey
I'm not worried about the buffer moving so much as I am about userspace
changing the tiling flags by calling the IOCTL. Can that happen when
it's pinned?
Nicholas Kazlauskas
In any case this has nothing to do with reserving the BO.
Andrey
Technically it does? See the function:
void amdgpu_bo_get_tiling_flags(struct amdgpu_bo *bo, u64 *tiling_flags)
{
lockdep_assert_held(&bo->tbo.resv->lock.base);
if (tiling_flags)
*tiling_flags = bo->tiling_flags;
}
Nicholas Kazlauskas
What I meant that it has nothing to do with this BO reservation in the
flip code while waiting for fences. Seems it's dafe to remove it.
Yeah, agree. That looks actually quite superfluous to me.
Following up on Michel's argument you would actually need to keep the BO
reserved until the scanout is completed when you really want to prevent
that the tilling flags don't change.
I would just completely drop that function and access bo->tilling_flags
directly here.
Christian.
Andrey
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
