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.