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

From: Daniel Vetter <daniel@ffwll.ch>
To: Keith Packard <keithp@keithp.com>
Cc: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2]
Date: Wed, 2 Aug 2017 10:53:58 +0200	[thread overview]
Message-ID: <20170802085358.cipsolpgxlb2e323@phenom.ffwll.local> (raw)
In-Reply-To: <20170801050306.24423-2-keithp@keithp.com>

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@daenzer.net>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Keith Packard <keithp@keithp.com>

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@ffwll.ch>

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