[Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support

[Chris Wilson]

Quoting Lionel Landwerlin (2019-07-01 12:34:33)
> +               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);

I'm very dubious about chain_find_seqno().

It returns -EINVAL if the point is older than the first in the chain --
it is in an unknown state, but may be signaled since we remove signaled
links from the chain. If we are waiting for an already signaled syncpt,
we should not be erring out!

Do we allow later requests to insert earlier syncpt into the chain? If
so, then the request we wait on here may be woefully inaccurate and
quite easily lead to cycles in the fence tree. We have no way of
resolving such deadlocks -- we would have to treat this fence as a
foreign fence and install a backup timer. Alternatively, we only allow
this to return the exact fence for a syncpt, and proxies for the rest.

> +                       if (err || !fence) {
> +                               DRM_DEBUG("Syncobj handle missing requested point\n");
> +                               drm_syncobj_put(syncobj);
> +                               err = err != 0 ? err : -EINVAL;
> +                               goto err;
> +                       }
> +               }
> +
> +               /*
> +                * For timeline syncobjs we need to preallocate chains for
> +                * later signaling.
> +                */
> +               if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) {
> +                       fences[n].chain_fence =
> +                               kmalloc(sizeof(*fences[n].chain_fence),
> +                                       GFP_KERNEL);
> +                       if (!fences[n].chain_fence) {
> +                               dma_fence_put(fence);
> +                               drm_syncobj_put(syncobj);
> +                               err = -ENOMEM;
> +                               DRM_DEBUG("Unable to alloc chain_fence\n");
> +                               goto err;
> +                       }

What happens if we later try to insert two fences for the same syncpt?
Should we not reserve the slot in the chain to reject duplicates?
-Chris