On 4/12/24 10:53 AM, Kevin Brodsky wrote:
On 11/04/2024 14:18, Carsten Haitzler wrote:
[...]
@@ -854,7 +860,7 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd, req.request.type = req32.request.type; req.request.sequence = req32.request.sequence; req.request.signal = req32.request.signal; - err = drm_ioctl_kernel(file, drm_wait_vblank_ioctl, &req, DRM_UNLOCKED); + err = drm_wait_vblank_ioctl_internal(dev, &req, file_priv, true);
I'm not sure bypassing drm_ioctl_kernel() is 100% correct, as it does extra things like calling drm_ioctl_permit(). Couldn't we define our own callback in this file that would itself call drm_wait_vblank_ioctl_internal(..., true)?
but compat_drm_wait_vblank() should have already been through these checks... right as this is called deeper down from the existing kernel ioctl handling infra and we should be done with all of that.
I don't have much understanding of the DRM infrastructure, but I can see that drm_ioctl_permit() is only called from drm_ioctl_kernel(), so I don't think it will ever get called if we call drm_wait_vblank_ioctl_internal() directly here.
req32.reply.type = req.reply.type; req32.reply.sequence = req.reply.sequence;
[...]
/** * struct drm_pending_vblank_event - pending vblank event tracking */ @@ -52,6 +68,10 @@ struct drm_pending_vblank_event { * @sequence: frame event should be triggered at */ u64 sequence; + /** + * @compat: This is a compat32 event
Aren't we precisely using this for compat64 (arguably not so useful in compat32)?
This is a naming thing - drm thinks compat is compat32 by name. we're re-using that to fake a compat64 system. there is an argument where there should be a whole separate compat64 impl in addition to compat32 but that would make our patchset a lot larger (and thus harder to maintain). So where you see these things it means here the vblank event being stored was requested by the drm_ioctl32 (compat3) subsystem which is actually for us compat64... but in the event this were built for a old armv7+armv8 system it'd still work and it would literally be the real compat32 code... :)
Sure I get that. My point is that saying "compat32" in the comment is misleading because what this really means is that we come from compat. It could be compat32 or compat64, that makes no difference to how we handle it.
[...] @@ -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. :)