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

[Keith Packard]

Daniel Vetter <daniel@xxxxxxxx> 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 <daniel.vetter@xxxxxxxx>

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 & 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) ?

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 <stdint.h>
#include <assert.h>
#include <stdlib.h>
#include <stdio.h>

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 < 100000; 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 < 100000; 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

Attachment: signature.asc
Description: PGP signature