* [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast @ 2023-05-16 17:30 Sui Jingfeng 2023-05-17 10:59 ` David Laight 0 siblings, 1 reply; 17+ messages in thread From: Sui Jingfeng @ 2023-05-16 17:30 UTC (permalink / raw) To: Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter, dri-devel, linux-kernel, loongson-kernel From: Sui Jingfeng <suijingfeng@loongson.cn> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. Using a u32 is enough to store the result, but considering that the result will be casted to u64 soon after. We use a u64 type directly. So there no need to cast it to signed type and cast back then. Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: loongson-kernel@lists.loongnix.cn --- drivers/gpu/drm/drm_vblank.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 877e2067534f..d99c404b181b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -622,7 +622,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, /* Valid dotclock? */ if (dotclock > 0) { - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; /* * Convert scanline length in pixels and video @@ -630,7 +630,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, * in nanoseconds: */ linedur_ns = div_u64((u64) mode->crtc_htotal * 1000000, dotclock); - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); + framedur_ns = div_u64(frame_size * 1000000, dotclock); /* * Fields of interlaced scanout modes are only half a frame duration. -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* RE: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-16 17:30 [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast Sui Jingfeng @ 2023-05-17 10:59 ` David Laight 2023-05-17 18:08 ` Sui Jingfeng 0 siblings, 1 reply; 17+ messages in thread From: David Laight @ 2023-05-17 10:59 UTC (permalink / raw) To: '15330273260@189.cn', Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, loongson-kernel@lists.loongnix.cn From: 15330273260@189.cn > Sent: 16 May 2023 18:30 > > From: Sui Jingfeng <suijingfeng@loongson.cn> > > Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, > mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. Nope, u16 gets promoted to 'signed int' and the result of the multiply is also signed. > Using a u32 is enough to store the result, but considering that the > result will be casted to u64 soon after. We use a u64 type directly. > So there no need to cast it to signed type and cast back then. .... > - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; > + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; ... > - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); > + framedur_ns = div_u64(frame_size * 1000000, dotclock); The (u64) cast is there to extend the value to 64bits, not because the original type is signed. The compiler will detect that the old code is a 32x32 multiply where a 64bit result is needed, that may not be true for the changed code (it would need to track back as far as the u16s). It is not uncommon to force a 64bit result from a multiply by making the constant 64bit. As in: div_u64(frame_size * 1000000ULL, dotclock); David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-17 10:59 ` David Laight @ 2023-05-17 18:08 ` Sui Jingfeng 2023-05-22 11:29 ` Jani Nikula 0 siblings, 1 reply; 17+ messages in thread From: Sui Jingfeng @ 2023-05-17 18:08 UTC (permalink / raw) To: David Laight, Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, loongson-kernel@lists.loongnix.cn On 2023/5/17 18:59, David Laight wrote: > From: 15330273260@189.cn >> Sent: 16 May 2023 18:30 >> >> From: Sui Jingfeng <suijingfeng@loongson.cn> >> >> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. > Nope, u16 gets promoted to 'signed int' and the result of the > multiply is also signed. I believe that signed or unsigned is dependent on the declaration. I am talk about the math, while you are talking about compiler. I admit that u16 gets promoted to 'signed int' is true, but this is irrelevant, the point is how to understand the returned value. How does the compiler generate the code is one thing, how do we interpret the result is another How does the compiler generate the code is NOT determined by us, while how do we interpret the result is determined by us. I believe that using a u32 type to interpret the result(u16 * u16) is always true, it is true in the perspective of *math*. Integer promotions is the details of C program language. If the result of the multiply is signed, then there are risks that the result is negative, what's the benefit to present this risk to the programmer? What's the benefit to tell me(and others) that u16 * u16 yield a signed value? and can be negative? Using int type as the return type bring concerns to the programmer and the user of the function, even though this is not impossible in practice. >> Using a u32 is enough to store the result, but considering that the >> result will be casted to u64 soon after. We use a u64 type directly. >> So there no need to cast it to signed type and cast back then. > .... >> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; > ... >> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >> + framedur_ns = div_u64(frame_size * 1000000, dotclock); > The (u64) cast is there to extend the value to 64bits, not > because the original type is signed. Sorry about my expression, I think my sentence did not mention anything about 'because the original type is signed'. In the contrary, my patch eliminated the concerns to the reviewer. It say that the results of the multiply can't be negative. My intent is to tell the compiler we want a unsigned return type, but GCC emit 'imul' instruction for the multiply...... I'm using u64 as the return type, because div_u64() function accept a u64 type value as its first argument. > The compiler will detect that the old code is a 32x32 multiply > where a 64bit result is needed, that may not be true for the > changed code (it would need to track back as far as the u16s). I don't believe my code could be wrong. when you use the word 'may', you are saying that it could be wrong after apply my patch. Then you have to find at least one test example to prove you point, in which case my codes generate wrong results. Again I don't believe you could find one. > It is not uncommon to force a 64bit result from a multiply > by making the constant 64bit. As in: > div_u64(frame_size * 1000000ULL, dotclock); In fact, After apply this patch, the ASM code generated is same with before. This may because the GCC is smart enough to generate optimized code in either case, I think It could be different with a different optimization-level. I have tested this patch on three different architecture, I can not find error still. Below is the assembly extract on x86-64: because GCC generate the same code in either case, so I pasted only one copy here. 0000000000000530 <drm_calc_timestamping_constants>: 530: f3 0f 1e fa endbr64 534: e8 00 00 00 00 callq 539 <drm_calc_timestamping_constants+0x9> 539: 55 push %rbp 53a: 48 89 e5 mov %rsp,%rbp 53d: 41 57 push %r15 53f: 41 56 push %r14 541: 41 55 push %r13 543: 41 54 push %r12 545: 53 push %rbx 546: 48 83 ec 18 sub $0x18,%rsp 54a: 4c 8b 3f mov (%rdi),%r15 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax 554: 85 c0 test %eax,%eax 556: 0f 84 ec 00 00 00 je 648 <drm_calc_timestamping_constants+0x118> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d 563: 49 89 fc mov %rdi,%r12 566: 44 39 c0 cmp %r8d,%eax 569: 0f 86 40 01 00 00 jbe 6af <drm_calc_timestamping_constants+0x17f> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx 57a: 48 89 f3 mov %rsi,%rbx 57d: 45 85 f6 test %r14d,%r14d 580: 0f 8e d5 00 00 00 jle 65b <drm_calc_timestamping_constants+0x12b> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax 58a: 49 63 f6 movslq %r14d,%rsi 58d: 31 d2 xor %edx,%edx 58f: 48 89 c7 mov %rax,%rdi 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax 599: 48 f7 f6 div %rsi 59c: 31 d2 xor %edx,%edx 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax 5a6: 0f af c7 imul %edi,%eax 5a9: 48 98 cltq 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax 5b2: 48 f7 f6 div %rsi 5b5: 41 89 c5 mov %eax,%r13d 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) 5bc: 74 0a je 5c8 <drm_calc_timestamping_constants+0x98> 5be: 41 c1 ed 1f shr $0x1f,%r13d 5c2: 41 01 c5 add %eax,%r13d 5c5: 41 d1 fd sar %r13d 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax 5cc: 48 89 de mov %rbx,%rsi 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi 5d3: 8b 45 d0 mov -0x30(%rbp),%eax 5d6: 48 c1 e7 04 shl $0x4,%rdi 5da: 48 01 cf add %rcx,%rdi 5dd: 89 47 78 mov %eax,0x78(%rdi) 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) 5e8: e8 00 00 00 00 callq 5ed <drm_calc_timestamping_constants+0xbd> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d 5ff: 4d 85 ff test %r15,%r15 602: 0f 84 87 00 00 00 je 68f <drm_calc_timestamping_constants+0x15f> 608: 49 8b 77 08 mov 0x8(%r15),%rsi 60c: 52 push %rdx 60d: 31 ff xor %edi,%edi 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx 616: 50 push %rax 617: 31 d2 xor %edx,%edx 619: e8 00 00 00 00 callq 61e <drm_calc_timestamping_constants+0xee> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 627: 5f pop %rdi 628: 41 59 pop %r9 62a: 8b 45 d0 mov -0x30(%rbp),%eax 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx 634: 4c 89 fe mov %r15,%rsi 637: 45 89 f1 mov %r14d,%r9d 63a: 31 d2 xor %edx,%edx 63c: 31 ff xor %edi,%edi 63e: 50 push %rax 63f: 41 55 push %r13 641: e8 00 00 00 00 callq 646 <drm_calc_timestamping_constants+0x116> 646: 59 pop %rcx 647: 5e pop %rsi 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp 64c: 5b pop %rbx 64d: 41 5c pop %r12 64f: 41 5d pop %r13 651: 41 5e pop %r14 653: 41 5f pop %r15 655: 5d pop %rbp 656: e9 00 00 00 00 jmpq 65b <drm_calc_timestamping_constants+0x12b> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx 660: 49 8b 7f 08 mov 0x8(%r15),%rdi 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) 668: 45 31 ed xor %r13d,%r13d 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) 676: e8 00 00 00 00 callq 67b <drm_calc_timestamping_constants+0x14b> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx 68a: e9 39 ff ff ff jmpq 5c8 <drm_calc_timestamping_constants+0x98> 68f: 52 push %rdx 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx 697: 31 d2 xor %edx,%edx 699: 31 f6 xor %esi,%esi 69b: 50 push %rax 69c: 31 ff xor %edi,%edi 69e: e8 00 00 00 00 callq 6a3 <drm_calc_timestamping_constants+0x173> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d 6a8: 58 pop %rax 6a9: 5a pop %rdx 6aa: e9 7b ff ff ff jmpq 62a <drm_calc_timestamping_constants+0xfa> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 6b7: 4d 85 e4 test %r12,%r12 6ba: 74 25 je 6e1 <drm_calc_timestamping_constants+0x1b1> 6bc: e8 00 00 00 00 callq 6c1 <drm_calc_timestamping_constants+0x191> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx 6c8: 4c 89 e2 mov %r12,%rdx 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi 6d2: 48 89 c6 mov %rax,%rsi 6d5: e8 00 00 00 00 callq 6da <drm_calc_timestamping_constants+0x1aa> 6da: 0f 0b ud2 6dc: e9 67 ff ff ff jmpq 648 <drm_calc_timestamping_constants+0x118> 6e1: 4c 8b 27 mov (%rdi),%r12 6e4: eb d6 jmp 6bc <drm_calc_timestamping_constants+0x18c> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) 6ed: 00 00 00 6f0: 90 nop 6f1: 90 nop 6f2: 90 nop 6f3: 90 nop 6f4: 90 nop 6f5: 90 nop 6f6: 90 nop 6f7: 90 nop 6f8: 90 nop 6f9: 90 nop 6fa: 90 nop 6fb: 90 nop 6fc: 90 nop 6fd: 90 nop 6fe: 90 nop 6ff: 90 nop > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-17 18:08 ` Sui Jingfeng @ 2023-05-22 11:29 ` Jani Nikula 2023-05-22 11:55 ` Sui Jingfeng 2023-05-23 4:26 ` Sui Jingfeng 0 siblings, 2 replies; 17+ messages in thread From: Jani Nikula @ 2023-05-22 11:29 UTC (permalink / raw) To: Sui Jingfeng, David Laight, Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: > On 2023/5/17 18:59, David Laight wrote: >> From: 15330273260@189.cn >>> Sent: 16 May 2023 18:30 >>> >>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>> >>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >> Nope, u16 gets promoted to 'signed int' and the result of the >> multiply is also signed. > > I believe that signed or unsigned is dependent on the declaration. > > I am talk about the math, while you are talking about compiler. > > I admit that u16 gets promoted to 'signed int' is true, but this is > irrelevant, > > the point is how to understand the returned value. > > > How does the compiler generate the code is one thing, how do we > interpret the result is another > > How does the compiler generate the code is NOT determined by us, while > how do we interpret the result is determined by us. > > > I believe that using a u32 type to interpret the result(u16 * u16) is > always true, it is true in the perspective of *math*. > > Integer promotions is the details of C program language. If the result > of the multiply is signed, then there are risks that > > the result is negative, what's the benefit to present this risk to the > programmer? > > What's the benefit to tell me(and others) that u16 * u16 yield a signed > value? and can be negative? > > Using int type as the return type bring concerns to the programmer and > the user of the function, > > even though this is not impossible in practice. In general, do not use unsigned types in arithmethic to avoid negative values, because most people will be tripped over by integer promotion rules, and you'll get negative values anyway. I'll bet most people will be surprised to see what this prints: #include <stdio.h> #include <stdint.h> int main(void) { uint16_t x = 0xffff; uint16_t y = 0xffff; uint64_t z = x * y; printf("0x%016lx\n", z); printf("%ld\n", z); printf("%d\n", x * y); } And it's not that different from what you have below. Your patch doesn't change anything, and doesn't make it any less confusing. BR, Jani. > >>> Using a u32 is enough to store the result, but considering that the >>> result will be casted to u64 soon after. We use a u64 type directly. >>> So there no need to cast it to signed type and cast back then. >> .... >>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >> ... >>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >>> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >> The (u64) cast is there to extend the value to 64bits, not >> because the original type is signed. > > Sorry about my expression, I think my sentence did not mention anything > about 'because the original type is signed'. > > In the contrary, my patch eliminated the concerns to the reviewer. It > say that the results of the multiply can't be negative. > > My intent is to tell the compiler we want a unsigned return type, but > GCC emit 'imul' instruction for the multiply...... > > I'm using u64 as the return type, because div_u64() function accept a > u64 type value as its first argument. > >> The compiler will detect that the old code is a 32x32 multiply >> where a 64bit result is needed, that may not be true for the >> changed code (it would need to track back as far as the u16s). > > I don't believe my code could be wrong. > > when you use the word 'may', you are saying that it could be wrong after > apply my patch. > > Then you have to find at least one test example to prove you point, in > which case my codes generate wrong results. > > Again I don't believe you could find one. > >> It is not uncommon to force a 64bit result from a multiply >> by making the constant 64bit. As in: >> div_u64(frame_size * 1000000ULL, dotclock); > > In fact, After apply this patch, the ASM code generated is same with before. > > This may because the GCC is smart enough to generate optimized code in > either case, > > I think It could be different with a different optimization-level. > > I have tested this patch on three different architecture, I can not > find error still. > > Below is the assembly extract on x86-64: because GCC generate the same > code in either case, > > so I pasted only one copy here. > > > 0000000000000530 <drm_calc_timestamping_constants>: > 530: f3 0f 1e fa endbr64 > 534: e8 00 00 00 00 callq 539 > <drm_calc_timestamping_constants+0x9> > 539: 55 push %rbp > 53a: 48 89 e5 mov %rsp,%rbp > 53d: 41 57 push %r15 > 53f: 41 56 push %r14 > 541: 41 55 push %r13 > 543: 41 54 push %r12 > 545: 53 push %rbx > 546: 48 83 ec 18 sub $0x18,%rsp > 54a: 4c 8b 3f mov (%rdi),%r15 > 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax > 554: 85 c0 test %eax,%eax > 556: 0f 84 ec 00 00 00 je 648 > <drm_calc_timestamping_constants+0x118> > 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d > 563: 49 89 fc mov %rdi,%r12 > 566: 44 39 c0 cmp %r8d,%eax > 569: 0f 86 40 01 00 00 jbe 6af > <drm_calc_timestamping_constants+0x17f> > 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d > 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx > 57a: 48 89 f3 mov %rsi,%rbx > 57d: 45 85 f6 test %r14d,%r14d > 580: 0f 8e d5 00 00 00 jle 65b > <drm_calc_timestamping_constants+0x12b> > 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax > 58a: 49 63 f6 movslq %r14d,%rsi > 58d: 31 d2 xor %edx,%edx > 58f: 48 89 c7 mov %rax,%rdi > 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax > 599: 48 f7 f6 div %rsi > 59c: 31 d2 xor %edx,%edx > 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) > 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax > 5a6: 0f af c7 imul %edi,%eax > 5a9: 48 98 cltq > 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax > 5b2: 48 f7 f6 div %rsi > 5b5: 41 89 c5 mov %eax,%r13d > 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) > 5bc: 74 0a je 5c8 > <drm_calc_timestamping_constants+0x98> > 5be: 41 c1 ed 1f shr $0x1f,%r13d > 5c2: 41 01 c5 add %eax,%r13d > 5c5: 41 d1 fd sar %r13d > 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax > 5cc: 48 89 de mov %rbx,%rsi > 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi > 5d3: 8b 45 d0 mov -0x30(%rbp),%eax > 5d6: 48 c1 e7 04 shl $0x4,%rdi > 5da: 48 01 cf add %rcx,%rdi > 5dd: 89 47 78 mov %eax,0x78(%rdi) > 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi > 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) > 5e8: e8 00 00 00 00 callq 5ed > <drm_calc_timestamping_constants+0xbd> > 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx > 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax > 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d > 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d > 5ff: 4d 85 ff test %r15,%r15 > 602: 0f 84 87 00 00 00 je 68f > <drm_calc_timestamping_constants+0x15f> > 608: 49 8b 77 08 mov 0x8(%r15),%rsi > 60c: 52 push %rdx > 60d: 31 ff xor %edi,%edi > 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx > 616: 50 push %rax > 617: 31 d2 xor %edx,%edx > 619: e8 00 00 00 00 callq 61e > <drm_calc_timestamping_constants+0xee> > 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d > 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 > 627: 5f pop %rdi > 628: 41 59 pop %r9 > 62a: 8b 45 d0 mov -0x30(%rbp),%eax > 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx > 634: 4c 89 fe mov %r15,%rsi > 637: 45 89 f1 mov %r14d,%r9d > 63a: 31 d2 xor %edx,%edx > 63c: 31 ff xor %edi,%edi > 63e: 50 push %rax > 63f: 41 55 push %r13 > 641: e8 00 00 00 00 callq 646 > <drm_calc_timestamping_constants+0x116> > 646: 59 pop %rcx > 647: 5e pop %rsi > 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp > 64c: 5b pop %rbx > 64d: 41 5c pop %r12 > 64f: 41 5d pop %r13 > 651: 41 5e pop %r14 > 653: 41 5f pop %r15 > 655: 5d pop %rbp > 656: e9 00 00 00 00 jmpq 65b > <drm_calc_timestamping_constants+0x12b> > 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx > 660: 49 8b 7f 08 mov 0x8(%r15),%rdi > 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) > 668: 45 31 ed xor %r13d,%r13d > 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi > 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) > 676: e8 00 00 00 00 callq 67b > <drm_calc_timestamping_constants+0x14b> > 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) > 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d > 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx > 68a: e9 39 ff ff ff jmpq 5c8 > <drm_calc_timestamping_constants+0x98> > 68f: 52 push %rdx > 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx > 697: 31 d2 xor %edx,%edx > 699: 31 f6 xor %esi,%esi > 69b: 50 push %rax > 69c: 31 ff xor %edi,%edi > 69e: e8 00 00 00 00 callq 6a3 > <drm_calc_timestamping_constants+0x173> > 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d > 6a8: 58 pop %rax > 6a9: 5a pop %rdx > 6aa: e9 7b ff ff ff jmpq 62a > <drm_calc_timestamping_constants+0xfa> > 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi > 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 > 6b7: 4d 85 e4 test %r12,%r12 > 6ba: 74 25 je 6e1 > <drm_calc_timestamping_constants+0x1b1> > 6bc: e8 00 00 00 00 callq 6c1 > <drm_calc_timestamping_constants+0x191> > 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx > 6c8: 4c 89 e2 mov %r12,%rdx > 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi > 6d2: 48 89 c6 mov %rax,%rsi > 6d5: e8 00 00 00 00 callq 6da > <drm_calc_timestamping_constants+0x1aa> > 6da: 0f 0b ud2 > 6dc: e9 67 ff ff ff jmpq 648 > <drm_calc_timestamping_constants+0x118> > 6e1: 4c 8b 27 mov (%rdi),%r12 > 6e4: eb d6 jmp 6bc > <drm_calc_timestamping_constants+0x18c> > 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) > 6ed: 00 00 00 > 6f0: 90 nop > 6f1: 90 nop > 6f2: 90 nop > 6f3: 90 nop > 6f4: 90 nop > 6f5: 90 nop > 6f6: 90 nop > 6f7: 90 nop > 6f8: 90 nop > 6f9: 90 nop > 6fa: 90 nop > 6fb: 90 nop > 6fc: 90 nop > 6fd: 90 nop > 6fe: 90 nop > 6ff: 90 nop > > >> David >> >> - >> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >> Registration No: 1397386 (Wales) >> -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 11:29 ` Jani Nikula @ 2023-05-22 11:55 ` Sui Jingfeng 2023-05-22 12:07 ` David Laight 2023-05-22 12:13 ` Jani Nikula 2023-05-23 4:26 ` Sui Jingfeng 1 sibling, 2 replies; 17+ messages in thread From: Sui Jingfeng @ 2023-05-22 11:55 UTC (permalink / raw) To: Jani Nikula, Sui Jingfeng, David Laight, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn Hi, On 2023/5/22 19:29, Jani Nikula wrote: > On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >> On 2023/5/17 18:59, David Laight wrote: >>> From: 15330273260@189.cn >>>> Sent: 16 May 2023 18:30 >>>> >>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>> >>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >>> Nope, u16 gets promoted to 'signed int' and the result of the >>> multiply is also signed. >> I believe that signed or unsigned is dependent on the declaration. >> >> I am talk about the math, while you are talking about compiler. >> >> I admit that u16 gets promoted to 'signed int' is true, but this is >> irrelevant, >> >> the point is how to understand the returned value. >> >> >> How does the compiler generate the code is one thing, how do we >> interpret the result is another >> >> How does the compiler generate the code is NOT determined by us, while >> how do we interpret the result is determined by us. >> >> >> I believe that using a u32 type to interpret the result(u16 * u16) is >> always true, it is true in the perspective of *math*. >> >> Integer promotions is the details of C program language. If the result >> of the multiply is signed, then there are risks that >> >> the result is negative, what's the benefit to present this risk to the >> programmer? >> >> What's the benefit to tell me(and others) that u16 * u16 yield a signed >> value? and can be negative? >> >> Using int type as the return type bring concerns to the programmer and >> the user of the function, >> >> even though this is not impossible in practice. > In general, do not use unsigned types in arithmethic to avoid negative > values, because most people will be tripped over by integer promotion > rules, and you'll get negative values anyway. > > I'll bet most people will be surprised to see what this prints: > > #include <stdio.h> > #include <stdint.h> > > int main(void) > { > uint16_t x = 0xffff; > uint16_t y = 0xffff; > uint64_t z = x * y; > > printf("0x%016lx\n", z); > printf("%ld\n", z); Here, please replace the "%ld\n" with the "%lu\n", then you will see the difference. you are casting the variable 'z' to signed value, "%d" is for printing signed value, and "%u" is for printing unsigned value. Your simple code explained exactly why you are still in confusion, that is u16 * u16 can yield a negative value if you use the int as the return type. Because it overflowed. > printf("%d\n", x * y); > } > > And it's not that different from what you have below. Your patch doesn't > change anything, and doesn't make it any less confusing. > > BR, > Jani. > > >>>> Using a u32 is enough to store the result, but considering that the >>>> result will be casted to u64 soon after. We use a u64 type directly. >>>> So there no need to cast it to signed type and cast back then. >>> .... >>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>> ... >>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >>> The (u64) cast is there to extend the value to 64bits, not >>> because the original type is signed. >> Sorry about my expression, I think my sentence did not mention anything >> about 'because the original type is signed'. >> >> In the contrary, my patch eliminated the concerns to the reviewer. It >> say that the results of the multiply can't be negative. >> >> My intent is to tell the compiler we want a unsigned return type, but >> GCC emit 'imul' instruction for the multiply...... >> >> I'm using u64 as the return type, because div_u64() function accept a >> u64 type value as its first argument. >> >>> The compiler will detect that the old code is a 32x32 multiply >>> where a 64bit result is needed, that may not be true for the >>> changed code (it would need to track back as far as the u16s). >> I don't believe my code could be wrong. >> >> when you use the word 'may', you are saying that it could be wrong after >> apply my patch. >> >> Then you have to find at least one test example to prove you point, in >> which case my codes generate wrong results. >> >> Again I don't believe you could find one. >> >>> It is not uncommon to force a 64bit result from a multiply >>> by making the constant 64bit. As in: >>> div_u64(frame_size * 1000000ULL, dotclock); >> In fact, After apply this patch, the ASM code generated is same with before. >> >> This may because the GCC is smart enough to generate optimized code in >> either case, >> >> I think It could be different with a different optimization-level. >> >> I have tested this patch on three different architecture, I can not >> find error still. >> >> Below is the assembly extract on x86-64: because GCC generate the same >> code in either case, >> >> so I pasted only one copy here. >> >> >> 0000000000000530 <drm_calc_timestamping_constants>: >> 530: f3 0f 1e fa endbr64 >> 534: e8 00 00 00 00 callq 539 >> <drm_calc_timestamping_constants+0x9> >> 539: 55 push %rbp >> 53a: 48 89 e5 mov %rsp,%rbp >> 53d: 41 57 push %r15 >> 53f: 41 56 push %r14 >> 541: 41 55 push %r13 >> 543: 41 54 push %r12 >> 545: 53 push %rbx >> 546: 48 83 ec 18 sub $0x18,%rsp >> 54a: 4c 8b 3f mov (%rdi),%r15 >> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax >> 554: 85 c0 test %eax,%eax >> 556: 0f 84 ec 00 00 00 je 648 >> <drm_calc_timestamping_constants+0x118> >> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d >> 563: 49 89 fc mov %rdi,%r12 >> 566: 44 39 c0 cmp %r8d,%eax >> 569: 0f 86 40 01 00 00 jbe 6af >> <drm_calc_timestamping_constants+0x17f> >> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d >> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx >> 57a: 48 89 f3 mov %rsi,%rbx >> 57d: 45 85 f6 test %r14d,%r14d >> 580: 0f 8e d5 00 00 00 jle 65b >> <drm_calc_timestamping_constants+0x12b> >> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax >> 58a: 49 63 f6 movslq %r14d,%rsi >> 58d: 31 d2 xor %edx,%edx >> 58f: 48 89 c7 mov %rax,%rdi >> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >> 599: 48 f7 f6 div %rsi >> 59c: 31 d2 xor %edx,%edx >> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) >> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax >> 5a6: 0f af c7 imul %edi,%eax >> 5a9: 48 98 cltq >> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >> 5b2: 48 f7 f6 div %rsi >> 5b5: 41 89 c5 mov %eax,%r13d >> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) >> 5bc: 74 0a je 5c8 >> <drm_calc_timestamping_constants+0x98> >> 5be: 41 c1 ed 1f shr $0x1f,%r13d >> 5c2: 41 01 c5 add %eax,%r13d >> 5c5: 41 d1 fd sar %r13d >> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax >> 5cc: 48 89 de mov %rbx,%rsi >> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi >> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax >> 5d6: 48 c1 e7 04 shl $0x4,%rdi >> 5da: 48 01 cf add %rcx,%rdi >> 5dd: 89 47 78 mov %eax,0x78(%rdi) >> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi >> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) >> 5e8: e8 00 00 00 00 callq 5ed >> <drm_calc_timestamping_constants+0xbd> >> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx >> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax >> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d >> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d >> 5ff: 4d 85 ff test %r15,%r15 >> 602: 0f 84 87 00 00 00 je 68f >> <drm_calc_timestamping_constants+0x15f> >> 608: 49 8b 77 08 mov 0x8(%r15),%rsi >> 60c: 52 push %rdx >> 60d: 31 ff xor %edi,%edi >> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >> 616: 50 push %rax >> 617: 31 d2 xor %edx,%edx >> 619: e8 00 00 00 00 callq 61e >> <drm_calc_timestamping_constants+0xee> >> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d >> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 >> 627: 5f pop %rdi >> 628: 41 59 pop %r9 >> 62a: 8b 45 d0 mov -0x30(%rbp),%eax >> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >> 634: 4c 89 fe mov %r15,%rsi >> 637: 45 89 f1 mov %r14d,%r9d >> 63a: 31 d2 xor %edx,%edx >> 63c: 31 ff xor %edi,%edi >> 63e: 50 push %rax >> 63f: 41 55 push %r13 >> 641: e8 00 00 00 00 callq 646 >> <drm_calc_timestamping_constants+0x116> >> 646: 59 pop %rcx >> 647: 5e pop %rsi >> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp >> 64c: 5b pop %rbx >> 64d: 41 5c pop %r12 >> 64f: 41 5d pop %r13 >> 651: 41 5e pop %r14 >> 653: 41 5f pop %r15 >> 655: 5d pop %rbp >> 656: e9 00 00 00 00 jmpq 65b >> <drm_calc_timestamping_constants+0x12b> >> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx >> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi >> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) >> 668: 45 31 ed xor %r13d,%r13d >> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi >> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) >> 676: e8 00 00 00 00 callq 67b >> <drm_calc_timestamping_constants+0x14b> >> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) >> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d >> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx >> 68a: e9 39 ff ff ff jmpq 5c8 >> <drm_calc_timestamping_constants+0x98> >> 68f: 52 push %rdx >> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >> 697: 31 d2 xor %edx,%edx >> 699: 31 f6 xor %esi,%esi >> 69b: 50 push %rax >> 69c: 31 ff xor %edi,%edi >> 69e: e8 00 00 00 00 callq 6a3 >> <drm_calc_timestamping_constants+0x173> >> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d >> 6a8: 58 pop %rax >> 6a9: 5a pop %rdx >> 6aa: e9 7b ff ff ff jmpq 62a >> <drm_calc_timestamping_constants+0xfa> >> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi >> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 >> 6b7: 4d 85 e4 test %r12,%r12 >> 6ba: 74 25 je 6e1 >> <drm_calc_timestamping_constants+0x1b1> >> 6bc: e8 00 00 00 00 callq 6c1 >> <drm_calc_timestamping_constants+0x191> >> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >> 6c8: 4c 89 e2 mov %r12,%rdx >> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi >> 6d2: 48 89 c6 mov %rax,%rsi >> 6d5: e8 00 00 00 00 callq 6da >> <drm_calc_timestamping_constants+0x1aa> >> 6da: 0f 0b ud2 >> 6dc: e9 67 ff ff ff jmpq 648 >> <drm_calc_timestamping_constants+0x118> >> 6e1: 4c 8b 27 mov (%rdi),%r12 >> 6e4: eb d6 jmp 6bc >> <drm_calc_timestamping_constants+0x18c> >> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) >> 6ed: 00 00 00 >> 6f0: 90 nop >> 6f1: 90 nop >> 6f2: 90 nop >> 6f3: 90 nop >> 6f4: 90 nop >> 6f5: 90 nop >> 6f6: 90 nop >> 6f7: 90 nop >> 6f8: 90 nop >> 6f9: 90 nop >> 6fa: 90 nop >> 6fb: 90 nop >> 6fc: 90 nop >> 6fd: 90 nop >> 6fe: 90 nop >> 6ff: 90 nop >> >> >>> David >>> >>> - >>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >>> Registration No: 1397386 (Wales) >>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 11:55 ` Sui Jingfeng @ 2023-05-22 12:07 ` David Laight 2023-05-22 12:13 ` Jani Nikula 1 sibling, 0 replies; 17+ messages in thread From: David Laight @ 2023-05-22 12:07 UTC (permalink / raw) To: '15330273260@189.cn', Jani Nikula, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn From: 15330273260@189.cn <15330273260@189.cn> > Sent: 22 May 2023 12:56 ... > > I'll bet most people will be surprised to see what this prints: > > > > #include <stdio.h> > > #include <stdint.h> > > > > int main(void) > > { > > uint16_t x = 0xffff; > > uint16_t y = 0xffff; > > uint64_t z = x * y; > > > > printf("0x%016lx\n", z); > > printf("%ld\n", z); > > Here, please replace the "%ld\n" with the "%lu\n", then you will see the > difference. > > you are casting the variable 'z' to signed value, "%d" is for printing > signed value, and "%u" is for printing unsigned value. That makes very little difference on 2's compliment systems. They both display the contents of the variable. > Your simple code explained exactly why you are still in confusion, > > that is u16 * u16 can yield a negative value if you use the int as the > return type. Because it overflowed. There is no 'return type', the type of 'u16 * u16' is signed int. When 'signed int' is promoted/cast to u64 it is first sign extended to 64 bits. You can get what you want/expect by either forcing an unsigned multiply or by explicitly casting the result of the multiply to u32. So the product in 'z = (x + 0u) * y' is 'unsigned int' it gets promoted to int64_t (ie a signed type) and then converted to unsigned. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 11:55 ` Sui Jingfeng 2023-05-22 12:07 ` David Laight @ 2023-05-22 12:13 ` Jani Nikula 2023-05-22 12:53 ` Sui Jingfeng 1 sibling, 1 reply; 17+ messages in thread From: Jani Nikula @ 2023-05-22 12:13 UTC (permalink / raw) To: Sui Jingfeng, Sui Jingfeng, David Laight, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn On Mon, 22 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: > Hi, > > On 2023/5/22 19:29, Jani Nikula wrote: >> On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >>> On 2023/5/17 18:59, David Laight wrote: >>>> From: 15330273260@189.cn >>>>> Sent: 16 May 2023 18:30 >>>>> >>>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>>> >>>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >>>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >>>> Nope, u16 gets promoted to 'signed int' and the result of the >>>> multiply is also signed. >>> I believe that signed or unsigned is dependent on the declaration. >>> >>> I am talk about the math, while you are talking about compiler. >>> >>> I admit that u16 gets promoted to 'signed int' is true, but this is >>> irrelevant, >>> >>> the point is how to understand the returned value. >>> >>> >>> How does the compiler generate the code is one thing, how do we >>> interpret the result is another >>> >>> How does the compiler generate the code is NOT determined by us, while >>> how do we interpret the result is determined by us. >>> >>> >>> I believe that using a u32 type to interpret the result(u16 * u16) is >>> always true, it is true in the perspective of *math*. >>> >>> Integer promotions is the details of C program language. If the result >>> of the multiply is signed, then there are risks that >>> >>> the result is negative, what's the benefit to present this risk to the >>> programmer? >>> >>> What's the benefit to tell me(and others) that u16 * u16 yield a signed >>> value? and can be negative? >>> >>> Using int type as the return type bring concerns to the programmer and >>> the user of the function, >>> >>> even though this is not impossible in practice. >> In general, do not use unsigned types in arithmethic to avoid negative >> values, because most people will be tripped over by integer promotion >> rules, and you'll get negative values anyway. >> >> I'll bet most people will be surprised to see what this prints: >> >> #include <stdio.h> >> #include <stdint.h> >> >> int main(void) >> { >> uint16_t x = 0xffff; >> uint16_t y = 0xffff; >> uint64_t z = x * y; >> >> printf("0x%016lx\n", z); >> printf("%ld\n", z); > > Here, please replace the "%ld\n" with the "%lu\n", then you will see the > difference. > > you are casting the variable 'z' to signed value, "%d" is for printing > signed value, and "%u" is for printing unsigned value. > > > Your simple code explained exactly why you are still in confusion, Am I? Take a look at the values, and explain the math. BR, Jani. > > that is u16 * u16 can yield a negative value if you use the int as the > return type. Because it overflowed. > >> printf("%d\n", x * y); >> } >> >> And it's not that different from what you have below. Your patch doesn't >> change anything, and doesn't make it any less confusing. >> >> BR, >> Jani. >> >> >>>>> Using a u32 is enough to store the result, but considering that the >>>>> result will be casted to u64 soon after. We use a u64 type directly. >>>>> So there no need to cast it to signed type and cast back then. >>>> .... >>>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>> ... >>>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >>>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >>>> The (u64) cast is there to extend the value to 64bits, not >>>> because the original type is signed. >>> Sorry about my expression, I think my sentence did not mention anything >>> about 'because the original type is signed'. >>> >>> In the contrary, my patch eliminated the concerns to the reviewer. It >>> say that the results of the multiply can't be negative. >>> >>> My intent is to tell the compiler we want a unsigned return type, but >>> GCC emit 'imul' instruction for the multiply...... >>> >>> I'm using u64 as the return type, because div_u64() function accept a >>> u64 type value as its first argument. >>> >>>> The compiler will detect that the old code is a 32x32 multiply >>>> where a 64bit result is needed, that may not be true for the >>>> changed code (it would need to track back as far as the u16s). >>> I don't believe my code could be wrong. >>> >>> when you use the word 'may', you are saying that it could be wrong after >>> apply my patch. >>> >>> Then you have to find at least one test example to prove you point, in >>> which case my codes generate wrong results. >>> >>> Again I don't believe you could find one. >>> >>>> It is not uncommon to force a 64bit result from a multiply >>>> by making the constant 64bit. As in: >>>> div_u64(frame_size * 1000000ULL, dotclock); >>> In fact, After apply this patch, the ASM code generated is same with before. >>> >>> This may because the GCC is smart enough to generate optimized code in >>> either case, >>> >>> I think It could be different with a different optimization-level. >>> >>> I have tested this patch on three different architecture, I can not >>> find error still. >>> >>> Below is the assembly extract on x86-64: because GCC generate the same >>> code in either case, >>> >>> so I pasted only one copy here. >>> >>> >>> 0000000000000530 <drm_calc_timestamping_constants>: >>> 530: f3 0f 1e fa endbr64 >>> 534: e8 00 00 00 00 callq 539 >>> <drm_calc_timestamping_constants+0x9> >>> 539: 55 push %rbp >>> 53a: 48 89 e5 mov %rsp,%rbp >>> 53d: 41 57 push %r15 >>> 53f: 41 56 push %r14 >>> 541: 41 55 push %r13 >>> 543: 41 54 push %r12 >>> 545: 53 push %rbx >>> 546: 48 83 ec 18 sub $0x18,%rsp >>> 54a: 4c 8b 3f mov (%rdi),%r15 >>> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax >>> 554: 85 c0 test %eax,%eax >>> 556: 0f 84 ec 00 00 00 je 648 >>> <drm_calc_timestamping_constants+0x118> >>> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d >>> 563: 49 89 fc mov %rdi,%r12 >>> 566: 44 39 c0 cmp %r8d,%eax >>> 569: 0f 86 40 01 00 00 jbe 6af >>> <drm_calc_timestamping_constants+0x17f> >>> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d >>> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx >>> 57a: 48 89 f3 mov %rsi,%rbx >>> 57d: 45 85 f6 test %r14d,%r14d >>> 580: 0f 8e d5 00 00 00 jle 65b >>> <drm_calc_timestamping_constants+0x12b> >>> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax >>> 58a: 49 63 f6 movslq %r14d,%rsi >>> 58d: 31 d2 xor %edx,%edx >>> 58f: 48 89 c7 mov %rax,%rdi >>> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>> 599: 48 f7 f6 div %rsi >>> 59c: 31 d2 xor %edx,%edx >>> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) >>> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>> 5a6: 0f af c7 imul %edi,%eax >>> 5a9: 48 98 cltq >>> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>> 5b2: 48 f7 f6 div %rsi >>> 5b5: 41 89 c5 mov %eax,%r13d >>> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) >>> 5bc: 74 0a je 5c8 >>> <drm_calc_timestamping_constants+0x98> >>> 5be: 41 c1 ed 1f shr $0x1f,%r13d >>> 5c2: 41 01 c5 add %eax,%r13d >>> 5c5: 41 d1 fd sar %r13d >>> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax >>> 5cc: 48 89 de mov %rbx,%rsi >>> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi >>> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax >>> 5d6: 48 c1 e7 04 shl $0x4,%rdi >>> 5da: 48 01 cf add %rcx,%rdi >>> 5dd: 89 47 78 mov %eax,0x78(%rdi) >>> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi >>> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) >>> 5e8: e8 00 00 00 00 callq 5ed >>> <drm_calc_timestamping_constants+0xbd> >>> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx >>> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d >>> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>> 5ff: 4d 85 ff test %r15,%r15 >>> 602: 0f 84 87 00 00 00 je 68f >>> <drm_calc_timestamping_constants+0x15f> >>> 608: 49 8b 77 08 mov 0x8(%r15),%rsi >>> 60c: 52 push %rdx >>> 60d: 31 ff xor %edi,%edi >>> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>> 616: 50 push %rax >>> 617: 31 d2 xor %edx,%edx >>> 619: e8 00 00 00 00 callq 61e >>> <drm_calc_timestamping_constants+0xee> >>> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 >>> 627: 5f pop %rdi >>> 628: 41 59 pop %r9 >>> 62a: 8b 45 d0 mov -0x30(%rbp),%eax >>> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>> 634: 4c 89 fe mov %r15,%rsi >>> 637: 45 89 f1 mov %r14d,%r9d >>> 63a: 31 d2 xor %edx,%edx >>> 63c: 31 ff xor %edi,%edi >>> 63e: 50 push %rax >>> 63f: 41 55 push %r13 >>> 641: e8 00 00 00 00 callq 646 >>> <drm_calc_timestamping_constants+0x116> >>> 646: 59 pop %rcx >>> 647: 5e pop %rsi >>> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp >>> 64c: 5b pop %rbx >>> 64d: 41 5c pop %r12 >>> 64f: 41 5d pop %r13 >>> 651: 41 5e pop %r14 >>> 653: 41 5f pop %r15 >>> 655: 5d pop %rbp >>> 656: e9 00 00 00 00 jmpq 65b >>> <drm_calc_timestamping_constants+0x12b> >>> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx >>> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi >>> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) >>> 668: 45 31 ed xor %r13d,%r13d >>> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi >>> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) >>> 676: e8 00 00 00 00 callq 67b >>> <drm_calc_timestamping_constants+0x14b> >>> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) >>> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d >>> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx >>> 68a: e9 39 ff ff ff jmpq 5c8 >>> <drm_calc_timestamping_constants+0x98> >>> 68f: 52 push %rdx >>> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>> 697: 31 d2 xor %edx,%edx >>> 699: 31 f6 xor %esi,%esi >>> 69b: 50 push %rax >>> 69c: 31 ff xor %edi,%edi >>> 69e: e8 00 00 00 00 callq 6a3 >>> <drm_calc_timestamping_constants+0x173> >>> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>> 6a8: 58 pop %rax >>> 6a9: 5a pop %rdx >>> 6aa: e9 7b ff ff ff jmpq 62a >>> <drm_calc_timestamping_constants+0xfa> >>> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi >>> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 >>> 6b7: 4d 85 e4 test %r12,%r12 >>> 6ba: 74 25 je 6e1 >>> <drm_calc_timestamping_constants+0x1b1> >>> 6bc: e8 00 00 00 00 callq 6c1 >>> <drm_calc_timestamping_constants+0x191> >>> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>> 6c8: 4c 89 e2 mov %r12,%rdx >>> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi >>> 6d2: 48 89 c6 mov %rax,%rsi >>> 6d5: e8 00 00 00 00 callq 6da >>> <drm_calc_timestamping_constants+0x1aa> >>> 6da: 0f 0b ud2 >>> 6dc: e9 67 ff ff ff jmpq 648 >>> <drm_calc_timestamping_constants+0x118> >>> 6e1: 4c 8b 27 mov (%rdi),%r12 >>> 6e4: eb d6 jmp 6bc >>> <drm_calc_timestamping_constants+0x18c> >>> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) >>> 6ed: 00 00 00 >>> 6f0: 90 nop >>> 6f1: 90 nop >>> 6f2: 90 nop >>> 6f3: 90 nop >>> 6f4: 90 nop >>> 6f5: 90 nop >>> 6f6: 90 nop >>> 6f7: 90 nop >>> 6f8: 90 nop >>> 6f9: 90 nop >>> 6fa: 90 nop >>> 6fb: 90 nop >>> 6fc: 90 nop >>> 6fd: 90 nop >>> 6fe: 90 nop >>> 6ff: 90 nop >>> >>> >>>> David >>>> >>>> - >>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >>>> Registration No: 1397386 (Wales) >>>> -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 12:13 ` Jani Nikula @ 2023-05-22 12:53 ` Sui Jingfeng 2023-05-22 15:01 ` Jani Nikula 0 siblings, 1 reply; 17+ messages in thread From: Sui Jingfeng @ 2023-05-22 12:53 UTC (permalink / raw) To: Jani Nikula, David Laight, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn Hi, On 2023/5/22 20:13, Jani Nikula wrote: > On Mon, 22 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >> Hi, >> >> On 2023/5/22 19:29, Jani Nikula wrote: >>> On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >>>> On 2023/5/17 18:59, David Laight wrote: >>>>> From: 15330273260@189.cn >>>>>> Sent: 16 May 2023 18:30 >>>>>> >>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>>>> >>>>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >>>>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >>>>> Nope, u16 gets promoted to 'signed int' and the result of the >>>>> multiply is also signed. >>>> I believe that signed or unsigned is dependent on the declaration. >>>> >>>> I am talk about the math, while you are talking about compiler. >>>> >>>> I admit that u16 gets promoted to 'signed int' is true, but this is >>>> irrelevant, >>>> >>>> the point is how to understand the returned value. >>>> >>>> >>>> How does the compiler generate the code is one thing, how do we >>>> interpret the result is another >>>> >>>> How does the compiler generate the code is NOT determined by us, while >>>> how do we interpret the result is determined by us. >>>> >>>> >>>> I believe that using a u32 type to interpret the result(u16 * u16) is >>>> always true, it is true in the perspective of *math*. >>>> >>>> Integer promotions is the details of C program language. If the result >>>> of the multiply is signed, then there are risks that >>>> >>>> the result is negative, what's the benefit to present this risk to the >>>> programmer? >>>> >>>> What's the benefit to tell me(and others) that u16 * u16 yield a signed >>>> value? and can be negative? >>>> >>>> Using int type as the return type bring concerns to the programmer and >>>> the user of the function, >>>> >>>> even though this is not impossible in practice. >>> In general, do not use unsigned types in arithmethic to avoid negative >>> values, because most people will be tripped over by integer promotion >>> rules, and you'll get negative values anyway. >>> >>> I'll bet most people will be surprised to see what this prints: >>> >>> #include <stdio.h> >>> #include <stdint.h> >>> >>> int main(void) >>> { >>> uint16_t x = 0xffff; >>> uint16_t y = 0xffff; >>> uint64_t z = x * y; >>> >>> printf("0x%016lx\n", z); >>> printf("%ld\n", z); >> Here, please replace the "%ld\n" with the "%lu\n", then you will see the >> difference. >> >> you are casting the variable 'z' to signed value, "%d" is for printing >> signed value, and "%u" is for printing unsigned value. >> >> >> Your simple code explained exactly why you are still in confusion, > Am I? > > Take a look at the values, and explain the math. I meant the value itself is represent with 2's compliment, when you print a value with '%ld', then you will get the signed version, when you print a value with '%lu', then you will get the unsigned version. The result of a u16*u16 couldn't be negative in math. But when you using a '%ld' or '%d' to print a unsigned value, then is wrong. This is also the case which you shouldn't using a int type to store the result of u16*u16. because when I seen a int type, I will choose '%d' to print it, when I seen a unsigned int type, I will choose '%u' to print it. when using a int type as the return type, this could lead people to using '%d' to print such a value. Then, it generate the confusion as this little test program shows. > > BR, > Jani. > >> that is u16 * u16 can yield a negative value if you use the int as the >> return type. Because it overflowed. >> >>> printf("%d\n", x * y); >>> } >>> >>> And it's not that different from what you have below. Your patch doesn't >>> change anything, and doesn't make it any less confusing. >>> >>> BR, >>> Jani. >>> >>> >>>>>> Using a u32 is enough to store the result, but considering that the >>>>>> result will be casted to u64 soon after. We use a u64 type directly. >>>>>> So there no need to cast it to signed type and cast back then. >>>>> .... >>>>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>> ... >>>>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >>>>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >>>>> The (u64) cast is there to extend the value to 64bits, not >>>>> because the original type is signed. >>>> Sorry about my expression, I think my sentence did not mention anything >>>> about 'because the original type is signed'. >>>> >>>> In the contrary, my patch eliminated the concerns to the reviewer. It >>>> say that the results of the multiply can't be negative. >>>> >>>> My intent is to tell the compiler we want a unsigned return type, but >>>> GCC emit 'imul' instruction for the multiply...... >>>> >>>> I'm using u64 as the return type, because div_u64() function accept a >>>> u64 type value as its first argument. >>>> >>>>> The compiler will detect that the old code is a 32x32 multiply >>>>> where a 64bit result is needed, that may not be true for the >>>>> changed code (it would need to track back as far as the u16s). >>>> I don't believe my code could be wrong. >>>> >>>> when you use the word 'may', you are saying that it could be wrong after >>>> apply my patch. >>>> >>>> Then you have to find at least one test example to prove you point, in >>>> which case my codes generate wrong results. >>>> >>>> Again I don't believe you could find one. >>>> >>>>> It is not uncommon to force a 64bit result from a multiply >>>>> by making the constant 64bit. As in: >>>>> div_u64(frame_size * 1000000ULL, dotclock); >>>> In fact, After apply this patch, the ASM code generated is same with before. >>>> >>>> This may because the GCC is smart enough to generate optimized code in >>>> either case, >>>> >>>> I think It could be different with a different optimization-level. >>>> >>>> I have tested this patch on three different architecture, I can not >>>> find error still. >>>> >>>> Below is the assembly extract on x86-64: because GCC generate the same >>>> code in either case, >>>> >>>> so I pasted only one copy here. >>>> >>>> >>>> 0000000000000530 <drm_calc_timestamping_constants>: >>>> 530: f3 0f 1e fa endbr64 >>>> 534: e8 00 00 00 00 callq 539 >>>> <drm_calc_timestamping_constants+0x9> >>>> 539: 55 push %rbp >>>> 53a: 48 89 e5 mov %rsp,%rbp >>>> 53d: 41 57 push %r15 >>>> 53f: 41 56 push %r14 >>>> 541: 41 55 push %r13 >>>> 543: 41 54 push %r12 >>>> 545: 53 push %rbx >>>> 546: 48 83 ec 18 sub $0x18,%rsp >>>> 54a: 4c 8b 3f mov (%rdi),%r15 >>>> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax >>>> 554: 85 c0 test %eax,%eax >>>> 556: 0f 84 ec 00 00 00 je 648 >>>> <drm_calc_timestamping_constants+0x118> >>>> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d >>>> 563: 49 89 fc mov %rdi,%r12 >>>> 566: 44 39 c0 cmp %r8d,%eax >>>> 569: 0f 86 40 01 00 00 jbe 6af >>>> <drm_calc_timestamping_constants+0x17f> >>>> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d >>>> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx >>>> 57a: 48 89 f3 mov %rsi,%rbx >>>> 57d: 45 85 f6 test %r14d,%r14d >>>> 580: 0f 8e d5 00 00 00 jle 65b >>>> <drm_calc_timestamping_constants+0x12b> >>>> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax >>>> 58a: 49 63 f6 movslq %r14d,%rsi >>>> 58d: 31 d2 xor %edx,%edx >>>> 58f: 48 89 c7 mov %rax,%rdi >>>> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>>> 599: 48 f7 f6 div %rsi >>>> 59c: 31 d2 xor %edx,%edx >>>> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) >>>> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>>> 5a6: 0f af c7 imul %edi,%eax >>>> 5a9: 48 98 cltq >>>> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>>> 5b2: 48 f7 f6 div %rsi >>>> 5b5: 41 89 c5 mov %eax,%r13d >>>> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) >>>> 5bc: 74 0a je 5c8 >>>> <drm_calc_timestamping_constants+0x98> >>>> 5be: 41 c1 ed 1f shr $0x1f,%r13d >>>> 5c2: 41 01 c5 add %eax,%r13d >>>> 5c5: 41 d1 fd sar %r13d >>>> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax >>>> 5cc: 48 89 de mov %rbx,%rsi >>>> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi >>>> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax >>>> 5d6: 48 c1 e7 04 shl $0x4,%rdi >>>> 5da: 48 01 cf add %rcx,%rdi >>>> 5dd: 89 47 78 mov %eax,0x78(%rdi) >>>> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi >>>> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) >>>> 5e8: e8 00 00 00 00 callq 5ed >>>> <drm_calc_timestamping_constants+0xbd> >>>> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx >>>> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>>> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d >>>> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>> 5ff: 4d 85 ff test %r15,%r15 >>>> 602: 0f 84 87 00 00 00 je 68f >>>> <drm_calc_timestamping_constants+0x15f> >>>> 608: 49 8b 77 08 mov 0x8(%r15),%rsi >>>> 60c: 52 push %rdx >>>> 60d: 31 ff xor %edi,%edi >>>> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>> 616: 50 push %rax >>>> 617: 31 d2 xor %edx,%edx >>>> 619: e8 00 00 00 00 callq 61e >>>> <drm_calc_timestamping_constants+0xee> >>>> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 >>>> 627: 5f pop %rdi >>>> 628: 41 59 pop %r9 >>>> 62a: 8b 45 d0 mov -0x30(%rbp),%eax >>>> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>> 634: 4c 89 fe mov %r15,%rsi >>>> 637: 45 89 f1 mov %r14d,%r9d >>>> 63a: 31 d2 xor %edx,%edx >>>> 63c: 31 ff xor %edi,%edi >>>> 63e: 50 push %rax >>>> 63f: 41 55 push %r13 >>>> 641: e8 00 00 00 00 callq 646 >>>> <drm_calc_timestamping_constants+0x116> >>>> 646: 59 pop %rcx >>>> 647: 5e pop %rsi >>>> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp >>>> 64c: 5b pop %rbx >>>> 64d: 41 5c pop %r12 >>>> 64f: 41 5d pop %r13 >>>> 651: 41 5e pop %r14 >>>> 653: 41 5f pop %r15 >>>> 655: 5d pop %rbp >>>> 656: e9 00 00 00 00 jmpq 65b >>>> <drm_calc_timestamping_constants+0x12b> >>>> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx >>>> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi >>>> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) >>>> 668: 45 31 ed xor %r13d,%r13d >>>> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi >>>> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) >>>> 676: e8 00 00 00 00 callq 67b >>>> <drm_calc_timestamping_constants+0x14b> >>>> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) >>>> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d >>>> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx >>>> 68a: e9 39 ff ff ff jmpq 5c8 >>>> <drm_calc_timestamping_constants+0x98> >>>> 68f: 52 push %rdx >>>> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>> 697: 31 d2 xor %edx,%edx >>>> 699: 31 f6 xor %esi,%esi >>>> 69b: 50 push %rax >>>> 69c: 31 ff xor %edi,%edi >>>> 69e: e8 00 00 00 00 callq 6a3 >>>> <drm_calc_timestamping_constants+0x173> >>>> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>> 6a8: 58 pop %rax >>>> 6a9: 5a pop %rdx >>>> 6aa: e9 7b ff ff ff jmpq 62a >>>> <drm_calc_timestamping_constants+0xfa> >>>> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi >>>> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 >>>> 6b7: 4d 85 e4 test %r12,%r12 >>>> 6ba: 74 25 je 6e1 >>>> <drm_calc_timestamping_constants+0x1b1> >>>> 6bc: e8 00 00 00 00 callq 6c1 >>>> <drm_calc_timestamping_constants+0x191> >>>> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>> 6c8: 4c 89 e2 mov %r12,%rdx >>>> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi >>>> 6d2: 48 89 c6 mov %rax,%rsi >>>> 6d5: e8 00 00 00 00 callq 6da >>>> <drm_calc_timestamping_constants+0x1aa> >>>> 6da: 0f 0b ud2 >>>> 6dc: e9 67 ff ff ff jmpq 648 >>>> <drm_calc_timestamping_constants+0x118> >>>> 6e1: 4c 8b 27 mov (%rdi),%r12 >>>> 6e4: eb d6 jmp 6bc >>>> <drm_calc_timestamping_constants+0x18c> >>>> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) >>>> 6ed: 00 00 00 >>>> 6f0: 90 nop >>>> 6f1: 90 nop >>>> 6f2: 90 nop >>>> 6f3: 90 nop >>>> 6f4: 90 nop >>>> 6f5: 90 nop >>>> 6f6: 90 nop >>>> 6f7: 90 nop >>>> 6f8: 90 nop >>>> 6f9: 90 nop >>>> 6fa: 90 nop >>>> 6fb: 90 nop >>>> 6fc: 90 nop >>>> 6fd: 90 nop >>>> 6fe: 90 nop >>>> 6ff: 90 nop >>>> >>>> >>>>> David >>>>> >>>>> - >>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >>>>> Registration No: 1397386 (Wales) >>>>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 12:53 ` Sui Jingfeng @ 2023-05-22 15:01 ` Jani Nikula 2023-05-23 3:55 ` Sui Jingfeng 0 siblings, 1 reply; 17+ messages in thread From: Jani Nikula @ 2023-05-22 15:01 UTC (permalink / raw) To: Sui Jingfeng, David Laight, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn On Mon, 22 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: > Hi, > > On 2023/5/22 20:13, Jani Nikula wrote: >> On Mon, 22 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >>> Hi, >>> >>> On 2023/5/22 19:29, Jani Nikula wrote: >>>> On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >>>>> On 2023/5/17 18:59, David Laight wrote: >>>>>> From: 15330273260@189.cn >>>>>>> Sent: 16 May 2023 18:30 >>>>>>> >>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>>>>> >>>>>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >>>>>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >>>>>> Nope, u16 gets promoted to 'signed int' and the result of the >>>>>> multiply is also signed. >>>>> I believe that signed or unsigned is dependent on the declaration. >>>>> >>>>> I am talk about the math, while you are talking about compiler. >>>>> >>>>> I admit that u16 gets promoted to 'signed int' is true, but this is >>>>> irrelevant, >>>>> >>>>> the point is how to understand the returned value. >>>>> >>>>> >>>>> How does the compiler generate the code is one thing, how do we >>>>> interpret the result is another >>>>> >>>>> How does the compiler generate the code is NOT determined by us, while >>>>> how do we interpret the result is determined by us. >>>>> >>>>> >>>>> I believe that using a u32 type to interpret the result(u16 * u16) is >>>>> always true, it is true in the perspective of *math*. >>>>> >>>>> Integer promotions is the details of C program language. If the result >>>>> of the multiply is signed, then there are risks that >>>>> >>>>> the result is negative, what's the benefit to present this risk to the >>>>> programmer? >>>>> >>>>> What's the benefit to tell me(and others) that u16 * u16 yield a signed >>>>> value? and can be negative? >>>>> >>>>> Using int type as the return type bring concerns to the programmer and >>>>> the user of the function, >>>>> >>>>> even though this is not impossible in practice. >>>> In general, do not use unsigned types in arithmethic to avoid negative >>>> values, because most people will be tripped over by integer promotion >>>> rules, and you'll get negative values anyway. >>>> >>>> I'll bet most people will be surprised to see what this prints: >>>> >>>> #include <stdio.h> >>>> #include <stdint.h> >>>> >>>> int main(void) >>>> { >>>> uint16_t x = 0xffff; >>>> uint16_t y = 0xffff; >>>> uint64_t z = x * y; >>>> >>>> printf("0x%016lx\n", z); >>>> printf("%ld\n", z); >>> Here, please replace the "%ld\n" with the "%lu\n", then you will see the >>> difference. >>> >>> you are casting the variable 'z' to signed value, "%d" is for printing >>> signed value, and "%u" is for printing unsigned value. >>> >>> >>> Your simple code explained exactly why you are still in confusion, >> Am I? >> >> Take a look at the values, and explain the math. > > I meant the value itself is represent with 2's compliment, > > when you print a value with '%ld', then you will get the signed version, > > when you print a value with '%lu', then you will get the unsigned version. > > The result of a u16*u16 couldn't be negative in math. > > > But when you using a '%ld' or '%d' to print a unsigned value, then is wrong. > > This is also the case which you shouldn't using a int type to store the result of u16*u16. > > because when I seen a int type, I will choose '%d' to print it, > > when I seen a unsigned int type, I will choose '%u' to print it. > > when using a int type as the return type, this could lead people to using '%d' to print > > such a value. Then, it generate the confusion as this little test program shows. Using 0x%016lx and %lu results in 0xfffffffffffe0001 and 18446744073709420545, respectively. They are equal. They are indeed not negative. However 0xffff * 0xffff = 0xfffe0001. Or 4294836225 in decimal. No matter what the math says, this is what actually happens in C. I don't know what more I could possibly tell you. BR, Jani. > >> >> BR, >> Jani. >> >>> that is u16 * u16 can yield a negative value if you use the int as the >>> return type. Because it overflowed. >>> >>>> printf("%d\n", x * y); >>>> } >>>> >>>> And it's not that different from what you have below. Your patch doesn't >>>> change anything, and doesn't make it any less confusing. >>>> >>>> BR, >>>> Jani. >>>> >>>> >>>>>>> Using a u32 is enough to store the result, but considering that the >>>>>>> result will be casted to u64 soon after. We use a u64 type directly. >>>>>>> So there no need to cast it to signed type and cast back then. >>>>>> .... >>>>>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>>> ... >>>>>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >>>>>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >>>>>> The (u64) cast is there to extend the value to 64bits, not >>>>>> because the original type is signed. >>>>> Sorry about my expression, I think my sentence did not mention anything >>>>> about 'because the original type is signed'. >>>>> >>>>> In the contrary, my patch eliminated the concerns to the reviewer. It >>>>> say that the results of the multiply can't be negative. >>>>> >>>>> My intent is to tell the compiler we want a unsigned return type, but >>>>> GCC emit 'imul' instruction for the multiply...... >>>>> >>>>> I'm using u64 as the return type, because div_u64() function accept a >>>>> u64 type value as its first argument. >>>>> >>>>>> The compiler will detect that the old code is a 32x32 multiply >>>>>> where a 64bit result is needed, that may not be true for the >>>>>> changed code (it would need to track back as far as the u16s). >>>>> I don't believe my code could be wrong. >>>>> >>>>> when you use the word 'may', you are saying that it could be wrong after >>>>> apply my patch. >>>>> >>>>> Then you have to find at least one test example to prove you point, in >>>>> which case my codes generate wrong results. >>>>> >>>>> Again I don't believe you could find one. >>>>> >>>>>> It is not uncommon to force a 64bit result from a multiply >>>>>> by making the constant 64bit. As in: >>>>>> div_u64(frame_size * 1000000ULL, dotclock); >>>>> In fact, After apply this patch, the ASM code generated is same with before. >>>>> >>>>> This may because the GCC is smart enough to generate optimized code in >>>>> either case, >>>>> >>>>> I think It could be different with a different optimization-level. >>>>> >>>>> I have tested this patch on three different architecture, I can not >>>>> find error still. >>>>> >>>>> Below is the assembly extract on x86-64: because GCC generate the same >>>>> code in either case, >>>>> >>>>> so I pasted only one copy here. >>>>> >>>>> >>>>> 0000000000000530 <drm_calc_timestamping_constants>: >>>>> 530: f3 0f 1e fa endbr64 >>>>> 534: e8 00 00 00 00 callq 539 >>>>> <drm_calc_timestamping_constants+0x9> >>>>> 539: 55 push %rbp >>>>> 53a: 48 89 e5 mov %rsp,%rbp >>>>> 53d: 41 57 push %r15 >>>>> 53f: 41 56 push %r14 >>>>> 541: 41 55 push %r13 >>>>> 543: 41 54 push %r12 >>>>> 545: 53 push %rbx >>>>> 546: 48 83 ec 18 sub $0x18,%rsp >>>>> 54a: 4c 8b 3f mov (%rdi),%r15 >>>>> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax >>>>> 554: 85 c0 test %eax,%eax >>>>> 556: 0f 84 ec 00 00 00 je 648 >>>>> <drm_calc_timestamping_constants+0x118> >>>>> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d >>>>> 563: 49 89 fc mov %rdi,%r12 >>>>> 566: 44 39 c0 cmp %r8d,%eax >>>>> 569: 0f 86 40 01 00 00 jbe 6af >>>>> <drm_calc_timestamping_constants+0x17f> >>>>> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d >>>>> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx >>>>> 57a: 48 89 f3 mov %rsi,%rbx >>>>> 57d: 45 85 f6 test %r14d,%r14d >>>>> 580: 0f 8e d5 00 00 00 jle 65b >>>>> <drm_calc_timestamping_constants+0x12b> >>>>> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax >>>>> 58a: 49 63 f6 movslq %r14d,%rsi >>>>> 58d: 31 d2 xor %edx,%edx >>>>> 58f: 48 89 c7 mov %rax,%rdi >>>>> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>>>> 599: 48 f7 f6 div %rsi >>>>> 59c: 31 d2 xor %edx,%edx >>>>> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) >>>>> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>>>> 5a6: 0f af c7 imul %edi,%eax >>>>> 5a9: 48 98 cltq >>>>> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>>>> 5b2: 48 f7 f6 div %rsi >>>>> 5b5: 41 89 c5 mov %eax,%r13d >>>>> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) >>>>> 5bc: 74 0a je 5c8 >>>>> <drm_calc_timestamping_constants+0x98> >>>>> 5be: 41 c1 ed 1f shr $0x1f,%r13d >>>>> 5c2: 41 01 c5 add %eax,%r13d >>>>> 5c5: 41 d1 fd sar %r13d >>>>> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax >>>>> 5cc: 48 89 de mov %rbx,%rsi >>>>> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi >>>>> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax >>>>> 5d6: 48 c1 e7 04 shl $0x4,%rdi >>>>> 5da: 48 01 cf add %rcx,%rdi >>>>> 5dd: 89 47 78 mov %eax,0x78(%rdi) >>>>> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi >>>>> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) >>>>> 5e8: e8 00 00 00 00 callq 5ed >>>>> <drm_calc_timestamping_constants+0xbd> >>>>> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx >>>>> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>>>> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d >>>>> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>>> 5ff: 4d 85 ff test %r15,%r15 >>>>> 602: 0f 84 87 00 00 00 je 68f >>>>> <drm_calc_timestamping_constants+0x15f> >>>>> 608: 49 8b 77 08 mov 0x8(%r15),%rsi >>>>> 60c: 52 push %rdx >>>>> 60d: 31 ff xor %edi,%edi >>>>> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>> 616: 50 push %rax >>>>> 617: 31 d2 xor %edx,%edx >>>>> 619: e8 00 00 00 00 callq 61e >>>>> <drm_calc_timestamping_constants+0xee> >>>>> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>>> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 >>>>> 627: 5f pop %rdi >>>>> 628: 41 59 pop %r9 >>>>> 62a: 8b 45 d0 mov -0x30(%rbp),%eax >>>>> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>> 634: 4c 89 fe mov %r15,%rsi >>>>> 637: 45 89 f1 mov %r14d,%r9d >>>>> 63a: 31 d2 xor %edx,%edx >>>>> 63c: 31 ff xor %edi,%edi >>>>> 63e: 50 push %rax >>>>> 63f: 41 55 push %r13 >>>>> 641: e8 00 00 00 00 callq 646 >>>>> <drm_calc_timestamping_constants+0x116> >>>>> 646: 59 pop %rcx >>>>> 647: 5e pop %rsi >>>>> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp >>>>> 64c: 5b pop %rbx >>>>> 64d: 41 5c pop %r12 >>>>> 64f: 41 5d pop %r13 >>>>> 651: 41 5e pop %r14 >>>>> 653: 41 5f pop %r15 >>>>> 655: 5d pop %rbp >>>>> 656: e9 00 00 00 00 jmpq 65b >>>>> <drm_calc_timestamping_constants+0x12b> >>>>> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx >>>>> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi >>>>> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) >>>>> 668: 45 31 ed xor %r13d,%r13d >>>>> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi >>>>> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) >>>>> 676: e8 00 00 00 00 callq 67b >>>>> <drm_calc_timestamping_constants+0x14b> >>>>> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) >>>>> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d >>>>> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx >>>>> 68a: e9 39 ff ff ff jmpq 5c8 >>>>> <drm_calc_timestamping_constants+0x98> >>>>> 68f: 52 push %rdx >>>>> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>> 697: 31 d2 xor %edx,%edx >>>>> 699: 31 f6 xor %esi,%esi >>>>> 69b: 50 push %rax >>>>> 69c: 31 ff xor %edi,%edi >>>>> 69e: e8 00 00 00 00 callq 6a3 >>>>> <drm_calc_timestamping_constants+0x173> >>>>> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>>> 6a8: 58 pop %rax >>>>> 6a9: 5a pop %rdx >>>>> 6aa: e9 7b ff ff ff jmpq 62a >>>>> <drm_calc_timestamping_constants+0xfa> >>>>> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi >>>>> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 >>>>> 6b7: 4d 85 e4 test %r12,%r12 >>>>> 6ba: 74 25 je 6e1 >>>>> <drm_calc_timestamping_constants+0x1b1> >>>>> 6bc: e8 00 00 00 00 callq 6c1 >>>>> <drm_calc_timestamping_constants+0x191> >>>>> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>> 6c8: 4c 89 e2 mov %r12,%rdx >>>>> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi >>>>> 6d2: 48 89 c6 mov %rax,%rsi >>>>> 6d5: e8 00 00 00 00 callq 6da >>>>> <drm_calc_timestamping_constants+0x1aa> >>>>> 6da: 0f 0b ud2 >>>>> 6dc: e9 67 ff ff ff jmpq 648 >>>>> <drm_calc_timestamping_constants+0x118> >>>>> 6e1: 4c 8b 27 mov (%rdi),%r12 >>>>> 6e4: eb d6 jmp 6bc >>>>> <drm_calc_timestamping_constants+0x18c> >>>>> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) >>>>> 6ed: 00 00 00 >>>>> 6f0: 90 nop >>>>> 6f1: 90 nop >>>>> 6f2: 90 nop >>>>> 6f3: 90 nop >>>>> 6f4: 90 nop >>>>> 6f5: 90 nop >>>>> 6f6: 90 nop >>>>> 6f7: 90 nop >>>>> 6f8: 90 nop >>>>> 6f9: 90 nop >>>>> 6fa: 90 nop >>>>> 6fb: 90 nop >>>>> 6fc: 90 nop >>>>> 6fd: 90 nop >>>>> 6fe: 90 nop >>>>> 6ff: 90 nop >>>>> >>>>> >>>>>> David >>>>>> >>>>>> - >>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >>>>>> Registration No: 1397386 (Wales) >>>>>> -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 15:01 ` Jani Nikula @ 2023-05-23 3:55 ` Sui Jingfeng 0 siblings, 0 replies; 17+ messages in thread From: Sui Jingfeng @ 2023-05-23 3:55 UTC (permalink / raw) To: Jani Nikula, David Laight, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn Hi, On 2023/5/22 23:01, Jani Nikula wrote: > On Mon, 22 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >> Hi, >> >> On 2023/5/22 20:13, Jani Nikula wrote: >>> On Mon, 22 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >>>> Hi, >>>> >>>> On 2023/5/22 19:29, Jani Nikula wrote: >>>>> On Thu, 18 May 2023, Sui Jingfeng <15330273260@189.cn> wrote: >>>>>> On 2023/5/17 18:59, David Laight wrote: >>>>>>> From: 15330273260@189.cn >>>>>>>> Sent: 16 May 2023 18:30 >>>>>>>> >>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>>>>>> >>>>>>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >>>>>>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >>>>>>> Nope, u16 gets promoted to 'signed int' and the result of the >>>>>>> multiply is also signed. >>>>>> I believe that signed or unsigned is dependent on the declaration. >>>>>> >>>>>> I am talk about the math, while you are talking about compiler. >>>>>> >>>>>> I admit that u16 gets promoted to 'signed int' is true, but this is >>>>>> irrelevant, >>>>>> >>>>>> the point is how to understand the returned value. >>>>>> >>>>>> >>>>>> How does the compiler generate the code is one thing, how do we >>>>>> interpret the result is another >>>>>> >>>>>> How does the compiler generate the code is NOT determined by us, while >>>>>> how do we interpret the result is determined by us. >>>>>> >>>>>> >>>>>> I believe that using a u32 type to interpret the result(u16 * u16) is >>>>>> always true, it is true in the perspective of *math*. >>>>>> >>>>>> Integer promotions is the details of C program language. If the result >>>>>> of the multiply is signed, then there are risks that >>>>>> >>>>>> the result is negative, what's the benefit to present this risk to the >>>>>> programmer? >>>>>> >>>>>> What's the benefit to tell me(and others) that u16 * u16 yield a signed >>>>>> value? and can be negative? >>>>>> >>>>>> Using int type as the return type bring concerns to the programmer and >>>>>> the user of the function, >>>>>> >>>>>> even though this is not impossible in practice. >>>>> In general, do not use unsigned types in arithmethic to avoid negative >>>>> values, because most people will be tripped over by integer promotion >>>>> rules, and you'll get negative values anyway. >>>>> >>>>> I'll bet most people will be surprised to see what this prints: >>>>> >>>>> #include <stdio.h> >>>>> #include <stdint.h> >>>>> >>>>> int main(void) >>>>> { >>>>> uint16_t x = 0xffff; >>>>> uint16_t y = 0xffff; >>>>> uint64_t z = x * y; >>>>> >>>>> printf("0x%016lx\n", z); >>>>> printf("%ld\n", z); >>>> Here, please replace the "%ld\n" with the "%lu\n", then you will see the >>>> difference. >>>> >>>> you are casting the variable 'z' to signed value, "%d" is for printing >>>> signed value, and "%u" is for printing unsigned value. >>>> >>>> >>>> Your simple code explained exactly why you are still in confusion, >>> Am I? >>> >>> Take a look at the values, and explain the math. >> I meant the value itself is represent with 2's compliment, >> >> when you print a value with '%ld', then you will get the signed version, >> >> when you print a value with '%lu', then you will get the unsigned version. >> >> The result of a u16*u16 couldn't be negative in math. >> >> >> But when you using a '%ld' or '%d' to print a unsigned value, then is wrong. >> >> This is also the case which you shouldn't using a int type to store the result of u16*u16. >> >> because when I seen a int type, I will choose '%d' to print it, >> >> when I seen a unsigned int type, I will choose '%u' to print it. >> >> when using a int type as the return type, this could lead people to using '%d' to print >> >> such a value. Then, it generate the confusion as this little test program shows. > Using 0x%016lx and %lu results in 0xfffffffffffe0001 and > 18446744073709420545, respectively. They are equal. They are indeed not > negative. > > However 0xffff * 0xffff = 0xfffe0001. Or 4294836225 in decimal. > > No matter what the math says, this is what actually happens in C. > > I don't know what more I could possibly tell you. Sorry, I realized something after rethink about it. Can we first assign the value to u32 first, then expend it to 64 bit then? Extend it to 64 bit from 32 bit explicitly, this enforce zero extend instead of sign extend. > > BR, > Jani. > > >>> BR, >>> Jani. >>> >>>> that is u16 * u16 can yield a negative value if you use the int as the >>>> return type. Because it overflowed. >>>> >>>>> printf("%d\n", x * y); >>>>> } >>>>> >>>>> And it's not that different from what you have below. Your patch doesn't >>>>> change anything, and doesn't make it any less confusing. >>>>> >>>>> BR, >>>>> Jani. >>>>> >>>>> >>>>>>>> Using a u32 is enough to store the result, but considering that the >>>>>>>> result will be casted to u64 soon after. We use a u64 type directly. >>>>>>>> So there no need to cast it to signed type and cast back then. >>>>>>> .... >>>>>>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>>>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >>>>>>> ... >>>>>>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >>>>>>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >>>>>>> The (u64) cast is there to extend the value to 64bits, not >>>>>>> because the original type is signed. >>>>>> Sorry about my expression, I think my sentence did not mention anything >>>>>> about 'because the original type is signed'. >>>>>> >>>>>> In the contrary, my patch eliminated the concerns to the reviewer. It >>>>>> say that the results of the multiply can't be negative. >>>>>> >>>>>> My intent is to tell the compiler we want a unsigned return type, but >>>>>> GCC emit 'imul' instruction for the multiply...... >>>>>> >>>>>> I'm using u64 as the return type, because div_u64() function accept a >>>>>> u64 type value as its first argument. >>>>>> >>>>>>> The compiler will detect that the old code is a 32x32 multiply >>>>>>> where a 64bit result is needed, that may not be true for the >>>>>>> changed code (it would need to track back as far as the u16s). >>>>>> I don't believe my code could be wrong. >>>>>> >>>>>> when you use the word 'may', you are saying that it could be wrong after >>>>>> apply my patch. >>>>>> >>>>>> Then you have to find at least one test example to prove you point, in >>>>>> which case my codes generate wrong results. >>>>>> >>>>>> Again I don't believe you could find one. >>>>>> >>>>>>> It is not uncommon to force a 64bit result from a multiply >>>>>>> by making the constant 64bit. As in: >>>>>>> div_u64(frame_size * 1000000ULL, dotclock); >>>>>> In fact, After apply this patch, the ASM code generated is same with before. >>>>>> >>>>>> This may because the GCC is smart enough to generate optimized code in >>>>>> either case, >>>>>> >>>>>> I think It could be different with a different optimization-level. >>>>>> >>>>>> I have tested this patch on three different architecture, I can not >>>>>> find error still. >>>>>> >>>>>> Below is the assembly extract on x86-64: because GCC generate the same >>>>>> code in either case, >>>>>> >>>>>> so I pasted only one copy here. >>>>>> >>>>>> >>>>>> 0000000000000530 <drm_calc_timestamping_constants>: >>>>>> 530: f3 0f 1e fa endbr64 >>>>>> 534: e8 00 00 00 00 callq 539 >>>>>> <drm_calc_timestamping_constants+0x9> >>>>>> 539: 55 push %rbp >>>>>> 53a: 48 89 e5 mov %rsp,%rbp >>>>>> 53d: 41 57 push %r15 >>>>>> 53f: 41 56 push %r14 >>>>>> 541: 41 55 push %r13 >>>>>> 543: 41 54 push %r12 >>>>>> 545: 53 push %rbx >>>>>> 546: 48 83 ec 18 sub $0x18,%rsp >>>>>> 54a: 4c 8b 3f mov (%rdi),%r15 >>>>>> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax >>>>>> 554: 85 c0 test %eax,%eax >>>>>> 556: 0f 84 ec 00 00 00 je 648 >>>>>> <drm_calc_timestamping_constants+0x118> >>>>>> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d >>>>>> 563: 49 89 fc mov %rdi,%r12 >>>>>> 566: 44 39 c0 cmp %r8d,%eax >>>>>> 569: 0f 86 40 01 00 00 jbe 6af >>>>>> <drm_calc_timestamping_constants+0x17f> >>>>>> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d >>>>>> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx >>>>>> 57a: 48 89 f3 mov %rsi,%rbx >>>>>> 57d: 45 85 f6 test %r14d,%r14d >>>>>> 580: 0f 8e d5 00 00 00 jle 65b >>>>>> <drm_calc_timestamping_constants+0x12b> >>>>>> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax >>>>>> 58a: 49 63 f6 movslq %r14d,%rsi >>>>>> 58d: 31 d2 xor %edx,%edx >>>>>> 58f: 48 89 c7 mov %rax,%rdi >>>>>> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>>>>> 599: 48 f7 f6 div %rsi >>>>>> 59c: 31 d2 xor %edx,%edx >>>>>> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp) >>>>>> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>>>>> 5a6: 0f af c7 imul %edi,%eax >>>>>> 5a9: 48 98 cltq >>>>>> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax >>>>>> 5b2: 48 f7 f6 div %rsi >>>>>> 5b5: 41 89 c5 mov %eax,%r13d >>>>>> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx) >>>>>> 5bc: 74 0a je 5c8 >>>>>> <drm_calc_timestamping_constants+0x98> >>>>>> 5be: 41 c1 ed 1f shr $0x1f,%r13d >>>>>> 5c2: 41 01 c5 add %eax,%r13d >>>>>> 5c5: 41 d1 fd sar %r13d >>>>>> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax >>>>>> 5cc: 48 89 de mov %rbx,%rsi >>>>>> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi >>>>>> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax >>>>>> 5d6: 48 c1 e7 04 shl $0x4,%rdi >>>>>> 5da: 48 01 cf add %rcx,%rdi >>>>>> 5dd: 89 47 78 mov %eax,0x78(%rdi) >>>>>> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi >>>>>> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi) >>>>>> 5e8: e8 00 00 00 00 callq 5ed >>>>>> <drm_calc_timestamping_constants+0xbd> >>>>>> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx >>>>>> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax >>>>>> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d >>>>>> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>>>> 5ff: 4d 85 ff test %r15,%r15 >>>>>> 602: 0f 84 87 00 00 00 je 68f >>>>>> <drm_calc_timestamping_constants+0x15f> >>>>>> 608: 49 8b 77 08 mov 0x8(%r15),%rsi >>>>>> 60c: 52 push %rdx >>>>>> 60d: 31 ff xor %edi,%edi >>>>>> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>>> 616: 50 push %rax >>>>>> 617: 31 d2 xor %edx,%edx >>>>>> 619: e8 00 00 00 00 callq 61e >>>>>> <drm_calc_timestamping_constants+0xee> >>>>>> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>>>> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15 >>>>>> 627: 5f pop %rdi >>>>>> 628: 41 59 pop %r9 >>>>>> 62a: 8b 45 d0 mov -0x30(%rbp),%eax >>>>>> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>>> 634: 4c 89 fe mov %r15,%rsi >>>>>> 637: 45 89 f1 mov %r14d,%r9d >>>>>> 63a: 31 d2 xor %edx,%edx >>>>>> 63c: 31 ff xor %edi,%edi >>>>>> 63e: 50 push %rax >>>>>> 63f: 41 55 push %r13 >>>>>> 641: e8 00 00 00 00 callq 646 >>>>>> <drm_calc_timestamping_constants+0x116> >>>>>> 646: 59 pop %rcx >>>>>> 647: 5e pop %rsi >>>>>> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp >>>>>> 64c: 5b pop %rbx >>>>>> 64d: 41 5c pop %r12 >>>>>> 64f: 41 5d pop %r13 >>>>>> 651: 41 5e pop %r14 >>>>>> 653: 41 5f pop %r15 >>>>>> 655: 5d pop %rbp >>>>>> 656: e9 00 00 00 00 jmpq 65b >>>>>> <drm_calc_timestamping_constants+0x12b> >>>>>> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx >>>>>> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi >>>>>> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp) >>>>>> 668: 45 31 ed xor %r13d,%r13d >>>>>> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi >>>>>> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp) >>>>>> 676: e8 00 00 00 00 callq 67b >>>>>> <drm_calc_timestamping_constants+0x14b> >>>>>> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp) >>>>>> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d >>>>>> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx >>>>>> 68a: e9 39 ff ff ff jmpq 5c8 >>>>>> <drm_calc_timestamping_constants+0x98> >>>>>> 68f: 52 push %rdx >>>>>> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>>> 697: 31 d2 xor %edx,%edx >>>>>> 699: 31 f6 xor %esi,%esi >>>>>> 69b: 50 push %rax >>>>>> 69c: 31 ff xor %edi,%edi >>>>>> 69e: e8 00 00 00 00 callq 6a3 >>>>>> <drm_calc_timestamping_constants+0x173> >>>>>> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d >>>>>> 6a8: 58 pop %rax >>>>>> 6a9: 5a pop %rdx >>>>>> 6aa: e9 7b ff ff ff jmpq 62a >>>>>> <drm_calc_timestamping_constants+0xfa> >>>>>> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi >>>>>> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12 >>>>>> 6b7: 4d 85 e4 test %r12,%r12 >>>>>> 6ba: 74 25 je 6e1 >>>>>> <drm_calc_timestamping_constants+0x1b1> >>>>>> 6bc: e8 00 00 00 00 callq 6c1 >>>>>> <drm_calc_timestamping_constants+0x191> >>>>>> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx >>>>>> 6c8: 4c 89 e2 mov %r12,%rdx >>>>>> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi >>>>>> 6d2: 48 89 c6 mov %rax,%rsi >>>>>> 6d5: e8 00 00 00 00 callq 6da >>>>>> <drm_calc_timestamping_constants+0x1aa> >>>>>> 6da: 0f 0b ud2 >>>>>> 6dc: e9 67 ff ff ff jmpq 648 >>>>>> <drm_calc_timestamping_constants+0x118> >>>>>> 6e1: 4c 8b 27 mov (%rdi),%r12 >>>>>> 6e4: eb d6 jmp 6bc >>>>>> <drm_calc_timestamping_constants+0x18c> >>>>>> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) >>>>>> 6ed: 00 00 00 >>>>>> 6f0: 90 nop >>>>>> 6f1: 90 nop >>>>>> 6f2: 90 nop >>>>>> 6f3: 90 nop >>>>>> 6f4: 90 nop >>>>>> 6f5: 90 nop >>>>>> 6f6: 90 nop >>>>>> 6f7: 90 nop >>>>>> 6f8: 90 nop >>>>>> 6f9: 90 nop >>>>>> 6fa: 90 nop >>>>>> 6fb: 90 nop >>>>>> 6fc: 90 nop >>>>>> 6fd: 90 nop >>>>>> 6fe: 90 nop >>>>>> 6ff: 90 nop >>>>>> >>>>>> >>>>>>> David >>>>>>> >>>>>>> - >>>>>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >>>>>>> Registration No: 1397386 (Wales) >>>>>>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-22 11:29 ` Jani Nikula 2023-05-22 11:55 ` Sui Jingfeng @ 2023-05-23 4:26 ` Sui Jingfeng 2023-05-23 8:12 ` Sui Jingfeng 2023-05-23 8:50 ` David Laight 1 sibling, 2 replies; 17+ messages in thread From: Sui Jingfeng @ 2023-05-23 4:26 UTC (permalink / raw) To: Jani Nikula, David Laight, Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn Hi, On 2023/5/22 19:29, Jani Nikula wrote: > In general, do not use unsigned types in arithmethic to avoid negative > values, because most people will be tripped over by integer promotion > rules, and you'll get negative values anyway. Here I'm sure about this, but there are plenty unsigned types arithmetic in the kernel. take kmalloc_array() function as an example in /tools/virto/linux/kernel.h static inline void *kmalloc_array(unsigned n, size_t s, gfp_t gfp) { return kmalloc(n * s, gfp); } NOTE that *size_t* is an unsigned integral data type. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-23 4:26 ` Sui Jingfeng @ 2023-05-23 8:12 ` Sui Jingfeng 2023-05-23 8:50 ` David Laight 1 sibling, 0 replies; 17+ messages in thread From: Sui Jingfeng @ 2023-05-23 8:12 UTC (permalink / raw) To: Sui Jingfeng, Jani Nikula, David Laight, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn Hi, On 2023/5/23 12:26, Sui Jingfeng wrote: > Hi, > > On 2023/5/22 19:29, Jani Nikula wrote: >> In general, do not use unsigned types in arithmethic to avoid negative >> values, because most people will be tripped over by integer promotion >> rules, and you'll get negative values anyway. > > > Here I'm sure about this, > Here, I'm NOT sure about this > but there are plenty unsigned types arithmetic in the kernel. > > take kmalloc_array() function as an example in > /tools/virto/linux/kernel.h > > > static inline void *kmalloc_array(unsigned n, size_t s, gfp_t gfp) > { > return kmalloc(n * s, gfp); > } > > > NOTE that *size_t* is an unsigned integral data type. ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-23 4:26 ` Sui Jingfeng 2023-05-23 8:12 ` Sui Jingfeng @ 2023-05-23 8:50 ` David Laight 2023-05-23 9:55 ` Sui Jingfeng 1 sibling, 1 reply; 17+ messages in thread From: David Laight @ 2023-05-23 8:50 UTC (permalink / raw) To: '15330273260@189.cn', Jani Nikula, Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn From: 15330273260@189.cn <15330273260@189.cn> > Sent: 23 May 2023 05:27 > > On 2023/5/22 19:29, Jani Nikula wrote: > > In general, do not use unsigned types in arithmethic to avoid negative > > values, because most people will be tripped over by integer promotion > > rules, and you'll get negative values anyway. > > > Here I'm sure about this, > > but there are plenty unsigned types arithmetic in the kernel. The real problem is (attempted) arithmetic on types smaller than int. Regardless of whether they are signed or unsigned. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-23 8:50 ` David Laight @ 2023-05-23 9:55 ` Sui Jingfeng 0 siblings, 0 replies; 17+ messages in thread From: Sui Jingfeng @ 2023-05-23 9:55 UTC (permalink / raw) To: David Laight, Jani Nikula, Sui Jingfeng, Li Yi Cc: Thomas Zimmermann, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, loongson-kernel@lists.loongnix.cn Hi, On 2023/5/23 16:50, David Laight wrote: > From: 15330273260@189.cn <15330273260@189.cn> >> Sent: 23 May 2023 05:27 >> >> On 2023/5/22 19:29, Jani Nikula wrote: >>> In general, do not use unsigned types in arithmethic to avoid negative >>> values, because most people will be tripped over by integer promotion >>> rules, and you'll get negative values anyway. >> >> Here I'm not sure about this, >> >> but there are plenty unsigned types arithmetic in the kernel. > The real problem is (attempted) arithmetic on types smaller than int. > Regardless of whether they are signed or unsigned. It is about sign extend. Yes, you may be right. I might create a wrong patch. But this will not happen in practice, because in general case: mode->crtc_htotal < 0x8fff; mode->crtc_vtotal < 0x8fff; u16 gets promoted to 'signed int' not 'unsigned int'. Sorry :/ > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast @ 2023-05-16 16:59 Sui Jingfeng 2023-05-16 17:06 ` Thomas Zimmermann 0 siblings, 1 reply; 17+ messages in thread From: Sui Jingfeng @ 2023-05-16 16:59 UTC (permalink / raw) To: suijingfeng Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, dri-devel, linux-kernel Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. Using a u32 is enough to store the result, but considering that the result will be casted to u64 soon after. We use a u64 type directly. So there no need to cast it to signed type and cast back then. Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org --- drivers/gpu/drm/drm_vblank.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 877e2067534f..d99c404b181b 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -622,7 +622,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, /* Valid dotclock? */ if (dotclock > 0) { - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; /* * Convert scanline length in pixels and video @@ -630,7 +630,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, * in nanoseconds: */ linedur_ns = div_u64((u64) mode->crtc_htotal * 1000000, dotclock); - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); + framedur_ns = div_u64(frame_size * 1000000, dotclock); /* * Fields of interlaced scanout modes are only half a frame duration. -- 2.25.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-16 16:59 Sui Jingfeng @ 2023-05-16 17:06 ` Thomas Zimmermann 2023-05-16 17:13 ` Sui Jingfeng 0 siblings, 1 reply; 17+ messages in thread From: Thomas Zimmermann @ 2023-05-16 17:06 UTC (permalink / raw) To: Sui Jingfeng, suijingfeng Cc: Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter, dri-devel, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2098 bytes --] Am 16.05.23 um 18:59 schrieb Sui Jingfeng: > Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, > mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. > Using a u32 is enough to store the result, but considering that the > result will be casted to u64 soon after. We use a u64 type directly. > > So there no need to cast it to signed type and cast back then. > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_vblank.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 877e2067534f..d99c404b181b 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -622,7 +622,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, > > /* Valid dotclock? */ > if (dotclock > 0) { > - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; > + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; > > /* > * Convert scanline length in pixels and video > @@ -630,7 +630,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, > * in nanoseconds: > */ > linedur_ns = div_u64((u64) mode->crtc_htotal * 1000000, dotclock); > - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); > + framedur_ns = div_u64(frame_size * 1000000, dotclock); > > /* > * Fields of interlaced scanout modes are only half a frame duration. -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast 2023-05-16 17:06 ` Thomas Zimmermann @ 2023-05-16 17:13 ` Sui Jingfeng 0 siblings, 0 replies; 17+ messages in thread From: Sui Jingfeng @ 2023-05-16 17:13 UTC (permalink / raw) To: Thomas Zimmermann, Sui Jingfeng Cc: Maarten Lankhorst, Maxime Ripard, David Airlie, Daniel Vetter, dri-devel, linux-kernel Hi, On 2023/5/17 01:06, Thomas Zimmermann wrote: > > > Am 16.05.23 um 18:59 schrieb Sui Jingfeng: >> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type, >> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type. >> Using a u32 is enough to store the result, but considering that the >> result will be casted to u64 soon after. We use a u64 type directly. >> >> So there no need to cast it to signed type and cast back then. >> >> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: David Airlie <airlied@gmail.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: dri-devel@lists.freedesktop.org >> Cc: linux-kernel@vger.kernel.org > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Thanks a lot, thanks for the response time also. >> --- >> drivers/gpu/drm/drm_vblank.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c >> index 877e2067534f..d99c404b181b 100644 >> --- a/drivers/gpu/drm/drm_vblank.c >> +++ b/drivers/gpu/drm/drm_vblank.c >> @@ -622,7 +622,7 @@ void drm_calc_timestamping_constants(struct >> drm_crtc *crtc, >> /* Valid dotclock? */ >> if (dotclock > 0) { >> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal; >> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal; >> /* >> * Convert scanline length in pixels and video >> @@ -630,7 +630,7 @@ void drm_calc_timestamping_constants(struct >> drm_crtc *crtc, >> * in nanoseconds: >> */ >> linedur_ns = div_u64((u64) mode->crtc_htotal * 1000000, >> dotclock); >> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock); >> + framedur_ns = div_u64(frame_size * 1000000, dotclock); >> /* >> * Fields of interlaced scanout modes are only half a frame >> duration. > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-05-23 9:55 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-16 17:30 [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast Sui Jingfeng 2023-05-17 10:59 ` David Laight 2023-05-17 18:08 ` Sui Jingfeng 2023-05-22 11:29 ` Jani Nikula 2023-05-22 11:55 ` Sui Jingfeng 2023-05-22 12:07 ` David Laight 2023-05-22 12:13 ` Jani Nikula 2023-05-22 12:53 ` Sui Jingfeng 2023-05-22 15:01 ` Jani Nikula 2023-05-23 3:55 ` Sui Jingfeng 2023-05-23 4:26 ` Sui Jingfeng 2023-05-23 8:12 ` Sui Jingfeng 2023-05-23 8:50 ` David Laight 2023-05-23 9:55 ` Sui Jingfeng -- strict thread matches above, loose matches on Subject: below -- 2023-05-16 16:59 Sui Jingfeng 2023-05-16 17:06 ` Thomas Zimmermann 2023-05-16 17:13 ` Sui Jingfeng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox