Thanks for the review and sorry I missed to reply back on this!
On 07-01-2026 16:33, Louis Chauvet wrote:
On 1/6/26 05:37, Arun R Murthy wrote:
Move atomic_state allocation to the beginning of the atomic_ioctl
to accommodate drm_mode_atomic_err_code usage for returning error
code on failures.
As atomic state is required for drm_mode_atomic_err_code to store the
error codes.
v7: Reframe commit message (Suraj)
Signed-off-by: Arun R Murthy <[email protected]>
---
drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index
7320db4b8489f10e24ed772094c77e2172951633..02029b5d7832eeaf4a225096a94947344083fc0b
100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1553,13 +1553,21 @@ int drm_mode_atomic_ioctl(struct drm_device
*dev,
struct drm_modeset_acquire_ctx ctx;
struct drm_out_fence_state *fence_state;
int ret = 0;
- unsigned int i, j, num_fences;
+ unsigned int i, j, num_fences = 0;
bool async_flip = false;
/* disallow for drivers not supporting atomic: */
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return -EOPNOTSUPP;
+ state = drm_atomic_state_alloc(dev);
+ if (!state)
+ return -ENOMEM;
It seems strange to add num_fences = 0 at the top and then don't use
it before the num_fences = 0. Did you forgot to replace return -ENOMEM
by goto out?
As part of this series in places where we return on error has been
changed to goto out, cleanup and then return with error code. In cleanup
we will be completing the fences. Hence setting this num_fences to 0
initially.
+
+ drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
+ state->acquire_ctx = &ctx;
+ state->allow_modeset = !!(arg->flags &
DRM_MODE_ATOMIC_ALLOW_MODESET);
+
/* disallow for userspace that has not enabled atomic cap (even
* though this may be a bit overkill, since legacy userspace
* wouldn't know how to call this ioctl)
@@ -1598,13 +1606,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
return -EINVAL;
}
- state = drm_atomic_state_alloc(dev);
- if (!state)
- return -ENOMEM;
-
- drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
- state->acquire_ctx = &ctx;
- state->allow_modeset = !!(arg->flags &
DRM_MODE_ATOMIC_ALLOW_MODESET);
state->plane_color_pipeline = file_priv->plane_color_pipeline;
retry:
@@ -1703,7 +1704,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
}
out:
- complete_signaling(dev, state, fence_state, num_fences, !ret);
+ if (num_fences)
+ complete_signaling(dev, state, fence_state, num_fences, !ret);
Hello Arun,
I am not familiar with this part of DRM, but this num_fences change
seems strange and unrelated to this patch.
If this is intentional, I think this change the previous behavior:
num_fences = 0;
for (...) {
if (ret)
goto out;
}
ret = prepare_signaling(dev, state, arg, file_priv,
fence_state, &num_fences);
out:
complete_signaling(dev, state, fence_state, num_fences, !ret);
Without your change:
=> no error -> prepare_signaling/complete_signaling are called with
num_fences=0
=> error in prepare_signaling -> complete_signaling is called in all
cases
=> error in loop = complete_signaling without prepare_signaling (very
strange, is it your fix?)
With your change:
=> no error -> same
=> error in prepare_signaling -> depends on prepare_signaling, only if
num_fences!=0 (a bit strange, but maybe expected)
=> error in loop -> don't call complete_signaling
I don't know if the previous behavior is broken, but if this change is
needed, maybe you can extract it in a different patch?
Yes even prior to this change it seems to have some issues and this
patch/series is not trying to fix that.
In this series before the for loop on error condition instead of exiting
goto error with cleanup is added for which num_fences is set to 0 in the
beginning.
Sure will move this change out of this patch.
Thanks and Regards,
Arun R Murthy
-------------------
Thanks,
Louis Chauvet
if (ret == -EDEADLK) {
drm_atomic_state_clear(state);
