On 4/12/24 3:55 PM, Kevin Brodsky wrote:
On 12/04/2024 16:46, Carsten Haitzler wrote:
On 4/12/24 2:57 PM, Kevin Brodsky wrote:
On 12/04/2024 14:49, Carsten Haitzler wrote:
On 4/12/24 12:40 PM, Kevin Brodsky wrote:
On 12/04/2024 13:20, Carsten Haitzler wrote:
[...] > >> [...] >> @@ -1277,7 +1277,7 @@ struct drm_event { >>>> struct drm_event_vblank { >>>> struct drm_event base; >>>> - __u64 user_data; >>>> + uintptr_t user_data; >>> >>> __kernel_uintptr_t surely? >>> >>> It is surprising that you didn't run into trouble here, because >>> the >>> kernel and userspace views will diverge in purecap (the former >>> will >>> see >>> user_data as 64-bit and the latter as a capability). >> >> hmmm how did i mess this up (and yet vblank tests work)... its the >> tools/include/uapi/drm/drm.h that should be uintptr_t and this one >> __kernel_uintptr_t. > > We shouldn't use uintptr_t in tools/include either. uintptr_t is > 32-bit > on a 32-bit arch, but this is always at least 64-bit. > __kernel_uintptr_t > can be used in those headers as well thanks to [1].
user data for vblank events was originally defined as a ulong - thus uintptr_t is actually correct here. it's called signal in public drm headers - but it is the user_data. vblank is special in drm because of this. :)
But struct drm_event_vblank::user_data is __u64 in include/uapi/drm/drm.h. Are you saying that userspace doesn't use that? In which case, was the uapi header broken all along and should it be fixed upstream?
it's not used. libdrm ships its own drm headers and thus i've patched those separately in libdrm itself. they have this as a ulong. :) it's always been this way... broken by design™ :)
Amazing... but then does it even make sense to bother patching any header in tools/include?
That's why I didn't notice the mix-up - as its an unused appendix thing. Nothing catches the "you forgot to change all the duplicates". I did it for completeness and correctness sake so it SHOULD be there. It's just easier to miss or get wrong.
I'd rather not touch tools/include/uapi/drm then. Changes there are completely untested and apparently these headers are unused anyway. We do not generally sync the headers in tools/include/uapi anyway (e.g. linux/bpf.h) since we don't need to.
well have a look over v3. i can strip those out if everything is ok.