[Intel-gfx] [PATCH 3/4] drm/i915/guc: kill doorbell code and selftests
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Mon Nov 18 22:20:03 UTC 2019
On 11/14/19 3:56 PM, John Harrison wrote:
> 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.
>
fixed
>> 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.
>
ack
>> - 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?
>
I've updated to leave a note that GuC might've already been reset.
>> - 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?
>
This goes away in the next patch, together with the rest of the client
stuff.
Daniele
> 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