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

[PATCH v2 0/2] 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.

For now this patchset only allow for async commits with IN_FENCE_ID and
FB_DAMAGE_CLIPS. Userspace can query if a driver supports this with TEST_ONLY
commits.

I will left overlay planes for a next iteration.

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

	André

André Almeida (2):
  drm/atomic: Allow drivers to write their own plane check for 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             | 62 ++++++++++++++-----
 include/drm/drm_atomic_uapi.h                 | 12 ++++
 include/drm/drm_plane.h                       |  5 ++
 4 files changed, 91 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] 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>
---
 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 v2 2/2] 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 IN_FENCE_ID and FB_DAMAGE_CLIPS
properties. For userspace to check if a driver support this two
properties, the strategy for now is to use TEST_ONLY commits.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
v2: Drop overlay plane option for now

 .../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..7afe8c1b62d4 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 &&
+	    prop != config->prop_fb_damage_clips) {
+		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;
+}
+
 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 v2 2/2] drm/amdgpu: Implement check_async_props for planes

[Ville Syrjälä]

On Fri, Jan 19, 2024 at 03:12:35PM -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 IN_FENCE_ID and FB_DAMAGE_CLIPS
> properties. For userspace to check if a driver support this two
> properties, the strategy for now is to use TEST_ONLY commits.
> 
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
> v2: Drop overlay plane option for now
> 
>  .../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..7afe8c1b62d4 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 &&

IN_FENCE should just be allowed always.

> +	    prop != config->prop_fb_damage_clips) {

This seems a bit dubious to me. How is amdgpu using the damage
information during async flips?

> +		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;
> +}
> +
>  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

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 2/2] drm/amdgpu: Implement check_async_props for planes

[Harry Wentland]

On 2024-01-19 13:25, Ville Syrjälä wrote:
> On Fri, Jan 19, 2024 at 03:12:35PM -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 IN_FENCE_ID and FB_DAMAGE_CLIPS
>> properties. For userspace to check if a driver support this two
>> properties, the strategy for now is to use TEST_ONLY commits.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---
>> v2: Drop overlay plane option for now
>>
>>   .../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..7afe8c1b62d4 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 &&
> 
> IN_FENCE should just be allowed always.
> 
>> +	    prop != config->prop_fb_damage_clips) {
> 
> This seems a bit dubious to me. How is amdgpu using the damage
> information during async flips?

Yeah, I'm also not sure this is right. Has anyone tested this
with a PSR SU panel?

Harry

> 
>> +		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;
>> +}
>> +
>>   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 v2 2/2] drm/amdgpu: Implement check_async_props for planes

[Xaver Hugl]

Am Mo., 22. Jan. 2024 um 16:50 Uhr schrieb Harry Wentland
<harry.wentland@amd.com>:
>
>
>
> On 2024-01-19 13:25, Ville Syrjälä wrote:
> > On Fri, Jan 19, 2024 at 03:12:35PM -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 IN_FENCE_ID and FB_DAMAGE_CLIPS
> >> properties. For userspace to check if a driver support this two
> >> properties, the strategy for now is to use TEST_ONLY commits.
> >>
> >> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> >> ---
> >> v2: Drop overlay plane option for now
> >>
> >>   .../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..7afe8c1b62d4 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 &&
> >
> > IN_FENCE should just be allowed always.
> >
> >> +        prop != config->prop_fb_damage_clips) {
> >
> > This seems a bit dubious to me. How is amdgpu using the damage
> > information during async flips?
>
> Yeah, I'm also not sure this is right. Has anyone tested this
> with a PSR SU panel?
>
> Harry

I attempted to, but according to
/sys/kernel/debug/dri/1/eDP-1/psr_state, PSR never kicks in on my
laptop at all. The only reason I wanted this property though is to
reduce the number of special cases for async pageflips compositors
have to implement; as it's not necessary for any functionality I think
it's also fine to leave it out.

> >> +            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;
> >> +}
> >> +
> >>   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 v2 2/2] drm/amdgpu: Implement check_async_props for planes

[Harry Wentland]

On 2024-01-23 13:02, Xaver Hugl wrote:
> Am Mo., 22. Jan. 2024 um 16:50 Uhr schrieb Harry Wentland
> <harry.wentland@amd.com>:
>>
>>
>>
>> On 2024-01-19 13:25, Ville Syrjälä wrote:
>>> On Fri, Jan 19, 2024 at 03:12:35PM -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 IN_FENCE_ID and FB_DAMAGE_CLIPS
>>>> properties. For userspace to check if a driver support this two
>>>> properties, the strategy for now is to use TEST_ONLY commits.
>>>>
>>>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>>>> ---
>>>> v2: Drop overlay plane option for now
>>>>
>>>>   .../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..7afe8c1b62d4 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 &&
>>>
>>> IN_FENCE should just be allowed always.
>>>
>>>> +        prop != config->prop_fb_damage_clips) {
>>>
>>> This seems a bit dubious to me. How is amdgpu using the damage
>>> information during async flips?
>>
>> Yeah, I'm also not sure this is right. Has anyone tested this
>> with a PSR SU panel?
>>
>> Harry
> 
> I attempted to, but according to
> /sys/kernel/debug/dri/1/eDP-1/psr_state, PSR never kicks in on my
> laptop at all. The only reason I wanted this property though is to
> reduce the number of special cases for async pageflips compositors
> have to implement; as it's not necessary for any functionality I think
> it's also fine to leave it out.
> 

Yeah, PSR panels aren't super common. PSR SU (Selective Update) panels
even less so.

I'd prefer to keep the damage clips out of async for now unless we
can actually test it with a PSR SU panel.

Harry

>>>> +            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;
>>>> +}
>>>> +
>>>>   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 v2 2/2] drm/amdgpu: Implement check_async_props for planes

[André Almeida]

Hi Ville,

Em 19/01/2024 15:25, Ville Syrjälä escreveu:
> On Fri, Jan 19, 2024 at 03:12:35PM -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 IN_FENCE_ID and FB_DAMAGE_CLIPS
>> properties. For userspace to check if a driver support this two
>> properties, the strategy for now is to use TEST_ONLY commits.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---
>> v2: Drop overlay plane option for now
>>
>>   .../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..7afe8c1b62d4 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 &&
> 
> IN_FENCE should just be allowed always.

Do you mean that the common path should allow IN_FENCE_FD for all drivers?

Re: [PATCH v2 2/2] drm/amdgpu: Implement check_async_props for planes

[Ville Syrjälä]

On Wed, Jan 24, 2024 at 11:14:40AM -0300, André Almeida wrote:
> Hi Ville,
> 
> Em 19/01/2024 15:25, Ville Syrjälä escreveu:
> > On Fri, Jan 19, 2024 at 03:12:35PM -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 IN_FENCE_ID and FB_DAMAGE_CLIPS
> >> properties. For userspace to check if a driver support this two
> >> properties, the strategy for now is to use TEST_ONLY commits.
> >>
> >> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> >> ---
> >> v2: Drop overlay plane option for now
> >>
> >>   .../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..7afe8c1b62d4 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 &&
> > 
> > IN_FENCE should just be allowed always.
> 
> Do you mean that the common path should allow IN_FENCE_FD for all drivers?

Yes.

-- 
Ville Syrjälä
Intel