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
