Hi Manuel,

It looks like, functionality-wise, this change primarily removes any trace of supporting swapinterval=1 in KMSDRM_GLES_SwapWindow because it's currently broken.

From grepping in the VC4 code it looks like it supports DRM_MODE_PAGE_FLIP_ASYNC:
https://github.com/intel/idxd-driver/blob/master/drivers/gpu/drm/vc4/vc4_crtc.c#L963

Given that, it seems that support for DRM_MODE_PAGE_FLIP_ASYNC could be added rather than breaking swapinterval=1 support in a different way.

@Anthony Pesch: No. The primary functionality of this patch is not to remove swapinterval support: its primary objetive is fixing dynamic vsync toggle in games, and to document DRM/EGL mechanics in a precise and explicit way, so the code is easier to maintain in the future. <---The second part on this is needed because not all contribs here are professional driver/backend developers and only have small bursts of time to invest on fixing low-level stuff, so having clear and extensively commented code is mandatory.

That swapinterval functionality is removed as a side-effect of drModePageFlip() not supporting the DRM_MODE_PAGE_FLIP_ASYNC flag in the drivers I have tested (Intel and VC4).
This is what Intel dri-devel developers say about that flag:

15:05 jekstrand: Vanfanel: No, we've never advertised async flips AFAIK
15:05 jekstrand: Hopefully soon though!
(Here are the logs of that day, May the 4th 2020, with all the embarrassing questions I asked and all the embarrassing experiments I did to find out that the feature is not yet ready: https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2020-05-04&show_html=true). 

It will take more time to be working on all main drivers, and even more time to get into the main distros, maybe months or even years.

So I would like to get this merged as it is now. Later, when ASYNC pageflips are possible, we will be able to issue them, and things will work without vsync. But that will take time from the MESA/libDRM people, and when I go back to this, I need the documented code to be there.

