thank you. Every effort to make the patches easier to follow is appreciated.
Jan
On Wed, Jun 13, 2018 at 10:39 AM, Zhu, Rex <Rex.Zhu at amd.com> wrote:
> 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> 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>
>
>
> 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>, 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.valid)
> 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);
> - 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
> 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/400a21a1/attachment.html>
