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

["Murthy, Arun R" <arun.r.murthy@intel.com>]

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 <arun.r.murthy@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?
>
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);
>>
>