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

[Michel Thierry]

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

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