Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751429AbdHFRfK (ORCPT ); Sun, 6 Aug 2017 13:35:10 -0400 Received: from home.keithp.com ([63.227.221.253]:56892 "EHLO elaine.keithp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751300AbdHFRfI (ORCPT ); Sun, 6 Aug 2017 13:35:08 -0400 From: "Keith Packard" To: Daniel Vetter Cc: linux-kernel@vger.kernel.org, Dave Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Subject: Re: [PATCH 1/3] drm: Widen vblank UAPI to 64 bits. Change vblank time to ktime_t [v2] In-Reply-To: <20170802085358.cipsolpgxlb2e323@phenom.ffwll.local> References: <20170705221013.27940-1-keithp@keithp.com> <20170801050306.24423-1-keithp@keithp.com> <20170801050306.24423-2-keithp@keithp.com> <20170802085358.cipsolpgxlb2e323@phenom.ffwll.local> Date: Sun, 06 Aug 2017 13:35:03 -0400 Message-ID: <87wp6geh14.fsf@keithp.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4936 Lines: 178 --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain 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 & 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. --=-=-= Content-Type: text/x-csrc Content-Disposition: inline; filename=near.c #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 < 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); } } } --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable =2D-=20 =2Dkeith --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEw4O3eCVWE9/bQJ2R2yIaaQAAABEFAlmHU0gACgkQ2yIaaQAA ABHSTw//e5gWypyaZI7W1wwJTDd+Sv6h1JsBiHrL4MVGCuOR1zBsPR8UVFxCcSpH 161ot+m+WZWV3Kt4Sb7oDm3sXZkDBmkB3tCnHFAAdqBmRgux5Y2IthKZ7gfyL98R Yx3qLomEozguQ7MtwFV55yrd1HaLs28H3jzCbvUibRT9xnanH9+uMR7vkLbFkhBv /QWLejTUvepIFKivNdzPClJQOwBmFt/BoaS6UDjuw1SsO3UZLKXHfBFQ7FogXQus /GpbdbQcOXYRI8SRimPC24leoiG5YTipesHkaSw4TvFpbFejAXA9Ny88mRRlGeQB 4C10hY4ytHy5LifwmzSNLhyBDV4FyAsdt/SEAp+wN2u4YP2IUYAccUf+g/iUYIw9 +UaJcnr0uP8rysxuItdi41M5nU57Sv5XtEA3/pyOOERXW90fdQPwnxSKZqXRQj4b N+5+vX0h6sSbucPuwENd/NcR0NJOc3+yQF9xcKOH5f593u40nnOGM2FRD65BCKMx 0BW2CyTTwL0Guw2DAOPuhKxPfxinUGu/OtIFQw+0Drz4nAOAHeYwGPA3EhevnZyj zNRAZZUGtRkn2of/t4OVAveP3Y/wpOliruickoPtNO8Zv0d16fQXaCLvRihhKb+8 KGPEkTcCoLHIARh6FOlz6Mc05KaMZzLZdlH53AVyEPNHuszb8CY= =ynZw -----END PGP SIGNATURE----- --==-=-=--