[PATCH v3 0/3] drm/atomic: Allow drivers to write their own plane check for async

[PATCH v3 0/3] drm/atomic: Allow drivers to write their own plane check for async

[André Almeida]

Hi,

AMD hardware can do more on the async flip path than just the primary plane, so
to lift up the current restrictions, this patchset allows drivers to write their
own check for planes for async flips.

This patchset allows for async commits with IN_FENCE_ID in any driver and
overlay planes on AMD. Userspace can query if a driver supports this with
TEST_ONLY commits.

Changes from v2:
 - Allow IN_FENCE_ID for any driver
 - Allow overlay planes again
v2: https://lore.kernel.org/lkml/20240119181235.255060-1-andrealmeid@igalia.com/

Changes from v1:
 - Drop overlay planes option for now
v1: https://lore.kernel.org/dri-devel/20240116045159.1015510-1-andrealmeid@igalia.com/

André Almeida (3):
  drm/atomic: Allow drivers to write their own plane check for async
    flips
  drm/atomic: Allow userspace to use explicit sync with atomic async
    flips
  drm/amdgpu: Implement check_async_props for planes

 .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 29 +++++++++
 drivers/gpu/drm/drm_atomic_uapi.c             | 63 ++++++++++++++-----
 include/drm/drm_atomic_uapi.h                 | 12 ++++
 include/drm/drm_plane.h                       |  5 ++
 4 files changed, 92 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] drm/atomic: Allow drivers to write their own plane check for async flips

[André Almeida]

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

[PATCH v3 2/3] drm/atomic: Allow userspace to use explicit sync with atomic async flips

[André Almeida]

