Op 2021-03-11 om 22:44 schreef Jason Ekstrand:
> On Thu, Mar 11, 2021 at 7:49 AM Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Reviewed-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/gt/intel_timeline.c | 40 +++++++++----
>> drivers/gpu/drm/i915/gt/intel_timeline.h | 2 +
>> drivers/gpu/drm/i915/gt/mock_engine.c | 22 ++++++-
>> drivers/gpu/drm/i915/gt/selftest_timeline.c | 63 +++++++++++----------
>> drivers/gpu/drm/i915/i915_selftest.h | 2 +
>> 5 files changed, 84 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
>> index efe2030cfe5e..032e1d1b4c5e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
>> @@ -52,14 +52,29 @@ static int __timeline_active(struct i915_active *active)
>> return 0;
>> }
>>
>> +I915_SELFTEST_EXPORT int
>> +intel_timeline_pin_map(struct intel_timeline *timeline)
>> +{
>> + struct drm_i915_gem_object *obj = timeline->hwsp_ggtt->obj;
>> + u32 ofs = offset_in_page(timeline->hwsp_offset);
>> + void *vaddr;
>> +
>> + vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
>> + if (IS_ERR(vaddr))
>> + return PTR_ERR(vaddr);
>> +
>> + timeline->hwsp_map = vaddr;
>> + timeline->hwsp_seqno = memset(vaddr + ofs, 0, CACHELINE_BYTES);
> What guarantees that hwsp_offset is cacheline-aligned? From what I
> saw in patch 1, it's incremented by 8 so only every 8th one is
> actually CL-aligned.
Yeah the name of the #define is wrong here. It was originally cacheline aligned because
the page was originally shared between different contexts. This is no longer the case,
but the name was kept. I think TIMELINE_SEQNO_ALIGN would be a better fit, I will respin.
>
>> + clflush(vaddr + ofs);
>> +
>> + return 0;
>> +}
>> +
>> static int intel_timeline_init(struct intel_timeline *timeline,
>> struct intel_gt *gt,
>> struct i915_vma *hwsp,
>> unsigned int offset)
>> {
>> - void *vaddr;
>> - u32 *seqno;
>> -
>> kref_init(&timeline->kref);
>> atomic_set(&timeline->pin_count, 0);
>>
>> @@ -76,14 +91,8 @@ static int intel_timeline_init(struct intel_timeline *timeline,
>> timeline->hwsp_ggtt = hwsp;
>> }
>>
>> - vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WB);
>> - if (IS_ERR(vaddr))
>> - return PTR_ERR(vaddr);
>> -
>> - timeline->hwsp_map = vaddr;
>> - seqno = vaddr + timeline->hwsp_offset;
>> - WRITE_ONCE(*seqno, 0);
>> - timeline->hwsp_seqno = seqno;
>> + timeline->hwsp_map = NULL;
>> + timeline->hwsp_seqno = (void *)(long)timeline->hwsp_offset;
> Maybe uintptr_t instead of long? I think they're always the same on
> Linux but uintptr_t seems like the more "correct" type for this sort
> of thing.
I did a non-scientific compare, void *)(uintptr_t vs void *)(long in the kernel, the latter was used 4x as often.
In general long is a pointer sized type for the kernel. I think for userspace it would be more correct, though.
>> GEM_BUG_ON(timeline->hwsp_offset >= hwsp->size);
>>
>> @@ -113,7 +122,8 @@ static void intel_timeline_fini(struct rcu_head *rcu)
>> struct intel_timeline *timeline =
>> container_of(rcu, struct intel_timeline, rcu);
>>
>> - i915_gem_object_unpin_map(timeline->hwsp_ggtt->obj);
>> + if (timeline->hwsp_map)
>> + i915_gem_object_unpin_map(timeline->hwsp_ggtt->obj);
>>
>> i915_vma_put(timeline->hwsp_ggtt);
>> i915_active_fini(&timeline->active);
>> @@ -173,6 +183,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;
>> + }
>> +
>> 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 b1f81d947f8d..57308c4d664a 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_timeline.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.h
>> @@ -98,4 +98,6 @@ intel_timeline_is_last(const struct intel_timeline *tl,
>> return list_is_last_rcu(&rq->link, &tl->requests);
>> }
>>
>> +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 5662f7c2f719..42fd86658ee7 100644
>> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
>> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
>> @@ -13,9 +13,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;
>> +
>> + err = intel_timeline_pin_map(tl);
>> + i915_gem_object_unlock(tl->hwsp_ggtt->obj);
>> + if (err)
>> + return err;
>> +
>> atomic_inc(&tl->pin_count);
>> + return 0;
>> }
>>
>> static void mock_timeline_unpin(struct intel_timeline *tl)
>> @@ -133,6 +144,8 @@ static void mock_context_destroy(struct kref *ref)
>>
>> static int mock_context_alloc(struct intel_context *ce)
>> {
>> + int err;
>> +
>> ce->ring = mock_ring(ce->engine);
>> if (!ce->ring)
>> return -ENOMEM;
>> @@ -143,7 +156,12 @@ static int mock_context_alloc(struct intel_context *ce)
>> return PTR_ERR(ce->timeline);
>> }
>>
>> - mock_timeline_pin(ce->timeline);
>> + err = mock_timeline_pin(ce->timeline);
>> + if (err) {
>> + intel_timeline_put(ce->timeline);
>> + ce->timeline = NULL;
>> + return err;
>> + }
>>
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> index a4c78062e92b..31b492eb2982 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> @@ -34,7 +34,7 @@ static unsigned long hwsp_cacheline(struct intel_timeline *tl)
>> {
>> unsigned long address = (unsigned long)page_address(hwsp_page(tl));
>>
>> - return (address + tl->hwsp_offset) / CACHELINE_BYTES;
>> + return (address + offset_in_page(tl->hwsp_offset)) / CACHELINE_BYTES;
> Does this belong in the previous commit? I've got no clue what I'm
> talking about here but it looks like it goes with the hwsp_offset
> wrapping changes in 01/69.
Yeah likely. Will move.
>
> --Jason
>
>> }
>>
>> #define CACHELINES_PER_PAGE (PAGE_SIZE / CACHELINE_BYTES)
>> @@ -58,6 +58,7 @@ static void __mock_hwsp_record(struct mock_hwsp_freelist *state,
>> tl = xchg(&state->history[idx], tl);
>> if (tl) {
>> radix_tree_delete(&state->cachelines, hwsp_cacheline(tl));
>> + intel_timeline_unpin(tl);
>> intel_timeline_put(tl);
>> }
>> }
>> @@ -77,6 +78,12 @@ static int __mock_hwsp_timeline(struct mock_hwsp_freelist *state,
>> if (IS_ERR(tl))
>> return PTR_ERR(tl);
>>
>> + err = intel_timeline_pin(tl, NULL);
>> + if (err) {
>> + intel_timeline_put(tl);
>> + return err;
>> + }
>> +
>> cacheline = hwsp_cacheline(tl);
>> err = radix_tree_insert(&state->cachelines, cacheline, tl);
>> if (err) {
>> @@ -84,6 +91,7 @@ static int __mock_hwsp_timeline(struct mock_hwsp_freelist *state,
>> pr_err("HWSP cacheline %lu already used; duplicate allocation!\n",
>> cacheline);
>> }
>> + intel_timeline_unpin(tl);
>> intel_timeline_put(tl);
>> return err;
>> }
>> @@ -451,7 +459,7 @@ static int emit_ggtt_store_dw(struct i915_request *rq, u32 addr, u32 value)
>> }
>>
>> static struct i915_request *
>> -tl_write(struct intel_timeline *tl, struct intel_engine_cs *engine, u32 value)
>> +checked_tl_write(struct intel_timeline *tl, struct intel_engine_cs *engine, u32 value)
>> {
>> struct i915_request *rq;
>> int err;
>> @@ -462,6 +470,13 @@ tl_write(struct intel_timeline *tl, struct intel_engine_cs *engine, u32 value)
>> goto out;
>> }
>>
>> + if (READ_ONCE(*tl->hwsp_seqno) != tl->seqno) {
>> + pr_err("Timeline created with incorrect breadcrumb, found %x, expected %x\n",
>> + *tl->hwsp_seqno, tl->seqno);
>> + intel_timeline_unpin(tl);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> rq = intel_engine_create_kernel_request(engine);
>> if (IS_ERR(rq))
>> goto out_unpin;
>> @@ -483,25 +498,6 @@ tl_write(struct intel_timeline *tl, struct intel_engine_cs *engine, u32 value)
>> return rq;
>> }
>>
>> -static struct intel_timeline *
>> -checked_intel_timeline_create(struct intel_gt *gt)
>> -{
>> - struct intel_timeline *tl;
>> -
>> - tl = intel_timeline_create(gt);
>> - if (IS_ERR(tl))
>> - return tl;
>> -
>> - if (READ_ONCE(*tl->hwsp_seqno) != tl->seqno) {
>> - pr_err("Timeline created with incorrect breadcrumb, found %x, expected %x\n",
>> - *tl->hwsp_seqno, tl->seqno);
>> - intel_timeline_put(tl);
>> - return ERR_PTR(-EINVAL);
>> - }
>> -
>> - return tl;
>> -}
>> -
>> static int live_hwsp_engine(void *arg)
>> {
>> #define NUM_TIMELINES 4096
>> @@ -534,13 +530,13 @@ static int live_hwsp_engine(void *arg)
>> struct intel_timeline *tl;
>> struct i915_request *rq;
>>
>> - tl = checked_intel_timeline_create(gt);
>> + tl = intel_timeline_create(gt);
>> if (IS_ERR(tl)) {
>> err = PTR_ERR(tl);
>> break;
>> }
>>
>> - rq = tl_write(tl, engine, count);
>> + rq = checked_tl_write(tl, engine, count);
>> if (IS_ERR(rq)) {
>> intel_timeline_put(tl);
>> err = PTR_ERR(rq);
>> @@ -607,14 +603,14 @@ static int live_hwsp_alternate(void *arg)
>> if (!intel_engine_can_store_dword(engine))
>> continue;
>>
>> - tl = checked_intel_timeline_create(gt);
>> + tl = intel_timeline_create(gt);
>> if (IS_ERR(tl)) {
>> err = PTR_ERR(tl);
>> goto out;
>> }
>>
>> intel_engine_pm_get(engine);
>> - rq = tl_write(tl, engine, count);
>> + rq = checked_tl_write(tl, engine, count);
>> intel_engine_pm_put(engine);
>> if (IS_ERR(rq)) {
>> intel_timeline_put(tl);
>> @@ -1257,6 +1253,10 @@ static int live_hwsp_rollover_user(void *arg)
>> if (!tl->has_initial_breadcrumb)
>> goto out;
>>
>> + err = intel_context_pin(ce);
>> + if (err)
>> + goto out;
>> +
>> tl->seqno = -4u;
>> WRITE_ONCE(*(u32 *)tl->hwsp_seqno, tl->seqno);
>>
>> @@ -1266,7 +1266,7 @@ static int live_hwsp_rollover_user(void *arg)
>> this = intel_context_create_request(ce);
>> if (IS_ERR(this)) {
>> err = PTR_ERR(this);
>> - goto out;
>> + goto out_unpin;
>> }
>>
>> pr_debug("%s: create fence.seqnp:%d\n",
>> @@ -1285,17 +1285,18 @@ static int live_hwsp_rollover_user(void *arg)
>> if (i915_request_wait(rq[2], 0, HZ / 5) < 0) {
>> pr_err("Wait for timeline wrap timed out!\n");
>> err = -EIO;
>> - goto out;
>> + goto out_unpin;
>> }
>>
>> for (i = 0; i < ARRAY_SIZE(rq); i++) {
>> if (!i915_request_completed(rq[i])) {
>> pr_err("Pre-wrap request not completed!\n");
>> err = -EINVAL;
>> - goto out;
>> + goto out_unpin;
>> }
>> }
>> -
>> +out_unpin:
>> + intel_context_unpin(ce);
>> out:
>> for (i = 0; i < ARRAY_SIZE(rq); i++)
>> i915_request_put(rq[i]);
>> @@ -1337,13 +1338,13 @@ static int live_hwsp_recycle(void *arg)
>> struct intel_timeline *tl;
>> struct i915_request *rq;
>>
>> - tl = checked_intel_timeline_create(gt);
>> + tl = intel_timeline_create(gt);
>> if (IS_ERR(tl)) {
>> err = PTR_ERR(tl);
>> break;
>> }
>>
>> - rq = tl_write(tl, engine, count);
>> + rq = checked_tl_write(tl, engine, count);
>> if (IS_ERR(rq)) {
>> intel_timeline_put(tl);
>> err = PTR_ERR(rq);
>> diff --git a/drivers/gpu/drm/i915/i915_selftest.h b/drivers/gpu/drm/i915/i915_selftest.h
>> index d53d207ab6eb..f54de0499be7 100644
>> --- a/drivers/gpu/drm/i915/i915_selftest.h
>> +++ b/drivers/gpu/drm/i915/i915_selftest.h
>> @@ -107,6 +107,7 @@ int __i915_subtests(const char *caller,
>>
>> #define I915_SELFTEST_DECLARE(x) x
>> #define I915_SELFTEST_ONLY(x) unlikely(x)
>> +#define I915_SELFTEST_EXPORT
>>
>> #else /* !IS_ENABLED(CONFIG_DRM_I915_SELFTEST) */
>>
>> @@ -116,6 +117,7 @@ static inline int i915_perf_selftests(struct pci_dev *pdev) { return 0; }
>>
>> #define I915_SELFTEST_DECLARE(x)
>> #define I915_SELFTEST_ONLY(x) 0
>> +#define I915_SELFTEST_EXPORT static
>>
>> #endif
>>
>> --
>> 2.30.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
