From: Carsten Haitzler carsten.haitzler@arm.com
This series starts to enable purecap support for drm ioctls. This series enables all the libdrm tests (tested and working). You will also need the purecap libdrm port as well to complement this.
Carsten Haitzler (3): drm: Fix copy to/from user so that caps trasnport in the region drm: Fix purecap vblank handling drm: fix up purecap handling of all iotcls in libdrm test tools
drivers/gpu/drm/drm_atomic_uapi.c | 3 +- drivers/gpu/drm/drm_connector.c | 108 +++++++++++++++++++------ drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioc32.c | 1 + drivers/gpu/drm/drm_ioctl.c | 8 +- drivers/gpu/drm/drm_mode_config.c | 126 +++++++++++++++++++++--------- drivers/gpu/drm/drm_mode_object.c | 37 ++++++++- drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++---- drivers/gpu/drm/drm_property.c | 89 +++++++++++++++++---- drivers/gpu/drm/drm_vblank.c | 39 ++++++--- include/drm/drm_vblank.h | 23 ++++++ include/uapi/drm/drm.h | 6 +- include/uapi/drm/drm_mode.h | 30 +++---- tools/include/uapi/drm/drm.h | 6 +- 14 files changed, 430 insertions(+), 137 deletions(-)
From: Carsten Haitzler carsten.haitzler@arm.com
DRM makes copies of ioctl payload data to/from userspace and some of this can also have pointers to other memory (or capabilities for morello) and thus it needs to assume the worst and copy while retaining capabilities. This also requires making sure the stack buffer for small icotls (128 bytes or less) is aligned to 16 bytes. This possibly a bit of wasted space on the stack but north it for simplicity of code.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com --- drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 446458aca8e9..76e3616158f3 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -651,7 +651,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; }
- if (copy_to_user(buffer + ret, e->event, length)) { + if (copy_to_user_with_ptr(buffer + ret, e->event, length)) { if (ret == 0) ret = -EFAULT; goto put_back_event; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f2cda382f8ed..72e83d87936d 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -469,7 +469,7 @@ EXPORT_SYMBOL(drm_invalid_op); /* * Copy and IOCTL return string to user space */ -static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) +static int drm_copy_field(char __user *buf, size_t __user *buf_len, const char *value) { size_t len;
@@ -821,7 +821,7 @@ long drm_ioctl(struct file *filp, drm_ioctl_t *func; unsigned int nr = DRM_IOCTL_NR(cmd); int retcode = -EINVAL; - char stack_kdata[128]; + char stack_kdata[128] __attribute__((aligned(16))); char *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl; @@ -884,7 +884,7 @@ long drm_ioctl(struct file *filp, } }
- if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) { + if (copy_from_user_with_ptr(kdata, (void __user *)arg, in_size) != 0) { retcode = -EFAULT; goto err_i1; } @@ -893,7 +893,7 @@ long drm_ioctl(struct file *filp, memset(kdata + in_size, 0, ksize - in_size);
retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags); - if (copy_to_user((void __user *)arg, kdata, out_size) != 0) + if (copy_to_user_with_ptr((void __user *)arg, kdata, out_size) != 0) retcode = -EFAULT;
err_i1:
On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Typo in title: "trasnport"
Or maybe simply say "to preserve capabilities"?
DRM makes copies of ioctl payload data to/from userspace and some of this can also have pointers to other memory (or capabilities for morello) and thus it needs to assume the worst and copy while retaining capabilities. This also requires making sure the stack buffer for small icotls (128 bytes or less) is aligned to 16 bytes.
s/icotls/ioctls/
This possibly a bit of wasted space on the stack but north it for
"This is", s/north/worth/
simplicity of code.
We could use alignof(__kernel_uintptr_t) instead of hardcoding 16, but I don't mind either way. Could be worth considering a patch for upstream though, especially since it could break all architectures that don't like unaligned loads/stores (surely there are some on which DRM is used?).
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com
drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 446458aca8e9..76e3616158f3 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -651,7 +651,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; }
if (copy_to_user(buffer + ret, e->event, length)) {
if (copy_to_user_with_ptr(buffer + ret, e->event, length)) { if (ret == 0) ret = -EFAULT; goto put_back_event;diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f2cda382f8ed..72e83d87936d 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -469,7 +469,7 @@ EXPORT_SYMBOL(drm_invalid_op); /*
- Copy and IOCTL return string to user space
*/ -static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) +static int drm_copy_field(char __user *buf, size_t __user *buf_len, const char *value)
We don't want this right (as discussed in v1)?
Kevin
{ size_t len; @@ -821,7 +821,7 @@ long drm_ioctl(struct file *filp, drm_ioctl_t *func; unsigned int nr = DRM_IOCTL_NR(cmd); int retcode = -EINVAL;
- char stack_kdata[128];
- char stack_kdata[128] __attribute__((aligned(16))); char *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl;
@@ -884,7 +884,7 @@ long drm_ioctl(struct file *filp, } }
- if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) {
- if (copy_from_user_with_ptr(kdata, (void __user *)arg, in_size) != 0) { retcode = -EFAULT; goto err_i1; }
@@ -893,7 +893,7 @@ long drm_ioctl(struct file *filp, memset(kdata + in_size, 0, ksize - in_size); retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
- if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
- if (copy_to_user_with_ptr((void __user *)arg, kdata, out_size) != 0) retcode = -EFAULT;
err_i1:
On 4/15/24 9:03 AM, Kevin Brodsky wrote:
On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Typo in title: "trasnport"
Or maybe simply say "to preserve capabilities"?
DRM makes copies of ioctl payload data to/from userspace and some of this can also have pointers to other memory (or capabilities for morello) and thus it needs to assume the worst and copy while retaining capabilities. This also requires making sure the stack buffer for small icotls (128 bytes or less) is aligned to 16 bytes.
s/icotls/ioctls/
This possibly a bit of wasted space on the stack but north it for
"This is", s/north/worth/
fixed fixed.
simplicity of code.
We could use alignof(__kernel_uintptr_t) instead of hardcoding 16, but I don't mind either way. Could be worth considering a patch for upstream though, especially since it could break all architectures that don't like unaligned loads/stores (surely there are some on which DRM is used?).
yeah - might be nicer
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com
drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 446458aca8e9..76e3616158f3 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -651,7 +651,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; }
if (copy_to_user(buffer + ret, e->event, length)) {
if (copy_to_user_with_ptr(buffer + ret, e->event, length)) { if (ret == 0) ret = -EFAULT; goto put_back_event;diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f2cda382f8ed..72e83d87936d 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -469,7 +469,7 @@ EXPORT_SYMBOL(drm_invalid_op); /*
- Copy and IOCTL return string to user space
*/ -static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) +static int drm_copy_field(char __user *buf, size_t __user *buf_len, const char *value)
We don't want this right (as discussed in v1)?
WTH ... I swore I changed this in my updates...
Kevin
{ size_t len; @@ -821,7 +821,7 @@ long drm_ioctl(struct file *filp, drm_ioctl_t *func; unsigned int nr = DRM_IOCTL_NR(cmd); int retcode = -EINVAL;
- char stack_kdata[128];
- char stack_kdata[128] __attribute__((aligned(16))); char *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl;
@@ -884,7 +884,7 @@ long drm_ioctl(struct file *filp, } }
- if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) {
- if (copy_from_user_with_ptr(kdata, (void __user *)arg, in_size) != 0) { retcode = -EFAULT; goto err_i1; }
@@ -893,7 +893,7 @@ long drm_ioctl(struct file *filp, memset(kdata + in_size, 0, ksize - in_size); retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
- if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
- if (copy_to_user_with_ptr((void __user *)arg, kdata, out_size) != 0) retcode = -EFAULT;
err_i1:
From: Carsten Haitzler carsten.haitzler@arm.com
Vblank is a bit different because it produces events that then can copy pointers/capabilities back to userspace via a read on the DRM 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_ioc32.c | 1 + drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++--------- include/drm/drm_vblank.h | 23 ++++++++++++++++++ include/uapi/drm/drm.h | 6 ++--- include/uapi/drm/drm_mode.h | 2 +- tools/include/uapi/drm/drm.h | 2 +- 7 files changed, 59 insertions(+), 17 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) { struct drm_pending_vblank_event *e = NULL;
diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..a081ee3f4dac 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" diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 877e2067534f..fbd60acc5eda 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1031,14 +1031,20 @@ static void send_vblank_event(struct drm_device *dev, case DRM_EVENT_VBLANK: case DRM_EVENT_FLIP_COMPLETE: tv = ktime_to_timespec64(now); - e->event.vbl.sequence = seq; /* * e->event is a user space structure, with hardcoded unsigned * 32-bit seconds/microseconds. This is safe as we always use * monotonic timestamps since linux-4.15 */ - e->event.vbl.tv_sec = tv.tv_sec; - e->event.vbl.tv_usec = tv.tv_nsec / 1000; + if (e->compat) { + e->event.vbl32.sequence = seq; + e->event.vbl32.tv_sec = tv.tv_sec; + e->event.vbl32.tv_usec = tv.tv_nsec / 1000; + } else { + e->event.vbl.sequence = seq; + e->event.vbl.tv_sec = tv.tv_sec; + e->event.vbl.tv_usec = tv.tv_nsec / 1000; + } break; case DRM_EVENT_CRTC_SEQUENCE: if (seq) @@ -1659,10 +1665,13 @@ int drm_legacy_modeset_ctl_ioctl(struct drm_device *dev, void *data, static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe, u64 req_seq, union drm_wait_vblank *vblwait, - struct drm_file *file_priv) + struct drm_file *file_priv, + bool compat + ) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; struct drm_pending_vblank_event *e; + struct drm_crtc *crtc = NULL; ktime_t now; u64 seq; int ret; @@ -1675,12 +1684,19 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
e->pipe = pipe; e->event.base.type = DRM_EVENT_VBLANK; - e->event.base.length = sizeof(e->event.vbl); - e->event.vbl.user_data = vblwait->request.signal; - e->event.vbl.crtc_id = 0; - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); - + if (drm_core_check_feature(dev, DRIVER_MODESET)) + crtc = drm_crtc_from_index(dev, pipe); + e->compat = compat; + if (compat) { + e->event.base.length = sizeof(e->event.vbl32); + e->event.vbl32.user_data = vblwait->request.signal; + e->event.vbl32.crtc_id = 0; + if (crtc) + e->event.vbl32.crtc_id = crtc->base.id; + } else { + e->event.base.length = sizeof(e->event.vbl); + e->event.vbl.user_data = vblwait->request.signal; + e->event.vbl.crtc_id = 0; if (crtc) e->event.vbl.crtc_id = crtc->base.id; } @@ -1886,7 +1902,8 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, /* must hold on to the vblank ref until the event fires * drm_vblank_put will be called asynchronously */ - return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv); + return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, + file_priv, in_compat64_syscall()); }
if (req_seq != seq) { diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 7f3957943dd1..ccbcea75ba13 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -36,6 +36,22 @@ struct drm_device; struct drm_crtc; struct drm_vblank_work;
+struct drm_event_vblank32 { + struct drm_event base; + __u64 user_data; + __u32 tv_sec; + __u32 tv_usec; + __u32 sequence; + __u32 crtc_id; /* 0 on older kernels that do not support this */ +}; + +struct drm_event_crtc_sequence32 { + struct drm_event base; + __u64 user_data; + __s64 time_ns; + __u64 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 compat event + */ + 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; } event; };
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index de723566c5ae..c5cc98abbffe 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; };
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 */ + __kernel_uintptr_t user_data; /* user data passed to event */ };
#if defined(__cplusplus) @@ -1277,7 +1277,7 @@ struct drm_event {
struct drm_event_vblank { struct drm_event base; - __u64 user_data; + __kernel_uintptr_t user_data; __u32 tv_sec; __u32 tv_usec; __u32 sequence; diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved; - __u64 user_data; + __kernel_uintptr_t user_data; };
struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..82797c95efa6 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -1277,7 +1277,7 @@ struct drm_event {
struct drm_event_vblank { struct drm_event base; - __u64 user_data; + uintptr_t user_data; __u32 tv_sec; __u32 tv_usec; __u32 sequence;
On 12/04/2024 18:06, 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 pointers/capabilities back to userspace via a read on the DRM 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_ioc32.c | 1 + drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++--------- include/drm/drm_vblank.h | 23 ++++++++++++++++++ include/uapi/drm/drm.h | 6 ++--- include/uapi/drm/drm_mode.h | 2 +- tools/include/uapi/drm/drm.h | 2 +- 7 files changed, 59 insertions(+), 17 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){ struct drm_pending_vblank_event *e = NULL; diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..a081ee3f4dac 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>
Looks like a leftover, this file is not otherwise touched anymore.
#include "drm_crtc_internal.h" #include "drm_internal.h"
[...]
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved;
- __u64 user_data;
- __kernel_uintptr_t user_data;
I think this change requires compat handling too (in drm_mode_atomic_ioctl()). In fact it looks a lot like the *_ptr members should also be changed like the other drm_mode_* structs in patch 3. Or maybe the intention is not to change it at all considering user_data is reverted to __u64 in patch 3? In which case it doesn't really make sense to change create_vblank_event() either.
}; struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..82797c95efa6 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -1277,7 +1277,7 @@ struct drm_event { struct drm_event_vblank { struct drm_event base;
- __u64 user_data;
- uintptr_t user_data;
As discussed in v1, it doesn't look like this header ever gets used in practice, so I'd rather leave it alone.
Kevin
__u32 tv_sec; __u32 tv_usec; __u32 sequence;
On 4/15/24 9:06 AM, Kevin Brodsky wrote:
On 12/04/2024 18:06, 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 pointers/capabilities back to userspace via a read on the DRM 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_ioc32.c | 1 + drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++--------- include/drm/drm_vblank.h | 23 ++++++++++++++++++ include/uapi/drm/drm.h | 6 ++--- include/uapi/drm/drm_mode.h | 2 +- tools/include/uapi/drm/drm.h | 2 +- 7 files changed, 59 insertions(+), 17 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)diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..a081ee3f4dac 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>
Looks like a leftover, this file is not otherwise touched anymore.
Ad before.. I swore I fixed this too... WTH?
#include "drm_crtc_internal.h" #include "drm_internal.h"
[...]
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved;
- __u64 user_data;
- __kernel_uintptr_t user_data;
I think this change requires compat handling too (in drm_mode_atomic_ioctl()). In fact it looks a lot like the *_ptr members should also be changed like the other drm_mode_* structs in patch 3. Or maybe the intention is not to change it at all considering user_data is reverted to __u64 in patch 3? In which case it doesn't really make sense to change create_vblank_event() either.
I'm now baffled. This is bizarre.
git diff shows nothing. empty. include/uapi/drm/drm_mode.h has:
struct drm_mode_atomic { __u32 flags; __u32 count_objs; __u64 objs_ptr; __u64 count_props_ptr; __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved; __u64 user_data; };
but git log -U shows:
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved; - __u64 user_data; + __kernel_uintptr_t user_data; };
struct drm_format_modifier_blob {
WTH? how... eh?
}; struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..82797c95efa6 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -1277,7 +1277,7 @@ struct drm_event { struct drm_event_vblank { struct drm_event base;
- __u64 user_data;
- uintptr_t user_data;
As discussed in v1, it doesn't look like this header ever gets used in practice, so I'd rather leave it alone.
yeah - it's on my "will get to" to strip out in the patch set.
Kevin
__u32 tv_sec; __u32 tv_usec; __u32 sequence;
On 4/15/24 9:06 AM, Kevin Brodsky wrote:
On 12/04/2024 18:06, 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 pointers/capabilities back to userspace via a read on the DRM 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_ioc32.c | 1 + drivers/gpu/drm/drm_vblank.c | 39 ++++++++++++++++++++++--------- include/drm/drm_vblank.h | 23 ++++++++++++++++++ include/uapi/drm/drm.h | 6 ++--- include/uapi/drm/drm_mode.h | 2 +- tools/include/uapi/drm/drm.h | 2 +- 7 files changed, 59 insertions(+), 17 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)diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..a081ee3f4dac 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>
Looks like a leftover, this file is not otherwise touched anymore.
#include "drm_crtc_internal.h" #include "drm_internal.h"
[...]
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved;
- __u64 user_data;
- __kernel_uintptr_t user_data;
I think this change requires compat handling too (in drm_mode_atomic_ioctl()). In fact it looks a lot like the *_ptr members should also be changed like the other drm_mode_* structs in patch 3. Or maybe the intention is not to change it at all considering user_data is reverted to __u64 in patch 3? In which case it doesn't really make sense to change create_vblank_event() either.
Ahh never mind - I undid that in patch 2 - well did it in patch 1, undid in patch 2... but yeah - this needs doing. I seem to have only partly begun this but nothing used/tested it so didn't finish it (didn't notice it wasn't finished).
}; struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..82797c95efa6 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -1277,7 +1277,7 @@ struct drm_event { struct drm_event_vblank { struct drm_event base;
- __u64 user_data;
- uintptr_t user_data;
As discussed in v1, it doesn't look like this header ever gets used in practice, so I'd rather leave it alone.
Kevin
__u32 tv_sec; __u32 tv_usec; __u32 sequence;
On 15/04/2024 12:31, Carsten Haitzler wrote:
On 4/15/24 9:06 AM, Kevin Brodsky wrote:
On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved; - __u64 user_data; + __kernel_uintptr_t user_data;
I think this change requires compat handling too (in drm_mode_atomic_ioctl()). In fact it looks a lot like the *_ptr members should also be changed like the other drm_mode_* structs in patch 3. Or maybe the intention is not to change it at all considering user_data is reverted to __u64 in patch 3? In which case it doesn't really make sense to change create_vblank_event() either.
Ahh never mind - I undid that in patch 2 - well did it in patch 1, undid in patch 2... but yeah - this needs doing. I seem to have only partly begun this but nothing used/tested it so didn't finish it (didn't notice it wasn't finished).
Yeah no worries, it's understood that this series is not comprehensive.
Kevin
On 4/15/24 12:10 PM, Kevin Brodsky wrote:
On 15/04/2024 12:31, Carsten Haitzler wrote:
On 4/15/24 9:06 AM, Kevin Brodsky wrote:
On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..3a426e7ed775 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved; - __u64 user_data; + __kernel_uintptr_t user_data;
I think this change requires compat handling too (in drm_mode_atomic_ioctl()). In fact it looks a lot like the *_ptr members should also be changed like the other drm_mode_* structs in patch 3. Or maybe the intention is not to change it at all considering user_data is reverted to __u64 in patch 3? In which case it doesn't really make sense to change create_vblank_event() either.
Ahh never mind - I undid that in patch 2 - well did it in patch 1, undid in patch 2... but yeah - this needs doing. I seem to have only partly begun this but nothing used/tested it so didn't finish it (didn't notice it wasn't finished).
Yeah no worries, it's understood that this series is not comprehensive.
oh ha - i see why...
commit 88f6b9b33e0fb1c0d7af2c7b1f5a1c19e2bc8d4c Author: Kevin Brodsky kevin.brodsky@arm.com Date: Thu Feb 9 15:43:44 2023 +0000
drm: Explicitly create user pointers
that's why i didn't see it fail and thus didn't dig in more... as it was "partially fixed" and by luck happened to not fall over... yeah - this needs fixing properly as per your commit log:
Various DRM uapi structs represent user pointers as __u64, which is not PCuABI-friendly. For now let's create a user pointer manually using uaddr_to_user_ptr(), but eventually the structs should be modified to represent the user pointer fields as __kernel_uintptr_t instead, allowing userspace to provide a whole user pointer instead of just an address.
From: Carsten Haitzler carsten.haitzler@arm.com
The modetest, proptest vbltest tools from drm all work with this as the ioctls they use with this patch now handle purecap ABI. The general design is for these ioctls to have an import + export code "blob" that converts to to/from in-kernel-data based on if it's a compat or purecap caller and embedded compat structs together with the ioctl handling. This is less invasive and less copy + paste based than alternative solutions, thus making it more maintainable as a patchset. There is no need to have special different ioctl number handling as drm already handles this in generic infrastructure to make sure the compat and native purecap ioctl numbers map to the same ioctl func unless its explicitly in the 32bit (compat) handling.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com --- drivers/gpu/drm/drm_connector.c | 108 +++++++++++++++++++------ drivers/gpu/drm/drm_mode_config.c | 126 +++++++++++++++++++++--------- drivers/gpu/drm/drm_mode_object.c | 37 ++++++++- drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++---- drivers/gpu/drm/drm_property.c | 89 +++++++++++++++++---- include/uapi/drm/drm_mode.h | 32 ++++---- tools/include/uapi/drm/drm.h | 4 +- 7 files changed, 368 insertions(+), 117 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index bc052b147172..e5838057b833 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2883,6 +2883,26 @@ drm_mode_expose_to_userspace(const struct drm_display_mode *mode, int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_connector32 { + __u64 encoders_ptr; + __u64 modes_ptr; + __u64 props_ptr; + __u64 prop_values_ptr; + __u32 count_modes; + __u32 count_props; + __u32 count_encoders; + + __u32 encoder_id; + __u32 connector_id; + __u32 connector_type; + __u32 connector_type_id; + __u32 connection; + __u32 mm_width; + __u32 mm_height; + __u32 subpixel; + __u32 pad; + }; + struct drm_mode_get_connector32 *out_resp32 = data; struct drm_mode_get_connector *out_resp = data; struct drm_connector *connector; struct drm_encoder *encoder; @@ -2895,21 +2915,46 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr; bool is_current_master; + __u32 connector_id; + __u32 count_encoders; + __u32 count_modes; + __u32 count_props; + uint32_t __user *prop_ptr; + uint64_t __user *prop_values; + + if (in_compat64_syscall()) { + connector_id = out_resp32->connector_id; + count_encoders = out_resp32->count_encoders; + count_modes = out_resp32->count_modes; + count_props = out_resp32->count_props; + encoder_ptr = compat_ptr(out_resp32->encoders_ptr); + mode_ptr = compat_ptr(out_resp32->modes_ptr); + prop_ptr = compat_ptr(out_resp32->props_ptr); + prop_values = compat_ptr(out_resp32->prop_values_ptr); + } else { + connector_id = out_resp->connector_id; + count_encoders = out_resp->count_encoders; + count_modes = out_resp->count_modes; + count_props = out_resp->count_props; + encoder_ptr = (uint32_t __user *)(out_resp->encoders_ptr); + mode_ptr = (struct drm_mode_modeinfo __user *)(out_resp->modes_ptr); + prop_ptr = (uint32_t __user *)(out_resp->props_ptr); + prop_values = (uint64_t __user *)(out_resp->prop_values_ptr); + }
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
- connector = drm_connector_lookup(dev, file_priv, out_resp->connector_id); + connector = drm_connector_lookup(dev, file_priv, connector_id); if (!connector) return -ENOENT;
encoders_count = hweight32(connector->possible_encoders);
- if ((out_resp->count_encoders >= encoders_count) && encoders_count) { + if ((count_encoders >= encoders_count) && encoders_count) { copied = 0; - encoder_ptr = uaddr_to_user_ptr(out_resp->encoders_ptr);
drm_connector_for_each_possible_encoder(connector, encoder) { if (put_user(encoder->base.id, encoder_ptr + copied)) { @@ -2919,16 +2964,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied++; } } - out_resp->count_encoders = encoders_count; - - out_resp->connector_id = connector->base.id; - out_resp->connector_type = connector->connector_type; - out_resp->connector_type_id = connector->connector_type_id; - is_current_master = drm_is_current_master(file_priv);
mutex_lock(&dev->mode_config.mutex); - if (out_resp->count_modes == 0) { + if (count_modes == 0) { if (is_current_master) connector->funcs->fill_modes(connector, dev->mode_config.max_width, @@ -2938,11 +2977,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, connector->base.id, connector->name); }
- out_resp->mm_width = connector->display_info.width_mm; - out_resp->mm_height = connector->display_info.height_mm; - out_resp->subpixel = connector->display_info.subpixel_order; - out_resp->connection = connector->status; - /* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head) { WARN_ON(mode->expose_to_userspace); @@ -2958,9 +2992,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ - if ((out_resp->count_modes >= mode_count) && mode_count) { + if ((count_modes >= mode_count) && mode_count) { copied = 0; - mode_ptr = uaddr_to_user_ptr(out_resp->modes_ptr); list_for_each_entry(mode, &connector->modes, head) { if (!mode->expose_to_userspace) continue; @@ -2998,25 +3031,50 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, mode->expose_to_userspace = false; }
- out_resp->count_modes = mode_count; mutex_unlock(&dev->mode_config.mutex);
drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); encoder = drm_connector_get_encoder(connector); - if (encoder) - out_resp->encoder_id = encoder->base.id; - else - out_resp->encoder_id = 0;
/* Only grab properties after probing, to make sure EDID and other * properties reflect the latest status. */ ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, - uaddr_to_user_ptr(out_resp->props_ptr), - uaddr_to_user_ptr(out_resp->prop_values_ptr), - &out_resp->count_props); + prop_ptr, prop_values, + &count_props); drm_modeset_unlock(&dev->mode_config.connection_mutex);
+ if (in_compat64_syscall()) { + out_resp32->count_encoders = encoders_count; + out_resp32->count_modes = mode_count; + out_resp32->count_props = count_props; + out_resp32->connector_id = connector->base.id; + out_resp32->connector_type = connector->connector_type; + out_resp32->connector_type_id = connector->connector_type_id; + if (encoder) + out_resp32->encoder_id = encoder->base.id; + else + out_resp32->encoder_id = 0; + out_resp32->mm_width = connector->display_info.width_mm; + out_resp32->mm_height = connector->display_info.height_mm; + out_resp32->subpixel = connector->display_info.subpixel_order; + out_resp32->connection = connector->status; + } else { + out_resp->count_encoders = encoders_count; + out_resp->count_modes = mode_count; + out_resp->count_props = count_props; + out_resp->connector_id = connector->base.id; + out_resp->connector_type = connector->connector_type; + out_resp->connector_type_id = connector->connector_type_id; + if (encoder) + out_resp->encoder_id = encoder->base.id; + else + out_resp->encoder_id = 0; + out_resp->mm_width = connector->display_info.width_mm; + out_resp->mm_height = connector->display_info.height_mm; + out_resp->subpixel = connector->display_info.subpixel_order; + out_resp->connection = connector->status; + } out: drm_connector_put(connector);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 8525ef851540..80d195328b77 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -92,11 +92,30 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data; + struct drm_mode_card_res32 { + __u64 fb_id_ptr; + __u64 crtc_id_ptr; + __u64 connector_id_ptr; + __u64 encoder_id_ptr; + __u32 count_fbs; + __u32 count_crtcs; + __u32 count_connectors; + __u32 count_encoders; + __u32 min_width; + __u32 max_width; + __u32 min_height; + __u32 max_height; + }; + struct drm_mode_card_res32 *card_res32 = data; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; struct drm_encoder *encoder; - int count, ret = 0; + int count_fb = 0, count_fbs; + int count_crtc = 0, count_crtcs; + int count_encoder = 0, count_encoders; + int count_connector = 0, count_connectors; + int ret = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; uint32_t __user *connector_id; @@ -107,49 +126,54 @@ int drm_mode_getresources(struct drm_device *dev, void *data, return -EOPNOTSUPP;
mutex_lock(&file_priv->fbs_lock); - count = 0; - fb_id = u64_to_user_ptr(card_res->fb_id_ptr); + if (in_compat64_syscall()) { + fb_id = compat_ptr(card_res32->fb_id_ptr); + crtc_id = compat_ptr(card_res32->crtc_id_ptr); + encoder_id = compat_ptr(card_res32->encoder_id_ptr); + connector_id = compat_ptr(card_res32->connector_id_ptr); + + count_fbs = card_res32->count_fbs; + count_crtcs = card_res32->count_crtcs; + count_encoders = card_res32->count_encoders; + count_connectors = card_res32->count_connectors; + } else { + fb_id = (uint32_t __user *)(card_res->fb_id_ptr); + crtc_id = (uint32_t __user *)(card_res->crtc_id_ptr); + encoder_id = (uint32_t __user *)(card_res->encoder_id_ptr); + connector_id = (uint32_t __user *)(card_res->connector_id_ptr); + + count_fbs = card_res->count_fbs; + count_crtcs = card_res->count_crtcs; + count_encoders = card_res->count_encoders; + count_connectors = card_res->count_connectors; + } list_for_each_entry(fb, &file_priv->fbs, filp_head) { - if (count < card_res->count_fbs && - put_user(fb->base.id, fb_id + count)) { - mutex_unlock(&file_priv->fbs_lock); - return -EFAULT; + if (count_fb < count_fbs && + put_user(fb->base.id, fb_id + count_fb)) { + ret = -EFAULT; + goto error; } - count++; + count_fb++; } - card_res->count_fbs = count; - mutex_unlock(&file_priv->fbs_lock); - - card_res->max_height = dev->mode_config.max_height; - card_res->min_height = dev->mode_config.min_height; - card_res->max_width = dev->mode_config.max_width; - card_res->min_width = dev->mode_config.min_width; - - count = 0; - crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr); drm_for_each_crtc(crtc, dev) { if (drm_lease_held(file_priv, crtc->base.id)) { - if (count < card_res->count_crtcs && - put_user(crtc->base.id, crtc_id + count)) - return -EFAULT; - count++; + if (count_crtc < count_crtcs && + put_user(crtc->base.id, crtc_id + count_crtc)) { + ret = -EFAULT; + goto error; + } + count_crtc++; } } - card_res->count_crtcs = count; - - count = 0; - encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr); drm_for_each_encoder(encoder, dev) { - if (count < card_res->count_encoders && - put_user(encoder->base.id, encoder_id + count)) - return -EFAULT; - count++; + if (count_encoder < count_encoders && + put_user(encoder->base.id, encoder_id + count_encoder)) { + ret = -EFAULT; + goto error; + } + count_encoder++; } - card_res->count_encoders = count; - drm_connector_list_iter_begin(dev, &conn_iter); - count = 0; - connector_id = u64_to_user_ptr(card_res->connector_id_ptr); drm_for_each_connector_iter(connector, &conn_iter) { /* only expose writeback connectors if userspace understands them */ if (!file_priv->writeback_connectors && @@ -157,17 +181,41 @@ int drm_mode_getresources(struct drm_device *dev, void *data, continue;
if (drm_lease_held(file_priv, connector->base.id)) { - if (count < card_res->count_connectors && - put_user(connector->base.id, connector_id + count)) { + if (count_connector < count_connectors && + put_user(connector->base.id, connector_id + count_connector)) { drm_connector_list_iter_end(&conn_iter); - return -EFAULT; + ret = -EFAULT; + goto error; } - count++; + count_connector++; } } - card_res->count_connectors = count; drm_connector_list_iter_end(&conn_iter);
+ if (in_compat64_syscall()) { + card_res32->count_fbs = count_fb; + card_res32->count_crtcs = count_crtc; + card_res32->count_encoders = count_encoder; + card_res32->count_connectors = count_connector; + + card_res32->max_height = dev->mode_config.max_height; + card_res32->min_height = dev->mode_config.min_height; + card_res32->max_width = dev->mode_config.max_width; + card_res32->min_width = dev->mode_config.min_width; + } else { + card_res->count_fbs = count_fb; + card_res->count_crtcs = count_crtc; + card_res->count_encoders = count_encoder; + card_res->count_connectors = count_connector; + + card_res->max_height = dev->mode_config.max_height; + card_res->min_height = dev->mode_config.min_height; + card_res->max_width = dev->mode_config.max_width; + card_res->min_width = dev->mode_config.min_width; + } + +error: + mutex_unlock(&file_priv->fbs_lock); return ret; }
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 29df6cbd9c99..10b1c74214e0 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -435,17 +435,41 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic, int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_obj_get_properties32 { + __u64 props_ptr; + __u64 prop_values_ptr; + __u32 count_props; + __u32 obj_id; + __u32 obj_type; + }; + struct drm_mode_obj_get_properties32 *arg32 = data; struct drm_mode_obj_get_properties *arg = data; struct drm_mode_object *obj; struct drm_modeset_acquire_ctx ctx; int ret = 0; + __u32 obj_id, obj_type, count_props; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); + if (in_compat64_syscall()) { + count_props = arg32->count_props; + obj_id = arg32->obj_id; + obj_type = arg32->obj_type; + props_ptr = compat_ptr(arg32->props_ptr); + prop_values_ptr = compat_ptr(arg32->prop_values_ptr); + } else { + count_props = arg->count_props; + obj_id = arg->obj_id; + obj_type = arg->obj_type; + props_ptr = (uint32_t __user *)(arg->props_ptr); + prop_values_ptr = (uint64_t __user *)(arg->prop_values_ptr); + }
- obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type); + obj = drm_mode_object_find(dev, file_priv, obj_id, obj_type); if (!obj) { ret = -ENOENT; goto out; @@ -456,13 +480,18 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, }
ret = drm_mode_object_get_properties(obj, file_priv->atomic, - uaddr_to_user_ptr(arg->props_ptr), - uaddr_to_user_ptr(arg->prop_values_ptr), - &arg->count_props); + props_ptr, + prop_values_ptr, + &count_props);
out_unref: drm_mode_object_put(obj); out: + if (in_compat64_syscall()) { + arg32->count_props = count_props; + } else { + arg->count_props = count_props; + } DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; } diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 3dba57a39197..c11358db7267 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -655,15 +655,27 @@ EXPORT_SYMBOL(drm_mode_plane_set_obj_prop); int drm_mode_getplane_res(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_plane_res32 { + __u64 plane_id_ptr; + __u32 count_planes; + }; + struct drm_mode_get_plane_res32 *plane_resp32 = data; struct drm_mode_get_plane_res *plane_resp = data; struct drm_plane *plane; uint32_t __user *plane_ptr; int count = 0; + __u32 count_planes;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- plane_ptr = u64_to_user_ptr(plane_resp->plane_id_ptr); + if (in_compat64_syscall()) { + plane_ptr = compat_ptr(plane_resp32->plane_id_ptr); + count_planes = plane_resp32->count_planes; + } else { + plane_ptr = (uint32_t __user *)(plane_resp->plane_id_ptr); + count_planes = plane_resp->count_planes; + }
/* * This ioctl is called twice, once to determine how much space is @@ -679,13 +691,18 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, continue;
if (drm_lease_held(file_priv, plane->base.id)) { - if (count < plane_resp->count_planes && + if (count < count_planes && put_user(plane->base.id, plane_ptr + count)) return -EFAULT; count++; } } - plane_resp->count_planes = count; + + if (in_compat64_syscall()) { + plane_resp32->count_planes = count; + } else { + plane_resp->count_planes = count; + }
return 0; } @@ -693,53 +710,91 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_plane32 { + __u32 plane_id; + + __u32 crtc_id; + __u32 fb_id; + + __u32 possible_crtcs; + __u32 gamma_size; + + __u32 count_format_types; + __u64 format_type_ptr; + }; + struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr; + __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- plane = drm_plane_find(dev, file_priv, plane_resp->plane_id); + if (in_compat64_syscall()) { + format_ptr = compat_ptr(plane_resp32->format_type_ptr); + plane_id = plane_resp32->plane_id; + count_format_types = plane_resp32->count_format_types; + } else { + format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr); + plane_id = plane_resp->plane_id; + count_format_types = plane_resp->count_format_types; + } + + plane = drm_plane_find(dev, file_priv, plane_id); if (!plane) return -ENOENT;
drm_modeset_lock(&plane->mutex, NULL); if (plane->state && plane->state->crtc && drm_lease_held(file_priv, plane->state->crtc->base.id)) - plane_resp->crtc_id = plane->state->crtc->base.id; + crtc_id = plane->state->crtc->base.id; else if (!plane->state && plane->crtc && drm_lease_held(file_priv, plane->crtc->base.id)) - plane_resp->crtc_id = plane->crtc->base.id; + crtc_id = plane->crtc->base.id; else - plane_resp->crtc_id = 0; + crtc_id = 0;
if (plane->state && plane->state->fb) - plane_resp->fb_id = plane->state->fb->base.id; + fb_id = plane->state->fb->base.id; else if (!plane->state && plane->fb) - plane_resp->fb_id = plane->fb->base.id; + fb_id = plane->fb->base.id; else - plane_resp->fb_id = 0; + fb_id = 0; drm_modeset_unlock(&plane->mutex);
- plane_resp->plane_id = plane->base.id; - plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv, - plane->possible_crtcs); + plane_id = plane->base.id; + possible_crtcs = drm_lease_filter_crtcs(file_priv, + plane->possible_crtcs);
- plane_resp->gamma_size = 0; + gamma_size = 0;
/* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ if (plane->format_count && - (plane_resp->count_format_types >= plane->format_count)) { - format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr); + (count_format_types >= plane->format_count)) { if (copy_to_user(format_ptr, plane->format_types, sizeof(uint32_t) * plane->format_count)) { return -EFAULT; } } - plane_resp->count_format_types = plane->format_count; + + if (in_compat64_syscall()) { + plane_resp32->crtc_id = crtc_id; + plane_resp32->fb_id = fb_id; + plane_resp32->plane_id = plane_id; + plane_resp32->possible_crtcs = possible_crtcs; + plane_resp32->gamma_size = gamma_size; + plane_resp32->count_format_types = count_format_types; + } else { + plane_resp->crtc_id = crtc_id; + plane_resp->fb_id = fb_id; + plane_resp->plane_id = plane_id; + plane_resp->possible_crtcs = possible_crtcs; + plane_resp->gamma_size = gamma_size; + plane_resp->count_format_types = count_format_types; + }
return 0; } diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..18435fc9df97 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -457,6 +457,16 @@ EXPORT_SYMBOL(drm_property_destroy); int drm_mode_getproperty_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_property32 { + __u64 values_ptr; + __u64 enum_blob_ptr; + __u32 prop_id; + __u32 flags; + char name[DRM_PROP_NAME_LEN]; + __u32 count_values; + __u32 count_enum_blobs; + }; + struct drm_mode_get_property32 *out_resp32 = data; struct drm_mode_get_property *out_resp = data; struct drm_property *property; int enum_count = 0; @@ -465,36 +475,51 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, struct drm_property_enum *prop_enum; struct drm_mode_property_enum __user *enum_ptr; uint64_t __user *values_ptr; + __u32 prop_id, flags, count_values, count_enum_blobs; + char *name;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- property = drm_property_find(dev, file_priv, out_resp->prop_id); + if (in_compat64_syscall()) { + values_ptr = compat_ptr(out_resp32->values_ptr); + enum_ptr = compat_ptr(out_resp32->enum_blob_ptr); + prop_id = out_resp32->prop_id; + name = out_resp32->name; + count_values = out_resp32->count_values; + count_enum_blobs = out_resp32->count_enum_blobs; + } else { + values_ptr = (uint64_t __user *)out_resp->values_ptr; + enum_ptr = (struct drm_mode_property_enum __user *)out_resp->enum_blob_ptr; + prop_id = out_resp->prop_id; + name = out_resp->name; + count_values = out_resp->count_values; + count_enum_blobs = out_resp->count_enum_blobs; + } + + property = drm_property_find(dev, file_priv, prop_id); if (!property) return -ENOENT;
- strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); - out_resp->flags = property->flags; + strscpy_pad(name, property->name, DRM_PROP_NAME_LEN); + flags = property->flags;
value_count = property->num_values; - values_ptr = u64_to_user_ptr(out_resp->values_ptr);
for (i = 0; i < value_count; i++) { - if (i < out_resp->count_values && + if (i < count_values && put_user(property->values[i], values_ptr + i)) { return -EFAULT; } } - out_resp->count_values = value_count;
copied = 0; - enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr);
if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { list_for_each_entry(prop_enum, &property->enum_list, head) { enum_count++; - if (out_resp->count_enum_blobs < enum_count) + if (count_enum_blobs < enum_count) continue;
if (copy_to_user(&enum_ptr[copied].value, @@ -506,7 +531,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, return -EFAULT; copied++; } - out_resp->count_enum_blobs = enum_count; + count_enum_blobs = enum_count; }
/* @@ -518,7 +543,19 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, * the property itself. */ if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) - out_resp->count_enum_blobs = 0; + count_enum_blobs = 0; + + if (in_compat64_syscall()) { + out_resp32->prop_id = prop_id; + out_resp32->flags = flags; + out_resp32->count_values = count_values; + out_resp32->count_enum_blobs = count_enum_blobs; + } else { + out_resp->prop_id = prop_id; + out_resp->flags = flags; + out_resp->count_values = count_values; + out_resp->count_enum_blobs = count_enum_blobs; + }
return 0; } @@ -754,29 +791,53 @@ EXPORT_SYMBOL(drm_property_replace_blob); int drm_mode_getblob_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_blob32 { + __u32 blob_id; + __u32 length; + __u64 data; + }; + struct drm_mode_get_blob32 *out_resp32 = data; struct drm_mode_get_blob *out_resp = data; struct drm_property_blob *blob; int ret = 0; + __u32 blob_id; + __u32 length; + uint8_t __user *blob_data; + + if (in_compat64_syscall()) { + blob_id = out_resp32->blob_id; + length = out_resp32->length; + blob_data = compat_ptr(out_resp32->data); + } else { + blob_id = out_resp->blob_id; + length = out_resp->length; + blob_data = (uint8_t __user *)(out_resp->data); + }
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- blob = drm_property_lookup_blob(dev, out_resp->blob_id); + blob = drm_property_lookup_blob(dev, blob_id); if (!blob) return -ENOENT;
- if (out_resp->length == blob->length) { - if (copy_to_user(u64_to_user_ptr(out_resp->data), + if (length == blob->length) { + if (copy_to_user(blob_data, blob->data, blob->length)) { ret = -EFAULT; goto unref; } } - out_resp->length = blob->length; + length = blob->length; unref: drm_property_blob_put(blob);
+ if (in_compat64_syscall()) { + out_resp32->length = length; + } else { + out_resp->length = length; + } return ret; }
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 3a426e7ed775..712ba1f5f912 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -260,10 +260,10 @@ struct drm_mode_modeinfo { };
struct drm_mode_card_res { - __u64 fb_id_ptr; - __u64 crtc_id_ptr; - __u64 connector_id_ptr; - __u64 encoder_id_ptr; + __kernel_uintptr_t fb_id_ptr; + __kernel_uintptr_t crtc_id_ptr; + __kernel_uintptr_t connector_id_ptr; + __kernel_uintptr_t encoder_id_ptr; __u32 count_fbs; __u32 count_crtcs; __u32 count_connectors; @@ -354,11 +354,11 @@ struct drm_mode_get_plane { * @format_type_ptr: Pointer to ``__u32`` array of formats that are * supported by the plane. These formats do not require modifiers. */ - __u64 format_type_ptr; + __kernel_uintptr_t format_type_ptr; };
struct drm_mode_get_plane_res { - __u64 plane_id_ptr; + __kernel_uintptr_t plane_id_ptr; __u32 count_planes; };
@@ -457,13 +457,13 @@ enum drm_mode_subconnector { */ struct drm_mode_get_connector { /** @encoders_ptr: Pointer to ``__u32`` array of object IDs. */ - __u64 encoders_ptr; + __kernel_uintptr_t encoders_ptr; /** @modes_ptr: Pointer to struct drm_mode_modeinfo array. */ - __u64 modes_ptr; + __kernel_uintptr_t modes_ptr; /** @props_ptr: Pointer to ``__u32`` array of property IDs. */ - __u64 props_ptr; + __kernel_uintptr_t props_ptr; /** @prop_values_ptr: Pointer to ``__u64`` array of property values. */ - __u64 prop_values_ptr; + __kernel_uintptr_t prop_values_ptr;
/** @count_modes: Number of modes. */ __u32 count_modes; @@ -589,9 +589,9 @@ struct drm_mode_property_enum { */ struct drm_mode_get_property { /** @values_ptr: Pointer to a ``__u64`` array. */ - __u64 values_ptr; + __kernel_uintptr_t values_ptr; /** @enum_blob_ptr: Pointer to a struct drm_mode_property_enum array. */ - __u64 enum_blob_ptr; + __kernel_uintptr_t enum_blob_ptr;
/** * @prop_id: Object ID of the property which should be retrieved. Set @@ -632,8 +632,8 @@ struct drm_mode_connector_set_property { #define DRM_MODE_OBJECT_ANY 0
struct drm_mode_obj_get_properties { - __u64 props_ptr; - __u64 prop_values_ptr; + __kernel_uintptr_t props_ptr; + __kernel_uintptr_t prop_values_ptr; __u32 count_props; __u32 obj_id; __u32 obj_type; @@ -649,7 +649,7 @@ struct drm_mode_obj_set_property { struct drm_mode_get_blob { __u32 blob_id; __u32 length; - __u64 data; + __kernel_uintptr_t data; };
struct drm_mode_fb_cmd { @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved; - __kernel_uintptr_t user_data; + __u64 user_data; };
struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index 82797c95efa6..e1957600a7f7 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -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 */ + uintptr_t user_data; /* user data passed to event */ };
#if defined(__cplusplus) @@ -1289,7 +1289,7 @@ struct drm_event_vblank { */ struct drm_event_crtc_sequence { struct drm_event base; - __u64 user_data; + uintptr_t user_data; __s64 time_ns; __u64 sequence; };
On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:
[...] @@ -693,53 +710,91 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) {
- struct drm_mode_get_plane32 {
__u32 plane_id;__u32 crtc_id;__u32 fb_id;__u32 possible_crtcs;__u32 gamma_size;__u32 count_format_types;__u64 format_type_ptr;- };
- struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr;
- __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- plane = drm_plane_find(dev, file_priv, plane_resp->plane_id);
- if (in_compat64_syscall()) {
format_ptr = compat_ptr(plane_resp32->format_type_ptr);plane_id = plane_resp32->plane_id;count_format_types = plane_resp32->count_format_types;- } else {
format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
Should be a simple cast (this is interpreting format_type_ptr as a 64-bit value, throwing away the capability, then creating a new one).
Kevin
On 4/15/24 9:08 AM, Kevin Brodsky wrote:
On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:
[...] @@ -693,53 +710,91 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) {
- struct drm_mode_get_plane32 {
__u32 plane_id;__u32 crtc_id;__u32 fb_id;__u32 possible_crtcs;__u32 gamma_size;__u32 count_format_types;__u64 format_type_ptr;- };
- struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr;
- __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- plane = drm_plane_find(dev, file_priv, plane_resp->plane_id);
- if (in_compat64_syscall()) {
format_ptr = compat_ptr(plane_resp32->format_type_ptr);plane_id = plane_resp32->plane_id;count_format_types = plane_resp32->count_format_types;- } else {
format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);Should be a simple cast (this is interpreting format_type_ptr as a 64-bit value, throwing away the capability, then creating a new one).
oh yeah - missed this.
linux-morello@op-lists.linaro.org
-
Carsten Haitzler -
carsten.haitzler＠foss.arm.com
-
Kevin Brodsky