(In reply to Anthony Pesch from comment #1)
> Hi Manuel,
> 
> It looks like, functionality-wise, this change primarily removes any trace
> of supporting swapinterval=1 in KMSDRM_GLES_SwapWindow because it's
> currently broken.
> 
> From grepping in the VC4 code it looks like it supports
> DRM_MODE_PAGE_FLIP_ASYNC:
> https://github.com/intel/idxd-driver/blob/master/drivers/gpu/drm/vc4/
> vc4_crtc.c#L963
> 
> Given that, it seems that support for DRM_MODE_PAGE_FLIP_ASYNC could be
> added rather than breaking swapinterval=1 support in a different way.

Oops, I meant to write swapinterval=0.

> @Anthony Pesch: No. The primary functionality of this patch is not to remove swapinterval support: its primary objetive is fixing dynamic vsync toggle in games

Sorry, what's the difference exactly? Is it that right now, the program misbehaves when you go from swapinterval=1 -> swapinterval=0 -> swapinterval=1? However by removing the broken support for swapinterval=0 at least the swapinterval=1 case never becomes broken when toggling them?

(In reply to Anthony Pesch from comment #3)
> (In reply to Anthony Pesch from comment #1)
> > Hi Manuel,
> > 
> > It looks like, functionality-wise, this change primarily removes any trace
> > of supporting swapinterval=1 in KMSDRM_GLES_SwapWindow because it's
> > currently broken.
> > 
> > From grepping in the VC4 code it looks like it supports
> > DRM_MODE_PAGE_FLIP_ASYNC:
> > https://github.com/intel/idxd-driver/blob/master/drivers/gpu/drm/vc4/
> > vc4_crtc.c#L963
> > 
> > Given that, it seems that support for DRM_MODE_PAGE_FLIP_ASYNC could be
> > added rather than breaking swapinterval=1 support in a different way.
> 
> Oops, I meant to write swapinterval=0.
> 
> > @Anthony Pesch: No. The primary functionality of this patch is not to remove swapinterval support: its primary objetive is fixing dynamic vsync toggle in games
> 
> Sorry, what's the difference exactly? Is it that right now, the program
> misbehaves when you go from swapinterval=1 -> swapinterval=0 ->
> swapinterval=1? However by removing the broken support for swapinterval=0 at
> least the swapinterval=1 case never becomes broken when toggling them?

In your first comment, you said is this: "It looks like, functionality-wise, this change primarily removes any trace of supporting swapinterval=1 in KMSDRM_GLES_SwapWindow because it's currently broken."
And I answered the same I am going to answer now. I will try to struct my answer better this time.

-NO. The primary function of this patch is NOT to remove the the support for swapinterval = 0, because current code does not support swapinterval = 0 to begin with. 
In current code, you see some kind of swapinterval = 0 support, but it has never worked: all that was being done is that pageflips were issued without the previous ones to complete. That is causing EBUSY returns on drmModePageFlip(), and buffer mismanagement. <--- That is what you have right now. You have not working swapinterval = 0, and even if ASYNC flips where possible (they are NOT), swapinterval = 0 will never work as its coded right now.

What happens is that swapinterval = 0 can NOT be supported right now because drmModePageFlip() returns an error with the DRM_MODE_PAGE_FLIP_ASYNC. This has been acknowledged by Mesa/libDRM devs as you saw.
It is IMPOSSIBLE to support swapinterval = 0 as things are, because we always have to wait for previous each pageflip to complete before issuing a new one. Not being able to issue ASYNC pageflips (since as I said DRM_MODE_PAGE_FLIP_ASYNC does NOT work), all we can do is issue synchronous pageflips with drmModePageFlip().
I have nothing against ASYNC pageflips. They will be supported as soon as major distros include versions of Mesa/libDRM that allow DRM_MODE_PAGE_FLIP_ASYNC flag drmModePageFlip() to work.
In current core, DRM_MODE_PAGE_FLIP_ASYNC is not even tried. Async pageflips are based on wrong assumptions that just break everything when activated, like passing 0 as timeout to poll() in WaitForPageflip() and then issue new pageflips before waiting for previous ones to complete, causing a ton of EBUSY returns. <--- This is what current code does. It is stupid because I did not understand all this stuff well time ago. It has to end and proper ASYNC pageflips will be implemented when it becomes possible.


Now lets see (again) what this patch does:
-One purpose of this patch is to prevent wrong code to be executed when the user toggles vsync in a program. With the new code, vsync can be disabled from programs, but it will not cause MAJOR errors on the KMSDRM interface anymore.
There are other things that are done WRONG in the current code. For example, poll() call on the WaitForPageflip() function should never receive a 0 value, because as I said we ALWAYS have to wait for each pageflip to complete, etc.
This rewrite is based on a deeper and more correct understanding of the EGL/GBM/KMSDRM mechanics, and that is why it is important.

-Other purpose of this patch (maybe the most important) is to reestruct and ***comment*** the code so I (or others) can come back to it later and improve it, without investing an insane amount of time re-learning what it does and how.
This patch also reestructs the code so it can be easier to read and understand, hence easier to maintain and correct in the same vein. Public code is not meant to be obfuscated and complicated: it has to be readable and understandable, because different people has to understand what it does by simply looking at it.  


NOW, I will repeat myself once more: swapinterval = 0 can NOT be supported right now because MESA and libDRM do NOT support this on most popular drivers. And when it can be supported, I will come back to this code and, thanks to the new comments in the code, I will be able to add support for it.

And I will clear things one time more: swapinterval = 0 was never supported before to start with, because it was based on wrong assumptions that broke the KMSDRM interface until it was re-initialized. 

Just in case, I will be clearer: in current code, there is no working support for swapinterval = 0. You see it there, but it does not work and it can not work, because it is plain wrong.

I would like to add that implementing a working swapinterval = 0 (I repeat: there is NO working/sane swapinterval = 0 support in current code) once DRM_MODE_PAGE_FLIP_ASYNC is trivial in my new code: it will be as easy as adding a drmModePageflip() call with the DRM_MODE_PAGE_FLIP_ASYNC flag to each of the two code paths in SwapWindow(): the rest of the functionality is in place. It will take minutes to add it, because things are now clear and well documented.
But adding it right now would lead to all major drivers to fail and stall when its activated.

I will anticipate new questions: Why doesnt current core "stall" when I try swapinterval = 0 then? Because there is no swapinterval = 0 support in current code. What you see there is wrong. Things done right need DRM_MODE_PAGE_FLIP_ASYNC, and if DRM_MODE_PAGE_FLIP_ASYNC is used as things are in Mesa/libDRM, the KMSDRM interface just stalls forever. Yeah, a timeout could be passed to poll(), but that is stupid because drmModePageFlip() fails with DRM_MODE_PAGE_FLIP_ASYNC anyway, so there will be no screen refresh even if you get pass poll().

Patch added, thanks!
https://hg.libsdl.org/SDL/rev/552176e6be03