[PATCH] Add freesync ioctl interface

[Christian König]

Am 10.08.2016 um 05:19 schrieb Michel Dänzer:
> On 09/08/16 07:44 PM, Christian König wrote:
>> Am 09.08.2016 um 12:03 schrieb Michel Dänzer:
>>> On 09/08/16 06:31 PM, Christian König wrote:
>>>> Am 09.08.2016 um 10:27 schrieb Michel Dänzer:
>>>>> On 09/08/16 05:12 PM, Christian König wrote:
>>>>> [SNIP]
>>>>> This would require extensive changes across the stack though, when more
>>>>> or less the same result could be achieved by just letting the kernel
>>>>> know what the current refresh rate is supposed to be, e.g. via output
>>>>> properties.
>>>> The problem is that you don't have a refresh rate any more.
>>> Maybe not below the video decoding API level, but the video player app
>>> certainly knows the refresh rate?
>> Sure, but as I said that isn't a constant refresh rate any more.
>>
>> E.g. the refresh rate from the video header doesn't need to be a
>> multiple of the time a vertical line takes to display.
>>
>> This results in sightly different display times for each frame. So you
>> don't have a constant frame rate any more but rather alternate between
>> two (or maybe more) different display times for each frame.
> And do you think we'll be able to program flips that accurately?

Yes, why not?

Even if the hardware can't do the flip at a specific vblank position 
(which as far as I know our hardware can) we can still use an HR timer 
for this.

>
>
>>>> When we add freesync support we just extend vblank_mode with a new enum
>>>> to enable it optionally for existing applications.
>>> "Just"... this will only be possible after the first 3 steps above.
>> Checking the present extension again we already got PresentOptionAsync
>> in there as well.
>>
>> So the whole thing boils down implementing a new vblank_mode enume which
>> sets PresentOptionAsync and then doing the right thing on the X side
>> with those information.
> PresentOptionAsync isn't for this. It's for not waiting for the vertical
> blank period before performing a flip, which may result in tearing. This
> distinction still applies with variable refresh rate, so this option
> cannot be re-purposed to distinguish between variable and fixed refresh
> rate.

Ok that makes sense. But we can still use PresentOptionUst for 
controlling this.

E.g. when you want to display a frame as fast as you can we send the 
PresentOptionUst flag and a zero timestamp (or in general something in 
the past/now).

When we want a constant frame rate of some value we send the 
PresentOptionUst flag and a monotonic increasing timestamp when the 
frame should be displayed.

> BTW, the only presentation time we can use for the vblank_mode override
> is "now" / "as soon as possible". So for that case (which is currently
> the case for basically all games, and will likely continue to be for the
> vast majority for a long time), the whole exercise doesn't provide any
> net benefit over using the existing vblank-based presentation
> infrastructure and just force-enabling variable refresh rate somehow.

Sure it does. My basic problem here is that just force-enabling it 
somehow is a really crude hack to just get it working fast and doesn't 
represent at all how the underlying hardware works.

This surely will get us into problems when we move forward because of 
the required backward compatibility. Or isn't an output property 
something we want to keep for backward compatibility?

Additional to that my fear is that when we allow such a flawed design in 
once at least the closed source driver will try to stick with that. 
Making things like DAL even harder to get merged upstream.

> Note that I'm not questioning the value of time-based presentation for
> video playback, and thus the need to implement it. I'm only questioning
> the point of delaying variable refresh rate for games by gating it on
> the time-based presentation infrastructure.
>
>
>> I'm going to give the UST option a try with VDPAU, let's see how well
>> that works and if it is implemented correctly or not.
> It's not implemented at all yet.

Yeah, unfortunately. Leo already tried it and it is indeed not 
implemented at all.

But at least we don't need to extend the spec in any way.

> BTW, it probably doesn't make sense to add support for time-based
> presentation to the pre-atomic KMS API. So another item to add to the
> list is adding support for the atomic KMS API to the DDX drivers.

struct drm_mode_crtc_page_flip is used only as an IOCTL parameter with 
size checks as far as I can see. So we could extend the structure 
without breaking backward compatibility.

But I agree that using the atomic API would indeed be a good idea here.

Regards,
Christian.