Re: [PATCH v8 02/69] drm/i915: Pin timeline map after first timeline pin, v3.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux