Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/guc/slpc: Add selftest for slpc tile-tile interaction

[Tauro, Riana]

On 10/28/2022 3:53 PM, Tvrtko Ursulin wrote:


Hi,

I peeked inside from curiosity and was pleasantly surprise to see 
kthread_work is used! Some comments below.


On 28/10/2022 10:50, Riana Tauro wrote:

Run a workload on tiles simultaneously by requesting for RP0 frequency.
Pcode can however limit the frequency being granted due to throttling
reasons. This test fails if there is any throttling

v2: Fix build error

Signed-off-by: Riana Tauro 
---
  drivers/gpu/drm/i915/gt/selftest_slpc.c | 63 +
  1 file changed, 63 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c 
b/drivers/gpu/drm/i915/gt/selftest_slpc.c

index 82ec95a299f6..d19486772f5a 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -13,6 +13,14 @@ enum test_type {
  VARY_MAX,
  MAX_GRANTED,
  SLPC_POWER,
+    TILE_INTERACTION,
+};
+
+struct slpc_thread {
+    struct kthread_worker *worker;
+    struct kthread_work work;
+    struct intel_gt *gt;
+    int result;
  };
  static int slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 freq)
@@ -310,6 +318,7 @@ static int run_test(struct intel_gt *gt, int 
test_type)

  break;
  case MAX_GRANTED:
+    case TILE_INTERACTION:
  /* Media engines have a different RP0 */
  if (engine->class == VIDEO_DECODE_CLASS ||
  engine->class == VIDEO_ENHANCEMENT_CLASS) {
@@ -325,6 +334,7 @@ static int run_test(struct intel_gt *gt, int 
test_type)

  case SLPC_POWER:
  err = slpc_power(gt, engine);
  break;
+


Noise.


  }
  if (test_type != SLPC_POWER) {
@@ -426,6 +436,58 @@ static int live_slpc_power(void *arg)
  return ret;
  }
+static void slpc_spinner_thread(struct kthread_work *work)
+{
+    struct slpc_thread *thread = container_of(work, typeof(*thread), 
work);

+
+    thread->result = run_test(thread->gt, TILE_INTERACTION);
+}
+
+static int live_slpc_tile_interaction(void *arg)
+{
+    struct drm_i915_private *i915 = arg;
+    struct intel_gt *gt;
+    struct slpc_thread *threads;
+    int i = 0, ret = 0;
+
+    threads = kcalloc(I915_MAX_GT, sizeof(*threads), GFP_KERNEL);
+    if (!threads)
+    return -ENOMEM;
+
+    for_each_gt(gt, i915, i) {
+    pr_info("Running on GT: %d\n", gt->info.id);


Not sure logging these makes sense since test runs on all tiles in 
parallel anyway.


+    threads[i].worker = kthread_create_worker(0, 
"igt/slpc_parallel:%d", gt->info.id);

+
+    if (IS_ERR(threads[i].worker)) {
+    ret = PTR_ERR(threads[i].worker);
+    break;
+    }
+
+    threads[i].gt = gt;
+    threads[i].result = 0;


No need to zero result due kcalloc and runs do not repeat.


+    kthread_init_work([i].work, slpc_spinner_thread);
+    kthread_queue_work(threads[i].worker, [i].work);
+    }
+
+    for_each_gt(gt, i915, i) {
+    int status;
+
+    if (!threads[i].worker)
+    continue;


Could be ERR_PTR by the look of it so it would crash below. Either gate 
on threads[i].gt or use IS_ERR_OR_NULL.


Regards,

Tvrtko


Thank you Tvrtko for the review comments.
Will fix these.

Thanks
Riana




+
+    kthread_flush_work([i].work);
+    status = READ_ONCE(threads[i].result);
+    if (status && !ret) {
+    pr_err("%s GT %d failed ", __func__, gt->info.id);
+    ret = status;
+    }
+    kthread_destroy_worker(threads[i].worker);
+    }
+
+    kfree(threads);
+    return ret;
+}
+
  int intel_slpc_live_selftests(struct drm_i915_private *i915)
  {
  static const struct i915_subtest tests[] = {
@@ -433,6 +495,7 @@ int intel_slpc_live_selftests(struct 
drm_i915_private *i915)

  SUBTEST(live_slpc_vary_min),
  SUBTEST(live_slpc_max_granted),
  SUBTEST(live_slpc_power),
+    SUBTEST(live_slpc_tile_interaction),
  };
  struct intel_gt *gt;

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/guc/slpc: Add selftest for slpc tile-tile interaction

[Tvrtko Ursulin]

Hi,

I peeked inside from curiosity and was pleasantly surprise to see 
kthread_work is used! Some comments below.


On 28/10/2022 10:50, Riana Tauro wrote:

Run a workload on tiles simultaneously by requesting for RP0 frequency.
Pcode can however limit the frequency being granted due to throttling
reasons. This test fails if there is any throttling

v2: Fix build error

Signed-off-by: Riana Tauro 
---
  drivers/gpu/drm/i915/gt/selftest_slpc.c | 63 +
  1 file changed, 63 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c 
b/drivers/gpu/drm/i915/gt/selftest_slpc.c
index 82ec95a299f6..d19486772f5a 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -13,6 +13,14 @@ enum test_type {
VARY_MAX,
MAX_GRANTED,
SLPC_POWER,
+   TILE_INTERACTION,
+};
+
+struct slpc_thread {
+   struct kthread_worker *worker;
+   struct kthread_work work;
+   struct intel_gt *gt;
+   int result;
  };
  
  static int slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 freq)

