Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time precision to ns

From: Daniel Vetter <daniel@ffwll.ch>
To: Keith Packard <keithp@keithp.com>
Cc: Daniel Vetter <daniel@ffwll.ch>,
	linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm: Widen vblank count to 64 bits. Change vblank time precision to ns
Date: Fri, 7 Jul 2017 14:16:40 +0200	[thread overview]
Message-ID: <20170707121640.4oedsdmpwhi4neyj@phenom.ffwll.local> (raw)
In-Reply-To: <86tw2pd314.fsf@keithp.com>

On Thu, Jul 06, 2017 at 07:59:51AM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > Extending the reported/sw vblank counter to u64 makes sense imo, but do we
> > have to extend the driver interfaces too? If there's no 64 bit hw vblank
> > currently I think I'd be good to postpone that part, simply because I'm
> > too lazy to audit all the drivers for correctly setting max_vblank_count
> > after your change :-)
> 
> As I said, it's easy enough to do that; I figured I'd do the obvious
> part and let you decide if you wanted that or not. We could also
> set max_vblank_count to 0xffffffff if it wasn't set by the driver,
> and/or add a WARN_ON_ONCE if it wasn't set. Given that it takes over two
> years to wrap this counter at 60Hz, we're never likely to hit a bug in
> testing.
> 
> Let me know what you think; I'm not invested in any particular solution
> at this point.

Setting a default would be what I suggested if it's possible. But for hw
without a vblank counter (gen2, and apparently every armsoc display ip
under the sun, dunno why), we need to set it to 0, while vblanks are
otherwise fully supported. Given that vblank counters aren't a thing
everywhere, and that it takes them forever to wrap, I don't think hw will
ever gain 64bit vblank counters.

I'd drop that part (but keep 64 everywhere else ofc).

> > Other thought on this, since you bother to change all the types: Afaik
> > both timespec and timeval suffer from the 32bit issues.
> 
> I'm not sure what 32bit issues you're concerned about here? We don't
> compare these values, just report them up to user space.

Year 2038 32bit wrap-around bug. Yes I believe/fear drm will still be
around then :-)

> > If we bother with changing everything I think it'd be neat to switch
> > all internal interfaces over to ktime, and only convert to the
> > userspace types once when we generate the event. I think that's how
> > cool hackers are supposed to do it, but not fully sure.
> 
> Yeah, I can definitely get behind that plan. A simple 64-bit value
> instead of a struct with two semi-related values which are hard to do
> arithmetic on.

So Arnd said on irc yesterday that one downside of ktime is that you get
to do a 64bit division when talking to old userspace interfaces that still
use the second/nanoseconds split. For super high-perf stuff where you need
to support old userspace interfaces there's a ktime_get_ts64 to optimize
that a bit. Given that we report vblank events on the order of 60fps to
userspace I think we can ignore that. 

Arnd also promised to update Documentation/ioctl/botching-up-ioctls.txt.

Anyway, ktime internally, converting to timeval/spec (old events) or s64 ns (new
events) sounds like the the approach to pick here.

> > Otherwise looks all good, but haven't yet carefully hunted for fumbles in
> > review before the above is clear.
> 
> Thanks. I'll switch over to ktime and wait to hear what your thoughts
> are on the vblank count interface changes.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch