| Message ID | 20171025205344.14469-1-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Winiarski <michal.winiarski@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
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@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Winiarski <michal.winiarski@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
On 25/10/17 14:19, Michel Thierry wrote: > 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@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Michal Winiarski <michal.winiarski@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. >> cc-ing people that may be interested, There is already an idr for the clients, guc->stage_ids, but its max range is GUC_MAX_STAGE_DESCRIPTORS, not GUC_NUM_DOORBELLS (this is because other drivers support the idea of 'leasing doorbells' and you could have more clients than doorbells). But we don't support that, and right now any client after GUC_NUM_DOORBELLS would be allocated but then get a -ENOSPC while creating/reserving its doorbell. I could change the limit of guc->stage_ids idr and reject earlier, any comments? -Michel
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 13d383cdc4c9..cb3139d9f4fe 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1280,3 +1280,7 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) guc_client_free(guc->execbuf_client); guc->execbuf_client = NULL; } + +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) +#include "selftests/intel_guc.c" +#endif diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h index 54a73534b37e..1b750e92dd08 100644 --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h @@ -19,3 +19,4 @@ selftest(evict, i915_gem_evict_live_selftests) selftest(hugepages, i915_gem_huge_page_live_selftests) selftest(contexts, i915_gem_context_live_selftests) selftest(hangcheck, intel_hangcheck_live_selftests) +selftest(guc, intel_guc_live_selftest) diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c new file mode 100644 index 000000000000..9f7be34acf83 --- /dev/null +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c @@ -0,0 +1,132 @@ +/* + * Copyright © 2017 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +#include "../i915_selftest.h" + +static int check_all_doorbells(struct intel_guc *guc) +{ + u16 db_id; + + pr_info_once("Max number of doorbells: %d", GUC_NUM_DOORBELLS); + for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id) { + if (!doorbell_ok(guc, db_id)) { + pr_err("doorbell %d, not ok\n", db_id); + return -EIO; + } + } + + return 0; +} + +/* 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]; + 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, + dev_priv->kernel_context); + + 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; + } + +out: + for (i = 0; i < GUC_CLIENT_PRIORITY_NUM; i++) + if (clients[i]) + guc_client_free(clients[i]); +unlock: + mutex_unlock(&dev_priv->drm.struct_mutex); + return err; +} + +int intel_guc_live_selftest(struct drm_i915_private *dev_priv) +{ + static const struct i915_subtest tests[] = { + SUBTEST(igt_guc_doorbells), + }; + + if (!i915_modparams.enable_guc_submission) + return 0; + + return i915_subtests(tests, dev_priv); +}
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@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 4 + .../gpu/drm/i915/selftests/i915_live_selftests.h | 1 + drivers/gpu/drm/i915/selftests/intel_guc.c | 132 +++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 drivers/gpu/drm/i915/selftests/intel_guc.c