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/
yah - typo :)
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.
this belongs here (forgot to reply in previous mail). what i did mess up is the splitting of the patch - the changes to drm_mode_atomic should be in this patch
{ 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.
#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)?
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)?
*/- 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?
}; 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.
}; #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).
Kevin
__u32 tv_sec; __u32 tv_usec; __u32 sequence;
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org