On 10/13/22 13:02, Simon Ser wrote:
So no tests that actually verify that the kernel properly rejects
stuff stuff like modesets, gamma LUT updates, plane movement,
etc.?
Pondering this a bit more, it just occurred to me the current driver
level checks might easily lead to confusing behaviour. Eg. is
the ioctl going to succeed if you ask for an async change of some
random property while the crtc disabled, but fails if you ask for
the same async property change when the crtc is active?
So another reason why rejecting most properties already at
the uapi level might be a good idea.
And just to be clear this is pretty much what I suggest:
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..471a2c703847 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
goto out;
}
+ if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
+ prop != dev->mode_config.prop_fb_id) {
+ drm_mode_object_put(obj);
+ ret = -EINVAL;
+ goto out;
+ }
+
if (copy_from_user(&prop_value,
prop_values_ptr + copied_props,
sizeof(prop_value))) {
That would actively discourage people from even attempting the
"just dump all the state into the ioctl" approach with async flips
since even the props whose value isn't even changing would be rejected.
How does this sound?
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 945761968428..ffd16bdc7b83 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
struct drm_file *file_priv,
struct drm_mode_object *obj,
struct drm_property *prop,
- uint64_t prop_value)
+ uint64_t prop_value,
+ bool async_flip)
{
struct drm_mode_object *ref;
int ret;
+ uint64_t old_val;
if (!drm_property_change_valid_get(prop, prop_value, &ref))
return -EINVAL;
+ if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
+ ret = drm_atomic_get_property(obj, prop, &old_val);
+ if (ret != 0 || old_val != prop_value) {
+ drm_dbg_atomic(prop->dev,
+ "[PROP:%d:%s] cannot be changed during async flip\n",
+ prop->base.id, prop->name);
I would write this as "[PROP:%d:%s] No prop can be changed during async
flips" to make it clear that it's not just this prop that can't, but any.
+ return -EINVAL;
+ }
+ }
+
switch (obj->type) {
case DRM_MODE_OBJECT_CONNECTOR: {
struct drm_connector *connector = obj_to_connector(obj);
