Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Michel Dänzer]

On 03.11.2015 15:43, Zhou, Jammy wrote:
>> If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate the 
>> userspace event immediately
> Yes, that works. But the round trip of the vblank handling makes the 
> performance worse.

Any chance you could share specific numbers? Otherwise, it's hard for me
to imagine how this could make a significant difference.


> -Original Message-
> From: Michel Dänzer [mailto:mic...@daenzer.net] 
> Sent: Monday, November 02, 2015 5:44 PM
> To: Zhou, Jammy
> Cc: xorg-driver-ati@lists.x.org
> Subject: Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if 
> target_msc <= crtc_msc (v2)
> 
> On 29.10.2015 14:34, Jammy Zhou wrote:
>> Do present immediately in this case.
>>
>> v2: fix the overflow
>>
>> Change-Id: I25b9212169ccbf572b88c033dc09c0ac5cfa4812
>> Signed-off-by: Jammy Zhou <jammy.z...@amd.com>
>> ---
>>  src/amdgpu_present.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c index 
>> 5e5ed72..b9cc1c7 100644
>> --- a/src/amdgpu_present.c
>> +++ b/src/amdgpu_present.c
>> @@ -160,6 +160,16 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t 
>> event_id, uint64_t msc)
>>  struct amdgpu_drm_queue_entry *queue;
>>  drmVBlank vbl;
>>  int ret;
>> +uint64_t crtc_ust, crtc_msc;
>> +
>> +ret = amdgpu_present_get_ust_msc(crtc, _ust, _msc);
>> +if (ret != Success)
>> +return ret;
>> +
>> +if (crtc_msc >= msc) {
>> +present_event_notify(event_id, crtc_ust, crtc_msc);
>> +return Success;
>> +}
> 
> If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate the 
> userspace event immediately, so I'm not sure anymore why this patch would 
> make any difference. Can you provide more details about what exactly happens 
> with and without this patch?
> 
> 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

RE: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Zhou, Jammy]

> Any chance you could share specific numbers?
When run some Vulkan application, the FPS jumps from ~80 to ~1000 with this 
patch for PresentOptionAsync. 

Regards,
Jammy

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Wednesday, November 04, 2015 11:23 AM
To: Zhou, Jammy
Cc: xorg-driver-ati@lists.x.org
Subject: Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if 
target_msc <= crtc_msc (v2)

On 03.11.2015 15:43, Zhou, Jammy wrote:
>> If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate 
>> the userspace event immediately
> Yes, that works. But the round trip of the vblank handling makes the 
> performance worse.

Any chance you could share specific numbers? Otherwise, it's hard for me to 
imagine how this could make a significant difference.


> -Original Message-
> From: Michel Dänzer [mailto:mic...@daenzer.net]
> Sent: Monday, November 02, 2015 5:44 PM
> To: Zhou, Jammy
> Cc: xorg-driver-ati@lists.x.org
> Subject: Re: [PATCH xf86-video-amdgpu] Call present_event_notify 
> directly if target_msc <= crtc_msc (v2)
> 
> On 29.10.2015 14:34, Jammy Zhou wrote:
>> Do present immediately in this case.
>>
>> v2: fix the overflow
>>
>> Change-Id: I25b9212169ccbf572b88c033dc09c0ac5cfa4812
>> Signed-off-by: Jammy Zhou <jammy.z...@amd.com>
>> ---
>>  src/amdgpu_present.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c index
>> 5e5ed72..b9cc1c7 100644
>> --- a/src/amdgpu_present.c
>> +++ b/src/amdgpu_present.c
>> @@ -160,6 +160,16 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t 
>> event_id, uint64_t msc)
>>  struct amdgpu_drm_queue_entry *queue;
>>  drmVBlank vbl;
>>  int ret;
>> +uint64_t crtc_ust, crtc_msc;
>> +
>> +ret = amdgpu_present_get_ust_msc(crtc, _ust, _msc);
>> +if (ret != Success)
>> +return ret;
>> +
>> +if (crtc_msc >= msc) {
>> +present_event_notify(event_id, crtc_ust, crtc_msc);
>> +return Success;
>> +}
> 
> If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate the 
> userspace event immediately, so I'm not sure anymore why this patch would 
> make any difference. Can you provide more details about what exactly happens 
> with and without this patch?
> 
> 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Michel Dänzer]

On 04.11.2015 14:36, Zhou, Jammy wrote:
>> Any chance you could share specific numbers?
> When run some Vulkan application, the FPS jumps from ~80 to ~1000
> with this patch for PresentOptionAsync.

I think that indicates an issue elsewhere — either the kernel doesn't
short-circuit the userspace event delivery as it should, or the event
delivery/processing is getting delayed for some reason. It would be good
to get to the bottom of that instead of just papering over it in the
Xorg driver.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

RE: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Zhou, Jammy]

> If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate the 
> userspace event immediately
Yes, that works. But the round trip of the vblank handling makes the 
performance worse.

Regards,
Jammy

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Monday, November 02, 2015 5:44 PM
To: Zhou, Jammy
Cc: xorg-driver-ati@lists.x.org
Subject: Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if 
target_msc <= crtc_msc (v2)

