[PATCH v3 0/6] Add support for atomic async page-flips
André Almeida
andrealmeid at igalia.com
Fri Oct 28 12:36:29 UTC 2022
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);
> + return -EINVAL;
> + }
> + }
> +
> switch (obj->type) {
> case DRM_MODE_OBJECT_CONNECTOR: {
> struct drm_connector *connector = obj_to_connector(obj);
drm_atomic_get_property() needs the object lock to be used, so we need
to check the property inside the switch-case like this:
-- >8 --
From f3ee5a1163bfe5a88109d7084208940fe5566967 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Almeida?= <andrealmeid at igalia.com>
Date: Thu, 27 Oct 2022 17:23:09 -0300
Subject: [PATCH] drm: Check for prop changes in atomic async flip
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
No prop changes are allowed during an async flip via atomic DRM API, so
make sure to reject them.
Signed-off-by: André Almeida <andrealmeid at igalia.com>
---
drivers/gpu/drm/drm_atomic_uapi.c | 47 +++++++++++++++++++++++++++--
drivers/gpu/drm/drm_crtc_internal.h | 2 +-
drivers/gpu/drm/drm_mode_object.c | 2 +-
3 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index ee24ed7e2edb..f63f23305621 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -964,13 +964,28 @@ int drm_atomic_connector_commit_dpms(struct
drm_atomic_state *state,
return ret;
}
+static bool drm_atomic_check_prop_changes(int ret, uint64_t old_val,
uint64_t prop_value,
+ struct drm_property *prop)
+{
+ if (ret != 0 || old_val != prop_value) {
+ drm_dbg_atomic(prop->dev,
+ "[PROP:%d:%s] No prop can be changed during async flip\n",
+ prop->base.id, prop->name);
+ return true;
+ }
+
+ return false;
+}
+
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;
+ uint64_t old_val;
int ret;
if (!drm_property_change_valid_get(prop, prop_value, &ref))
@@ -987,6 +1002,15 @@ int drm_atomic_set_property(struct
drm_atomic_state *state,
break;
}
+ if (async_flip) {
+ ret = drm_atomic_connector_get_property(connector, connector_state,
+ prop, &old_val);
+ if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+
ret = drm_atomic_connector_set_property(connector,
connector_state, file_priv,
prop, prop_value);
@@ -1002,6 +1026,14 @@ int drm_atomic_set_property(struct
drm_atomic_state *state,
break;
}
+ if (async_flip) {
+ ret = drm_atomic_crtc_get_property(crtc, crtc_state, prop, &old_val);
+ if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+
ret = drm_atomic_crtc_set_property(crtc,
crtc_state, prop, prop_value);
break;
@@ -1016,6 +1048,14 @@ int drm_atomic_set_property(struct
drm_atomic_state *state,
break;
}
+ if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
+ ret = drm_atomic_plane_get_property(plane, plane_state, prop, &old_val);
+ if (drm_atomic_check_prop_changes(ret, old_val, prop_value, prop)) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+
ret = drm_atomic_plane_set_property(plane,
plane_state, file_priv,
prop, prop_value);
@@ -1304,6 +1344,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_out_fence_state *fence_state;
int ret = 0;
unsigned int i, j, num_fences;
+ bool async = false;
/* disallow for drivers not supporting atomic: */
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1340,6 +1381,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
"commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported with
atomic\n");
return -EINVAL;
}
+
+ async = true;
}
/* can't test and expect an event at the same time. */
@@ -1420,7 +1463,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
}
ret = drm_atomic_set_property(state, file_priv,
- obj, prop, prop_value);
+ obj, prop, prop_value, async);
if (ret) {
drm_mode_object_put(obj);
goto out;
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index 56041b604881..42ff11706fd4 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -250,7 +250,7 @@ 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);
int drm_atomic_get_property(struct drm_mode_object *obj,
struct drm_property *property, uint64_t *val);
diff --git a/drivers/gpu/drm/drm_mode_object.c
b/drivers/gpu/drm/drm_mode_object.c
index ba1608effc0f..64f519254895 100644
--- a/drivers/gpu/drm/drm_mode_object.c
+++ b/drivers/gpu/drm/drm_mode_object.c
@@ -536,7 +536,7 @@ static int set_property_atomic(struct
drm_mode_object *obj,
obj_to_connector(obj),
prop_value);
} else {
- ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value);
+ ret = drm_atomic_set_property(state, file_priv, obj, prop,
prop_value, false);
if (ret)
goto out;
ret = drm_atomic_commit(state);
--
2.37.3
More information about the wayland-devel
mailing list