Re: [PATCH 07/27] drm/i915: Coordinate i915_active with its own mutex

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

On 27/09/2019 13:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-27 13:25:23)
>>
>> On 27/09/2019 13:16, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-09-27 13:08:51)
>>>>
>>>> On 27/09/2019 12:25, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-09-27 12:10:29)
>>>>>>
>>>>>> On 25/09/2019 11:01, Chris Wilson wrote:
>>>>>>> +struct dma_fence *
>>>>>>> +__i915_active_fence_set(struct i915_active_fence *active,
>>>>>>> +                     struct dma_fence *fence)
>>>>>>
>>>>>> Can you add a short comment for this function explaining the racyness
>>>>>> and so why it returns prev and what should the callers do with it?
>>>>>
>>>>> Before using this function, we had the callers declare what mutex guards
>>>>> this timeline and we check here that is held.
>>>>
>>>> No, I mean because it has to reload prev and return it to the caller,
>>>> which implies that is to handle some designed-in racy-ness which I think
>>>> should be mentioned.
>>>
>>> But that's not racing with the caller, that just racing with the
>>> callback from the interrupt handler and the reason why we have to check
>>> after serialising is called out. /* serialise with prev->cb_list */ ?
>>>
>>> The caller is responsible for ensuring that prev is executed before
>>> fence to keep the timeline in the same order as recorded.
>>
>> I did not say it is racing with the caller, but that the caller has to
>> handle a race.
> 
> But the caller only has to care about "was there already an active fence
> on this timeline? If so, I must make sure it executes before me and take
> that into consideration for the flow along the timeline"
> 
> I'm not seeing what race the caller has to be concerned with here. If
> they replace the last request in the timeline, they have it returned.
> If there was no request previously in the timeline, they have NULL.
> (That just seems straightforward.)
>   
>> "Serialise with prev->cb_list" is too low level for me. Trust me, I
>> always struggle with the active tracking code since there is so many
>> indirections and relationships. So in the absence of a visual diagram a
>> higher level comment would be beneficial for the future self. Just
>> expanding on what you replied here talking about how interrupts
>> interacts with new stuff entering tracking would do it.
> 
> It's just about that the callback may be executing and so we need to
> serialise the list manipulation under the lock; having performed that
> list manipulation, we then know whether or not we were successful in
> capturing the previous fence.

Ok ok :), just expand the comment next to re-fetch of prev = 
active->fence to that effect, that's all I'm asking. :)

It will make it easier for future reader to understand why it is 
required and under what circumstances could active->fence have became 
NULL in there.

Regards,

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