<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">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: </div> <blockquote cite="mid:FD5696F5CDD4B04D92C97DB96167DECA265365CB@SCYBEXDAG04.amd.com" type="cite"> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> <div class="WordSection1"> 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 class="moz-txt-link-freetext" 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 class="moz-txt-link-abbreviated" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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 moz-do-not-send="true" 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> </div> </blockquote> </body> </html>