I wrote a little program to simulate how ms_kernel_msc_to_crtc_msc()
is implemented in servers 1.16 - 1.20
and how it responds to msc's delivered by the kernel.
If i didn't make a mistake, then according to the compiler it turns
out that ms_kernel_msc_to_crtc_msc is broken in all servers.
The wraparound handling never triggers when it should, so the function
is essentially a no-op. If it would, it wouldn't deal with vblank
events that arrive with slightly out-of-order msc's (ie. sometimes not
monotonically increasing), as probably can happen during dpms off/on
or suspend/resume cycles, when the kernel blasts out all pending
vblank events in fifo order. If it would, it would be more broken on
server 1.20 due to improper handling of true 64-bit msc input from the
new sequence api's of Linux 4.15+.
The attached program simulates the different variants. I'll try to
write a patch later today that implements the spirit of the
"mapit_good" function in the program.
The method of 32-bit wraparound handling is stolen from mesa commit
cc5ddd584d17abd422ae4d8e83805969485740d9 ("glx: Handle out-of-sequence
swap completion events correctly. (v2)") which happens to solve the
same problem for mapping 32-bit sbc to 64-bit sbc and dealing with
wraparound.
Just to say, maybe wait a little bit longer with server 1.20 release
to get this fixed.
thanks,
-mario
On Fri, May 4, 2018 at 2:41 PM, Daniel Stone <dan...@fooishbar.org> wrote:
> Hi Mario,
>
> On 4 May 2018 at 13:14, Mario Kleiner <mario.kleiner...@gmail.com> wrote:
>> Indeed, i found a Mesa bug yesterday which can cause Mesa's
>> PresentPixmap request to spuriously feed garbage targetMSC's
>> into the driver under some conditions. However, while other
>> video drivers seem to cope relatively well with that, modesetting
>> ddx causes KDE-5's plasmashell to lock up badly quite frequently,
>> and my suspicion is that the code removed in this commit is one
>> major source of the extra fragility.
>
> Thanks a lot for tracking this down! Adding Eero to Cc, who I think
> stumbled on this very problem.
>
> Cheers,
> Daniel
/* gcc -o vblankcalc vblankcalc.c
*
* vblankcalc 0 == Simulate Linux < 4.15 behaviour
* vblankcalc 1 == Simulate Linux >=4.15 non-pageflipped
* vblankcalc 2 == Simulate Linux >=4.15 pageflipped
*
* inseq = simulated true 64-bit counts for testing.
*/
#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>
uint64_t inseq[] = { 1, 2, 3, 4, 0x10000000, 0x20000000, 0x30000000, 0x40000000, 0x50000000, 0x60000000, 0x70000000, 0x80000000,
0x90000000, 0xa0000000, 0xb0000000, 0xc0000000, 0xd0000000, 0xe0000000, 0xf0000000, 0xfffffffd, 0xffffffff,
0x100000000, 0x100000001, 0x100000002, 0x100000003, 0x100000004, 0x100000005, 0xe0000000, 0xb0000000, 0xf0000000,
0xf00000000, 0xf00001000};
uint64_t msc_high = 0;
uint32_t msc_prev = 0;
/* Current server 1.20 implementation -- more broken than 1.19 */
uint64_t mapit_20(uint64_t sequence)
{
if ((int32_t) (sequence - msc_prev) < -0x40000000)
msc_high += 0x100000000L;
msc_prev = sequence;
return msc_high + sequence;
}
/* Server 1.16 - 1.19, only somewhat broken */
uint64_t mapit_19(uint32_t sequence)
{
/* Wrong in server 1.19: if ((int32_t) (sequence - msc_prev) < -0x40000000) */
/* Below one is better, but still not robust against unordered events */
if (((int64_t) sequence - msc_prev) < -0x40000000)
msc_high += 0x100000000L;
msc_prev = sequence;
return msc_high + sequence;
}
/* Best one could do on server 1.19 with only 32-bit kernel api */
uint64_t mapit_fixed(uint32_t sequence)
{
/* Deal with wraparound and slightly misordered events, like the
* ones that might happen in practice due to dpms off/on or
* suspend/resume iirc */
if ((int64_t) sequence < ((int64_t) msc_prev - 0x40000000))
msc_high += 0x100000000L;
if ((int64_t) sequence > ((int64_t) msc_prev + 0x40000000))
msc_high -= 0x100000000L;
msc_prev = sequence;
return msc_high + sequence;
}
/* Almost decent for taking advantage of Linux 4.15+ new 64-bit sequence api */
uint64_t mapit_good(uint64_t sequence)
{
/* 32-Bit count, probably provided by 32-Bit kernel api? */
/* Could do better by telling the function if sequence comes from
* a true 64-bit source, or not. */
if (sequence <= 0xffffffff) {
if ((int64_t) sequence < ((int64_t) msc_prev - 0x40000000))
msc_high += 0x100000000L;
if ((int64_t) sequence > ((int64_t) msc_prev + 0x40000000))
msc_high -= 0x100000000L;
msc_prev = sequence;
return msc_high + sequence;
}
/* True 64-Bit count from true 64-Bit sequence kernel 4.15+ api */
msc_prev = sequence;
msc_high = sequence & 0xffffffff00000000;
return sequence;
}
void main(int argc, char* argv[])
{
int mode = atoi(argv[1]);
for (int i = 0; i < sizeof(inseq) / sizeof(uint64_t); i++) {
uint64_t sequence;
uint64_t retseq;
printf("HW: %lu ", inseq[i]);
switch (mode) {
case 0: /* pre - Linux 4.15 or server <= 1.19 */
sequence = (uint32_t) inseq[i];
break;
case 1: /* Linux 4.15+ no pageflipping */
sequence = inseq[i];
break;
case 2: /* Linux 4.15+ with pageflipping */
if (i % 2 == 1)
/* pageflip event always 32 bit */
sequence = (uint32_t) inseq[i];
else
/* value via new 64-bit api */
sequence = inseq[i];
break;
}
printf("IN: %lu [msc_high %lu, msc_prev %u] --> ", sequence, msc_high, msc_prev);
retseq = mapit_good(sequence);
printf("OUT: %lu ", retseq);
if (retseq == inseq[i])
printf("OK\n");
else
printf("FAIL\n");
}
}
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