On 29.10.2015 14:34, Jammy Zhou wrote:
> Do present immediately in this case.
> 
> v2: fix the overflow
> 
> Change-Id: I25b9212169ccbf572b88c033dc09c0ac5cfa4812
> Signed-off-by: Jammy Zhou <jammy.z...@amd.com>
> ---
>  src/amdgpu_present.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c index 
> 5e5ed72..b9cc1c7 100644
> --- a/src/amdgpu_present.c
> +++ b/src/amdgpu_present.c
> @@ -160,6 +160,16 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t 
> event_id, uint64_t msc)
>   struct amdgpu_drm_queue_entry *queue;
>   drmVBlank vbl;
>   int ret;
> + uint64_t crtc_ust, crtc_msc;
> +
> + ret = amdgpu_present_get_ust_msc(crtc, _ust, _msc);
> + if (ret != Success)
> + return ret;
> +
> + if (crtc_msc >= msc) {
> + present_event_notify(event_id, crtc_ust, crtc_msc);
> + return Success;
> + }

If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate the 
userspace event immediately, so I'm not sure anymore why this patch would make 
any difference. Can you provide more details about what exactly happens with 
and without this patch?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Michel Dänzer]

On 29.10.2015 14:34, Jammy Zhou wrote:
> Do present immediately in this case.
> 
> v2: fix the overflow
> 
> Change-Id: I25b9212169ccbf572b88c033dc09c0ac5cfa4812
> Signed-off-by: Jammy Zhou 
> ---
>  src/amdgpu_present.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c
> index 5e5ed72..b9cc1c7 100644
> --- a/src/amdgpu_present.c
> +++ b/src/amdgpu_present.c
> @@ -160,6 +160,16 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t 
> event_id, uint64_t msc)
>   struct amdgpu_drm_queue_entry *queue;
>   drmVBlank vbl;
>   int ret;
> + uint64_t crtc_ust, crtc_msc;
> +
> + ret = amdgpu_present_get_ust_msc(crtc, _ust, _msc);
> + if (ret != Success)
> + return ret;
> +
> + if (crtc_msc >= msc) {
> + present_event_notify(event_id, crtc_ust, crtc_msc);
> + return Success;
> + }

If crtc_msc >= msc, the DRM_IOCTL_WAIT_VBLANK ioctl should generate the
userspace event immediately, so I'm not sure anymore why this patch
would make any difference. Can you provide more details about what
exactly happens with and without this patch?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

RE: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Zhou, Jammy]

> This has basically the same effect as the xserver patch you posted yesterday, 
> right?
Yes, they are for the same issue. For existing distros/xservers without the 
xserver patch I posted, we need to fix this issue from driver side. 

> [0] BTW, what's your test-case for this?
It is a simple vulkan test application with PresentOptionAsync option 
specified, in which case we don't need to wait for the vblank.

Regards,
Jammy

-Original Message-
From: Michel Dänzer [mailto:mic...@daenzer.net] 
Sent: Thursday, October 29, 2015 2:09 PM
To: Zhou, Jammy
Cc: xorg-driver-ati@lists.x.org
Subject: Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if 
target_msc <= crtc_msc (v2)

On 29.10.2015 14:34, Jammy Zhou wrote:
> Do present immediately in this case.

This has basically the same effect as the xserver patch you posted yesterday, 
right? Is the idea to fix the problem[0] regardless of whether xserver or the 
driver has the fix?

[0] BTW, what's your test-case for this?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Michel Dänzer]

On 29.10.2015 15:51, Zhou, Jammy wrote:
>> This has basically the same effect as the xserver patch you posted
>> yesterday, right?
> Yes, they are for the same issue. For existing distros/xservers
> without the xserver patch I posted, we need to fix this issue from
> driver side.

Makes sense, but I'd like to wait for the xserver patch to land first,
to avoid diverging behaviour between the driver and xserver.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

Re: [PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Michel Dänzer]

On 29.10.2015 14:34, Jammy Zhou wrote:
> Do present immediately in this case.

This has basically the same effect as the xserver patch you posted
yesterday, right? Is the idea to fix the problem[0] regardless of
whether xserver or the driver has the fix?

[0] BTW, what's your test-case for this?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

[PATCH xf86-video-amdgpu] Call present_event_notify directly if target_msc <= crtc_msc (v2)

[Jammy Zhou]

Do present immediately in this case.

v2: fix the overflow

Change-Id: I25b9212169ccbf572b88c033dc09c0ac5cfa4812
Signed-off-by: Jammy Zhou 
---
 src/amdgpu_present.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c
index 5e5ed72..b9cc1c7 100644
--- a/src/amdgpu_present.c
+++ b/src/amdgpu_present.c
@@ -160,6 +160,16 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t 
event_id, uint64_t msc)
struct amdgpu_drm_queue_entry *queue;
drmVBlank vbl;
int ret;
+   uint64_t crtc_ust, crtc_msc;
+
+   ret = amdgpu_present_get_ust_msc(crtc, _ust, _msc);
+   if (ret != Success)
+   return ret;
+
+   if (crtc_msc >= msc) {
+   present_event_notify(event_id, crtc_ust, crtc_msc);
+   return Success;
+   }
 
event = calloc(sizeof(struct amdgpu_present_vblank_event), 1);
if (!event)
-- 
1.9.1

___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati