Some hardware are more flexible on what they can flip asynchronously, so
rework the plane check so drivers can implement their own check, lifting
up some of the restrictions.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
v3: no changes

 drivers/gpu/drm/drm_atomic_uapi.c | 62 ++++++++++++++++++++++---------
 include/drm/drm_atomic_uapi.h     | 12 ++++++
 include/drm/drm_plane.h           |  5 +++
 3 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index aee4a65d4959..6d5b9fec90c7 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -620,7 +620,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 	return 0;
 }
 
-static int
+int
 drm_atomic_plane_get_property(struct drm_plane *plane,
 		const struct drm_plane_state *state,
 		struct drm_property *property, uint64_t *val)
@@ -683,6 +683,7 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_atomic_plane_get_property);
 
 static int drm_atomic_set_writeback_fb_for_connector(
 		struct drm_connector_state *conn_state,
@@ -1026,18 +1027,54 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
 	return ret;
 }
 
-static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
+int 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:%d:%s] This prop cannot be changed during async flip\n",
 			       prop->base.id, prop->name);
 		return -EINVAL;
 	}
 
 	return 0;
 }
+EXPORT_SYMBOL(drm_atomic_check_prop_changes);
+
+/* plane changes may have exceptions, so we have a special function for them */
+static int drm_atomic_check_plane_changes(struct drm_property *prop,
+					  struct drm_plane *plane,
+					  struct drm_plane_state *plane_state,
+					  struct drm_mode_object *obj,
+					  u64 prop_value, u64 old_val)
+{
+	struct drm_mode_config *config = &plane->dev->mode_config;
+	int ret;
+
+	if (plane->funcs->check_async_props)
+		return plane->funcs->check_async_props(prop, plane, plane_state,
+							     obj, prop_value, old_val);
+
+	/*
+	 * if you are trying to change something other than the FB ID, your
+	 * change will be either rejected or ignored, so we can stop the check
+	 * here
+	 */
+	if (prop != config->prop_fb_id) {
+		ret = drm_atomic_plane_get_property(plane, plane_state,
+						    prop, &old_val);
+		return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+	}
+
+	if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
+		drm_dbg_atomic(prop->dev,
+			       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
+			       obj->id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
 
 int drm_atomic_set_property(struct drm_atomic_state *state,
 			    struct drm_file *file_priv,
@@ -1100,7 +1137,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 	case DRM_MODE_OBJECT_PLANE: {
 		struct drm_plane *plane = obj_to_plane(obj);
 		struct drm_plane_state *plane_state;
-		struct drm_mode_config *config = &plane->dev->mode_config;
 
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
@@ -1108,19 +1144,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			break;
 		}
 
-		if (async_flip && prop != config->prop_fb_id) {
-			ret = drm_atomic_plane_get_property(plane, plane_state,
-							    prop, &old_val);
-			ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
-			break;
-		}
-
-		if (async_flip && plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
-			drm_dbg_atomic(prop->dev,
-				       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
-				       obj->id);
-			ret = -EINVAL;
-			break;
+		if (async_flip) {
+			ret = drm_atomic_check_plane_changes(prop, plane, plane_state,
+							     obj, prop_value, old_val);
+			if (ret)
+				break;
 		}
 
 		ret = drm_atomic_plane_set_property(plane,
diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
index 4c6d39d7bdb2..d65fa8fbbca0 100644
--- a/include/drm/drm_atomic_uapi.h
+++ b/include/drm/drm_atomic_uapi.h
@@ -29,6 +29,8 @@
 #ifndef DRM_ATOMIC_UAPI_H_
 #define DRM_ATOMIC_UAPI_H_
 
+#include <linux/types.h>
+
 struct drm_crtc_state;
 struct drm_display_mode;
 struct drm_property_blob;
@@ -37,6 +39,9 @@ struct drm_crtc;
 struct drm_connector_state;
 struct dma_fence;
 struct drm_framebuffer;
+struct drm_mode_object;
+struct drm_property;
+struct drm_plane;
 
 int __must_check
 drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
@@ -53,4 +58,11 @@ int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
 
+int drm_atomic_plane_get_property(struct drm_plane *plane,
+				  const struct drm_plane_state *state,
+				  struct drm_property *property, uint64_t *val);
+
+int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
+				  struct drm_property *prop);
+
 #endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c6565a6f9324..73dac8d76831 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -540,6 +540,11 @@ struct drm_plane_funcs {
 	 */
 	bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
 				     uint64_t modifier);
+
+	int (*check_async_props)(struct drm_property *prop, struct drm_plane *plane,
+				 struct drm_plane_state *plane_state,
+				 struct drm_mode_object *obj,
+				 u64 prop_value, u64 old_val);
 };
 
 /**
-- 
2.43.0

On Sun, 28 Jan 2024 18:25:13 -0300
André Almeida <andrealmeid@igalia.com> wrote:

> Some hardware are more flexible on what they can flip asynchronously, so
> rework the plane check so drivers can implement their own check, lifting
> up some of the restrictions.
> 
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
> v3: no changes
> 
>  drivers/gpu/drm/drm_atomic_uapi.c | 62 ++++++++++++++++++++++---------
>  include/drm/drm_atomic_uapi.h     | 12 ++++++
>  include/drm/drm_plane.h           |  5 +++
>  3 files changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index aee4a65d4959..6d5b9fec90c7 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -620,7 +620,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  	return 0;
>  }
>  
> -static int
> +int
>  drm_atomic_plane_get_property(struct drm_plane *plane,
>  		const struct drm_plane_state *state,
>  		struct drm_property *property, uint64_t *val)
> @@ -683,6 +683,7 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(drm_atomic_plane_get_property);
>  
>  static int drm_atomic_set_writeback_fb_for_connector(
>  		struct drm_connector_state *conn_state,
> @@ -1026,18 +1027,54 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>  	return ret;
>  }
>  
> -static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
> +int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,

Hi,

should the word "async" be somewhere in the function name?

>  					 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:%d:%s] This prop cannot be changed during async flip\n",
>  			       prop->base.id, prop->name);
>  		return -EINVAL;
>  	}
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(drm_atomic_check_prop_changes);
> +
> +/* plane changes may have exceptions, so we have a special function for them */
> +static int drm_atomic_check_plane_changes(struct drm_property *prop,
> +					  struct drm_plane *plane,
> +					  struct drm_plane_state *plane_state,
> +					  struct drm_mode_object *obj,
> +					  u64 prop_value, u64 old_val)
> +{
> +	struct drm_mode_config *config = &plane->dev->mode_config;
> +	int ret;
> +
> +	if (plane->funcs->check_async_props)
> +		return plane->funcs->check_async_props(prop, plane, plane_state,
> +							     obj, prop_value, old_val);

Is it really ok to allow drivers to opt-in to also *reject* the FB_ID
and IN_FENCE_FD props on async commits?

Either intentionally or by accident.

> +
> +	/*
> +	 * if you are trying to change something other than the FB ID, your
> +	 * change will be either rejected or ignored, so we can stop the check
> +	 * here
> +	 */
> +	if (prop != config->prop_fb_id) {
> +		ret = drm_atomic_plane_get_property(plane, plane_state,
> +						    prop, &old_val);
> +		return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);

When I first read this code, it seemed to say: "If the prop is not
FB_ID, then do the usual prop change checking that happens on all
changes, not only async.". Reading this patch a few more times over, I
finally realized drm_atomic_check_prop_changes() is about async
specifically.

> +	}
> +
> +	if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
> +		drm_dbg_atomic(prop->dev,
> +			       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
> +			       obj->id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
>  
>  int drm_atomic_set_property(struct drm_atomic_state *state,
>  			    struct drm_file *file_priv,
> @@ -1100,7 +1137,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  	case DRM_MODE_OBJECT_PLANE: {
>  		struct drm_plane *plane = obj_to_plane(obj);
>  		struct drm_plane_state *plane_state;
> -		struct drm_mode_config *config = &plane->dev->mode_config;
>  
>  		plane_state = drm_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state)) {
> @@ -1108,19 +1144,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  			break;
>  		}
>  
> -		if (async_flip && prop != config->prop_fb_id) {
> -			ret = drm_atomic_plane_get_property(plane, plane_state,
> -							    prop, &old_val);
> -			ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
> -			break;
> -		}
> -
> -		if (async_flip && plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
> -			drm_dbg_atomic(prop->dev,
> -				       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
> -				       obj->id);
> -			ret = -EINVAL;
> -			break;
> +		if (async_flip) {
> +			ret = drm_atomic_check_plane_changes(prop, plane, plane_state,

Should there be "async" somewhere in the name of
drm_atomic_check_plane_changes()?


Thanks,
pq

> +							     obj, prop_value, old_val);
> +			if (ret)
> +				break;
>  		}
>  
>  		ret = drm_atomic_plane_set_property(plane,
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 4c6d39d7bdb2..d65fa8fbbca0 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -29,6 +29,8 @@
>  #ifndef DRM_ATOMIC_UAPI_H_
>  #define DRM_ATOMIC_UAPI_H_
>  
> +#include <linux/types.h>
> +
>  struct drm_crtc_state;
>  struct drm_display_mode;
>  struct drm_property_blob;
> @@ -37,6 +39,9 @@ struct drm_crtc;
>  struct drm_connector_state;
>  struct dma_fence;
>  struct drm_framebuffer;
> +struct drm_mode_object;
> +struct drm_property;
> +struct drm_plane;
>  
>  int __must_check
>  drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> @@ -53,4 +58,11 @@ int __must_check
>  drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  				  struct drm_crtc *crtc);
>  
> +int drm_atomic_plane_get_property(struct drm_plane *plane,
> +				  const struct drm_plane_state *state,
> +				  struct drm_property *property, uint64_t *val);
> +
> +int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
> +				  struct drm_property *prop);
> +
>  #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index c6565a6f9324..73dac8d76831 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -540,6 +540,11 @@ struct drm_plane_funcs {
>  	 */
>  	bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
>  				     uint64_t modifier);
> +
> +	int (*check_async_props)(struct drm_property *prop, struct drm_plane *plane,
> +				 struct drm_plane_state *plane_state,
> +				 struct drm_mode_object *obj,
> +				 u64 prop_value, u64 old_val);
>  };
>  
>  /**

Hi Pekka,

Em 29/01/2024 05:49, Pekka Paalanen escreveu:
> On Sun, 28 Jan 2024 18:25:13 -0300
> André Almeida <andrealmeid@igalia.com> wrote:
> 
>> Some hardware are more flexible on what they can flip asynchronously, so
>> rework the plane check so drivers can implement their own check, lifting
>> up some of the restrictions.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---
>> v3: no changes
>>
>>   drivers/gpu/drm/drm_atomic_uapi.c | 62 ++++++++++++++++++++++---------
>>   include/drm/drm_atomic_uapi.h     | 12 ++++++
>>   include/drm/drm_plane.h           |  5 +++
>>   3 files changed, 62 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index aee4a65d4959..6d5b9fec90c7 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -620,7 +620,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>>   	return 0;
>>   }
>>   
>> -static int
>> +int
>>   drm_atomic_plane_get_property(struct drm_plane *plane,
>>   		const struct drm_plane_state *state,
>>   		struct drm_property *property, uint64_t *val)
>> @@ -683,6 +683,7 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(drm_atomic_plane_get_property);
>>   
>>   static int drm_atomic_set_writeback_fb_for_connector(
>>   		struct drm_connector_state *conn_state,
>> @@ -1026,18 +1027,54 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state,
>>   	return ret;
>>   }
>>   
>> -static int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
>> +int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
> 
> Hi,
> 
> should the word "async" be somewhere in the function name?
> 
>>   					 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:%d:%s] This prop cannot be changed during async flip\n",
>>   			       prop->base.id, prop->name);
>>   		return -EINVAL;
>>   	}
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(drm_atomic_check_prop_changes);
>> +
>> +/* plane changes may have exceptions, so we have a special function for them */
>> +static int drm_atomic_check_plane_changes(struct drm_property *prop,
>> +					  struct drm_plane *plane,
>> +					  struct drm_plane_state *plane_state,
>> +					  struct drm_mode_object *obj,
>> +					  u64 prop_value, u64 old_val)
>> +{
>> +	struct drm_mode_config *config = &plane->dev->mode_config;
>> +	int ret;
>> +
>> +	if (plane->funcs->check_async_props)
>> +		return plane->funcs->check_async_props(prop, plane, plane_state,
>> +							     obj, prop_value, old_val);
> 
> Is it really ok to allow drivers to opt-in to also *reject* the FB_ID
> and IN_FENCE_FD props on async commits?
> 
> Either intentionally or by accident.
> 

Right, perhaps I should write this function in a way that you can only 
lift restrictions, and not add new ones.

>> +
>> +	/*
>> +	 * if you are trying to change something other than the FB ID, your
>> +	 * change will be either rejected or ignored, so we can stop the check
>> +	 * here
>> +	 */
>> +	if (prop != config->prop_fb_id) {
>> +		ret = drm_atomic_plane_get_property(plane, plane_state,
>> +						    prop, &old_val);
>> +		return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
> 
> When I first read this code, it seemed to say: "If the prop is not
> FB_ID, then do the usual prop change checking that happens on all
> changes, not only async.". Reading this patch a few more times over, I
> finally realized drm_atomic_check_prop_changes() is about async
> specifically.
> 

I see that the lack of the async word is giving some confusion, so I'll 
add it to the functions.

Thanks,
	André

>> +	}
>> +
>> +	if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> +		drm_dbg_atomic(prop->dev,
>> +			       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
>> +			       obj->id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>>   
>>   int drm_atomic_set_property(struct drm_atomic_state *state,
>>   			    struct drm_file *file_priv,
>> @@ -1100,7 +1137,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>   	case DRM_MODE_OBJECT_PLANE: {
>>   		struct drm_plane *plane = obj_to_plane(obj);
>>   		struct drm_plane_state *plane_state;
>> -		struct drm_mode_config *config = &plane->dev->mode_config;
>>   
>>   		plane_state = drm_atomic_get_plane_state(state, plane);
>>   		if (IS_ERR(plane_state)) {
>> @@ -1108,19 +1144,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>   			break;
>>   		}
>>   
>> -		if (async_flip && prop != config->prop_fb_id) {
>> -			ret = drm_atomic_plane_get_property(plane, plane_state,
>> -							    prop, &old_val);
>> -			ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
>> -			break;
>> -		}
>> -
>> -		if (async_flip && plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> -			drm_dbg_atomic(prop->dev,
>> -				       "[OBJECT:%d] Only primary planes can be changed during async flip\n",
>> -				       obj->id);
>> -			ret = -EINVAL;
>> -			break;
>> +		if (async_flip) {
>> +			ret = drm_atomic_check_plane_changes(prop, plane, plane_state,
> 
> Should there be "async" somewhere in the name of
> drm_atomic_check_plane_changes()?
> 
> 
> Thanks,
> pq
> 
>> +							     obj, prop_value, old_val);
>> +			if (ret)
>> +				break;
>>   		}
>>   
>>   		ret = drm_atomic_plane_set_property(plane,
>> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
>> index 4c6d39d7bdb2..d65fa8fbbca0 100644
>> --- a/include/drm/drm_atomic_uapi.h
>> +++ b/include/drm/drm_atomic_uapi.h
>> @@ -29,6 +29,8 @@
>>   #ifndef DRM_ATOMIC_UAPI_H_
>>   #define DRM_ATOMIC_UAPI_H_
>>   
>> +#include <linux/types.h>
>> +
>>   struct drm_crtc_state;
>>   struct drm_display_mode;
>>   struct drm_property_blob;
>> @@ -37,6 +39,9 @@ struct drm_crtc;
>>   struct drm_connector_state;
>>   struct dma_fence;
>>   struct drm_framebuffer;
>> +struct drm_mode_object;
>> +struct drm_property;
>> +struct drm_plane;
>>   
>>   int __must_check
>>   drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>> @@ -53,4 +58,11 @@ int __must_check
>>   drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>>   				  struct drm_crtc *crtc);
>>   
>> +int drm_atomic_plane_get_property(struct drm_plane *plane,
>> +				  const struct drm_plane_state *state,
>> +				  struct drm_property *property, uint64_t *val);
>> +
>> +int drm_atomic_check_prop_changes(int ret, uint64_t old_val, uint64_t prop_value,
>> +				  struct drm_property *prop);
>> +
>>   #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index c6565a6f9324..73dac8d76831 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -540,6 +540,11 @@ struct drm_plane_funcs {
>>   	 */
>>   	bool (*format_mod_supported)(struct drm_plane *plane, uint32_t format,
>>   				     uint64_t modifier);
>> +
>> +	int (*check_async_props)(struct drm_property *prop, struct drm_plane *plane,
>> +				 struct drm_plane_state *plane_state,
>> +				 struct drm_mode_object *obj,
>> +				 u64 prop_value, u64 old_val);
>>   };
>>   
>>   /**
>