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.
> + 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.
>
> 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.
--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
