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

[Kazlauskas, Nicholas]

On 10/30/18 11:34 AM, Kazlauskas, Nicholas wrote:
> On 10/30/18 5:29 AM, Michel Dänzer wrote:
>> On 2018-10-29 7:03 p.m., Ville Syrjälä wrote:
>>> On Mon, Oct 29, 2018 at 05:37:49PM +0100, Michel Dänzer wrote:
>>>> On 2018-10-26 7:59 p.m., Ville Syrjälä wrote:
>>>>> On Fri, Oct 26, 2018 at 05:34:15PM +0000, Kazlauskas, Nicholas wrote:
>>>>>> On 10/26/18 10:53 AM, Ville Syrjälä wrote:
>>>>>>>
>>>>>>> Speaking of timestamps. What is the expected behaviour of vblank
>>>>>>> timestamps when vrr is enabled?
>>>>>>
>>>>>> When vrr is enabled the duration of the vertical front porch will be
>>>>>> extended until flip or timeout occurs. The vblank timestamp will vary
>>>>>> based on duration of the vertical front porch. The min/max duration for
>>>>>> the front porch can be specified by the driver via the min/max range.
>>>>>>
>>>>>> No changes to vblank timestamping handling should be necessary to
>>>>>> accommodate variable refresh rate.
>>>>>
>>>>> The problem is that the timestamp is supposed to correspond to the first
>>>>> active pixel. And since we don't know how long the front porch will be
>>>>> we can't realistically report the true value. So I guess just assuming
>>>>> min front porch length is as good as anything else?
>>>>
>>>> That (and documenting that the timestamp corresponds to the earliest
>>>> possible first active pixel, not necessarily the actual one, with VRR)
>>>> might be good enough for the actual vblank event timestamps.
>>>>
>>>>
>>>> However, I'm not so sure about the timestamps of page flip completion
>>>> events. Those could be very misleading if the flip completes towards the
>>>> timeout, which could result in bad behaviour of applications which use
>>>> them for animation timing.
>>>>
>>>> Maybe the timestamp could be updated appropriately (yes, I'm hand-waving
>>>> :) in drm_crtc_send_vblank_event?
>>>
>>> Hmm. Updated how? Whether it's a page flip event or vblank even we won't
>>> know when the first active pixel will come. Although I suppose if
>>> there is some kind of vrr slew rate limit we could at least account
>>> for that to report a more correct "this is the earliest you migth be
>>> able to see your frame" timestamp.
>>>
>>> Oh, or are you actually saying that shceduling a new flip before the
>>> timeout is actually going to latch that flip immediately? I figured
>>> that the flip would get latched on the next start of vblank regardless,
>>> and the act of scheduling a flip will just kick the hardware to start
>>> scanning the previously latched frame earlier.
>>>
>>> scanout A | ... vblank | scanout A | ... vblank | scanout B | ... vblank
>>>                     ^                ^        ^               ^
>>>                     |                |        flip C          latch C
>>>                     flip B           latch B
>>
>> This would kind of defeat the point of VRR, wouldn't it? If a flip was
>> scheduled after the start of vblank, the vblank would always time out,
>> resulting in the minimum refresh rate.
>>
>>
>>> scanout A | ... vblank | scanout B | ... vblank | scanout C | ... vblank
>>>                     ^ ^                      ^ ^
>>>                     | latch B                | latch C
>>>                     flip B                   flip C
>>
>> So this is what happens.
>>
>> Regardless, when the flip is latched, AMD hardware generates a "pflip"
>> interrupt, and its handler calls drm_crtc_send_vblank_event (and in the
>> case of DC drm_crtc_accurate_vblank_count before that). So the time when
>> drm_crtc_send_vblank_event is called might be a good approximation of
>> when scanout of the next frame starts.
>>
>> Another possibility might be to wait for the hardware vline counter to
>> wrap around to 0 before calling drm_crtc_accurate_vblank_count, then the
>> calculations should be based on 0 instead of crtc_vtotal.
>>
>>
> 
> 
> I understand the issue you're describing now. The timestamp is supposed
> to signify the end of the current vblank. The call to get scanout
> position is supposed to return the number of lines until scanout (a
> negative value) or the number of lines since the next scanout began (a
> positive value).
> 
> The amdgpu driver calculates the number of lines based on a hardware
> register status position which returns an increasing value from 0 that
> indicates the current vpos/hpos for the display.
> 
> For any vpos below vbl_start we know the value is correct since the next
> vblank hasn't begun yet. But for anythign about vbl_start it's probably
> wrong since it applies a corrective offset based on the fixed value of
> crtc_vtotal. It's even worse when the value is above crtc_vtotal since
> it'll be calculating the number of lines since the last scanout since
> it'll be a positive value.
> 
> So the issue becomes determining when the vfront porch will end.
> 
> When the flip address gets written the vfront porch will end at the
> start of the next line leaving only the back porch plus part of the
> line. But we don't know when the flip will occur, if at all. It hasn't
> occurred yet in this case.
> 
> Waiting for the wrap around to 0 might be the best choice here since
> there's no guarantee the flip will occur.
> 
> Nicholas Kazlauskas
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

I put some more thought into this and I don't think this is as bad as I 
had originally thought.

I think the vblank timestamp is supposed to be for the first active 
pixel of the next scanout. The usage of which is for clients to time 
their content/animation/etc.

The client likely doesn't care when they issue their flip, just that 
their content is matched for that vblank timestamp. The fixed refresh 
model works really well for that kind of application.

Utilizing variable refresh rate would be a mistake in that case since 
the client would then have to time based on when they flip which is a 
lot harder to "predict" precisely.

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 - making the timestamp useless and probably burning a 
ton of CPU to do it.

What this means is that we're going to have to make some sort of 
prediction based on a fixed duration to have a reasonable result. I 
think assuming the min front porch duration (max refresh) is the logical 
approach here - in most cases this will be crtc_vtotal since it's the 
monitor's fastest refresh. For clients that flip early this will end up 
being the correct timestamp. No need to explicitly check for variable 
refresh enabled in the get scanout pos handler either.

There is one little caveat with get scanout pos with that approach, 
however. For usages other than the timestamp the number of lines until 
scanout will still be incorrect when going above crtc_vtotal. We can 
either assume the worst in this case (the max front porch duration, the 
min refresh/timeout) or clamp and just return the size of the back porch 
instead The vpos will look stalled, but the flip will likely happen 
sooner than the timeout in most cases so this will likely work out better.

Nicholas Kazlauskas