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

[Keith Packard]

Daniel Vetter  writes:

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

Yeah, thanks.

> Reviewed-by: Daniel Vetter 

Added.

> 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.

I also need to re-write the reply.sequence value in the queue
function; seems like passing in the vblwait is a simpler plan.

>> +static u64 widen_32_to_64(u32 narrow, u64 near)
>> +{
>> +u64 wide = narrow | (near & 0xULL);
>> +if ((int64_t) (wide - near) > 0x8000LL)
>> +wide -= 0x1ULL;
>> +else if ((int64_t) (near - wide) > 0x8000LL)
>> +wide += 0x1ULL;
>> +return wide;
>
> return near + (int32_s) ((uint32_t)wide - near) ?

Oh, yes, that makes perfect sense -- an int32_t will obviously hold the
shortest distance between the two, whether negative or positive. Of
course, '(uint32_t) wide' is just 'narrow'.

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

Your version is a lot shorter, and I think it's actually clearer. How
about

static inline uint64_t widen_32_to_64(uint32_t narrow, uint64_t near)
{
return near + (int32_t) (narrow - (uint32_t) near);
}

Here's a test program which validates the widen function.

#include 
#include 
#include 
#include 

static inline uint64_t widen_32_to_64(uint32_t narrow, uint64_t near)
{
	return near + (int32_t) (narrow - (uint32_t) near);
}

uint64_t
random_u64(void) {
	assert(sizeof (long int) == 8);
	return (uint32_t) mrand48() | (((uint64_t) (uint32_t) mrand48()) << 32);
}

uint32_t
random_u32(int bits) {
	return random_u64() & ((1UL << bits) - 1);
}

int32_t
random_s32(int bits) {
	int32_t	value = (int32_t) random_u32(bits);

	return value << (32 - bits) >> (32 - bits);
}

int
main(int argc, char **argv) {
	int bits;
	int tries;

	/* Validate random number generators */
	for (bits = 1; bits <= 32; bits++) {
		int64_t	max = ((1L << (bits-1)) - 1);
		int32_t min = -max - 1;
		int32_t range_min = INT32_MAX;
		int32_t range_max = INT32_MIN;

		for (tries = 0; tries < 10; tries++) {
			int32_t	i = random_s32(bits);

			if (i < min || max < i) {
printf ("min %d i %d max %d\n", min, i, max);
exit(1);
			}
			if (i < range_min)
range_min = i;
			if (i > range_max)
range_max = i;
		}
		if (range_min >= min/2 || (range_max <= max/2 && max != 0)) {
			printf ("bits %d min %d max %d range min %d range max %d\n",
bits, min, max, range_min, range_max);
			exit(1);
		}
	}

	/* Check to make sure the 'widen' function generates the right answer */
	for (bits = 1; bits < 32; bits++) {
		for (tries = 0; tries < 10; tries++) {

			/* A random 64-bit value */
			uint64_t	near = random_u64();

			/* Compute a nearby value, within a 32-bit delta of the target*/
			int32_t		delta = random_s32(bits);
			uint64_t	value = near + delta;

			/* Narrow the value to 32-bits */
			uint32_t	narrow = (uint32_t) (value);

			/* Use our 'widen' function to reconstruct the wider value */
			uint64_t	wide = widen_32_to_64(narrow, near);

			/* Make sure the reconstruction worked */
			if (wide != value)
printf ("widen failed near %ld value %ld wide %ld\n",
	near, value, wide);
		}
	}
}

-- 
-keith


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 02/08/17 05:53 PM, Daniel Vetter wrote:
> 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 
>> Suggested-by: Daniel Vetter 
>> Signed-off-by: Keith Packard 
> 
> 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 
> 
> Two more optional comments below, feel free to adapt or ignore. I'll wait
> for Michel's r-b before merging either way.

I don't think changing max_vblank_count to u64 is necessary/useful;
other than that, AFAICT the issues I raised before for this patch have
been addressed. I'm afraid I don't know if/when I'll get a chance to
review the whole patch in detail though.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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 
> Suggested-by: Daniel Vetter 
> Signed-off-by: Keith Packard 

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 

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 & 0xULL);
> + if ((int64_t) (wide - near) > 0x8000LL)
> + wide -= 0x1ULL;
> + else if ((int64_t) (near - wide) > 0x8000LL)
> + wide += 0x1ULL;
> + 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel