<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body bgcolor="white" lang="EN-US" link="blue" vlink="purple"> <div class="WordSection1"> Hi,<o:p></o:p> <o:p> </o:p> If it is expected that the present_execute should be done when target_msc==crtc_msc in this case, I think it is better to keep the code in X side instead of various DDX drivers. Without this change, there is no functionality problem, but it has big impact on performance. Maybe the performance problem was not noticed at the beginning.<o:p></o:p> <o:p> </o:p> Going through the code below, there seems no difference between “divisor==1 && remainder==0” and “divisor==0” for the target_msc calculation.<o:p></o:p> <o:p> </o:p> if (crtc_msc >= target_msc) {<o:p></o:p> if (divisor != 0) {<o:p></o:p> target_msc = crtc_msc - (crtc_msc % divisor) + remainder;<o:p></o:p> if (options & PresentOptionAsync) {<o:p></o:p> if (target_msc < crtc_msc)<o:p></o:p> target_msc += divisor;<o:p></o:p> } else {<o:p></o:p> if (target_msc <= crtc_msc)<o:p></o:p> target_msc += divisor;<o:p></o:p> }<o:p></o:p> } else {<o:p></o:p> target_msc = crtc_msc;<o:p></o:p> if (!(options & PresentOptionAsync))<o:p></o:p> target_msc++;<o:p></o:p> }<o:p></o:p> }<o:p></o:p> <o:p> </o:p> <div> Regards,<o:p></o:p> Jammy<o:p></o:p> </div> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Axel Davy [mailto:axel.davy@ens.fr] Sent: Tuesday, November 03, 2015 3:52 PM To: Zhou, Jammy; Michel Dänzer Cc: xorg-devel@lists.x.org; Mario Kleiner Subject: Re: [PATCH] present: Execute right away if target_msc equals current_msc<o:p></o:p> </div> </div> <o:p> </o:p> <div> Hi, I realize reading what I wrote that I made the case for the 'target_msc > crtc_msc' check, and did not explain the 'target_msc >= crtc_msc' check. I guess the only possible explanation could be that DDXs were expected to trigger present_execute straight away when target_msc == crtc_msc, but it doesn't make much sense. It is strange the issue hasn't been noticed before. What should be the behaviour difference when 'divisor == 1' and 'remainder == 0' versus 'divisor == 0' ? Yours, Axel On 03/11/2015 06:18, Zhou, Jammy wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> Actually my current problem happens when ‘divisor==1’ and ‘remainder==0’, and target_msc always equals crtc_msc if PresentOptionAsync is specified in this case. It seems the same even if we set ‘divisor=0’. With current implementation, the code path of present_queue_vblank is always triggered when they are equal. But it is not the expected behavior according to the present protocol.<o:p></o:p> <o:p></o:p> <div> Regards,<o:p></o:p> Jammy<o:p></o:p> </div> <o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Axel Davy [<a href="mailto:axel.davy@ens.fr">mailto:axel.davy@ens.fr</a>] Sent: Monday, November 02, 2015 5:28 PM To: Zhou, Jammy; Michel Dänzer Cc: <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>; Mario Kleiner Subject: Re: [PATCH] present: Execute right away if target_msc equals current_msc<o:p></o:p> </div> </div> <o:p></o:p> <div> Yes, I assume divisor = 0. Initially the code was like that: . if crtc_msc > target_msc, set target_msc to crtc_msc, and if PresentOptionAsync not requested, then increase target_msc. . if ddx doing sync flips only, and we plan to do a flip, then decrease target_msc . if target_msc was already reached, do a copy immediately (or async flip) Lets give as example a few scenarios: the screen is currently at msc crtc_msc. Let's assume ddx is not able to do async flips: . Client presents for target_msc <= crtc_msc -> target_msc set to crtc_msc -> if Async not requested, target_msc increased by one => We present immediately (copy) if Async requested else if Async not requested: -> if we plan to do a flip, we decrement target_msc (because sync flips delay effect by one msc) -> if we don't plan to flip, we wait next vblank and copy then. . Client presents for target_msc = crtc_msc + 1 -> PresentOptionAsync doesn't change anything -> if doing flip then target_msc is decremented (same reason than before). In these two cases, when at the end of present_pixmap, we have target_msc = crtc_msc, then we are in the case where we must copy immediately and won't be doing flip. That explains the " target_msc > crtc_msc" check. I don't know if that system worked perfectly when ddx was async flips able. I haven't checked yet if the new code is ok for sync flips. Yours, Axel On 02/11/2015 09:56, Zhou, Jammy wrote :<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>Thanks Axel. It is consistent with my understanding. <o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>For the 'pixmap' check in your previous change, it is used to differentiate present_pixmap and present_notify_msc, right? It looks like "target_msc >= crtc_msc" was used at the first place for present_pixmap, do you know the background about this?<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Regards,<o:p></o:p></pre> <pre>Jammy<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>-----Original Message-----<o:p></o:p></pre> <pre>From: Axel Davy [<a href="mailto:axel.davy@ens.fr">mailto:axel.davy@ens.fr</a>] <o:p></o:p></pre> <pre>Sent: Monday, November 02, 2015 4:45 PM<o:p></o:p></pre> <pre>To: Zhou, Jammy; Michel Dänzer<o:p></o:p></pre> <pre>Cc: <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>; Mario Kleiner<o:p></o:p></pre> <pre>Subject: Re: [PATCH] present: Execute right away if target_msc equals current_msc<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Hi,<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>As you can see here:<o:p></o:p></pre> <pre><a href="http://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt#n212">http://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt#n212</a><o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>It means to present immediately instead of waiting for the next vblank (if needed).<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>In practice when needed to present immediately, if the ddx is not able to do async flips, then we do a copy to the screen buffer, else we use that ability.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Yours,<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Axel<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>On 02/11/2015 09:26, Zhou, Jammy wrote :<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>By the way, does PresentOptionAsync mean the same thing with the async flip capability in kernel side? Or is it just a flag to indicate X/DDX to do present immediately instead of waiting for the next vblank?<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Regards,<o:p></o:p></pre> <pre>Jammy<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>-----Original Message-----<o:p></o:p></pre> <pre>From: Axel Davy [<a href="mailto:axel.davy@ens.fr">mailto:axel.davy@ens.fr</a>]<o:p></o:p></pre> <pre>Sent: Thursday, October 29, 2015 3:42 PM<o:p></o:p></pre> <pre>To: Michel Dänzer; Zhou, Jammy<o:p></o:p></pre> <pre>Cc: <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>; Mario Kleiner<o:p></o:p></pre> <pre>Subject: Re: [PATCH] present: Execute right away if target_msc equals <o:p></o:p></pre> <pre>current_msc<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Hi,<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>What is the current conscensus for how should PresentOptionAsync work ?<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>If I remember correctly, the semantic used to be:<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>. if we present at the current or past msc with the flag, two options:<o:p></o:p></pre> <pre>-> if the ddx doesn't support async swap, we do copy to the screen<o:p></o:p></pre> <pre>pixmap right away<o:p></o:p></pre> <pre>-> if the ddx does support it, we do an async swap right away<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>. if we present at another msc, things behave as without PresentOptionAsync, that is we schedule a swap, and ask at msc-1 to the ddx to swap for msc.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>With "Fix use of vsynced pageflips", I get the impression it shifted to:<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>. Async requested, but driver not having the async option -> do screen copy (no flips, whatever the msc) . if Async not requested and flip planned, present at msc-1, else at msc. (so for flips and Async flag, we do always plan to flip at msc ?<o:p></o:p></pre> <pre>That means always tear, right ?)<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>That doesn't make a lot of sense to me, can someone clarify ?<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>I'm afraid this patch could be a workaround to currently broken behaviour, and not the correct fix.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>I CCed Mario Kleiner.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Yours,<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Axel Davy<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>On 29/10/2015 03:53, Michel Dänzer wrote:<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>On 28.10.2015 19:39, Jammy Zhou wrote:<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>It is according to the protocol:<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>"If 'options' contains PresentOptionAsync, and the 'target-msc'<o:p></o:p></pre> <pre>is less than or equal to the current msc for 'window', then the <o:p></o:p></pre> <pre>operation will be performed as soon as possible, not necessarily <o:p></o:p></pre> <pre>waiting for the next vertical blank interval."<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Signed-off-by: Jammy Zhou <a href="mailto:Jammy.Zhou@amd.com"><Jammy.Zhou@amd.com></a><o:p></o:p></pre> <pre>---<o:p></o:p></pre> <pre> present/present.c | 2 +-<o:p></o:p></pre> <pre> 1 file changed, 1 insertion(+), 1 deletion(-)<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>diff --git a/present/present.c b/present/present.c index<o:p></o:p></pre> <pre>beb4ff0..5900c22 100644<o:p></o:p></pre> <pre>--- a/present/present.c<o:p></o:p></pre> <pre>+++ b/present/present.c<o:p></o:p></pre> <pre>@@ -871,7 +871,7 @@ present_pixmap(WindowPtr window,<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> xorg_list_add(&vblank->event_queue, &present_exec_queue);<o:p></o:p></pre> <pre> vblank->queued = TRUE;<o:p></o:p></pre> <pre>- if ((pixmap && target_msc >= crtc_msc) || (!pixmap && target_msc > crtc_msc)) {<o:p></o:p></pre> <pre>+ if (target_msc > crtc_msc) {<o:p></o:p></pre> <pre> ret = present_queue_vblank(screen, target_crtc, vblank->event_id, target_msc);<o:p></o:p></pre> <pre> if (ret == Success)<o:p></o:p></pre> <pre> return Success;<o:p></o:p></pre> <pre> <o:p></o:p></pre> </blockquote> <pre>Looks good to me, but Cc'ing Axel Davy, who made the last change to <o:p></o:p></pre> <pre>this code.<o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre>Reviewed-by: Michel Dänzer <a href="mailto:michel.daenzer@amd.com"><michel.daenzer@amd.com></a><o:p></o:p></pre> <pre> <o:p></o:p></pre> <pre> <o:p></o:p></pre> </blockquote> </blockquote> <pre> <o:p></o:p></pre> </blockquote> <o:p></o:p> </blockquote> <o:p> </o:p> </div> </body> </html>