[Intel-gfx] [PATCH v3 04/63] drm/i915: Pin timeline map after first timeline pin, v3.

[Thomas Hellström (Intel)]

On 10/27/20 3:31 PM, Maarten Lankhorst wrote:
> Op 27-10-2020 om 12:03 schreef Thomas Hellström (Intel):
>> On 10/15/20 1:25 PM, Maarten Lankhorst wrote:
>>> We're starting to require the reservation lock for pinning,
>>> so wait until we have that.
>>>
>>> Update the selftests to handle this correctly, and ensure pin is
>>> called in live_hwsp_rollover_user() and mock_hwsp_freelist().
>>>
>>> Changes since v1:
>>> - Fix NULL + XX arithmatic, use casts. (kbuild)
>>> Changes since v2:
>>> - Clear entire cacheline when pinning.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> Reported-by: kernel test robot <lkp at intel.com>
>> ...
>>> @@ -150,6 +161,12 @@ int intel_timeline_pin(struct intel_timeline *tl, struct i915_gem_ww_ctx *ww)
>>>        if (atomic_add_unless(&tl->pin_count, 1, 0))
>>>            return 0;
>>>    +    if (!tl->hwsp_map) {
>>> +        err = intel_timeline_pin_map(tl);
>>> +        if (err)
>>> +            return err;
>>> +    }
>>> +
>> On subsequent errors or if somebody beats us to the 0->1 transition, we need to unpin_map to avoid leaking pins.
> No? tl->hwsp_map can stay set. We hold a lock to tl->hwsp_ggtt to prevent any races. :)

OK, but in that case we need to be consistent. Either there are no races 
and we can remove the code that handles them, or there are races and we 
should take this into account.

>
> ~Maarten
>
>>>        err = i915_ggtt_pin(tl->hwsp_ggtt, ww, 0, PIN_HIGH);
>>>        if (err)
>>>            return err;
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.h b/drivers/gpu/drm/i915/gt/intel_timeline.h
>>> index 9882cd911d8e..1cfdc4679b62 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_timeline.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.h
>>> @@ -106,4 +106,6 @@ int intel_timeline_read_hwsp(struct i915_request *from,
>>>    void intel_gt_init_timelines(struct intel_gt *gt);
>>>    void intel_gt_fini_timelines(struct intel_gt *gt);
>>>    +I915_SELFTEST_DECLARE(int intel_timeline_pin_map(struct intel_timeline *tl));
>>> +
>>>    #endif
>>> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
>>> index 2f830017c51d..effbac877eec 100644
>>> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
>>> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
>>> @@ -32,9 +32,20 @@
>>>    #include "mock_engine.h"
>>>    #include "selftests/mock_request.h"
>>>    -static void mock_timeline_pin(struct intel_timeline *tl)
>>> +static int mock_timeline_pin(struct intel_timeline *tl)
>>>    {
>>> +    int err;
>>> +
>>> +    if (WARN_ON(!i915_gem_object_trylock(tl->hwsp_ggtt->obj)))
>>> +        return -EBUSY;
>> I think we should either annotate this properly as an isolated lock, or allow a silent -EBUSY.
> This is done in a controlled selftest where we mock the entire i915 device, so normally this can't happen. :)

But if it does, we get erratic BAT failures, and if it indeed doesn't 
happen, then an annotated isolated lock would guarantee that.

/Thomas