Hi Jan,
Thanks for your suggestion. Obvious the patch title was inappropriate. I will update it. In fact, I originally plan to send this patch internally for discussion. So just thought that "Revert xxxxx" can cause the attention. Best Regards Rex ________________________________ From: [email protected] <[email protected]> on behalf of Jan Vesely <[email protected]> Sent: Wednesday, June 13, 2018 9:30 PM To: Koenig, Christian Cc: Zhu, Rex; amd-gfx list Subject: Re: [PATCH] drm/amdgpu: Partially revert commit 2dc80b006 Hi, can you please improve the commit message? seeing "Revert $HASH" conveys zero information about the code change. I'm sorry for bringing this up again, but following AMDGPU/Radeon driver development is an exercise in frustration for anyone who is not on AMD's payroll. git commit logs like: "revert XYZ" or "fix bug #123" make it really cumbersome to actually look at history and pick interesting/breaking commits. thanks, Jan On Wed, Jun 13, 2018 at 8:46 AM, Christian König <[email protected]<mailto:[email protected]>> wrote: Am 13.06.2018 um 13:40 schrieb Rex Zhu: Move the CG enablement out of delay worker thread. 1. CG/PG enablement are part of gpu hw ip initialize, we should wait for them complete. otherwise, there are some potential conflicts, for example, Suspend and CG enablement concurrently. 2. better run ib test after hw initialize completely. That is to say, ib test should be after CG/PG enablement. otherwise, the test will not cover the cg/pg/poweroff enable case. Signed-off-by: Rex Zhu <[email protected]<mailto:[email protected]>> Yeah, that thought came to my mind as well. Essentially the IB test should simulate a submission from userspace to make sure that the stack is working as expected. I think it was just moved before CG/PG to avoid issues with that, which is actually not very clever. Patch is Reviewed-by: Christian König <[email protected]<mailto:[email protected]>>, but there could be some fallout we could need to deal with. Thanks, Christian. --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 9647f54..90b78c7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1709,10 +1709,6 @@ static int amdgpu_device_ip_late_set_cg_state(struct amdgpu_device *adev) if (amdgpu_emu_mode == 1) return 0; - r = amdgpu_ib_ring_tests(adev); - if (r) - DRM_ERROR("ib ring test failed (%d).\n", r); - for (i = 0; i < adev->num_ip_blocks; i++) { if (!adev->ip_blocks[i].status.va<http://status.va>lid) continue; @@ -1793,6 +1789,9 @@ static int amdgpu_device_ip_late_init(struct amdgpu_device *adev) } } + amdgpu_device_ip_late_set_cg_state(adev); + amdgpu_device_ip_late_set_pg_state(adev); + queue_delayed_work(system_wq, &adev->late_init_work, msecs_to_jiffies(AMDGPU_RESUME_MS)); @@ -1921,8 +1920,11 @@ static void amdgpu_device_ip_late_init_func_handler(struct work_struct *work) { struct amdgpu_device *adev = container_of(work, struct amdgpu_device, late_init_work.work<http://late_init_work.work>); - amdgpu_device_ip_late_set_cg_state(adev); - amdgpu_device_ip_late_set_pg_state(adev); + int r; + + r = amdgpu_ib_ring_tests(adev); + if (r) + DRM_ERROR("ib ring test failed (%d).\n", r); } /** _______________________________________________ amd-gfx mailing list [email protected]<mailto:[email protected]> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
