[PATCH v2] drm: don't run atomic_async_check for disabled planes
Murthy, Arun R
arun.r.murthy at intel.com
Fri Aug 1 05:09:42 UTC 2025
On 31-07-2025 20:47, 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, 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 +++++++++++++++++++----------
> drivers/gpu/drm/drm_crtc_internal.h | 3 +-
> drivers/gpu/drm/drm_mode_object.c | 3 +-
> 3 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index c2726af6698e..df298ac49dcd 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1011,7 +1011,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> struct drm_mode_object *obj,
> struct drm_property *prop,
> u64 prop_value,
> - bool async_flip)
> + bool async_flip,
> + bool *needs_async_plane_check)
> {
> struct drm_mode_object *ref;
> u64 old_val;
> @@ -1068,7 +1069,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,22 +1084,14 @@ 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);
> + if (ret)
> + break;
> }
>
> - /* 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;
> - }
> - }
> + /* Need to ask the driver if this non-primary plane is supported.
> + * Note that this can't happen here, as the full state of the plane
> + * is not known yet */
Multiple lines comment stye
/*
* <text here>
*/
> + *needs_async_plane_check |= plane->type != DRM_PLANE_TYPE_PRIMARY;
> }
>
> ret = drm_atomic_plane_set_property(plane,
> @@ -1394,6 +1386,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> int ret = 0;
> unsigned int i, j, num_fences;
> bool async_flip = false;
> + bool needs_async_plane_check = false;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state;
> + struct drm_plane_state *new_plane_state;
>
> /* disallow for drivers not supporting atomic: */
> if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1450,6 +1446,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> copied_props = 0;
> fence_state = NULL;
> num_fences = 0;
> + needs_async_plane_check = false;
This is already set to false in the declaration.
> for (i = 0; i < arg->count_objs; i++) {
> uint32_t obj_id, count_props;
> @@ -1512,7 +1509,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> }
>
> ret = drm_atomic_set_property(state, file_priv, obj,
> - prop, prop_value, async_flip);
> + prop, prop_value, async_flip,
> + &needs_async_plane_check);
> if (ret) {
> drm_mode_object_put(obj);
> goto out;
> @@ -1521,6 +1519,25 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> copied_props++;
> }
>
> + if (needs_async_plane_check) {
> + 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);
> + /* only do the check if the plane was or is enabled */
> + if (old_plane_state->visible || new_plane_state->visible)
As said in my earlier comment, this new_state->visible is not yet
populated. It will be viable to have these after atomic_check where
state->visible gets updated.
Instead fb can be checked to see if its changed to NULL then it means
disable the plane and instead of rejecting the change, can proceed.
These kind of checks will be much easier with having the proposed change
in RFC https://patchwork.freedesktop.org/series/150081/
Thanks and Regards,
Arun R Murthy
--------------------
> + 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);
> }
>
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 89706aa8232f..111907a55d9b 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -260,7 +260,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> struct drm_file *file_priv,
> struct drm_mode_object *obj,
> struct drm_property *prop,
> - u64 prop_value, bool async_flip);
> + u64 prop_value, bool async_flip,
> + bool *needs_async_plane_check);
> 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 e943205a2394..ec471536cfbc 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -540,7 +540,8 @@ 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, false);
> + ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value,
> + false, NULL);
> if (ret)
> goto out;
> ret = drm_atomic_commit(state);
More information about the amd-gfx
mailing list