@@ -310,6 +318,7 @@ static int run_test(struct intel_gt *gt, int test_type)
break;
  
  		case MAX_GRANTED:

+   case TILE_INTERACTION:
/* Media engines have a different RP0 */
if (engine->class == VIDEO_DECODE_CLASS ||
engine->class == VIDEO_ENHANCEMENT_CLASS) {
@@ -325,6 +334,7 @@ static int run_test(struct intel_gt *gt, int test_type)
case SLPC_POWER:
err = slpc_power(gt, engine);
break;
+


Noise.


}
  
  		if (test_type != SLPC_POWER) {

@@ -426,6 +436,58 @@ static int live_slpc_power(void *arg)
return ret;
  }
  
+static void slpc_spinner_thread(struct kthread_work *work)

+{
+   struct slpc_thread *thread = container_of(work, typeof(*thread), work);
+
+   thread->result = run_test(thread->gt, TILE_INTERACTION);
+}
+
+static int live_slpc_tile_interaction(void *arg)
+{
+   struct drm_i915_private *i915 = arg;
+   struct intel_gt *gt;
+   struct slpc_thread *threads;
+   int i = 0, ret = 0;
+
+   threads = kcalloc(I915_MAX_GT, sizeof(*threads), GFP_KERNEL);
+   if (!threads)
+   return -ENOMEM;
+
+   for_each_gt(gt, i915, i) {
+   pr_info("Running on GT: %d\n", gt->info.id);


Not sure logging these makes sense since test runs on all tiles in 
parallel anyway.



+   threads[i].worker = kthread_create_worker(0, 
"igt/slpc_parallel:%d", gt->info.id);
+
+   if (IS_ERR(threads[i].worker)) {
+   ret = PTR_ERR(threads[i].worker);
+   break;
+   }
+
+   threads[i].gt = gt;
+   threads[i].result = 0;


No need to zero result due kcalloc and runs do not repeat.


+   kthread_init_work([i].work, slpc_spinner_thread);
+   kthread_queue_work(threads[i].worker, [i].work);
+   }
+
+   for_each_gt(gt, i915, i) {
+   int status;
+
+   if (!threads[i].worker)
+   continue;


Could be ERR_PTR by the look of it so it would crash below. Either gate 
on threads[i].gt or use IS_ERR_OR_NULL.


Regards,

Tvrtko


+
+   kthread_flush_work([i].work);
+   status = READ_ONCE(threads[i].result);
+   if (status && !ret) {
+   pr_err("%s GT %d failed ", __func__, gt->info.id);
+   ret = status;
+   }
+   kthread_destroy_worker(threads[i].worker);
+   }
+
+   kfree(threads);
+   return ret;
+}
+
  int intel_slpc_live_selftests(struct drm_i915_private *i915)
  {
static const struct i915_subtest tests[] = {
@@ -433,6 +495,7 @@ int intel_slpc_live_selftests(struct drm_i915_private *i915)
SUBTEST(live_slpc_vary_min),
SUBTEST(live_slpc_max_granted),
SUBTEST(live_slpc_power),
+   SUBTEST(live_slpc_tile_interaction),
};
  
  	struct intel_gt *gt;