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

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

[Xaver Hugl]

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@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);
 	}
 
-- 
2.50.1

Re: [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@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);
>   	}
>   

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

[Xaver Hugl]

Am Mo., 4. Aug. 2025 um 11:54 Uhr schrieb Murthy, Arun R
<arun.r.murthy@intel.com>:
>
> 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))
This would allow disabling a plane in an async commit that was
previously enabled, not sure that should be allowed? Also, if the
property is fb_id, ret would be used uninitialized. But you're right,
this should be fixable with smaller changes. Probably best to keep it
minimal for the bugfix.

Looking more at this code, I also notice that it currently allows you
to change *any* property on overlay planes in an async flip, which
doesn't seem right.

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

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

[Kumar, Naveen1]

Hi Xaver,

>-----Original Message-----
>From: Xaver Hugl <xaver.hugl@kde.org>
>Sent: Tuesday, August 5, 2025 3:03 AM
>To: Murthy, Arun R <arun.r.murthy@intel.com>
>Cc: dri-devel@lists.freedesktop.org; andrealmeid@igalia.com;
>chris@kode54.net; Kumar, Naveen1 <naveen1.kumar@intel.com>;
>ville.syrjala@linux.intel.com; mdaenzer@redhat.com; intel-
>gfx@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
>alexdeucher@gmail.com
>Subject: Re: [PATCH v3] drm: don't run atomic_async_check for disabled planes
>
>Am Mo., 4. Aug. 2025 um 11:54 Uhr schrieb Murthy, Arun R
><arun.r.murthy@intel.com>:
>>
>> 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))
>This would allow disabling a plane in an async commit that was previously
>enabled, not sure that should be allowed? Also, if the property is fb_id, ret
>would be used uninitialized. But you're right, this should be fixable with smaller
>changes. Probably best to keep it minimal for the bugfix.

As I commented earlier in the gitlab issue [1], any change of property, including disabling a plane is not allowed in the async commit. 
We must disable a plane (e.g. HW cursor) during the first (synchronized) flip, and allowing later flips to proceed asynchronously. 
This change should be done in the compositor. As per Ville's opinion in related series [2], kernel driver should reject all these disabled
Planes in the drm core and driver should only get the planes which is supported with async flip. Based on his comment, I have started
Working and will be addressing it in the next version of my series [3].
	
[1]. https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13834#note_2994595 
[2]. https://lore.kernel.org/all/aHAg2J-uFLLWINqp@intel.com/
[3]. https://patchwork.freedesktop.org/series/151280/

Regards,
Naveen Kumar
>
>Looking more at this code, I also notice that it currently allows you to change
>*any* property on overlay planes in an async flip, which doesn't seem right.
>
>> +  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

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

[Murthy, Arun R]

On 05-08-2025 03:02, Xaver Hugl wrote:
> Am Mo., 4. Aug. 2025 um 11:54 Uhr schrieb Murthy, Arun R
> <arun.r.murthy@intel.com>:
>> 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))
> This would allow disabling a plane in an async commit that was
> previously enabled, not sure that should be allowed?
Yes, I assumes this is what you intended to do as you had if 
(curr_state->visible or prev_state->visible) in your 1st patchset changes.
If disabling of plane is not suppose to be allowed then prop_value == 0 
condition should be removed.
>   Also, if the
> property is fb_id, ret would be used uninitialized. But you're right,
> this should be fixable with smaller changes. Probably best to keep it
> minimal for the bugfix.
>
> Looking more at this code, I also notice that it currently allows you
> to change *any* property on overlay planes in an async flip, which
> doesn't seem right.

As part of ret = drm_atomci_check_prop_changes() ret will be -EINVAL.
ret should be checked for failure and returned over here.

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

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

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

[Xaver Hugl]

> As I commented earlier in the gitlab issue [1], any change of property, including disabling a plane is not allowed in the async commit.
> We must disable a plane (e.g. HW cursor) during the first (synchronized) flip, and allowing later flips to proceed asynchronously.
> This change should be done in the compositor.
No change is needed there, compositors already do that.
> As per Ville's opinion in related series [2], kernel driver should reject all these disabled
> Planes in the drm core and driver should only get the planes which is supported with async flip. Based on his comment, I have started
> Working and will be addressing it in the next version of my series [3].
As long as it only filters out planes that were and still are
disabled, I think that could work out fine - in theory they shouldn't
have any side effects.
Note though that my patch intends to specifically fix a regression for
amdgpu in 6.15, which I think we can do with fewer changes.

> [1]. https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13834#note_2994595
> [2]. https://lore.kernel.org/all/aHAg2J-uFLLWINqp@intel.com/
> [3]. https://patchwork.freedesktop.org/series/151280/
>
> Regards,
> Naveen Kumar