Re: [PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().

[Adam Jackson]

On Fri, 2018-05-04 at 14:14 +0200, Mario Kleiner wrote:
> The function is ported from intel-ddx uxa backend around
> 2013, where its stated purpose was to apply a vblank_offset
> to msc values to correct for problems with those kernel
> provided msc values. Some (somewhat magic and puzzling to
> myself) heuristic tried to guess if provided values were
> unreasonable and tried to adapt the corrective vblank_offset
> to account for that.
> 
> Except: It wasn't applied to kernel provided msc values,
> but the values delivered by clients via DRI2 or Present,
> so valid client targetmsc values, e.g., requesting a vblank
> event > 1000 vblanks in the future, triggered the offset
> correction in arbitrarily wrong ways, leading to wrong msc
> values being returned and thereby vblank events queued to
> the kernel for the wrong time. This causes glXSwapBuffersMscOML
> and glXWaitForMscOML to swap / return immediately whenever a
> swap/wait in > 1000 vblanks is requested.
> 
> The original code was also written to only deal with 32 bit mscs,
> but server 1.20 modesetting ddx can now use new Linux 4.15+ kernel
> vblank api to process true 64 bit msc's, which may confuse the
> heuristic even more due to 32 bit integer truncation/wrapping.
> 
> This code caused various problems in the intel-ddx in the
> past since year 2013, and was removed there in 2015 by Chris
> Wilson in commit 42ebe2ef9646be5c4586868cf332b4cd79bb4618:
> 
> "uxa: Remove the filtering of bogus Present MSC values
> 
> If the intention was to filter the return values from the kernel, the
> filtering would have been applied to the kernel values and not to the
> incoming values from Present. This filtering introduces crazy integer
> promotion and truncation bugs all because Present feeds garbage into its
> vblank requests.
> 
> "
> 
> 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.
> 
> Also my own tests fail for any swap scheduled more than 1000
> vblanks into the future, which is not uncommon for some scientific
> applications.
> 
> Iow. modesetting's swap scheduling seems to be more robust without
> this function afaics.
> 
> Signed-off-by: Mario Kleiner 
> Cc: Chris Wilson 
> Cc: Keith Packard 
> Cc: Adam Jackson 

Fixing things by deleting them! The best kind of patch. Merged, thanks:

remote: I: patch #220463 updated using rev 
73f0ed2d928afc692ed057eb3d7627328a6e5b12.
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   f5ded22e14..73f0ed2d92  master -> master

- ajax
___
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

Re: [PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().

[Mario Kleiner]

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  wrote:
> Hi Mario,
>
> On 4 May 2018 at 13:14, Mario Kleiner  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 
#include 
#include 

uint64_t inseq[] = { 1, 2, 3, 4, 0x1000, 0x2000, 0x3000, 0x4000, 0x5000, 0x6000, 0x7000, 0x8000,
 0x9000, 0xa000, 0xb000, 0xc000, 0xd000, 0xe000, 0xf000, 0xfffd, 0x,
 0x1, 0x10001, 0x10002, 0x10003, 0x10004, 0x10005, 0xe000, 0xb000, 0xf000,
 0xf, 0xf1000};

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) < -0x4000)
msc_high += 0x1L;

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) < -0x4000) */
/* Below one is better, but still not robust against unordered events */
if (((int64_t) sequence - msc_prev) < -0x4000)
msc_high += 0x1L;

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 - 0x4000))
msc_high += 0x1L;

if ((int64_t) sequence > ((int64_t) msc_prev + 0x4000))
msc_high -= 0x1L;

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 <= 0x) {
if ((int64_t) sequence < ((int64_t) msc_prev - 0x4000))
msc_high += 0x1L;

if ((int64_t) sequence > ((int64_t) msc_prev + 0x4000))
msc_high -= 0x1L;

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 & 0x;
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;
		

Re: [PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().

[Mario Kleiner]

On Fri, May 4, 2018 at 6:16 PM, Mike Lothian  wrote:
> Hi Mario
>
> Thanks for finding this
>
> Tested-by: Mike Lothian 
>

That tested-by means it doesn't cause new harm, right? Not that it
solves the plasmashell or steam freezes on itself?
-mario

> Cheers
>
> Mike
>
> On 4 May 2018 at 14:02, Mario Kleiner  wrote:
>> Hi Daniel, Eero,
>>
>> On Fri, May 4, 2018 at 2:41 PM, Daniel Stone  wrote:
>>> Hi Mario,
>>>
>>> On 4 May 2018 at 13:14, Mario Kleiner  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.
>>
>> You mean the Mesa bug? I'll quickly send out something half finished
>> for discussion to mesa-devel, cc'ed to xorg-devel, which "fixes" the
>> bug in two different ways, both are not optimal, but better than
>> nothing. Lacking this fix, KDE-5 is pretty much unusable due to
>> frequent lockup on server 1.20 with the modesetting-ddx atm.
>>
>> -mario
>>
>>>
>>> Cheers,
>>> Daniel
>> ___
>> 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
___
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

Re: [PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().

[Mike Lothian]

Hi Mario

Thanks for finding this

Tested-by: Mike Lothian 

Cheers

Mike

On 4 May 2018 at 14:02, Mario Kleiner  wrote:
> Hi Daniel, Eero,
>
> On Fri, May 4, 2018 at 2:41 PM, Daniel Stone  wrote:
>> Hi Mario,
>>
>> On 4 May 2018 at 13:14, Mario Kleiner  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.
>
> You mean the Mesa bug? I'll quickly send out something half finished
> for discussion to mesa-devel, cc'ed to xorg-devel, which "fixes" the
> bug in two different ways, both are not optimal, but better than
> nothing. Lacking this fix, KDE-5 is pretty much unusable due to
> frequent lockup on server 1.20 with the modesetting-ddx atm.
>
> -mario
>
>>
>> Cheers,
>> Daniel
> ___
> 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
___
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

Re: [PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().

[Mario Kleiner]

Hi Daniel, Eero,

On Fri, May 4, 2018 at 2:41 PM, Daniel Stone  wrote:
> Hi Mario,
>
> On 4 May 2018 at 13:14, Mario Kleiner  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.

You mean the Mesa bug? I'll quickly send out something half finished
for discussion to mesa-devel, cc'ed to xorg-devel, which "fixes" the
bug in two different ways, both are not optimal, but better than
nothing. Lacking this fix, KDE-5 is pretty much unusable due to
frequent lockup on server 1.20 with the modesetting-ddx atm.

-mario

>
> Cheers,
> Daniel
___
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

Re: [PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().

[Daniel Stone]

Hi Mario,

On 4 May 2018 at 13:14, Mario Kleiner  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
___
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