Re: [Intel-gfx] [CI] drm/i915/gt: Refactor hangcheck selftest to use igt_spinner
On 8/19/2023 15:50, Andi Shyti wrote:
From: Jonathan Cavitt
The hangcheck live selftest contains duplicate declarations of some
functions that already exist in igt_spinner.c, such as the creation and
deconstruction of a spinning batch buffer (spinner) that hangs an engine.
It's undesireable to have such code duplicated, as the requirements for
the spinner may change with hardware updates, necessitating both
execution paths be updated. To avoid this, have the hangcheck live
selftest use the declaration from igt_spinner. This eliminates the need
for the declarations in the selftest itself, as well as the associated
local helper structures, so we can erase those.
Suggested-by: Matt Roper
Signed-off-by: Jonathan Cavitt
---
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 457 ++-
drivers/gpu/drm/i915/selftests/igt_spinner.c | 15 +-
drivers/gpu/drm/i915/selftests/igt_spinner.h | 9 +
3 files changed, 155 insertions(+), 326 deletions(-)
[snip]
- pr_err("[%s] Hang init failed: %d!\n", engine->name, err);
+ pr_err("[%s] Spinner init failed: %d!\n", engine->name, err);
If this code is being touched, can you also change it to use gt_err
instead of pr_err? And gt_info instead of pr_info, etc. The pr_err
functions are the worst of the worst for message prints, they don't even
tag the output with 'i915' let alone anything useful like which GT it
was or which card in a multi-card system.
John.
Re: [Intel-gfx] [CI] drm/i915/gt: Refactor hangcheck selftest to use igt_spinner
Hi Jonathan, > > The hangcheck live selftest contains duplicate declarations of some > > functions that already exist in igt_spinner.c, such as the creation and > > deconstruction of a spinning batch buffer (spinner) that hangs an engine. > > It's undesireable to have such code duplicated, as the requirements for > > the spinner may change with hardware updates, necessitating both > > execution paths be updated. To avoid this, have the hangcheck live > > selftest use the declaration from igt_spinner. This eliminates the need > > for the declarations in the selftest itself, as well as the associated > > local helper structures, so we can erase those. > > > > Suggested-by: Matt Roper > > Signed-off-by: Jonathan Cavitt > > > Test fails with -62 (ETIME) on intel_selftest_wait_for_rq > It looks like the cause might be me calling intel_context_put too early? > Let me move those calls to later and see if that helps. > Give me some time to implement those changes. Thanks! Andi
Re: [Intel-gfx] [CI] drm/i915/gt: Refactor hangcheck selftest to use igt_spinner
-Original Message-
From: Andi Shyti
Sent: Saturday, August 19, 2023 3:50 PM
To: Cavitt, Jonathan
Cc: intel-gfx
Subject: [CI] drm/i915/gt: Refactor hangcheck selftest to use igt_spinner
>
> From: Jonathan Cavitt
>
> The hangcheck live selftest contains duplicate declarations of some
> functions that already exist in igt_spinner.c, such as the creation and
> deconstruction of a spinning batch buffer (spinner) that hangs an engine.
> It's undesireable to have such code duplicated, as the requirements for
> the spinner may change with hardware updates, necessitating both
> execution paths be updated. To avoid this, have the hangcheck live
> selftest use the declaration from igt_spinner. This eliminates the need
> for the declarations in the selftest itself, as well as the associated
> local helper structures, so we can erase those.
>
> Suggested-by: Matt Roper
> Signed-off-by: Jonathan Cavitt
Test fails with -62 (ETIME) on intel_selftest_wait_for_rq
It looks like the cause might be me calling intel_context_put too early?
Let me move those calls to later and see if that helps.
Give me some time to implement those changes.
-Jonathan Cavitt
> ---
> drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 457 ++-
> drivers/gpu/drm/i915/selftests/igt_spinner.c | 15 +-
> drivers/gpu/drm/i915/selftests/igt_spinner.h | 9 +
> 3 files changed, 155 insertions(+), 326 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> index 0dd4d00ee894e..36376a4ade8e4 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -29,281 +29,40 @@
>
> #define IGT_IDLE_TIMEOUT 50 /* ms; time to wait after flushing between tests
> */
>
> -struct hang {
> - struct intel_gt *gt;
> - struct drm_i915_gem_object *hws;
> - struct drm_i915_gem_object *obj;
> - struct i915_gem_context *ctx;
> - u32 *seqno;
> - u32 *batch;
> -};
> -
> -static int hang_init(struct hang *h, struct intel_gt *gt)
> -{
> - void *vaddr;
> - int err;
> -
> - memset(h, 0, sizeof(*h));
> - h->gt = gt;
> -
> - h->ctx = kernel_context(gt->i915, NULL);
> - if (IS_ERR(h->ctx))
> - return PTR_ERR(h->ctx);
> -
> - GEM_BUG_ON(i915_gem_context_is_bannable(h->ctx));
> -
> - h->hws = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> - if (IS_ERR(h->hws)) {
> - err = PTR_ERR(h->hws);
> - goto err_ctx;
> - }
> -
> - h->obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> - if (IS_ERR(h->obj)) {
> - err = PTR_ERR(h->obj);
> - goto err_hws;
> - }
> -
> - i915_gem_object_set_cache_coherency(h->hws, I915_CACHE_LLC);
> - vaddr = i915_gem_object_pin_map_unlocked(h->hws, I915_MAP_WB);
> - if (IS_ERR(vaddr)) {
> - err = PTR_ERR(vaddr);
> - goto err_obj;
> - }
> - h->seqno = memset(vaddr, 0xff, PAGE_SIZE);
> -
> - vaddr = i915_gem_object_pin_map_unlocked(h->obj,
> - intel_gt_coherent_map_type(gt,
> h->obj, false));
> - if (IS_ERR(vaddr)) {
> - err = PTR_ERR(vaddr);
> - goto err_unpin_hws;
> - }
> - h->batch = vaddr;
> -
> - return 0;
> -
> -err_unpin_hws:
> - i915_gem_object_unpin_map(h->hws);
> -err_obj:
> - i915_gem_object_put(h->obj);
> -err_hws:
> - i915_gem_object_put(h->hws);
> -err_ctx:
> - kernel_context_close(h->ctx);
> - return err;
> -}
> -
> -static u64 hws_address(const struct i915_vma *hws,
> -const struct i915_request *rq)
> -{
> - return i915_vma_offset(hws) +
> -offset_in_page(sizeof(u32) * rq->fence.context);
> -}
> -
> -static struct i915_request *
> -hang_create_request(struct hang *h, struct intel_engine_cs *engine)
> -{
> - struct intel_gt *gt = h->gt;
> - struct i915_address_space *vm = i915_gem_context_get_eb_vm(h->ctx);
> - struct drm_i915_gem_object *obj;
> - struct i915_request *rq = NULL;
> - struct i915_vma *hws, *vma;
> - unsigned int flags;
> - void *vaddr;
> - u32 *batch;
> - int err;
> -
> - obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> - if (IS_ERR(obj)) {
> - i915_vm_put(vm);
> - return ERR_CAST(obj);
> - }
> -
> - vaddr = i915_gem_object_pin_map_unlocked(obj,
> intel_gt_coherent_map_type(gt
[Intel-gfx] [CI] drm/i915/gt: Refactor hangcheck selftest to use igt_spinner
From: Jonathan Cavitt
The hangcheck live selftest contains duplicate declarations of some
functions that already exist in igt_spinner.c, such as the creation and
deconstruction of a spinning batch buffer (spinner) that hangs an engine.
It's undesireable to have such code duplicated, as the requirements for
the spinner may change with hardware updates, necessitating both
execution paths be updated. To avoid this, have the hangcheck live
selftest use the declaration from igt_spinner. This eliminates the need
for the declarations in the selftest itself, as well as the associated
local helper structures, so we can erase those.
Suggested-by: Matt Roper
Signed-off-by: Jonathan Cavitt
---
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 457 ++-
drivers/gpu/drm/i915/selftests/igt_spinner.c | 15 +-
drivers/gpu/drm/i915/selftests/igt_spinner.h | 9 +
3 files changed, 155 insertions(+), 326 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 0dd4d00ee894e..36376a4ade8e4 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -29,281 +29,40 @@
#define IGT_IDLE_TIMEOUT 50 /* ms; time to wait after flushing between tests */
-struct hang {
- struct intel_gt *gt;
- struct drm_i915_gem_object *hws;
- struct drm_i915_gem_object *obj;
- struct i915_gem_context *ctx;
- u32 *seqno;
- u32 *batch;
-};
-
-static int hang_init(struct hang *h, struct intel_gt *gt)
-{
- void *vaddr;
- int err;
-
- memset(h, 0, sizeof(*h));
- h->gt = gt;
-
- h->ctx = kernel_context(gt->i915, NULL);
- if (IS_ERR(h->ctx))
- return PTR_ERR(h->ctx);
-
- GEM_BUG_ON(i915_gem_context_is_bannable(h->ctx));
-
- h->hws = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
- if (IS_ERR(h->hws)) {
- err = PTR_ERR(h->hws);
- goto err_ctx;
- }
-
- h->obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
- if (IS_ERR(h->obj)) {
- err = PTR_ERR(h->obj);
- goto err_hws;
- }
-
- i915_gem_object_set_cache_coherency(h->hws, I915_CACHE_LLC);
- vaddr = i915_gem_object_pin_map_unlocked(h->hws, I915_MAP_WB);
- if (IS_ERR(vaddr)) {
- err = PTR_ERR(vaddr);
- goto err_obj;
- }
- h->seqno = memset(vaddr, 0xff, PAGE_SIZE);
-
- vaddr = i915_gem_object_pin_map_unlocked(h->obj,
-intel_gt_coherent_map_type(gt,
h->obj, false));
- if (IS_ERR(vaddr)) {
- err = PTR_ERR(vaddr);
- goto err_unpin_hws;
- }
- h->batch = vaddr;
-
- return 0;
-
-err_unpin_hws:
- i915_gem_object_unpin_map(h->hws);
-err_obj:
- i915_gem_object_put(h->obj);
-err_hws:
- i915_gem_object_put(h->hws);
-err_ctx:
- kernel_context_close(h->ctx);
- return err;
-}
-
-static u64 hws_address(const struct i915_vma *hws,
- const struct i915_request *rq)
-{
- return i915_vma_offset(hws) +
- offset_in_page(sizeof(u32) * rq->fence.context);
-}
-
-static struct i915_request *
-hang_create_request(struct hang *h, struct intel_engine_cs *engine)
-{
- struct intel_gt *gt = h->gt;
- struct i915_address_space *vm = i915_gem_context_get_eb_vm(h->ctx);
- struct drm_i915_gem_object *obj;
- struct i915_request *rq = NULL;
- struct i915_vma *hws, *vma;
- unsigned int flags;
- void *vaddr;
- u32 *batch;
- int err;
-
- obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
- if (IS_ERR(obj)) {
- i915_vm_put(vm);
- return ERR_CAST(obj);
- }
-
- vaddr = i915_gem_object_pin_map_unlocked(obj,
intel_gt_coherent_map_type(gt, obj, false));
- if (IS_ERR(vaddr)) {
- i915_gem_object_put(obj);
- i915_vm_put(vm);
- return ERR_CAST(vaddr);
- }
-
- i915_gem_object_unpin_map(h->obj);
- i915_gem_object_put(h->obj);
-
- h->obj = obj;
- h->batch = vaddr;
-
- vma = i915_vma_instance(h->obj, vm, NULL);
- if (IS_ERR(vma)) {
- i915_vm_put(vm);
- return ERR_CAST(vma);
- }
-
- hws = i915_vma_instance(h->hws, vm, NULL);
- if (IS_ERR(hws)) {
- i915_vm_put(vm);
- return ERR_CAST(hws);
- }
-
- err = i915_vma_pin(vma, 0, 0, PIN_USER);
- if (err) {
- i915_vm_put(vm);
- return ERR_PTR(err);
- }
-
- err = i915_vma_pin(hws, 0, 0, PIN_USER);
- if (err)
- goto unpin_vma;
-
- rq = igt_request_alloc(h->ctx, engine);
- if (IS_ERR(rq)) {
- err = PTR_ERR(rq);
- goto unpin_hws;
- }
-
- err =
[Intel-gfx] [CI] drm/i915/gt: Refactor hangcheck selftest to use igt_spinner
From: Jonathan Cavitt
The hangcheck live selftest contains duplicate declarations of some
functions that already exist in igt_spinner.c, such as the creation and
deconstruction of a spinning batch buffer (spinner) that hangs an engine.
It's undesireable to have such code duplicated, as the requirements for
the spinner may change with hardware updates, necessitating both
execution paths be updated. To avoid this, have the hangcheck live
selftest use the declaration from igt_spinner. This eliminates the need
for the declarations in the selftest itself, as well as the associated
local helper structures, so we can erase those.
Suggested-by: Matt Roper
Signed-off-by: Jonathan Cavitt
---
Hi,
I'm resending this patch in order to be sure that CI doesn't complain.
Andi
drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 457 ++-
drivers/gpu/drm/i915/selftests/igt_spinner.c | 15 +-
drivers/gpu/drm/i915/selftests/igt_spinner.h | 9 +
3 files changed, 155 insertions(+), 326 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 0dd4d00ee894e..36376a4ade8e4 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -29,281 +29,40 @@
#define IGT_IDLE_TIMEOUT 50 /* ms; time to wait after flushing between tests */
-struct hang {
- struct intel_gt *gt;
- struct drm_i915_gem_object *hws;
- struct drm_i915_gem_object *obj;
- struct i915_gem_context *ctx;
- u32 *seqno;
- u32 *batch;
-};
-
-static int hang_init(struct hang *h, struct intel_gt *gt)
-{
- void *vaddr;
- int err;
-
- memset(h, 0, sizeof(*h));
- h->gt = gt;
-
- h->ctx = kernel_context(gt->i915, NULL);
- if (IS_ERR(h->ctx))
- return PTR_ERR(h->ctx);
-
- GEM_BUG_ON(i915_gem_context_is_bannable(h->ctx));
-
- h->hws = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
- if (IS_ERR(h->hws)) {
- err = PTR_ERR(h->hws);
- goto err_ctx;
- }
-
- h->obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
- if (IS_ERR(h->obj)) {
- err = PTR_ERR(h->obj);
- goto err_hws;
- }
-
- i915_gem_object_set_cache_coherency(h->hws, I915_CACHE_LLC);
- vaddr = i915_gem_object_pin_map_unlocked(h->hws, I915_MAP_WB);
- if (IS_ERR(vaddr)) {
- err = PTR_ERR(vaddr);
- goto err_obj;
- }
- h->seqno = memset(vaddr, 0xff, PAGE_SIZE);
-
- vaddr = i915_gem_object_pin_map_unlocked(h->obj,
-intel_gt_coherent_map_type(gt,
h->obj, false));
- if (IS_ERR(vaddr)) {
- err = PTR_ERR(vaddr);
- goto err_unpin_hws;
- }
- h->batch = vaddr;
-
- return 0;
-
-err_unpin_hws:
- i915_gem_object_unpin_map(h->hws);
-err_obj:
- i915_gem_object_put(h->obj);
-err_hws:
- i915_gem_object_put(h->hws);
-err_ctx:
- kernel_context_close(h->ctx);
- return err;
-}
-
-static u64 hws_address(const struct i915_vma *hws,
- const struct i915_request *rq)
-{
- return i915_vma_offset(hws) +
- offset_in_page(sizeof(u32) * rq->fence.context);
-}
-
-static struct i915_request *
-hang_create_request(struct hang *h, struct intel_engine_cs *engine)
-{
- struct intel_gt *gt = h->gt;
- struct i915_address_space *vm = i915_gem_context_get_eb_vm(h->ctx);
- struct drm_i915_gem_object *obj;
- struct i915_request *rq = NULL;
- struct i915_vma *hws, *vma;
- unsigned int flags;
- void *vaddr;
- u32 *batch;
- int err;
-
- obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
- if (IS_ERR(obj)) {
- i915_vm_put(vm);
- return ERR_CAST(obj);
- }
-
- vaddr = i915_gem_object_pin_map_unlocked(obj,
intel_gt_coherent_map_type(gt, obj, false));
- if (IS_ERR(vaddr)) {
- i915_gem_object_put(obj);
- i915_vm_put(vm);
- return ERR_CAST(vaddr);
- }
-
- i915_gem_object_unpin_map(h->obj);
- i915_gem_object_put(h->obj);
-
- h->obj = obj;
- h->batch = vaddr;
-
- vma = i915_vma_instance(h->obj, vm, NULL);
- if (IS_ERR(vma)) {
- i915_vm_put(vm);
- return ERR_CAST(vma);
- }
-
- hws = i915_vma_instance(h->hws, vm, NULL);
- if (IS_ERR(hws)) {
- i915_vm_put(vm);
- return ERR_CAST(hws);
- }
-
- err = i915_vma_pin(vma, 0, 0, PIN_USER);
- if (err) {
- i915_vm_put(vm);
- return ERR_PTR(err);
- }
-
- err = i915_vma_pin(hws, 0, 0, PIN_USER);
- if (err)
- goto unpin_vma;
-
- rq = igt_request_alloc(h->ctx, engine);
- if (IS_ERR(rq)) {
- err =
