[Intel-gfx] [RFC 3/9] drm/i915: Convert requests to use struct fence

[Tvrtko Ursulin]

On 07/28/2015 11:10 AM, John Harrison wrote:
[snip]
>>>   static inline void
>>> @@ -2267,7 +2284,7 @@ i915_gem_request_unreference__unlocked(struct
>>> drm_i915_gem_request *req)
>>>           return;
>>>
>>>       dev = req->ring->dev;
>>> -    if (kref_put_mutex(&req->ref, i915_gem_request_free,
>>> &dev->struct_mutex))
>>> +    if (kref_put_mutex(&req->fence.refcount, fence_release,
>>> &dev->struct_mutex))
>>>           mutex_unlock(&dev->struct_mutex);
>>
>> Would it be nicer to add fence_put_mutex(struct fence *, struct mutex
>> *) for this? It would avoid the layering violation of requests peeking
>> into fence implementation details.
>
> Maarten pointed out that adding 'fence_put_mutex()' is breaking the
> fence ABI itself. It requires users of any random fence to know and
> worry about what mutex objects that fence's internal implementation
> might require.

I don't see what it has to do with the ABI? I dislike both approaches 
actually and don't like the question of what is the lesser evil. :)

> This is a bit more hacky from our point of view but it is only a
> temporary measure until the mutex lock is not required for
> dereferencing. At that point all the nasty stuff disappears completely.

Yes saw that in later patches. No worries then, just a consequence of 
going patch by patch.

>>> +
>>>       if (req->file_priv)
>>>           i915_gem_request_remove_from_client(req);
>>>
>>> @@ -2637,6 +2639,47 @@ void i915_gem_request_free(struct kref *req_ref)
>>>       kmem_cache_free(req->i915->requests, req);
>>>   }
>>>
>>> +static const char *i915_gem_request_get_driver_name(struct fence
>>> *req_fence)
>>> +{
>>> +    return "i915_request";
>>> +}
>>
>> I think this becomes kind of ABI once added so we need to make sure
>> the best name is chosen to start with. I couldn't immediately figure
>> out why not just "i915"?
>
> The thought was that we might start using fences for other purposes at
> some point in the future. At which point it is helpful to differentiate
> them. If nothing else, a previous iteration of the native sync
> implementation was using different fence objects due to worries about
> allowing user land to get its grubby mitts on important driver internal
> structures.

I don't follow on the connection between these names and the last 
concern? If I did, would it explain why driver name "i915" and 
differentiation by timeline names would be problematic? Looks much 
cleaner to me.

>>> +
>>> +static const char *i915_gem_request_get_timeline_name(struct fence
>>> *req_fence)
>>> +{
>>> +    struct drm_i915_gem_request *req = container_of(req_fence,
>>> +                         typeof(*req), fence);
>>> +    return req->ring->name;
>>> +}
>>> +
>>> +static bool i915_gem_request_enable_signaling(struct fence *req_fence)
>>> +{
>>> +    /* Interrupt driven fences are not implemented yet.*/
>>> +    WARN(true, "This should not be called!");
>>> +    return true;
>>
>> I suppose WARN is not really needed in the interim patch. Would return
>> false work?
>
> The point is to keep the driver 'safe' if that patch is viewed as stand
> alone. I.e., if the interrupt follow up does not get merged immediately
> after (or not at all) then this prevents people accidentally trying to
> use an unsupported interface without first implementing it.

I assumed true means "signaling enabled successfully" but "false" 
actually means "fence already passed" so you are right. I blame the 
un-intuitive API. :)

>>> +    fence_base = fence_context_alloc(I915_NUM_RINGS);
>>> +
>>>       /* Now it is safe to go back round and do everything else: */
>>>       for_each_ring(ring, dev_priv, i) {
>>>           struct drm_i915_gem_request *req;
>>>
>>>           WARN_ON(!ring->default_context);
>>>
>>> +        ring->fence_context = fence_base + i;
>>
>> Could you store fence_base in dev_priv and then ring->init_hw could
>> set up the fence_context on its own?
>
> Probably. It seemed to make more sense to me to keep the fence
> allocation all in one place rather than splitting it in half and
> distributing it. It gets removed again with the per context per ring
> timelines anyway.

Yes saw that later, never mind then.

Tvrtko