On 4/9/24 3:58 PM, Kevin Brodsky wrote:
On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Vblank is a bit different because it produces events that then can copy bpointers/capabilities back to userspace via a read on the DRM
s/bpointers/pointers/
device as opposed to the ioctl. The compat handler called the normal native/purecap one and this happened to work for compat but not for purecap. Make the shared "native" purecap handling work for both and share that infra properly to avoid copy and paste.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com
drivers/gpu/drm/drm_atomic_uapi.c | 3 +- drivers/gpu/drm/drm_internal.h | 4 +++ drivers/gpu/drm/drm_ioc32.c | 8 +++++- drivers/gpu/drm/drm_vblank.c | 46 +++++++++++++++++++++++-------- include/drm/drm_vblank.h | 23 ++++++++++++++++ include/uapi/drm/drm.h | 6 ++-- 6 files changed, 74 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 3e4668a20157..08474a1ea90e 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -940,7 +940,8 @@ int drm_atomic_get_property(struct drm_mode_object *obj, */ static struct drm_pending_vblank_event *create_vblank_event(
struct drm_crtc *crtc, uint64_t user_data)
struct drm_crtc *crtc,__kernel_uintptr_t user_data)We're not changing struct drm_mode_atomic in this patch so I suspect this belongs to the next one.
{ struct drm_pending_vblank_event *e = NULL; diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 8462b657c375..2d588df41cb5 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -23,6 +23,7 @@ #include <linux/kthread.h> +#include <drm/drm.h>
Doesn't seem to be needed.
I think this snuck through in one of my dead-end experiments and I didn't undo it. Good catch
#include <drm/drm_ioctl.h> #include <drm/drm_vblank.h> @@ -113,6 +114,9 @@ void drm_vblank_cancel_pending_works(struct drm_vblank_crtc *vblank); void drm_handle_vblank_works(struct drm_vblank_crtc *vblank); /* IOCTLS */ +int drm_wait_vblank_ioctl_internal(struct drm_device *dev, void *data,
struct drm_file *filp,bool compat);diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..d6a8c7250dda 100644 --- a/drivers/gpu/drm/drm_ioc32.c +++ b/drivers/gpu/drm/drm_ioc32.c @@ -33,6 +33,7 @@ #include <drm/drm_file.h> #include <drm/drm_print.h> +#include <drm/drm_drv.h> #include "drm_crtc_internal.h" #include "drm_internal.h" @@ -841,11 +842,16 @@ typedef union drm_wait_vblank32 { static int compat_drm_wait_vblank(struct file *file, unsigned int cmd, user_uintptr_t arg) {
- struct drm_file *file_priv = file->private_data;
- struct drm_device *dev = file_priv->minor->dev; drm_wait_vblank32_t __user *argp = (void __user *)arg; drm_wait_vblank32_t req32; union drm_wait_vblank req; int err;
- if (drm_dev_is_unplugged(dev))
return -ENODEV;- if (copy_from_user(&req32, argp, sizeof(req32))) return -EFAULT;
@@ -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.
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 eventAren'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... :)
*/- bool compat; /**
*/
- @event: Actual event which will be sent to userspace.
@@ -75,6 +95,9 @@ struct drm_pending_vblank_event { * @event.seq: Event payload for the MODE_QUEUEU_SEQUENCE IOCTL. */ struct drm_event_crtc_sequence seq;
struct drm_event_vblank32 vbl32;struct drm_event_crtc_sequence32 seq32;diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index de723566c5ae..5347b76005ee 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -494,7 +494,7 @@ enum drm_vblank_seq_type { struct drm_wait_vblank_request { enum drm_vblank_seq_type type; unsigned int sequence;
- unsigned long signal;
- __kernel_uintptr_t signal;
Just checking I'm getting this right, this is used to hold some user_data, and as a result needs to be expanded too, right?
yes - it's a userspace capability/ptr the kernel has to store and send back via a drm event later on that is read() from the drm device.
}; struct drm_wait_vblank_reply { @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */
- __u64 user_data; /* user data passed to event */
- __u64 user_data; /* user data passed to event */
The actual type change (to __kernel_uintptr_t) is in patch 3, it should be in this patch instead.
gah - something broke in my splitting of this into multiple patches. will re-jig. i think this was a manual change i had to do in the split. copy & paste problem.
}; #if defined(__cplusplus) @@ -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.
Kevin
__u32 tv_sec; __u32 tv_usec; __u32 sequence;