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

[Lionel Landwerlin]

On 03/07/2019 11:56, Chris Wilson wrote:
> 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!


You're right, I got this wrong.

We can get fence = NULL if the point is already signaled.

The easiest would be to skip it from the list, or add the stub fence.


I guess the CTS got lucky that it always got the point needed before it 
was garbage collected...


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


Adding points < latest added point is forbidden.

I wish we enforced it a bit more than what's currently done in 
drm_syncobj_add_point().

In my view we should :

     - lock the syncobj in get_timeline_fence_array() do the sanity 
check there.

     - keep the lock until we add the point to the timeline

     - unlock once added


That way we would ensure that the application cannot generate invalid 
timelines and error out if it does.

We could do the same for host signaling in 
drm_syncobj_timeline_signal_ioctl/drm_syncobj_transfer_to_timeline 
(there the locking a lot shorter).

That requires holding the lock for longer than maybe other driver would 
prefer.


Ccing Christian who can tell whether that's out of question for AMD.


Cheers,


-Lionel


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