[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 amd-gfx mailing list