[Intel-gfx] [PATCH] drm/i915/selftests: Add a GuC doorbells selftest

[Chris Wilson]

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 at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> ---
> +/* 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 :)

> +                                             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 :)
-Chris