[Mesa-dev] Upstream support for FreeSync / Adaptive Sync

Thu Oct 19 18:51:32 UTC 2017

On Wed, Oct 18, 2017 at 09:28:01PM +0200, Daniel Vetter wrote:
> On Wed, Oct 18, 2017 at 6:59 PM, Michel Dänzer <michel at daenzer.net> wrote:
> > On 18/10/17 12:15 PM, Nicolai Hähnle wrote:
> >> On 18.10.2017 10:10, Daniel Vetter wrote:
> >>> On Tue, Oct 17, 2017 at 09:01:52PM +0200, Nicolai Hähnle wrote:
> >>>> On 17.10.2017 19:16, Daniel Vetter wrote:
> >>>>> On Tue, Oct 17, 2017 at 5:40 PM, Michel Dänzer <michel at daenzer.net>
> >>>>> wrote:
> >>>>>> On 17/10/17 05:04 PM, Daniel Vetter wrote:
> >>>>>>> On Tue, Oct 17, 2017 at 03:46:24PM +0200, Michel Dänzer wrote:
> >>>>>>>> On 17/10/17 02:22 PM, Daniel Vetter wrote:
> >>>>>>>>> On Tue, Oct 17, 2017 at 12:28:17PM +0200, Michel Dänzer wrote:
> >>>>>>>>>> On 17/10/17 11:34 AM, Nicolai Hähnle wrote:
> >>>>>>>>>
> >>>>>>>>>>> Common sense suggests that there need to be two side to
> >>>>>>>>>>> FreeSync / VESA
> >>>>>>>>>>> Adaptive Sync support:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. Query the display capabilities. This means querying minimum
> >>>>>>>>>>> / maximum
> >>>>>>>>>>> refresh duration, plus possibly a query for when the
> >>>>>>>>>>> earliest/latest
> >>>>>>>>>>> timing of the *next* refresh.
> >>>>>>>>>>>
> >>>>>>>>>>> 2. Signal desired present time. This means passing a target
> >>>>>>>>>>> timer value
> >>>>>>>>>>> instead of a target vblank count, e.g. something like this for
> >>>>>>>>>>> the KMS
> >>>>>>>>>>> interface:
> >>>>>>>>>>>
> >>>>>>>>>>>     int drmModePageFlipTarget64(int fd, uint32_t crtc_id,
> >>>>>>>>>>> uint32_t fb_id,
> >>>>>>>>>>>                                 uint32_t flags, void *user_data,
> >>>>>>>>>>>                                 uint64_t target);
> >>>>>>>>>>>
> >>>>>>>>>>>     + a flag to indicate whether target is the vblank count or
> >>>>>>>>>>> the
> >>>>>>>>>>> CLOCK_MONOTONIC (?) time in ns.
> >>>>>>>>>>
> >>>>>>>>>> drmModePageFlip(Target) is part of the pre-atomic KMS API, but
> >>>>>>>>>> adapative
> >>>>>>>>>> sync should probably only be supported via the atomic API,
> >>>>>>>>>> presumably
> >>>>>>>>>> via output properties.
> >>>>>>>>>
> >>>>>>>>> +1
> >>>>>>>>>
> >>>>>>>>> At least now that DC is on track to land properly, and you want
> >>>>>>>>> to do this
> >>>>>>>>> for DC-only anyway there's no reason to pimp the legacy interfaces
> >>>>>>>>> further. And atomic is soooooo much easier to extend.
> >>>>>>>>>
> >>>>>>>>> The big question imo is where we need to put the flag on the kms
> >>>>>>>>> side,
> >>>>>>>>> since freesync is not just about presenting earlier, but also about
> >>>>>>>>> presenting later. But for backwards compat we can't stretch the
> >>>>>>>>> refresh
> >>>>>>>>> rate by default for everyone, or clients that rely on high
> >>>>>>>>> precision
> >>>>>>>>> timestamps and regular refresh will get a bad surprise.
> >>>>>>>>
> >>>>>>>> The idea described above is that adaptive sync would be used for
> >>>>>>>> flips
> >>>>>>>> with a target timestamp. Apps which don't want to use adaptive sync
> >>>>>>>> wouldn't set a target timestamp.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> I think a boolean enable_freesync property is probably what we
> >>>>>>>>> want, which
> >>>>>>>>> enables freesync for as long as it's set.
> >>>>>>>>
> >>>>>>>> The question then becomes under what circumstances the property
> >>>>>>>> is (not)
> >>>>>>>> set. Not sure offhand this will actually solve any problem, or
> >>>>>>>> just push
> >>>>>>>> it somewhere else.
> >>>>>>>
> >>>>>>> I thought that's what the driconf switch is for, with a policy of
> >>>>>>> "please
> >>>>>>> schedule asap" instead of a specific timestamp.
> >>>>>>
> >>>>>> The driconf switch is just for the user's intention to use adaptive
> >>>>>> sync
> >>>>>> when possible. A property as you suggest cannot be set by the client
> >>>>>> directly, because it can't know when adaptive sync can actually be
> >>>>>> used
> >>>>>> (only when its window is fullscreen and using page flipping). So the
> >>>>>> property would have to be set by the X server/driver / Wayland
> >>>>>> compositor / ... instead. The question is whether such a property is
> >>>>>> actually needed, or whether the kernel could just enable adaptive sync
> >>>>>> when there's a flip with a target timestamp, and disable it when
> >>>>>> there's
> >>>>>> a flip without a target timestamp, or something like that.
> >>>>>
> >>>>> If your adaptive sync also supports extending the vblank beyond the
> >>>>> nominal limit, then you can't do that with a per-flip flag. Because
> >>>>> absent of a userspace requesting adaptive sync you must flip at the
> >>>>> nominal vrefresh rate. So if your userspace is a tad bit late with the
> >>>>> frame and would like to extend the frame to avoid missing a frame
> >>>>> entirely it'll be too late by the time the vblank actually gets
> >>>>> submitted. That's a bit a variation of what Ville brought up about
> >>>>> what we're going to do when the timestamp was missed by the time all
> >>>>> the depending fences signalled.
> >>>>
> >>>> These are very good points. It does sound like we'd need both an
> >>>> "AdaptiveSync" boolean property and an (optional) "DesiredPresentTime"
> >>>> property.
> >>>>
> >>>> The DesiredPresentTime property applies only to a single commit and
> >>>> could
> >>>> perhaps be left out in a first version. The AdaptiveSync property is
> >>>> persistent. When enabled, it means:
> >>>>
> >>>> - handle page flip requests as soon as possible
> >>>> - while no page flip is requested, delay vblank as long as possible
> >>>>
> >>>> How does that sound?
> >>>
> >>> Yeah, that's what I had in mind. No idea it'll work out on real hw/full
> >>> stack.
> >>
> >> Here's another question that occurred to me while thinking about how all
> >> this could be prototyped.
> >>
> >> What happens when a FreeSync aware application / compositor enables the
> >> FreeSync property and then shuts down (crashes) without disabling it again?
> >>
> >> It seems to me that a non-FreeSync aware compositor would then be
> >> operating in FreeSync mode without expecting it.
> >
> > AFAIU the atomic KMS API (Daniel et al please correct me if I'm wrong),
> > this might not be an issue: When a process which enabled the property
> > dies, I think any other process has to do a modeset to display anything
> > else on that CRTC. The modeset will set the property's value to what's
> > expected by that process.
> 
> All the atomic commits are incremental (this was needed to be able to
> implement the legacy stuff on top of atomic), so if you don't reset
> the prop, you'll inherit whatever's there.
> 
> But one design principle for new properties we're tryng to uphold is
> that userspace can just read them all and force-restore all atomic
> properties when it takes over again, even if it doesn't understand
> them, to be able to recover from something else having crashed. That
> means special properties that cause a one-off action like fences, or
> the link status stuff give you a value on read that won't cause
> anything to happen when writing it back. But the kernel won't restore
> stuff for you. fbcon does restore some of the things it deems
> relevant, and we migh want to add a reset for the freesync to the list
> of things it sets. But you can't rely on fbcon everywhere.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thanks for initiating this discussion. I had some initial patches of creating these
properties in DRM for Adaptive Sync but never got a chance to complete the testing and
submitting them upstream.
So the initial thought was to add two properties:

1. bool enable_adaptive_sync - one for allowing userspace to
enable VRR and
2. bool supports_adaptive_sync - the other immutable property for exposing
hardware's capability of supporting VRR.

The second property will be set based on the VRR capabilities of the connected
monitor. If this is set, userspace can request to enable adaptive-sync by setting
the first property and through the atomic modeset, the driver will check if the
requested frame rate falls within the monitor range and enable it.

I have some prototype patches as well...But I would like to see where this
discussion heads and then submit those some day.

Manasi