On 2018-04-10 05:35 PM, Cyr, Aric wrote: >> On 2018-04-10 03:37 AM, Michel Dänzer wrote: >>> On 2018-04-10 08:45 AM, Christian König wrote: >>>> Am 09.04.2018 um 23:45 schrieb Manasi Navare: >>>>> Thanks for initiating the discussion. Find my comments >>>>> below: On Mon, Apr 09, 2018 at 04:00:21PM -0400, Harry >>>>> Wentland wrote: >>>>>> On 2018-04-09 03:56 PM, Harry Wentland wrote: >>>>>>> >>>>>>> === A DRM render API to support variable refresh rates >>>>>>> === >>>>>>> >>>>>>> In order to benefit from adaptive sync and VRR userland >>>>>>> needs a way to let us know whether to vary frame timings >>>>>>> or to target a different frame time. These can be >>>>>>> provided as atomic properties on a CRTC: * bool >>>>>>> variable_refresh_compatible * int >>>>>>> target_frame_duration_ns (nanosecond frame duration) >>>>>>> >>>>>>> This gives us the following cases: >>>>>>> >>>>>>> variable_refresh_compatible = 0, target_frame_duration_ns >>>>>>> = 0 * drive monitor at timing's normal refresh rate >>>>>>> >>>>>>> variable_refresh_compatible = 1, target_frame_duration_ns >>>>>>> = 0 * send new frame to monitor as soon as it's >>>>>>> available, if within min/max of monitor's reported >>>>>>> capabilities >>>>>>> >>>>>>> variable_refresh_compatible = 0/1, >>>>>>> target_frame_duration_ns = > 0 * send new frame to >>>>>>> monitor with the specified target_frame_duration_ns >>>>>>> >>>>>>> When a target_frame_duration_ns or >>>>>>> variable_refresh_compatible cannot be supported the >>>>>>> atomic check will reject the commit. >>>>>>> >>>>> What I would like is two sets of properties on a CRTC or >>>>> preferably on a connector: >>>>> >>>>> KMD properties that UMD can query: * vrr_capable - This will >>>>> be an immutable property for exposing hardware's capability >>>>> of supporting VRR. This will be set by the kernel after >>>>> reading the EDID mode information and monitor range >>>>> capabilities. * vrr_vrefresh_max, vrr_vrefresh_min - To >>>>> expose the min and max refresh rates supported. These >>>>> properties are optional and will be created and attached to >>>>> the DP/eDP connector when the connector is getting >>>>> intialized. >>>> >>>> Mhm, aren't those properties actually per mode and not per >>>> CRTC/connector? >>>> >>>>> Properties that you mentioned above that the UMD can set >>>>> before kernel can enable VRR functionality *bool vrr_enable >>>>> or vrr_compatible target_frame_duration_ns >>>> >>>> Yeah, that certainly makes sense. But target_frame_duration_ns >>>> is a bad name/semantics. >>>> >>>> We should use an absolute timestamp where the frame should be >>>> presented, otherwise you could run into a bunch of trouble with >>>> IOCTL restarts or missed blanks. >>> >>> Also, a fixed target frame duration isn't suitable even for >>> video playback, due to drift between the video and audio clocks. > > Why? Even if they drift, you know you want to show your 24Hz video > frame for 41.6666ms and adaptive sync can ensure that with reasonable > accuracy. Due to the drift, the video player has to occasionally either skip a frame or present it twice to prevent audio and video going out of sync, resulting in visual artifacts. With time-based presentation and variable refresh rate, audio and video can stay in sync without occasional visual artifacts. It would be a pity to create a "variable refresh rate API" which doesn't allow harnessing this strength of variable refresh rate. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
