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 :)
> + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