Allow userspace to use explicit synchronization with atomic async flips.
That means that the flip will wait for some hardware fence, and then
will flip as soon as possible (async) in regard of the vblank.

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

 drivers/gpu/drm/drm_atomic_uapi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 6d5b9fec90c7..edae7924ad69 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1060,7 +1060,8 @@ static int drm_atomic_check_plane_changes(struct drm_property *prop,
 	 * change will be either rejected or ignored, so we can stop the check
 	 * here
 	 */
-	if (prop != config->prop_fb_id) {
+	if (prop != config->prop_fb_id &&
+	    prop != config->prop_in_fence_fd) {
 		ret = drm_atomic_plane_get_property(plane, plane_state,
 						    prop, &old_val);
 		return drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
-- 
2.43.0

[PATCH v3 3/3] drm/amdgpu: Implement check_async_props for planes

[André Almeida]

AMD GPUs can do async flips with changes on more properties than just
the FB ID, so implement a custom check_async_props for AMD planes.

Allow amdgpu to do async flips with overlay planes as well.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
v3: allow overlay planes

 .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index 116121e647ca..ed75b69636b4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
@@ -25,6 +25,7 @@
  */
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_atomic_uapi.h>
 #include <drm/drm_blend.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
@@ -1430,6 +1431,33 @@ static void amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
 
+static int amdgpu_dm_plane_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)
+{
+	struct drm_mode_config *config = &plane->dev->mode_config;
+	int ret;
+
+	if (prop != config->prop_fb_id &&
+	    prop != config->prop_in_fence_fd) {
+		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 &&
+	    plane_state->plane->type != DRM_PLANE_TYPE_OVERLAY) {
+		drm_dbg_atomic(prop->dev,
+			       "[OBJECT:%d] Only primary or overlay planes can be changed during async flip\n",
+			       obj->id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct drm_plane_funcs dm_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
@@ -1438,6 +1466,7 @@ static const struct drm_plane_funcs dm_plane_funcs = {
 	.atomic_duplicate_state = amdgpu_dm_plane_drm_plane_duplicate_state,
 	.atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
 	.format_mod_supported = amdgpu_dm_plane_format_mod_supported,
+	.check_async_props = amdgpu_dm_plane_check_async_props,
 };
 
 int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
-- 
2.43.0

Re: [PATCH v3 1/3] drm/atomic: Allow drivers to write their own plane check for async flips

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7014 bytes --]

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);
>  };
>  
>  /**


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] drm/amdgpu: Implement check_async_props for planes

[Daniel Vetter]

On Sun, Jan 28, 2024 at 06:25:15PM -0300, André Almeida wrote:
> AMD GPUs can do async flips with changes on more properties than just
> the FB ID, so implement a custom check_async_props for AMD planes.
> 
> Allow amdgpu to do async flips with overlay planes as well.
> 
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
> v3: allow overlay planes

This comment very much written with a lack of clearly better ideas, but:

Do we really need this much flexibility, especially for the first driver
adding the first few additional properties?

A simple bool on struct drm_plane to indicate whether async flips are ok
or not should also do this job here? Maybe a bit of work to roll that out
to the primary planes for current drivers, but not much. And wouldn't need
drivers to implement some very uapi-marshalling atomic code ...

Also we could probably remove the current drm_mode_config.async_flip flag
and entirely replace it with the per-plane one.
-Sima
> 
>  .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 116121e647ca..ed75b69636b4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_blend.h>
>  #include <drm/drm_gem_atomic_helper.h>
>  #include <drm/drm_plane_helper.h>
> @@ -1430,6 +1431,33 @@ static void amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> +static int amdgpu_dm_plane_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)
> +{
> +	struct drm_mode_config *config = &plane->dev->mode_config;
> +	int ret;
> +
> +	if (prop != config->prop_fb_id &&
> +	    prop != config->prop_in_fence_fd) {
> +		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 &&
> +	    plane_state->plane->type != DRM_PLANE_TYPE_OVERLAY) {
> +		drm_dbg_atomic(prop->dev,
> +			       "[OBJECT:%d] Only primary or overlay planes can be changed during async flip\n",
> +			       obj->id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct drm_plane_funcs dm_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
> @@ -1438,6 +1466,7 @@ static const struct drm_plane_funcs dm_plane_funcs = {
>  	.atomic_duplicate_state = amdgpu_dm_plane_drm_plane_duplicate_state,
>  	.atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
>  	.format_mod_supported = amdgpu_dm_plane_format_mod_supported,
> +	.check_async_props = amdgpu_dm_plane_check_async_props,
>  };
>  
>  int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 3/3] drm/amdgpu: Implement check_async_props for planes

[Simon Ser]

> Do we really need this much flexibility, especially for the first driver
> adding the first few additional properties?

AFAIU we'd like to allow more props as well, e.g. cursor position…

Re: [PATCH v3 1/3] drm/atomic: Allow drivers to write their own plane check for async flips

[André Almeida]

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);
>>   };
>>   
>>   /**
> 

Re: [PATCH v3 3/3] drm/amdgpu: Implement check_async_props for planes

[André Almeida]

Hi Sima,

Em 30/01/2024 07:56, Daniel Vetter escreveu:
> On Sun, Jan 28, 2024 at 06:25:15PM -0300, André Almeida wrote:
>> AMD GPUs can do async flips with changes on more properties than just
>> the FB ID, so implement a custom check_async_props for AMD planes.
>>
>> Allow amdgpu to do async flips with overlay planes as well.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---
>> v3: allow overlay planes
> 
> This comment very much written with a lack of clearly better ideas, but:
> 
> Do we really need this much flexibility, especially for the first driver
> adding the first few additional properties?
> 
> A simple bool on struct drm_plane to indicate whether async flips are ok
> or not should also do this job here? Maybe a bit of work to roll that out
> to the primary planes for current drivers, but not much. And wouldn't need
> drivers to implement some very uapi-marshalling atomic code ...

Right, perhaps I can first write in the way you suggest, and later 
expand to the form I have proposed here if/when new properties arise.

> 
> Also we could probably remove the current drm_mode_config.async_flip flag
> and entirely replace it with the per-plane one.
> -Sima
>>
>>   .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 29 +++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
>> index 116121e647ca..ed75b69636b4 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
>> @@ -25,6 +25,7 @@
>>    */
>>   
>>   #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_atomic_uapi.h>
>>   #include <drm/drm_blend.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>>   #include <drm/drm_plane_helper.h>
>> @@ -1430,6 +1431,33 @@ static void amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
>>   	drm_atomic_helper_plane_destroy_state(plane, state);
>>   }
>>   
>> +static int amdgpu_dm_plane_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)
>> +{
>> +	struct drm_mode_config *config = &plane->dev->mode_config;
>> +	int ret;
>> +
>> +	if (prop != config->prop_fb_id &&
>> +	    prop != config->prop_in_fence_fd) {
>> +		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 &&
>> +	    plane_state->plane->type != DRM_PLANE_TYPE_OVERLAY) {
>> +		drm_dbg_atomic(prop->dev,
>> +			       "[OBJECT:%d] Only primary or overlay planes can be changed during async flip\n",
>> +			       obj->id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct drm_plane_funcs dm_plane_funcs = {
>>   	.update_plane	= drm_atomic_helper_update_plane,
>>   	.disable_plane	= drm_atomic_helper_disable_plane,
>> @@ -1438,6 +1466,7 @@ static const struct drm_plane_funcs dm_plane_funcs = {
>>   	.atomic_duplicate_state = amdgpu_dm_plane_drm_plane_duplicate_state,
>>   	.atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
>>   	.format_mod_supported = amdgpu_dm_plane_format_mod_supported,
>> +	.check_async_props = amdgpu_dm_plane_check_async_props,
>>   };
>>   
>>   int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>> -- 
>> 2.43.0
>>
>