Messages in this thread | | | From | "Keith Packard" <> | Subject | Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2] | Date | Sun, 06 Aug 2017 13:35:03 -0400 |
| |
Daniel Vetter <daniel@ffwll.ch> 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@ffwll.ch>
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 [unhandled content-type:application/pgp-signature] | |