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

[Chris Wilson]

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.

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

> >> @@ -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