[PATCH v4 1/1] drm/syncobj: add sideband payload

[Lionel Landwerlin]

On 09/08/2019 14:58, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-08-09 12:30:30)
>> +int drm_syncobj_binary_ioctl(struct drm_device *dev, void *data,
>> +                            struct drm_file *file_private)
>> +{
>> +       struct drm_syncobj_binary_array *args = data;
>> +       struct drm_syncobj **syncobjs;
>> +       u32 __user *access_flags = u64_to_user_ptr(args->access_flags);
>> +       u64 __user *values = u64_to_user_ptr(args->values);
>> +       u32 i;
>> +       int ret;
>> +
>> +       if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
>> +               return -EOPNOTSUPP;
>> +
>> +       if (args->pad != 0)
>> +               return -EINVAL;
>> +
>> +       if (args->count_handles == 0)
>> +               return -EINVAL;
> You may find it easier to just return success for 0 handles. Slightly less
> obnoxious error handling?


All the other ioctls in this file return EINVAL in that case. I'm just 
going for consistency.

It's also a good indication for the application it can save itself an 
ioctl really :)


>
>> +       ret = drm_syncobj_array_find(file_private,
>> +                                    u64_to_user_ptr(args->handles),
>> +                                    args->count_handles,
>> +                                    &syncobjs);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       for (i = 0; i < args->count_handles; i++) {
>> +               u32 flags;
>> +
>> +               copy_from_user(&flags, &access_flags[i], sizeof(flags));
>> +               ret = ret ? -EFAULT : 0;
> Magic?
>
> if (get_user(flags, &access_flags[i[))
> 	return -EFAULT;


I give this no testing, I'm just trying to get some feedback about the 
direction.

Thanks though :)


>
>> +               if (ret)
>> +                       break;
>> +
>> +               if (flags & I915_DRM_SYNCOBJ_BINARY_ITEM_VALUE_READ) {
>> +                       copy_to_user(&values[i], &syncobjs[i]->binary_payload, sizeof(values[i]));
>> +                       ret = ret ? -EFAULT : 0;
> More magic.
>
> if (put_user(&syncobjs[i]->binary_payload, &values[i]))
> 	return -EFAULT;
>
>> +                       if (ret)
>> +                               break;
>> +               }
>> +
>> +               if (flags & I915_DRM_SYNCOBJ_BINARY_ITEM_VALUE_INC)
>> +                       syncobjs[i]->binary_payload++;
> So if an error occurs how does the user know which syncobj were
> advanced before the error? (Or explain why it doesn't actually matter)
> The clue I guess is with read/inc, but confirmation of design would be
> nice.


I guess we could toggle the access flag bits to notify that the actions 
were completed.


>
> Not atomic (the u64 write should really be to avoid total corruption)
> and nothing prevents userspace from racing. How safe is that in the
> overall design?


Atomic would prevent issue related to 2 processes/threads seeing 
different values because of caching?


If not then it's not really interesting for the use case. The increment 
should happen during the vkQueueSubmit() call and the value is only 
valid upon returning.

The application is responsible for not having 
vkQueueSubmit()/vkWaitForFences() race.


Not opposed to switch to atomic though.


>
> What would happen if the binary_payload was initialised to -1?


The 0 value is problematic because it's also used for "whatever fence in 
the syncobj".

I think we need to stick to the same rules as the timeline values : 0 is 
always signaled


Thanks,


-Lionel


>
>> +       }
>> +
>>          drm_syncobj_array_free(syncobjs, args->count_handles);
>>   
>>          return ret;