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

2018-06-13 Thread Jan Vesely 
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  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 x" can cause the attention.
>
> Best Regards
> Rex
>
>
>
>
> --
> *From:* jv...@scarletmail.rutgers.edu  on
> behalf of Jan Vesely 
> *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.leichtzumer...@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 
>
>
> 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 , 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, >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@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

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

2018-06-13 Thread 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 x" can cause the attention.

Best Regards
Rex





From: jv...@scarletmail.rutgers.edu  on behalf 
of Jan Vesely 
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 
mailto:ckoenig.leichtzumer...@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 mailto:rex@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 
mailto:christian.koe...@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, >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@lists.freedesktop.org<mailto: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

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

2018-06-13 Thread William Lewis 


On 06/13/2018 08:30 AM, Jan Vesely wrote:
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
Just a polite agreement and seconding here.  The abundance of acronyms is 
confusing but understandable, and the weekly dumps of DC updates show not a bit 
of dialog about how or why the patches were written.  Although I read the rest 
of the emails to the list, I can usually just mark that set of 18 to 30 mails 
as read and send it straight to the trash.  It's too much work to glean useful 
information from them.

Regards,
Will


On Wed, Jun 13, 2018 at 8:46 AM, Christian König 
mailto:ckoenig.leichtzumer...@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 mailto:rex@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 
mailto:christian.koe...@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, >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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx=02%7C01%7C%7C3869c12d0e1d489d0f1b08d5d131d2d5%7C84df9e7fe9f640afb435%7C1%7C0%7C636644934270387474=EllXqELZzZXDFafao7ingUyBAK4bl2T4Ja54uqdzR%2BE%3D=0


___
amd-gfx mailing list

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

2018-06-13 Thread Jan Vesely 
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.leichtzumer...@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 
>>
>
> 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 , 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, >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@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

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

2018-06-13 Thread Christian König 

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 


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 , 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, >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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2018-06-13 Thread 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 
---
 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, >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);
 }
 
 /**
-- 
1.9.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx