[PATCH v4 3/4] drm: Document variable refresh properties

[Kazlauskas, Nicholas]

On 11/1/18 1:05 PM, Michel Dänzer wrote:
> On 2018-11-01 3:58 p.m., Kazlauskas, Nicholas wrote:
>> On 11/1/18 6:58 AM, Michel Dänzer wrote:
>>> On 2018-10-31 6:54 p.m., Kazlauskas, Nicholas wrote:
>>>> On 10/31/18 12:20 PM, Michel Dänzer wrote:
>>>>> On 2018-10-31 3:41 p.m., Kazlauskas, Nicholas wrote:
>>>>>> On 10/31/18 10:12 AM, Michel Dänzer wrote:
>>>>>>> On 2018-10-31 2:38 p.m., Kazlauskas, Nicholas wrote:
>>>>>>>>
>>>>>>>> I did some more investigation into when amdgpu gets the scanout position
>>>>>>>> and what values we get back out of it. The timestamp is updated shortly
>>>>>>>> after the crtc irq vblank which is typically within a few lines of
>>>>>>>> vbl_start. This makes sense, since we can provide the prediction
>>>>>>>> timestamp early. Waiting for the vblank to wrap around to 0 doesn't
>>>>>>>> really make sense here since that would mean we already hit timeout or
>>>>>>>> the flip occurred
>>>>>>>
>>>>>>> Sounds like you're mixing up the two cases of "actual" vblank events
>>>>>>> (triggered by the "vblank" interrupt => drm_(crtc_)handle_vblank) and
>>>>>>> flip completion events (triggered by the PFLIP interrupt with our
>>>>>>> hardware => drm_crtc_send_vblank_event).
>>>>>>>
>>>>>>> Actual vblank events need to be delivered to userspace at the start of
>>>>>>> vblank, so we indeed can't wait until the timestamp is accurate for
>>>>>>> them. We just need to document the exact semantics of their timestamp
>>>>>>> with VRR.
>>>>>>>
>>>>>>> For page flip completion events though, the timestamp needs to be
>>>>>>> accurate and correspond to when the flipped frame starts being scanned
>>>>>>> out, otherwise we'll surely break at least some userspace relying on
>>>>>>> this information.
>>>>>>>
>>>>>> Yeah, I was. I guess what's sent is the estimated vblank timestamp
>>>>>> calculated at the start of the interrupt.
>>>>>
>>>>> s/interrupt/vblank/, yeah.
>>>>>
>>>>>
>>>>>> And since that's just a guess rather than what's actually going to
>>>>>> happen it's going to be wrong in a lot of cases.
>>>>>>
>>>>>> I could see the wrap-around method working if the vblank timestamp was
>>>>>> somehow updated in amdgpu or in drm_crtc_send_vblank_event.
>>>>>
>>>>> DC already calls drm_crtc_accurate_vblank_count before
>>>>> drm_crtc_send_vblank_event, we "just" need to make sure that results in
>>>>> an accurate timestamp.
>>>>>
>>>>>
>>>>>> This would be a relatively simple fix but would break anything in
>>>>>> userspace that relied on the timestamp for vblank interrupt and the
>>>>>> flip completion being the same value.
>>>>>
>>>>> Hmm, that's a good point. So while VRR is enabled, maybe it is safer to
>>>>> defer delivery of vblank events until the accurate timestamp is known as
>>>>> well, at least by default. If there is userspace which needs the events
>>>>> earlier even with VRR but can live with the guessed timestamp, a flag or
>>>>> something could be added for that.
>>>>
>>>> I was under the impression that the vblank timestamp was reused but it's
>>>> already going to differ since we call drm_crtc_accurate_vblank_count
>>>> before drm_crtc_send_vblank_event. Thanks for pointing that out.
>>>>
>>>> Since that works for updating timestamp what's left is making sure that
>>>> it waits for the wrap around if it's above crtc_vtotal. It might make
>>>> sense to add a new flag for this that's only used within
>>>> amdgpu_get_crtc_scanout_position so the other call sites aren't affected.
>>>>
>>>> There isn't a way to get an accurate timestamp with VRR enabled until
>>>> after the flip happens. So deferring it kind of defeats the purpose of a
>>>> client using it to make predictions before the flip for displaying their
>>>> content.
>>>
>>> That's generally not a valid use-case. The KMS API is defined such that
>>> if userspace receives the vblank event for vblank period N and then
>>> submits a page flip, the page flip cannot (normally[0]) take effect
>>> before vblank period N+1.
>>>
>>>
>>> There are mainly two valid use-cases for waiting for vblank:
>>>
>>> 1. Wait for vblank N, then submit page flip for vblank N+1.
>>>
>>> 2. Wait for vblank N, then try to do something before vblank N ends,
>>> e.g. draw directly to the front buffer, as a poor man's way of avoiding
>>> tearing or similar artifacts.
>>>
>>>
>>> Use-case 1 is used by Xorg (both for Present and DRI2) and at least
>>> Weston, but presumably most if not all Wayland compositors. From Pekka's
>>> description of how Weston uses it (thanks!), it's clear that the
>>> timestamp of vblank events does need to accurately correspond to the end
>>> of the vblank period.
>>>
>>> Use-case 2 is also used by Xorg, but only when not flipping, which rules
>>> out VRR. Also, I'd argue that blocking waits for vblank are more
>>> suitable for this use-case than vblank events.
>>>
>>> Therefore, in summary I propose this is how it could work:
>>>
>>> * Without VRR, timestamps continue to work the same way they do now.
>>>
>>> * With VRR, delivery of both vblank and page flip completion events must
>>> be deferred until the timestamp accurately corresponds to the actual end
>>> of vblank. For blocking vblank waits, I'm not sure, but I'm leaning
>>> towards continuing to return to userspace at the beginning of vblank by
>>> default, even if the timestamp is inaccurate.
>>>
>>> Deferring delivery of events should avoid busy-loops. Bonus points for
>>> minimizing the number of interrupts needed. :)
>>>
>>> Note that both blocking vblank waits and vblank events are currently
>>> triggered from drm_(crtc_)handle_vblank, so some rework would be
>>> required for this.
>>>
>>> It's possible that there's other userspace which doesn't care about the
>>> accuracy of the timestamps, but cares about getting events as early as
>>> possible. In that case, flags might need to be added.
>>>
>>>
>>> [0] Our drivers can complete the flip in the same vblank period under
>>> some circumstances if userspace specifies it as the target using the
>>> DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flag, but there's no
>>> guarantee for it to work. And no other driver supports this yet. Without
>>> those flags, the flip cannot complete in the same vblank period, or it
>>> would break existing userspace using use-case 1 above.
>>>
>> Deferring the pageflip timestamp is something we can do today in the
>> driver with no impact on existing behavior/semantics. Since hpos is used
>> in the calculation as well I think we don't need even need to wait for
>> the roll-over necessarily.
> 
> Not sure offhand how hpos helps for this, but I'll trust your word for
> it. :)

If we're still in the vfront porch when on the pageflip irq timestamp 
query then it should be going over to the back porch on the next line.

So there's still the full back porch to go through, plus a little bit of 
the current line horizontally. That horizontal bit is included in the 
timestamp calculation already so it should be fine.

> 
> 
>> Deferring the event that comes from drm_handle_vblank worries me though.
>> The implementation for that would be sending that event on the roll-over
>> at timeout or grouping it together on the pageflip. Those are pretty
>> long delays for both.
>>
>> To me it still makes more sense to just define the vblank timestamp
>> coming out of drm_crtc_handle_vblank as an estimate when VRR is enabled.
>> An estimation which ends up being the same value as what userspace gets
>> today, the estimation for the current mode.
>>
>> I think my opinion would be different if the timestamps were supposed to
>> be the same value but that's incredibly unlikely given that they're
>> offset from the ktime_get in most cases.
> 
> Right now, without VRR, they're supposed to be the same within measuring
> accuracy. If you're seeing bigger differences than that, can you provide
> some examples? You might have discovered a bug.

There's a little bit of uncertainty since it's two separate calls to 
get_crtc_scanoutpos and two different calls to ktime_get for the end 
time. The numbers differ but not by a significant amount. I wouldn't 
consider it a bug at least.

> 
> 
> Anyway, I re-read Pekka's description, and it sounds like Weston only
> uses the vblank timestamp if no flip completed in the last vblank
> period, which normally shouldn't happen with VRR. Otherwise it uses the
> timestamp from the flip completion event.
> 
> So, it sounds like having different timestamps between the two cases
> should be fine for Weston after all. Sorry for the confusion above.
> 
> So, I'm okay with trying to change only the timestamps of the flip
> completion events. That'll certainly make things easier. :)
> 
> 

Sounds good.

The only change needed to handle this in the pageflip is a small bugfix 
for the case where vpos >= crtc_vtotal on the pageflip query. I'll 
include it as part of the next patch series with the updated documentation.

Nicholas Kazlauskas