[PATCH] drm/amdgpu: Partially revert commit 2dc80b006

[Zhu, Rex]

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: jv356 at scarletmail.rutgers.edu <jv356 at scarletmail.rutgers.edu> on behalf of Jan Vesely <jan.vesely at rutgers.edu>
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 <ckoenig.leichtzumerken at gmail.com<mailto:ckoenig.leichtzumerken at gmail.com>> 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 <Rex.Zhu at amd.com<mailto:Rex.Zhu at amd.com>>

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 <christian.koenig at amd.com<mailto:christian.koenig at amd.com>>, 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
amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180613/13f6dc39/attachment-0001.html>