[PATCH] present: Execute right away if target_msc equals current_msc
Axel Davy
axel.davy at ens.fr
Mon Nov 2 23:52:26 PST 2015
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:
>
> 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.
>
> Regards,
>
> Jammy
>
> *From:*Axel Davy [mailto:axel.davy at ens.fr]
> *Sent:* Monday, November 02, 2015 5:28 PM
> *To:* Zhou, Jammy; Michel Dänzer
> *Cc:* xorg-devel at lists.x.org; Mario Kleiner
> *Subject:* Re: [PATCH] present: Execute right away if target_msc
> equals current_msc
>
> 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 :
>
> Thanks Axel. It is consistent with my understanding.
>
> 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?
>
> Regards,
>
> Jammy
>
> -----Original Message-----
>
> From: Axel Davy [mailto:axel.davy at ens.fr]
>
> Sent: Monday, November 02, 2015 4:45 PM
>
> To: Zhou, Jammy; Michel Dänzer
>
> Cc:xorg-devel at lists.x.org <mailto:xorg-devel at lists.x.org>; Mario Kleiner
>
> Subject: Re: [PATCH] present: Execute right away if target_msc equals current_msc
>
> Hi,
>
> As you can see here:
>
> http://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt#n212
>
> It means to present immediately instead of waiting for the next vblank (if needed).
>
> 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.
>
> Yours,
>
> Axel
>
> On 02/11/2015 09:26, Zhou, Jammy wrote :
>
> 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?
>
> Regards,
>
> Jammy
>
> -----Original Message-----
>
> From: Axel Davy [mailto:axel.davy at ens.fr]
>
> Sent: Thursday, October 29, 2015 3:42 PM
>
> To: Michel Dänzer; Zhou, Jammy
>
> Cc:xorg-devel at lists.x.org <mailto:xorg-devel at lists.x.org>; Mario Kleiner
>
> Subject: Re: [PATCH] present: Execute right away if target_msc equals
>
> current_msc
>
> Hi,
>
> What is the current conscensus for how should PresentOptionAsync work ?
>
> If I remember correctly, the semantic used to be:
>
> . if we present at the current or past msc with the flag, two options:
>
> -> if the ddx doesn't support async swap, we do copy to the screen
>
> pixmap right away
>
> -> if the ddx does support it, we do an async swap right away
>
> . 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.
>
> With "Fix use of vsynced pageflips", I get the impression it shifted to:
>
> . 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 ?
>
> That means always tear, right ?)
>
> That doesn't make a lot of sense to me, can someone clarify ?
>
> I'm afraid this patch could be a workaround to currently broken behaviour, and not the correct fix.
>
> I CCed Mario Kleiner.
>
> Yours,
>
> Axel Davy
>
> On 29/10/2015 03:53, Michel Dänzer wrote:
>
> On 28.10.2015 19:39, Jammy Zhou wrote:
>
> It is according to the protocol:
>
> "If 'options' contains PresentOptionAsync, and the 'target-msc'
>
> is less than or equal to the current msc for 'window', then the
>
> operation will be performed as soon as possible, not necessarily
>
> waiting for the next vertical blank interval."
>
> Signed-off-by: Jammy Zhou<Jammy.Zhou at amd.com> <mailto:Jammy.Zhou at amd.com>
>
> ---
>
> present/present.c | 2 +-
>
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/present/present.c b/present/present.c index
>
> beb4ff0..5900c22 100644
>
> --- a/present/present.c
>
> +++ b/present/present.c
>
> @@ -871,7 +871,7 @@ present_pixmap(WindowPtr window,
>
>
>
> xorg_list_add(&vblank->event_queue, &present_exec_queue);
>
> vblank->queued = TRUE;
>
> - if ((pixmap && target_msc >= crtc_msc) || (!pixmap && target_msc > crtc_msc)) {
>
> + if (target_msc > crtc_msc) {
>
> ret = present_queue_vblank(screen, target_crtc, vblank->event_id, target_msc);
>
> if (ret == Success)
>
> return Success;
>
> Looks good to me, but Cc'ing Axel Davy, who made the last change to
>
> this code.
>
> Reviewed-by: Michel Dänzer<michel.daenzer at amd.com> <mailto:michel.daenzer at amd.com>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20151103/27bc5d51/attachment-0001.html>
More information about the xorg-devel
mailing list