Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2]

From: Daniel Vetter
Date: Wed Aug 02 2017 - 04:54:59 EST


On Mon, Jul 31, 2017 at 10:03:04PM -0700, Keith Packard wrote:
> This modifies the datatypes used by the vblank code to provide both 64
> bits of vblank count and switch to using ktime_t for timestamps to
> increase resolution from microseconds to nanoseconds.
>
> The driver interfaces have been left using 32 bits of vblank count;
> all of the code necessary to widen that value for the user API was
> already included to handle devices returning fewer than 32-bits.
>
> This will provide the necessary datatypes for the Vulkan API.
>
> v2:
>
> * Re-write wait_vblank ioctl to ABSOLUTE sequence
>
> When an application uses the WAIT_VBLANK ioctl with RELATIVE
> or NEXTONMISS bits set, the target vblank interval is updated
> within the kernel. We need to write that target back to the
> ioctl buffer and update the flags bits so that if the wait is
> interrupted by a signal, when it is re-started, it will target
> precisely the same vblank count as before.
>
> * Leave driver API with 32-bit vblank count
>
> Suggested-by: Michel Dänzer <michel@xxxxxxxxxxx>
> Suggested-by: Daniel Vetter <daniel@xxxxxxxx>
> Signed-off-by: Keith Packard <keithp@xxxxxxxxxx>

Subject is a bit confusing since you say uapi, but this is just the
internal prep work. Dropping UAPI fixes that. With that fixed:

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

Two more optional comments below, feel free to adapt or ignore. I'll wait
for Michel's r-b before merging either way.

> static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> + u64 req_seq,
> union drm_wait_vblank *vblwait,

Minor bikeshed: Since you pass the requested vblank number explicit, mabye
also pass the user_data explicit and remove the vblwait struct from the
parameter list? Restricts the old uapi cruft a bit.

> /*
> + * Widen a 32-bit param to 64-bits.
> + *
> + * \param narrow 32-bit value (missing upper 32 bits)
> + * \param near 64-bit value that should be 'close' to near
> + *
> + * This function returns a 64-bit value using the lower 32-bits from
> + * 'narrow' and constructing the upper 32-bits so that the result is
> + * as close as possible to 'near'.
> + */
> +
> +static u64 widen_32_to_64(u32 narrow, u64 near)
> +{
> + u64 wide = narrow | (near & 0xffffffff00000000ULL);
> + if ((int64_t) (wide - near) > 0x80000000LL)
> + wide -= 0x100000000ULL;
> + else if ((int64_t) (near - wide) > 0x80000000LL)
> + wide += 0x100000000ULL;
> + return wide;

return near + (int32_s) ((uint32_t)wide - near) ?

But then it took me way too long to think about this one, so maybe leave
it at that.

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