[Intel-gfx] [PATCH v5 06/10] drm/i915: add syncobj timeline support

[Lionel Landwerlin]

On 27/06/2019 13:59, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-06-27 11:49:56)
>> Thanks a lot for your comments.
>>
>> On 27/06/2019 12:15, Chris Wilson wrote:
>>>> +               syncobj = drm_syncobj_find(eb->file, user_fence.handle);
>>>> +               if (!syncobj) {
>>>> +                       DRM_DEBUG("Invalid syncobj handle provided\n");
>>>> +                       err = -EINVAL;
>>>> +                       goto err;
>>>> +               }
>>>> +
>>>> +               if (user_fence.flags & I915_EXEC_FENCE_WAIT) {
>>>> +                       fence = drm_syncobj_fence_get(syncobj);
>>>> +                       if (!fence) {
>>>> +                               DRM_DEBUG("Syncobj handle has no fence\n");
>>>> +                               drm_syncobj_put(syncobj);
>>>> +                               err = -EINVAL;
>>>> +                               goto err;
>>>> +                       }
>>>> +
>>>> +                       err = dma_fence_chain_find_seqno(&fence, point);
>>> Is an imported syncobj always a fence-chain?
>>>
>>> drm_syncobj_import_sync_file_fence() says no.
>>
>> drm_syncobj_import_sync_file_fence() would turn a timeline semaphore
>> into a binary semaphore by breaking the chain.
>>
>> drm_syncobj_transfer_to_timeline() is what you should use if you wish to
>> import a fence into the timeline.
> What I am worrying about is the legality of the user passing in a
> non-timeline fence here. It looks like the interface will itself
> generate non-timeline fences... Oh, ignore me, I overlooked the early
> return for !seqno.


I wrote that down in my reply then deleted it ;)


>
> So timelines are not allowed to use 0. Ever. That's a bit more of a hard
> rule than implied by the uapi.h :)


Anv turns any signal/wait on 0 into NOOP, those never reach i915.

The spec is pretty clear that point 0 is always signaled.


I'll add a statement with regard to timeline semaphores.


Thanks!


-Lionel


>
>>>> @@ -1014,9 +1020,40 @@ struct drm_i915_gem_exec_fence {
>>>>    };
>>>>    
>>>>    enum drm_i915_gem_execbuffer_ext {
>>>> +       /**
>>>> +        * This identifier is associated with
>>>> +        * drm_i915_gem_execbuf_ext_timeline_fences.
>>> Or just "See drm_i915_gem_execbuf_ext_timeline_fences
>>>> +        */
>>>> +       DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES,
>>>    = 0
>>> don't leave it up to the compiler.
>>
>> Should we set every single item in the enum or just the first one?
> We've been setting the first one to be explicit (doubly useful when we
> want a certain value to be documented to be 0), and then gaps. The spec
> says that each id is incremented by one from the previous, it's just the
> first value and natural type of the enum is up to the compiler.
> -Chris
>