Re: [Intel-gfx] [PATCH 4/4] drm/i915: Implement SINGLE_TIMELINE with a syncobj

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/03/2021 16:10, Jason Ekstrand wrote:
> On Mon, Mar 22, 2021 at 7:28 AM Tvrtko Ursulin

[snip]

>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> index 96403130a373d..2c56796f6a71b 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> @@ -3295,6 +3295,15 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>>>                goto err_vma;
>>>        }
>>>
>>> +     if (eb.gem_context->syncobj) {
>>> +             struct dma_fence *fence;
>>> +
>>> +             fence = drm_syncobj_fence_get(eb.gem_context->syncobj);
>>
>> Who drops this reference?
> 
> i915_request_await_dma_fence() below consumes a reference.

Not sure, please check on difference wrt input fence handling.

>>> +             err = i915_request_await_dma_fence(eb.request, fence);
>>> +             if (err)
>>> +                     goto err_ext;
>>> +     }
>>> +
>>>        if (in_fence) {
>>>                if (args->flags & I915_EXEC_FENCE_SUBMIT)
>>>                        err = i915_request_await_execution(eb.request,
>>> @@ -3351,6 +3360,12 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>>>                        fput(out_fence->file);
>>>                }
>>>        }
>>> +
>>> +     if (eb.gem_context->syncobj) {
>>> +             drm_syncobj_replace_fence(eb.gem_context->syncobj,
>>> +                                       &eb.request->fence);
>>> +     }
>>> +
>>>        i915_request_put(eb.request);
>>>
>>>    err_vma:
>>>
>>
>> So essentially moving the synchronisation to top level which is extra
>> work, but given limited and questionable usage of the uapi may be
>> acceptable. Need full picture on motivation to understand.
> 
> For one thing, the GuC scheduler doesn't natively have a concept of
> "timelines" which can be shared like this.  To work with the GuC

Confused - neither does execlists. It is handled in common layer in 
i915. GuC scheduler has the same concept of one hw context is one 
timeline because that is the hw concept and not backend specific.

> scheduler as currently proposed in DII, they've asked the media driver
> to stop using this flag in favor of passing a sync file from batch to
> batch.  If we want to slide GuC scheduling in smoothly, we've got to
> keep it working.  This means either making timelines a concept there
> or doing an emulation like this.

Hm not aware and don't see that GuC backend can't or doesn't implement 
this. Perhaps this would be best discussed once GuC patches are posted.

>> Semantics are also not 1:1 since dma fence context will be different.
> 
> Could you elaborate?

Exported dma fence context as an "timeline" id will be single with the 
current patch and multiple contexts with this implementation.

Daniel also raised another difference caused by lack of serialisation 
due multiple tl->mutex here.

I don't think this is important, it was never part of a contract what 
happens with racing execbufs, but it is definitely required covering 
both topics in the commit message.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx