[Intel-gfx] [PATCH 3/4] drm/i915/guc: kill doorbell code and selftests
John Harrison
John.C.Harrison at Intel.com
Thu Nov 14 23:56:41 UTC 2019
On 11/6/2019 14:25, Daniele Ceraolo Spurio wrote:
> Instead of relying on the workqueue, the upcoming reworked GuC
> submission flow will offer the host driver indipendent control over
independent
> the execution status of each context submitted to GuC. As part of this,
> the doorbell usage model has been reworked, with each doorbell being
> paired to a single lrc and a doorbell ring representing new work
> available for that specific context. This mechanism, however, limits
> the number of contexts that can be registered with GuC to the number of
> doorbells, which is an undesired limitation. Luckily, GuC will also
Not exactly 'luckily'. More a case of, we said the doorbells won't work
for linux so can we have a H2G instead and they listened.
> provide a H2G that will allow the host to notify the GuC of work
> available for a specified lrc, so we can use that mechanism instead of
> relying on the doorbells. We can therefore drop the doorbell code we
> currently have, also given the fact that in the unlikely case we'd want
> to switch back to using doorbells we'd have to heavily rework it.
> The workqueue will still have a use in the new interface to pass special
> commands, so that code has been retained for now.
>
> With the doorbells gone and the GuC client becoming even simpler, the
> existing GuC selftests don't give us any meaningful coverage so we can
> remove them as well. Some selftests might come with the new code, but
> they will look different from what we have now so if doesn't seem worth
> it to keep the file around in the meantime.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: John Harrison <John.C.Harrison at Intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 8 +-
> drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 2 +-
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 373 ++----------------
> .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 9 +-
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 +-
> drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 299 --------------
> drivers/gpu/drm/i915/i915_debugfs.c | 11 +-
> .../drm/i915/selftests/i915_live_selftests.h | 1 -
> 8 files changed, 42 insertions(+), 668 deletions(-)
> delete mode 100644 drivers/gpu/drm/i915/gt/uc/selftest_guc.c
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index bf438f820c35..b2d1766e689a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -20,8 +20,8 @@ struct __guc_ads_blob;
>
> /*
> * Top level structure of GuC. It handles firmware loading and manages client
> - * pool and doorbells. intel_guc owns a intel_guc_client to replace the legacy
> - * ExecList submission.
> + * pool. intel_guc owns a intel_guc_client to replace the legacy ExecList
> + * submission.
> */
> struct intel_guc {
> struct intel_uc_fw fw;
> @@ -50,10 +50,6 @@ struct intel_guc {
>
> struct intel_guc_client *execbuf_client;
>
> - DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> - /* Cyclic counter mod pagesize */
> - u32 db_cacheline;
> -
> /* Control params for fw initialization */
> u32 params[GUC_CTL_MAX_DWORDS];
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index a26a85d50209..1e8e4af7d9ca 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -31,7 +31,7 @@
>
> #define GUC_DOORBELL_INVALID 256
>
> -#define GUC_DB_SIZE (PAGE_SIZE)
> +#define GUC_PD_SIZE (PAGE_SIZE)
> #define GUC_WQ_SIZE (PAGE_SIZE * 2)
>
> /* Work queue item header definitions */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 6ac213ddbfa3..0088c3417641 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -30,8 +30,8 @@
> * GuC client:
> * A intel_guc_client refers to a submission path through GuC. Currently, there
> * is only one client, which is charged with all submissions to the GuC. This
> - * struct is the owner of a doorbell, a process descriptor and a workqueue (all
> - * of them inside a single gem object that contains all required pages for these
> + * struct is the owner of a process descriptor and a workqueue (both of them
> + * inside a single gem object that contains all required pages for these
> * elements).
> *
> * GuC stage descriptor:
> @@ -39,13 +39,13 @@
> * descriptors, and shares them with the GuC.
> * Currently, there exists a 1:1 mapping between a intel_guc_client and a
> * guc_stage_desc (via the client's stage_id), so effectively only one
> - * gets used. This stage descriptor lets the GuC know about the doorbell,
> - * workqueue and process descriptor. Theoretically, it also lets the GuC
> - * know about our HW contexts (context ID, etc...), but we actually
> - * employ a kind of submission where the GuC uses the LRCA sent via the work
> - * item instead (the single guc_stage_desc associated to execbuf client
> - * contains information about the default kernel context only, but this is
> - * essentially unused). This is called a "proxy" submission.
> + * gets used. This stage descriptor lets the GuC know about the workqueue and
> + * process descriptor. Theoretically, it also lets the GuC know about our HW
> + * contexts (context ID, etc...), but we actually employ a kind of submission
> + * where the GuC uses the LRCA sent via the work item instead (the single
> + * guc_stage_desc associated to execbuf client contains information about the
> + * default kernel context only, but this is essentially unused). This is called
> + * a "proxy" submission.
> *
> * The Scratch registers:
> * There are 16 MMIO-based registers start from 0xC180. The kernel driver writes
> @@ -56,10 +56,6 @@
> * then proceeds.
> * See intel_guc_send()
> *
> - * Doorbells:
> - * Doorbells are interrupts to uKernel. A doorbell is a single cache line (QW)
> - * mapped into process space.
> - *
> * Work Items:
> * There are several types of work items that the host may place into a
> * workqueue, each with its own requirements and limitations. Currently only
> @@ -81,78 +77,6 @@ static inline bool is_high_priority(struct intel_guc_client *client)
> client->priority == GUC_CLIENT_PRIORITY_HIGH);
> }
>
> -static int reserve_doorbell(struct intel_guc_client *client)
> -{
> - unsigned long offset;
> - unsigned long end;
> - u16 id;
> -
> - GEM_BUG_ON(client->doorbell_id != GUC_DOORBELL_INVALID);
> -
> - /*
> - * The bitmap tracks which doorbell registers are currently in use.
> - * It is split into two halves; the first half is used for normal
> - * priority contexts, the second half for high-priority ones.
> - */
> - offset = 0;
> - end = GUC_NUM_DOORBELLS / 2;
> - if (is_high_priority(client)) {
> - offset = end;
> - end += offset;
> - }
> -
> - id = find_next_zero_bit(client->guc->doorbell_bitmap, end, offset);
> - if (id == end)
> - return -ENOSPC;
> -
> - __set_bit(id, client->guc->doorbell_bitmap);
> - client->doorbell_id = id;
> - DRM_DEBUG_DRIVER("client %u (high prio=%s) reserved doorbell: %d\n",
> - client->stage_id, yesno(is_high_priority(client)),
> - id);
> - return 0;
> -}
> -
> -static bool has_doorbell(struct intel_guc_client *client)
> -{
> - if (client->doorbell_id == GUC_DOORBELL_INVALID)
> - return false;
> -
> - return test_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> -}
> -
> -static void unreserve_doorbell(struct intel_guc_client *client)
> -{
> - GEM_BUG_ON(!has_doorbell(client));
> -
> - __clear_bit(client->doorbell_id, client->guc->doorbell_bitmap);
> - client->doorbell_id = GUC_DOORBELL_INVALID;
> -}
> -
> -/*
> - * Tell the GuC to allocate or deallocate a specific doorbell
> - */
> -
> -static int __guc_allocate_doorbell(struct intel_guc *guc, u32 stage_id)
> -{
> - u32 action[] = {
> - INTEL_GUC_ACTION_ALLOCATE_DOORBELL,
> - stage_id
> - };
> -
> - return intel_guc_send(guc, action, ARRAY_SIZE(action));
> -}
> -
> -static int __guc_deallocate_doorbell(struct intel_guc *guc, u32 stage_id)
> -{
> - u32 action[] = {
> - INTEL_GUC_ACTION_DEALLOCATE_DOORBELL,
> - stage_id
> - };
> -
> - return intel_guc_send(guc, action, ARRAY_SIZE(action));
> -}
> -
> static struct guc_stage_desc *__get_stage_desc(struct intel_guc_client *client)
> {
> struct guc_stage_desc *base = client->guc->stage_desc_pool_vaddr;
> @@ -160,118 +84,10 @@ static struct guc_stage_desc *__get_stage_desc(struct intel_guc_client *client)
> return &base[client->stage_id];
> }
>
> -/*
> - * Initialise, update, or clear doorbell data shared with the GuC
> - *
> - * These functions modify shared data and so need access to the mapped
> - * client object which contains the page being used for the doorbell
> - */
> -
> -static void __update_doorbell_desc(struct intel_guc_client *client, u16 new_id)
> -{
> - struct guc_stage_desc *desc;
> -
> - /* Update the GuC's idea of the doorbell ID */
> - desc = __get_stage_desc(client);
> - desc->db_id = new_id;
> -}
> -
> -static struct guc_doorbell_info *__get_doorbell(struct intel_guc_client *client)
> -{
> - return client->vaddr + client->doorbell_offset;
> -}
> -
> -static bool __doorbell_valid(struct intel_guc *guc, u16 db_id)
> -{
> - struct intel_uncore *uncore = guc_to_gt(guc)->uncore;
> -
> - GEM_BUG_ON(db_id >= GUC_NUM_DOORBELLS);
> - return intel_uncore_read(uncore, GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID;
> -}
> -
> -static void __init_doorbell(struct intel_guc_client *client)
> -{
> - struct guc_doorbell_info *doorbell;
> -
> - doorbell = __get_doorbell(client);
> - doorbell->db_status = GUC_DOORBELL_ENABLED;
> - doorbell->cookie = 0;
> -}
> -
> -static void __fini_doorbell(struct intel_guc_client *client)
> -{
> - struct guc_doorbell_info *doorbell;
> - u16 db_id = client->doorbell_id;
> -
> - doorbell = __get_doorbell(client);
> - doorbell->db_status = GUC_DOORBELL_DISABLED;
> -
> - /* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
> - * to go to zero after updating db_status before we call the GuC to
> - * release the doorbell
> - */
> - if (wait_for_us(!__doorbell_valid(client->guc, db_id), 10))
> - WARN_ONCE(true, "Doorbell never became invalid after disable\n");
> -}
> -
> -static int create_doorbell(struct intel_guc_client *client)
> -{
> - int ret;
> -
> - if (WARN_ON(!has_doorbell(client)))
> - return -ENODEV; /* internal setup error, should never happen */
> -
> - __update_doorbell_desc(client, client->doorbell_id);
> - __init_doorbell(client);
> -
> - ret = __guc_allocate_doorbell(client->guc, client->stage_id);
> - if (ret) {
> - __fini_doorbell(client);
> - __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
> - DRM_DEBUG_DRIVER("Couldn't create client %u doorbell: %d\n",
> - client->stage_id, ret);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static int destroy_doorbell(struct intel_guc_client *client)
> -{
> - int ret;
> -
> - GEM_BUG_ON(!has_doorbell(client));
> -
> - __fini_doorbell(client);
> - ret = __guc_deallocate_doorbell(client->guc, client->stage_id);
> - if (ret)
> - DRM_ERROR("Couldn't destroy client %u doorbell: %d\n",
> - client->stage_id, ret);
> -
> - __update_doorbell_desc(client, GUC_DOORBELL_INVALID);
> -
> - return ret;
> -}
> -
> -static unsigned long __select_cacheline(struct intel_guc *guc)
> -{
> - unsigned long offset;
> -
> - /* Doorbell uses a single cache line within a page */
> - offset = offset_in_page(guc->db_cacheline);
> -
> - /* Moving to next cache line to reduce contention */
> - guc->db_cacheline += cache_line_size();
> -
> - DRM_DEBUG_DRIVER("reserved cacheline 0x%lx, next 0x%x, linesize %u\n",
> - offset, guc->db_cacheline, cache_line_size());
> - return offset;
> -}
> -
> static inline struct guc_process_desc *
> __get_process_desc(struct intel_guc_client *client)
> {
> - return client->vaddr + client->proc_desc_offset;
> + return client->vaddr;
> }
>
> /*
> @@ -332,8 +148,8 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
> * Initialise/clear the stage descriptor shared with the GuC firmware.
> *
> * This descriptor tells the GuC where (in GGTT space) to find the important
> - * data structures relating to this client (doorbell, process descriptor,
> - * write queue, etc).
> + * data structures relating to this client (process descriptor, write queue,
> + * etc).
> */
> static void guc_stage_desc_init(struct intel_guc_client *client)
> {
> @@ -350,19 +166,14 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> desc->attribute |= GUC_STAGE_DESC_ATTR_PREEMPT;
> desc->stage_id = client->stage_id;
> desc->priority = client->priority;
> - desc->db_id = client->doorbell_id;
>
> /*
> - * The doorbell, process descriptor, and workqueue are all parts
> - * of the client object, which the GuC will reference via the GGTT
> + * The process descriptor and workqueue are all parts of the client
> + * object, which the GuC will reference via the GGTT
> */
> gfx_addr = intel_guc_ggtt_offset(guc, client->vma);
> - desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
> - client->doorbell_offset;
> - desc->db_trigger_cpu = ptr_to_u64(__get_doorbell(client));
> - desc->db_trigger_uk = gfx_addr + client->doorbell_offset;
> - desc->process_desc = gfx_addr + client->proc_desc_offset;
> - desc->wq_addr = gfx_addr + GUC_DB_SIZE;
> + desc->process_desc = gfx_addr;
> + desc->wq_addr = gfx_addr + GUC_PD_SIZE;
> desc->wq_size = GUC_WQ_SIZE;
>
> desc->desc_private = ptr_to_u64(client);
> @@ -408,48 +219,23 @@ static void guc_wq_item_append(struct intel_guc_client *client,
> GUC_WQ_SIZE) < wqi_size);
> GEM_BUG_ON(wq_off & (wqi_size - 1));
>
> - /* WQ starts from the page after doorbell / process_desc */
> - wqi = client->vaddr + wq_off + GUC_DB_SIZE;
> -
> - if (I915_SELFTEST_ONLY(client->use_nop_wqi)) {
> - wqi->header = WQ_TYPE_NOOP | (wqi_len << WQ_LEN_SHIFT);
> - } else {
> - /* Now fill in the 4-word work queue item */
> - wqi->header = WQ_TYPE_INORDER |
> - (wqi_len << WQ_LEN_SHIFT) |
> - (target_engine << WQ_TARGET_SHIFT) |
> - WQ_NO_WCFLUSH_WAIT;
> - wqi->context_desc = context_desc;
> - wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
> - GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
> - wqi->fence_id = fence_id;
> - }
> + /* WQ starts from the page after process_desc */
> + wqi = client->vaddr + wq_off + GUC_PD_SIZE;
> +
> + /* Now fill in the 4-word work queue item */
> + wqi->header = WQ_TYPE_INORDER |
> + (wqi_len << WQ_LEN_SHIFT) |
> + (target_engine << WQ_TARGET_SHIFT) |
> + WQ_NO_WCFLUSH_WAIT;
> + wqi->context_desc = context_desc;
> + wqi->submit_element_info = ring_tail << WQ_RING_TAIL_SHIFT;
> + GEM_BUG_ON(ring_tail > WQ_RING_TAIL_MAX);
> + wqi->fence_id = fence_id;
>
> /* Make the update visible to GuC */
> WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1));
> }
>
> -static void guc_ring_doorbell(struct intel_guc_client *client)
> -{
> - struct guc_doorbell_info *db;
> - u32 cookie;
> -
> - lockdep_assert_held(&client->wq_lock);
> -
> - /* pointer of current doorbell cacheline */
> - db = __get_doorbell(client);
> -
> - /*
> - * We're not expecting the doorbell cookie to change behind our back,
> - * we also need to treat 0 as a reserved value.
> - */
> - cookie = READ_ONCE(db->cookie);
> - WARN_ON_ONCE(xchg(&db->cookie, cookie + 1 ?: cookie + 2) != cookie);
> -
> - /* XXX: doorbell was lost and need to acquire it again */
> - GEM_BUG_ON(db->db_status != GUC_DOORBELL_ENABLED);
> -}
> -
> static void guc_add_request(struct intel_guc *guc, struct i915_request *rq)
> {
> struct intel_guc_client *client = guc->execbuf_client;
> @@ -459,7 +245,6 @@ static void guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>
> guc_wq_item_append(client, engine->guc_id, ctx_desc,
> ring_tail, rq->fence.seqno);
> - guc_ring_doorbell(client);
> }
>
> /*
> @@ -744,36 +529,6 @@ static void guc_reset_finish(struct intel_engine_cs *engine)
> * path of guc_submit() above.
> */
>
> -/* Check that a doorbell register is in the expected state */
> -static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
> -{
> - bool valid;
> -
> - GEM_BUG_ON(db_id >= GUC_NUM_DOORBELLS);
> -
> - valid = __doorbell_valid(guc, db_id);
> -
> - if (test_bit(db_id, guc->doorbell_bitmap) == valid)
> - return true;
> -
> - DRM_DEBUG_DRIVER("Doorbell %u has unexpected state: valid=%s\n",
> - db_id, yesno(valid));
> -
> - return false;
> -}
> -
> -static bool guc_verify_doorbells(struct intel_guc *guc)
> -{
> - bool doorbells_ok = true;
> - u16 db_id;
> -
> - for (db_id = 0; db_id < GUC_NUM_DOORBELLS; ++db_id)
> - if (!doorbell_ok(guc, db_id))
> - doorbells_ok = false;
> -
> - return doorbells_ok;
> -}
> -
> /**
> * guc_client_alloc() - Allocate an intel_guc_client
> * @guc: the intel_guc structure
> @@ -798,7 +553,6 @@ guc_client_alloc(struct intel_guc *guc, u32 priority)
>
> client->guc = guc;
> client->priority = priority;
> - client->doorbell_id = GUC_DOORBELL_INVALID;
> spin_lock_init(&client->wq_lock);
>
> ret = ida_simple_get(&guc->stage_ids, 0, GUC_MAX_STAGE_DESCRIPTORS,
> @@ -809,7 +563,7 @@ guc_client_alloc(struct intel_guc *guc, u32 priority)
> client->stage_id = ret;
>
> /* The first page is doorbell/proc_desc. Two followed pages are wq. */
Need to update this comment as well.
> - vma = intel_guc_allocate_vma(guc, GUC_DB_SIZE + GUC_WQ_SIZE);
> + vma = intel_guc_allocate_vma(guc, GUC_PD_SIZE + GUC_WQ_SIZE);
> if (IS_ERR(vma)) {
> ret = PTR_ERR(vma);
> goto err_id;
> @@ -825,31 +579,11 @@ guc_client_alloc(struct intel_guc *guc, u32 priority)
> }
> client->vaddr = vaddr;
>
> - ret = reserve_doorbell(client);
> - if (ret)
> - goto err_vaddr;
> -
> - client->doorbell_offset = __select_cacheline(guc);
> -
> - /*
> - * Since the doorbell only requires a single cacheline, we can save
> - * space by putting the application process descriptor in the same
> - * page. Use the half of the page that doesn't include the doorbell.
> - */
> - if (client->doorbell_offset >= (GUC_DB_SIZE / 2))
> - client->proc_desc_offset = 0;
> - else
> - client->proc_desc_offset = (GUC_DB_SIZE / 2);
> -
> DRM_DEBUG_DRIVER("new priority %u client %p: stage_id %u\n",
> priority, client, client->stage_id);
> - DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
> - client->doorbell_id, client->doorbell_offset);
>
> return client;
>
> -err_vaddr:
> - i915_gem_object_unpin_map(client->vma->obj);
> err_vma:
> i915_vma_unpin_and_release(&client->vma, 0);
> err_id:
> @@ -861,7 +595,6 @@ guc_client_alloc(struct intel_guc *guc, u32 priority)
>
> static void guc_client_free(struct intel_guc_client *client)
> {
> - unreserve_doorbell(client);
> i915_vma_unpin_and_release(&client->vma, I915_VMA_RELEASE_MAP);
> ida_simple_remove(&client->guc->stage_ids, client->stage_id);
> kfree(client);
> @@ -892,44 +625,21 @@ static void guc_clients_destroy(struct intel_guc *guc)
> guc_client_free(client);
> }
>
> -static int __guc_client_enable(struct intel_guc_client *client)
> +static void __guc_client_enable(struct intel_guc_client *client)
> {
> - int ret;
> -
> guc_proc_desc_init(client);
> guc_stage_desc_init(client);
> -
> - ret = create_doorbell(client);
> - if (ret)
> - goto fail;
> -
> - return 0;
> -
> -fail:
> - guc_stage_desc_fini(client);
> - guc_proc_desc_fini(client);
> - return ret;
> }
>
> static void __guc_client_disable(struct intel_guc_client *client)
> {
> - /*
> - * By the time we're here, GuC may have already been reset. if that is
> - * the case, instead of trying (in vain) to communicate with it, let's
> - * just cleanup the doorbell HW and our internal state.
> - */
This comment should be kept, only dropping the 'doorell HW and' phrase?
> - if (intel_guc_is_running(client->guc))
> - destroy_doorbell(client);
> - else
> - __fini_doorbell(client);
> -
> guc_stage_desc_fini(client);
> guc_proc_desc_fini(client);
> }
>
> -static int guc_clients_enable(struct intel_guc *guc)
> +static void guc_clients_enable(struct intel_guc *guc)
> {
> - return __guc_client_enable(guc->execbuf_client);
> + __guc_client_enable(guc->execbuf_client);
> }
>
This seems like a pretty pointless wrapper. I'm guessing there was a
mutex lock or something in here originally? Maybe time to drop the '__'
version and just move the actual work into this function?
Otherwise, looks good to me. So with some corrected comments:
Reviewed-by: John Harrison <John.C.Harrison at Intel.com>
> static void guc_clients_disable(struct intel_guc *guc)
> @@ -958,7 +668,6 @@ int intel_guc_submission_init(struct intel_guc *guc)
> */
> GEM_BUG_ON(!guc->stage_desc_pool);
>
> - WARN_ON(!guc_verify_doorbells(guc));
> ret = guc_clients_create(guc);
> if (ret)
> goto err_pool;
> @@ -973,7 +682,6 @@ int intel_guc_submission_init(struct intel_guc *guc)
> void intel_guc_submission_fini(struct intel_guc *guc)
> {
> guc_clients_destroy(guc);
> - WARN_ON(!guc_verify_doorbells(guc));
>
> if (guc->stage_desc_pool)
> guc_stage_desc_pool_destroy(guc);
> @@ -1089,16 +797,11 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
> GEM_BUG_ON(engine->irq_enable || engine->irq_disable);
> }
>
> -int intel_guc_submission_enable(struct intel_guc *guc)
> +void intel_guc_submission_enable(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> - int err;
> -
> - err = i915_inject_probe_error(gt->i915, -ENXIO);
> - if (err)
> - return err;
>
> /*
> * We're using GuC work items for submitting work through GuC. Since
> @@ -1115,9 +818,7 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>
> GEM_BUG_ON(!guc->execbuf_client);
>
> - err = guc_clients_enable(guc);
> - if (err)
> - return err;
> + guc_clients_enable(guc);
>
> /* Take over from manual control of ELSP (execlists) */
> guc_interrupts_capture(gt);
> @@ -1126,8 +827,6 @@ int intel_guc_submission_enable(struct intel_guc *guc)
> engine->set_default_submission = guc_set_default_submission;
> engine->set_default_submission(engine);
> }
> -
> - return 0;
> }
>
> void intel_guc_submission_disable(struct intel_guc *guc)
> @@ -1155,7 +854,3 @@ void intel_guc_submission_init_early(struct intel_guc *guc)
> {
> guc->submission_supported = __guc_submission_support(guc);
> }
> -
> -#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> -#include "selftest_guc.c"
> -#endif
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> index 54d716828352..e2deb4e6f42a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> @@ -44,21 +44,14 @@ struct intel_guc_client {
> /* bitmap of (host) engine ids */
> u32 priority;
> u32 stage_id;
> - u32 proc_desc_offset;
> -
> - u16 doorbell_id;
> - unsigned long doorbell_offset;
>
> /* Protects GuC client's WQ access */
> spinlock_t wq_lock;
> -
> - /* For testing purposes, use nop WQ items instead of real ones */
> - I915_SELFTEST_DECLARE(bool use_nop_wqi);
> };
>
> void intel_guc_submission_init_early(struct intel_guc *guc);
> int intel_guc_submission_init(struct intel_guc *guc);
> -int intel_guc_submission_enable(struct intel_guc *guc);
> +void intel_guc_submission_enable(struct intel_guc *guc);
> void intel_guc_submission_disable(struct intel_guc *guc);
> void intel_guc_submission_fini(struct intel_guc *guc);
> int intel_guc_preempt_work_create(struct intel_guc *guc);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 629b19377a29..c6519066a0f6 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -486,11 +486,8 @@ int intel_uc_init_hw(struct intel_uc *uc)
> if (ret)
> goto err_communication;
>
> - if (intel_uc_supports_guc_submission(uc)) {
> - ret = intel_guc_submission_enable(guc);
> - if (ret)
> - goto err_communication;
> - }
> + if (intel_uc_supports_guc_submission(uc))
> + intel_guc_submission_enable(guc);
>
> dev_info(i915->drm.dev, "%s firmware %s version %u.%u %s:%s\n",
> intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC), guc->fw.path,
> diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> deleted file mode 100644
> index d8a80388bd31..000000000000
> --- a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c
> +++ /dev/null
> @@ -1,299 +0,0 @@
> -// SPDX-License-Identifier: MIT
> -/*
> - * Copyright © 2017 Intel Corporation
> - */
> -
> -#include "i915_selftest.h"
> -#include "gem/i915_gem_pm.h"
> -
> -/* max doorbell number + negative test for each client type */
> -#define ATTEMPTS (GUC_NUM_DOORBELLS + GUC_CLIENT_PRIORITY_NUM)
> -
> -static struct intel_guc_client *clients[ATTEMPTS];
> -
> -static bool available_dbs(struct intel_guc *guc, u32 priority)
> -{
> - unsigned long offset;
> - unsigned long end;
> - u16 id;
> -
> - /* first half is used for normal priority, second half for high */
> - offset = 0;
> - end = GUC_NUM_DOORBELLS / 2;
> - if (priority <= GUC_CLIENT_PRIORITY_HIGH) {
> - offset = end;
> - end += offset;
> - }
> -
> - id = find_next_zero_bit(guc->doorbell_bitmap, end, offset);
> - if (id < end)
> - return true;
> -
> - return false;
> -}
> -
> -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;
> -}
> -
> -static int ring_doorbell_nop(struct intel_guc_client *client)
> -{
> - struct guc_process_desc *desc = __get_process_desc(client);
> - int err;
> -
> - client->use_nop_wqi = true;
> -
> - spin_lock_irq(&client->wq_lock);
> -
> - guc_wq_item_append(client, 0, 0, 0, 0);
> - guc_ring_doorbell(client);
> -
> - spin_unlock_irq(&client->wq_lock);
> -
> - client->use_nop_wqi = false;
> -
> - /* if there are no issues GuC will update the WQ head and keep the
> - * WQ in active status
> - */
> - err = wait_for(READ_ONCE(desc->head) == READ_ONCE(desc->tail), 10);
> - if (err) {
> - pr_err("doorbell %u ring failed!\n", client->doorbell_id);
> - return -EIO;
> - }
> -
> - if (desc->wq_status != WQ_STATUS_ACTIVE) {
> - pr_err("doorbell %u ring put WQ in bad state (%u)!\n",
> - client->doorbell_id, desc->wq_status);
> - return -EIO;
> - }
> -
> - return 0;
> -}
> -
> -/*
> - * Basic client sanity check, handy to validate create_clients.
> - */
> -static int validate_client(struct intel_guc_client *client, int client_priority)
> -{
> - if (client->priority != client_priority ||
> - client->doorbell_id == GUC_DOORBELL_INVALID)
> - return -EINVAL;
> - else
> - return 0;
> -}
> -
> -static bool client_doorbell_in_sync(struct intel_guc_client *client)
> -{
> - return !client || doorbell_ok(client->guc, client->doorbell_id);
> -}
> -
> -/*
> - * Check that we're able to synchronize guc_clients with their doorbells
> - *
> - * We're creating clients and reserving doorbells once, at module load. During
> - * module lifetime, GuC, doorbell HW, and i915 state may go out of sync due to
> - * GuC being reset. In other words - GuC clients are still around, but the
> - * status of their doorbells may be incorrect. This is the reason behind
> - * validating that the doorbells status expected by the driver matches what the
> - * GuC/HW have.
> - */
> -static int igt_guc_clients(void *arg)
> -{
> - struct intel_gt *gt = arg;
> - struct intel_guc *guc = >->uc.guc;
> - intel_wakeref_t wakeref;
> - int err = 0;
> -
> - GEM_BUG_ON(!HAS_GT_UC(gt->i915));
> - wakeref = intel_runtime_pm_get(gt->uncore->rpm);
> -
> - err = check_all_doorbells(guc);
> - if (err)
> - goto unlock;
> -
> - /*
> - * Get rid of clients created during driver load because the test will
> - * recreate them.
> - */
> - guc_clients_disable(guc);
> - guc_clients_destroy(guc);
> - if (guc->execbuf_client) {
> - pr_err("guc_clients_destroy lied!\n");
> - err = -EINVAL;
> - goto unlock;
> - }
> -
> - err = guc_clients_create(guc);
> - if (err) {
> - pr_err("Failed to create clients\n");
> - goto unlock;
> - }
> - GEM_BUG_ON(!guc->execbuf_client);
> -
> - err = validate_client(guc->execbuf_client,
> - GUC_CLIENT_PRIORITY_KMD_NORMAL);
> - if (err) {
> - pr_err("execbug client validation failed\n");
> - goto out;
> - }
> -
> - /* the client should now have reserved a doorbell */
> - if (!has_doorbell(guc->execbuf_client)) {
> - pr_err("guc_clients_create didn't reserve doorbells\n");
> - err = -EINVAL;
> - goto out;
> - }
> -
> - /* Now enable the clients */
> - guc_clients_enable(guc);
> -
> - /* each client should now have received a doorbell */
> - if (!client_doorbell_in_sync(guc->execbuf_client)) {
> - pr_err("failed to initialize the doorbells\n");
> - err = -EINVAL;
> - goto out;
> - }
> -
> - /*
> - * Basic test - an attempt to reallocate a valid doorbell to the
> - * client it is currently assigned should not cause a failure.
> - */
> - err = create_doorbell(guc->execbuf_client);
> -
> -out:
> - /*
> - * Leave clean state for other test, plus the driver always destroy the
> - * clients during unload.
> - */
> - guc_clients_disable(guc);
> - guc_clients_destroy(guc);
> - guc_clients_create(guc);
> - guc_clients_enable(guc);
> -unlock:
> - intel_runtime_pm_put(gt->uncore->rpm, wakeref);
> - return err;
> -}
> -
> -/*
> - * Create as many clients as number of doorbells. Note that there's already
> - * client(s)/doorbell(s) created during driver load, but this test creates
> - * its own and do not interact with the existing ones.
> - */
> -static int igt_guc_doorbells(void *arg)
> -{
> - struct intel_gt *gt = arg;
> - struct intel_guc *guc = >->uc.guc;
> - intel_wakeref_t wakeref;
> - int i, err = 0;
> - u16 db_id;
> -
> - GEM_BUG_ON(!HAS_GT_UC(gt->i915));
> - wakeref = intel_runtime_pm_get(gt->uncore->rpm);
> -
> - err = check_all_doorbells(guc);
> - if (err)
> - goto unlock;
> -
> - for (i = 0; i < ATTEMPTS; i++) {
> - clients[i] = guc_client_alloc(guc, i % GUC_CLIENT_PRIORITY_NUM);
> -
> - if (!clients[i]) {
> - pr_err("[%d] No guc client\n", i);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - if (IS_ERR(clients[i])) {
> - if (PTR_ERR(clients[i]) != -ENOSPC) {
> - pr_err("[%d] unexpected error\n", i);
> - err = PTR_ERR(clients[i]);
> - goto out;
> - }
> -
> - if (available_dbs(guc, i % GUC_CLIENT_PRIORITY_NUM)) {
> - pr_err("[%d] non-db related alloc fail\n", i);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - /* expected, ran out of dbs for this client type */
> - continue;
> - }
> -
> - /*
> - * The check below is only valid because we keep a doorbell
> - * assigned during the whole life of the client.
> - */
> - if (clients[i]->stage_id >= GUC_NUM_DOORBELLS) {
> - pr_err("[%d] more clients than doorbells (%d >= %d)\n",
> - i, clients[i]->stage_id, GUC_NUM_DOORBELLS);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - err = validate_client(clients[i], i % GUC_CLIENT_PRIORITY_NUM);
> - if (err) {
> - pr_err("[%d] client_alloc sanity check failed!\n", i);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - db_id = clients[i]->doorbell_id;
> -
> - err = __guc_client_enable(clients[i]);
> - if (err) {
> - pr_err("[%d] Failed to create a doorbell\n", i);
> - goto out;
> - }
> -
> - /* doorbell id shouldn't change, we are holding the mutex */
> - if (db_id != clients[i]->doorbell_id) {
> - pr_err("[%d] doorbell id changed (%d != %d)\n",
> - i, db_id, clients[i]->doorbell_id);
> - err = -EINVAL;
> - goto out;
> - }
> -
> - err = check_all_doorbells(guc);
> - if (err)
> - goto out;
> -
> - err = ring_doorbell_nop(clients[i]);
> - if (err)
> - goto out;
> - }
> -
> -out:
> - for (i = 0; i < ATTEMPTS; i++)
> - if (!IS_ERR_OR_NULL(clients[i])) {
> - __guc_client_disable(clients[i]);
> - guc_client_free(clients[i]);
> - }
> -unlock:
> - intel_runtime_pm_put(gt->uncore->rpm, wakeref);
> - return err;
> -}
> -
> -int intel_guc_live_selftest(struct drm_i915_private *i915)
> -{
> - static const struct i915_subtest tests[] = {
> - SUBTEST(igt_guc_clients),
> - SUBTEST(igt_guc_doorbells),
> - };
> -
> - if (!USES_GUC_SUBMISSION(i915))
> - return 0;
> -
> - return intel_gt_live_subtests(tests, &i915->gt);
> -}
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index cab632791f73..5d5974e7f3ed 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1815,17 +1815,10 @@ static int i915_guc_info(struct seq_file *m, void *data)
>
> GEM_BUG_ON(!guc->execbuf_client);
>
> - seq_printf(m, "\nDoorbell map:\n");
> - seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
> - seq_printf(m, "Doorbell next cacheline: 0x%x\n", guc->db_cacheline);
> -
> seq_printf(m, "\nGuC execbuf client @ %p:\n", client);
> - seq_printf(m, "\tPriority %d, GuC stage index: %u, PD offset 0x%x\n",
> + seq_printf(m, "\tPriority %d, GuC stage index: %u\n",
> client->priority,
> - client->stage_id,
> - client->proc_desc_offset);
> - seq_printf(m, "\tDoorbell id %d, offset: 0x%lx\n",
> - client->doorbell_id, client->doorbell_offset);
> + client->stage_id);
> /* Add more as required ... */
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> index 4b3cac73e291..fb03f8a90cac 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> @@ -36,5 +36,4 @@ selftest(reset, intel_reset_live_selftests)
> selftest(memory_region, intel_memory_region_live_selftests)
> selftest(hangcheck, intel_hangcheck_live_selftests)
> selftest(execlists, intel_execlists_live_selftests)
> -selftest(guc, intel_guc_live_selftest)
> selftest(perf, i915_perf_live_selftests)
More information about the Intel-gfx
mailing list