[PATCH v3] drm: don't run atomic_async_check for disabled planes

[Murthy, Arun R]

On 01-08-2025 18:40, Xaver Hugl wrote:
> It's entirely valid and correct for compositors to include disabled
> planes in the atomic commit, and doing that should not prevent async
> flips from working. To fix that, this commit moves the plane check
> to after all the properties of the object have been set,
I dont think this is required. Again the plane states will have to be 
fetched outside the set_prop()

Alternate approach
@@ -1091,8 +1091,16 @@ int drm_atomic_set_property(struct 
drm_atomic_state *state,

                         /* ask the driver if this non-primary plane is 
supported */
                         if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
-                               ret = -EINVAL;
+                               /*
+                                * continue if no change in prop on 
non-supported async planes as well
+                                * or when disabling the plane
+                                */
+                               if (ret == 0 || (prop == 
config->prop_fb_id && prop_value == 0))
+  drm_dbg_atomic(prop->dev,
+ "[PLANE:%d:%s] continue async as there is no prop change\n",
+                                                      obj->id, 
plane->name);
+                               else
+                                       ret = -EINVAL;

                                 if (plane_funcs && 
plane_funcs->atomic_async_check)

Thanks and Regards,
Arun R Murthy
--------------------

>   and skips
> the async checks if the plane was and still is not visible.
>
> Fixes: fd40a63c drm/atomic (Let drivers decide which planes to async flip)
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4263
>
> Signed-off-by: Xaver Hugl <xaver.hugl at kde.org>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 51 +++++++++++++++++++++----------
>   1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c2726af6698e..2ae41a522e92 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1068,7 +1068,6 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   		struct drm_plane *plane = obj_to_plane(obj);
>   		struct drm_plane_state *plane_state;
>   		struct drm_mode_config *config = &plane->dev->mode_config;
> -		const struct drm_plane_helper_funcs *plane_funcs = plane->helper_private;
>   
>   		plane_state = drm_atomic_get_plane_state(state, plane);
>   		if (IS_ERR(plane_state)) {
> @@ -1084,21 +1083,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   				ret = drm_atomic_plane_get_property(plane, plane_state,
>   								    prop, &old_val);
>   				ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
> -			}
> -
> -			/* ask the driver if this non-primary plane is supported */
> -			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> -				ret = -EINVAL;
> -
> -				if (plane_funcs && plane_funcs->atomic_async_check)
> -					ret = plane_funcs->atomic_async_check(plane, state, true);
> -
> -				if (ret) {
> -					drm_dbg_atomic(prop->dev,
> -						       "[PLANE:%d:%s] does not support async flips\n",
> -						       obj->id, plane->name);
> -					break;
> -				}
> +				if (ret)
> +				    break;
>   			}
>   		}
>   
> @@ -1394,6 +1380,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>   	int ret = 0;
>   	unsigned int i, j, num_fences;
>   	bool async_flip = false;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state = NULL;
> +	struct drm_plane_state *new_plane_state = NULL;
> +	u64 fb_id = 0;
>   
>   	/* disallow for drivers not supporting atomic: */
>   	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1521,6 +1511,35 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>   			copied_props++;
>   		}
>   
> +		if (async_flip && obj->type == DRM_MODE_OBJECT_PLANE &&
> +		    obj_to_plane(obj)->type != DRM_PLANE_TYPE_PRIMARY) {
> +			/* need to ask the driver if this plane is supported */
> +			plane = obj_to_plane(obj);
> +			old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +			new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +			ret = drm_atomic_plane_get_property(plane, new_plane_state,
> +							    dev->mode_config.prop_fb_id,
> +							    &fb_id);
> +			if (ret)
> +				break;
> +			/*
> +			 * Only do the check if the plane was or is enabled.
> +			 * Note that the new state doesn't have "visible" set yet,
> +			 * so this uses fb_id instead.
> +			 */
> +			if (old_plane_state->visible || fb_id)
> +				ret = -EINVAL;
> +			if (ret && plane->helper_private &&
> +			    plane->helper_private->atomic_async_check) {
> +				ret = plane->helper_private->atomic_async_check(plane, state, true);
> +			}
> +			if (ret) {
> +				drm_dbg_atomic(dev, "[PLANE:%d:%s] does not support async flips\n",
> +						obj->id, plane->name);
> +				break;
> +			}
> +		}
> +
>   		drm_mode_object_put(obj);
>   	}
>