Re: [PATCH v7 3/5] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl

[Louis Chauvet]

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 <arun.r.mur...@intel.com>
---
  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?

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

Thanks,
Louis Chauvet


        if (ret == -EDEADLK) {
                drm_atomic_state_clear(state);