Re: [PATCH] drm/i915/selftests: Add a GuC doorbells selftest

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

 



On 25/10/17 14:08, Chris Wilson wrote:
Quoting Michel Thierry (2017-10-25 21:53:44)
Try to create multiple clients (one of each kind) and exercise the
doorbell alloc/dealloc code.

Since our usage mode require very few clients/doorbells, this code has
been exercised very lightly and it's good to have a simple test for it.

As reference, this test already helped identify the bug fixed by
commit 7f1ea2ac3017 ("drm/i915/guc: Fix doorbell id selection").

Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
---
+/* create a client of each type and their doorbells */
+static int igt_guc_doorbells(void *arg)
+{
+       struct drm_i915_private *dev_priv = arg;
+       struct intel_guc *guc;
+       struct i915_guc_client *clients[GUC_CLIENT_PRIORITY_NUM];

Don't you actually want to test up to GUC_NUM_DOORBELLS? (-reserved?)
And check failure when full?

I'm thinking an idr for the clients, that will give you an universal
-EEXISTS test as well for a repeated id, as well as an easy structure to
cleanup.

+       int i, err = 0;
+
+       DRM_INFO("GuC Doorbells selftest\n");
+       GEM_BUG_ON(!HAS_GUC(dev_priv));
+       mutex_lock(&dev_priv->drm.struct_mutex);
+
+       guc = &dev_priv->guc;
+       if (!guc) {
+               pr_err("No guc object!\n");
+               err = -EINVAL;
+               goto unlock;
+       }
+
+       err = check_all_doorbells(guc);
+       if (err)
+               goto unlock;
+
+       for (i = 0; i < GUC_CLIENT_PRIORITY_NUM; i++) {
+               clients[i] = guc_client_alloc(dev_priv,
+                                             INTEL_INFO(dev_priv)->ring_mask,
+                                             i % GUC_CLIENT_PRIORITY_NUM,

Definitely says to me you want a larger loop :)


I was trying to test doorbells not clients, but I'll expand it as you suggest.

+                                             dev_priv->kernel_context);

Is there any sanity check you want to do on the return client? Matches
requested priority, ring, context? I know that seems very superficial.

+               if (!clients[i]) {
+                       pr_err("[%d] No guc client!\n", i);
+                       err = -EINVAL;
+                       goto out;
+               }
+
+               if (i > 0 &&
+                   clients[i - 1]->doorbell_id == clients[i]->doorbell_id) {
+                       pr_err("doorbell ids must be unique!\n");
+                       err = -EINVAL;
+                       goto out;
+               }
+
+               /*
+                * Client alloc gives us a doorbell, but we want to exercise
+                * this ourselves.
+                */
+               destroy_doorbell(clients[i]);
+               if (clients[i]->doorbell_id != GUC_DOORBELL_INVALID) {
+                       pr_err("[%d] destroy db did not work!\n", i);
+                       err = -EINVAL;
+                       goto out;
+               }
+
+               err = __reserve_doorbell(clients[i]);
+               if (err) {
+                       pr_err("Failed to reserve a doorbell\n");
+                       goto out;
+               }
+
+               __update_doorbell_desc(clients[i], clients[i]->doorbell_id);
+               err = __create_doorbell(clients[i]);
+               if (err) {
+                       pr_err("Failed to create a doorbell\n");
+                       goto out;
+               }
+
+               err = check_all_doorbells(guc);
+               if (err)
+                       goto out;
+       }

I would love for something to test guc_init_doorbell_hw() to be worked
into here. That function scares me :)

You noticed this is more less a copy of guc_init_doorbell_hw, I'll try to use it directly.

Thanks
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux