Re: [PATCH 1/2] drm: use monotonic time in drm_calc_vbltimestamp_from_scanoutpos

[Mario Kleiner <mario.kleiner@tuebingen.mpg.de>]

On 25.10.12 12:28, Imre Deak wrote:
> On Thu, 2012-10-25 at 01:05 +0200, Mario Kleiner wrote:
>> On 23.10.12 20:53, Imre Deak wrote:
>>> For measuring duration we want to avoid that our start/end timestamps
>>> jump, so use monotonic instead of real time for that.
>>>
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_irq.c |   18 ++++++++++++------
>>>    1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>>> index 89b830d..7dc203d 100644
>>> --- a/drivers/gpu/drm/drm_irq.c
>>> +++ b/drivers/gpu/drm/drm_irq.c
>>> @@ -576,7 +576,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>>>    					  unsigned flags,
>>>    					  struct drm_crtc *refcrtc)
>>>    {
>>> -	struct timeval stime, raw_time;
>>> +	ktime_t stime, etime, mono_time_offset;
>>> +	struct timeval tv_etime;
>>>    	struct drm_display_mode *mode;
>>>    	int vbl_status, vtotal, vdisplay;
>>>    	int vpos, hpos, i;
>>> @@ -625,13 +626,14 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>>>    		preempt_disable();
>>>
>>>    		/* Get system timestamp before query. */
>>> -		do_gettimeofday(&stime);
>>> +		stime = ktime_get();
>>>
>>>    		/* Get vertical and horizontal scanout pos. vpos, hpos. */
>>>    		vbl_status = dev->driver->get_scanout_position(dev, crtc, &vpos, &hpos);
>>>
>>>    		/* Get system timestamp after query. */
>>> -		do_gettimeofday(&raw_time);
>>> +		etime = ktime_get();
>>
>> Here is possibly a tiny race: The wall_to_monotonic offset value could
>> change between the ktime_get() - which uses it internally for wallclock
>> -> monotonic clock conversion, and the ktime_get_monotonic_offset()
>> query below, so the later subtraction of mono_time_offset from etime
>> would not cancel out the addition to etime inside ktime_get() and you
>> wouldn't get correct walltime back. There seem to be multiple sources of
>> change to the value, e.g., do_settimeofday(), do_adjtimex() - the admin
>> or ntp changing the system clock. The internal code, e.g., ktime_get()
>> use a seqlock to protect against this race.
>>
>> There's a function ktime_get_real(void) which directly gives you the
>> wall time you want as ktime_t, but then you'd still need to do the
>> ktime_get() query in the !drm_timestamp_monotonic case to calculate
>> duration_ns below.
>>
>> Same problem in the 2nd patch for get_drm_timestamp(). Otoh, the time
>> window for the race is small and it can only happen in the non-default
>> case of !drm_timestamp_monotonic, so i don't know if it is worth fixing it?
>
> I was also hold up by this for a while, since there is no function to
> get both clocks atomically. But it isn't really a problem if you think
> about it: etime - mono_time_offset is a correct wall time value
> regardless whether mono_time_offset has changed or not after
> ktime_get(). The only difference is whether the user sees the time value
> before or after the adjustment, but you can't guard against that anyway
> (except using monotonic time values always).
>
> It would be a problem if as ktime_get() we would do the reverse and
> calculate the monotonic time from the wall time. There not getting the
> wall time and the wall_to_monotonic offset atomically could result in a
> incorrect monotonic time value, for example one that jumps backwards.
>

Yes, agreed. Your patches should be good to go as they are - i like them :)

-mario


> --Imre
>
>> Other than that:
>>
>> Reviewed-by: mario.kleiner
>>