[PATCH v3 0/6] Add support for atomic async page-flips

[André Almeida]

On 10/13/22 13:02, Simon Ser wrote:
>>>> So no tests that actually verify that the kernel properly rejects
>>>> stuff stuff like modesets, gamma LUT updates, plane movement,
>>>> etc.?
>>>
>>> Pondering this a bit more, it just occurred to me the current driver
>>> level checks might easily lead to confusing behaviour. Eg. is
>>> the ioctl going to succeed if you ask for an async change of some
>>> random property while the crtc disabled, but fails if you ask for
>>> the same async property change when the crtc is active?
>>>
>>> So another reason why rejecting most properties already at
>>> the uapi level might be a good idea.
>>
>> And just to be clear this is pretty much what I suggest:
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 79730fa1dd8e..471a2c703847 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   				goto out;
>>   			}
>>
>> +			if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
>> +			    prop != dev->mode_config.prop_fb_id) {
>> +				drm_mode_object_put(obj);
>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>>   			if (copy_from_user(&prop_value,
>>   					   prop_values_ptr + copied_props,
>>   					   sizeof(prop_value))) {
>>
>>
>> That would actively discourage people from even attempting the
>> "just dump all the state into the ioctl" approach with async flips
>> since even the props whose value isn't even changing would be rejected.
> 
> How does this sound?
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 945761968428..ffd16bdc7b83 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -972,14 +972,26 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   			    struct drm_file *file_priv,
>   			    struct drm_mode_object *obj,
>   			    struct drm_property *prop,
> -			    uint64_t prop_value)
> +			    uint64_t prop_value,
> +			    bool async_flip)
>   {
>   	struct drm_mode_object *ref;
>   	int ret;
> +	uint64_t old_val;
>   
>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>   		return -EINVAL;
>   
> +	if (async_flip && prop != prop->dev->mode_config.prop_fb_id) {
> +		ret = drm_atomic_get_property(obj, prop, &old_val);
> +		if (ret != 0 || old_val != prop_value) {
> +			drm_dbg_atomic(prop->dev,
> +				       "[PROP:%d:%s] cannot be changed during async flip\n",
> +				       prop->base.id, prop->name);

I would write this as "[PROP:%d:%s] No prop can be changed during async 
flips" to make it clear that it's not just this prop that can't, but any.

> +			return -EINVAL;
> +		}
> +	}
> +
>   	switch (obj->type) {
>   	case DRM_MODE_OBJECT_CONNECTOR: {
>   		struct drm_connector *connector = obj_to_connector(obj);