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 overindependentNot 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.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
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@xxxxxxxxx> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> Cc: John Harrison <John.C.Harrison@xxxxxxxxx> Cc: Matthew Brost <matthew.brost@xxxxxxxxx> --- 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.cdiff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.hindex 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.hindex 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.cindex 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.
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?- 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 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@xxxxxxxxx>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" -#endifdiff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.hindex 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.cindex 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.cdeleted 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.cindex 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.hindex 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)
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
