[Intel-gfx] [PATCH 05/12] drm/i915/scheduler: Record all dependencies upon request construction

[Tvrtko Ursulin]

On 07/11/2016 13:39, Chris Wilson wrote:
> On Mon, Nov 07, 2016 at 01:30:43PM +0000, Tvrtko Ursulin wrote:
>>
>> On 07/11/2016 09:30, Chris Wilson wrote:
>>> On Mon, Nov 07, 2016 at 09:12:57AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 04/11/2016 15:11, Chris Wilson wrote:
>>>>> On Fri, Nov 04, 2016 at 02:44:44PM +0000, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 03/11/2016 11:55, Chris Wilson wrote:
>>>>>>> On Thu, Nov 03, 2016 at 11:03:47AM +0000, Tvrtko Ursulin wrote:
>>>>>>>>
>>>>>>>> On 02/11/2016 17:50, Chris Wilson wrote:
>>>>>>>>> +struct i915_dependency {
>>>>>>>>> +	struct i915_priotree *signal;
>>>>>>>>> +	struct list_head pre_link, post_link;
>>>>>>>>> +	unsigned long flags;
>>>>>>>>> +#define I915_DEPENDENCY_ALLOC BIT(0)
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +struct i915_priotree {
>>>>>>>>> +	struct list_head pre_list; /* who is before us, we depend upon */
>>>>>>>>> +	struct list_head post_list; /* who is after us, they depend upon us */
>>>>>>>>> +};
>>>>>>>>
>>>>>>>> I need a picture to imagine this data structure. :(
>>>>>>>
>>>>>>> The names suck.
>>>>>>
>>>>>> When you wrote this I assumed you would respin shortly with some
>>>>>> better names?
>>>>>
>>>>> Not yet. I kind of like
>>>>>
>>>>> struct i915_dependency {
>>>>> 	struct i915_priotree *signaler;
>>>>> 	struct list_head signaler_link;
>>>>> 	struct list_head listener_link;
>>>>> };
>>>>>
>>>>> struct i915_priotree {
>>>>> 	struct list_head signalers_list; /* before us, we depend on them */
>>>>> 	struct list_head listeners_list; /* those after, who depend on us */
>>>>> };
>>>>>
>>>>
>>>> What is the signaler in i915_dependency?
>>>
>>> That would be the actual dependency. The fences have a notion of
>>> waiters, but we need to track the signalers in order to perform PI.
>>> Fwiw,
>>>
>>> +struct i915_dependency {
>>> +       struct i915_priotree *signaler;
>>> +       struct list_head signal_link;
>>> +       struct list_head wait_link;
>>> +       unsigned long flags;
>>> +#define I915_DEPENDENCY_ALLOC BIT(0)
>>> +};
>>> +
>>> +/* Requests exist in a complex web of interdependencies. Each request
>>> + * has to wait for some other request to complete before it is ready to be run
>>> + * (e.g. we have to wait until the pixels have been rendering into a texture
>>> + * before we can copy from it). We track the readiness of a request in terms
>>> + * of fences, but we also need to keep the dependency tree for the lifetime
>>> + * of the request (beyond the life of an individual fence). We use the tree
>>> + * at various points to reorder the requests whilst keeping the requests
>>> + * in order with respect to their various dependencies.
>>> + */
>>> +struct i915_priotree {
>>> +       struct list_head signalers_list; /* those before us, we depend upon */
>>> +       struct list_head waiters_list; /* those after us, they depend upon us */
>>> +};
>>>
>>>
>>
>> req->depq is just an optimisation to avoid one allocation in the
>> common case?
>
> Because we can't handle an allocation failure at the point of use, so we
> need to preallocate it at the time of request construction - so easiest
> to stuff it into the request (and reasonable since we almost always need
> to use - the only time we don't is on the very first request on a new
> timeline).

Yes, all clear now. Think I've figured it out. :)

Awaiting for a new series now.

Regards,

Tvrtko