From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 764E9D2169D for ; Tue, 15 Oct 2024 15:20:25 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 40D1610E598; Tue, 15 Oct 2024 15:20:25 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="jixKTazF"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id A886610E598 for ; Tue, 15 Oct 2024 15:20:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1729005624; x=1760541624; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=opd0Fcm5OzEsTFOs1kNoMdSgwKu1XidEACFCL0RUQMc=; b=jixKTazFfhlbgAvSQR31zPOIp0YqDAdO8bZ7DrJkljKI2HVxWC5JFvbu 56IK/2SvHIZs76n+fOUzYeUc64Zwpn55+K7dfv9kC7wp3tXXSFt7GlTf3 pp2o2jIpzE7033dhFpcl4j0+++aa70lNFzqSPyzPuFYMsac98Ok40bQhz tWM0JB+0ivUM6+je9YLKfkeQjHbgy5tSwbAcNqA/M0JhtBFLaM4bNf9e6 vpHF6H6/qz9l5c6x8SlGygn7fuv3pGFk2Hg4cgHQv4SoQlwO6BKGqxCZK u11wrgcAOBUFyv3APKDDKAxi/kahZZABI+A5qLAVIN8s+68KM7whHhHNN g==; X-CSE-ConnectionGUID: C2sewkj4RcCnVEbJ+ZEIYA== X-CSE-MsgGUID: 0NT63QhlRy+aJAfkxR7hhg== X-IronPort-AV: E=McAfee;i="6700,10204,11222"; a="28198357" X-IronPort-AV: E=Sophos;i="6.11,199,1725346800"; d="scan'208";a="28198357" Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by orvoesa112.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2024 08:20:24 -0700 X-CSE-ConnectionGUID: /034jykKTiC4W+m608LOkg== X-CSE-MsgGUID: W/6UupdYQOSlYqkBscjugQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,205,1725346800"; d="scan'208";a="77549581" Received: from nirmoyda-mobl.ger.corp.intel.com (HELO [10.245.177.104]) ([10.245.177.104]) by fmviesa006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2024 08:20:22 -0700 Message-ID: <73ff671b-53c3-4a48-a499-44e18f2d2b7f@linux.intel.com> Date: Tue, 15 Oct 2024 17:20:17 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 16/26] drm/xe/guc: Update handling of xe_force_wake_get return To: Himal Prasad Ghimiray , intel-xe@lists.freedesktop.org Cc: Matthew Brost , Rodrigo Vivi , Lucas De Marchi References: <20241014075601.2324382-1-himal.prasad.ghimiray@intel.com> <20241014075601.2324382-17-himal.prasad.ghimiray@intel.com> Content-Language: en-US From: Nirmoy Das In-Reply-To: <20241014075601.2324382-17-himal.prasad.ghimiray@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On 10/14/2024 9:55 AM, Himal Prasad Ghimiray wrote: > xe_force_wake_get() now returns the reference count-incremented domain > mask. If it fails for individual domains, the return value will always > be 0. However, for XE_FORCEWAKE_ALL, it may return a non-zero value even > in the event of failure. Use helper xe_force_wake_ref_has_domain to > verify all domains are initialized or not. Update the return handling of > xe_force_wake_get() to reflect this behavior, and ensure that the return > value is passed as input to xe_force_wake_put(). > > v3 > - return xe_wakeref_t instead of int in xe_force_wake_get() > - xe_force_wake_put() error doesn't need to be checked. It internally > WARNS on domain ack failure. > > v5 > - return unsigned int from xe_force_wake_get() > - Remove redundant xe_gt_WARN_ON > > v6 > - use helper xe_force_wake_ref_has_domain() > > v7 > - Fix commit message > > v9 > - Rebase > > Cc: Matthew Brost > Cc: Rodrigo Vivi > Cc: Lucas De Marchi > Signed-off-by: Himal Prasad Ghimiray Reviewed-by: Nirmoy Das > --- > drivers/gpu/drm/xe/xe_guc.c | 13 ++++---- > drivers/gpu/drm/xe/xe_guc_log.c | 9 +++--- > drivers/gpu/drm/xe/xe_guc_pc.c | 50 ++++++++++++++++++------------ > drivers/gpu/drm/xe/xe_guc_submit.c | 6 ++-- > 4 files changed, 47 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c > index 8570b1218287..76437d42b8a1 100644 > --- a/drivers/gpu/drm/xe/xe_guc.c > +++ b/drivers/gpu/drm/xe/xe_guc.c > @@ -248,10 +248,11 @@ static void guc_fini_hw(void *arg) > { > struct xe_guc *guc = arg; > struct xe_gt *gt = guc_to_gt(guc); > + unsigned int fw_ref; > > - xe_gt_WARN_ON(gt, xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > xe_uc_fini_hw(&guc_to_gt(guc)->uc); > - xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > } > > /** > @@ -1155,14 +1156,14 @@ int xe_guc_start(struct xe_guc *guc) > void xe_guc_print_info(struct xe_guc *guc, struct drm_printer *p) > { > struct xe_gt *gt = guc_to_gt(guc); > + unsigned int fw_ref; > u32 status; > - int err; > int i; > > xe_uc_fw_print(&guc->fw, p); > > - err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT); > - if (err) > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT); > + if (!fw_ref) > return; > > status = xe_mmio_read32(>->mmio, GUC_STATUS); > @@ -1183,7 +1184,7 @@ void xe_guc_print_info(struct xe_guc *guc, struct drm_printer *p) > i, xe_mmio_read32(>->mmio, SOFT_SCRATCH(i))); > } > > - xe_force_wake_put(gt_to_fw(gt), XE_FW_GT); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > > xe_guc_ct_print(&guc->ct, p); > xe_guc_submit_print(guc, p); > diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c > index cc70f448d879..fead96216243 100644 > --- a/drivers/gpu/drm/xe/xe_guc_log.c > +++ b/drivers/gpu/drm/xe/xe_guc_log.c > @@ -145,8 +145,9 @@ struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct xe_guc_log *log, > struct xe_device *xe = log_to_xe(log); > struct xe_guc *guc = log_to_guc(log); > struct xe_gt *gt = log_to_gt(log); > + unsigned int fw_ref; > size_t remain; > - int i, err; > + int i; > > if (!log->bo) { > xe_gt_err(gt, "GuC log buffer not allocated\n"); > @@ -168,12 +169,12 @@ struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct xe_guc_log *log, > remain -= size; > } > > - err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT); > - if (err) { > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT); > + if (!fw_ref) { > snapshot->stamp = ~0; > } else { > snapshot->stamp = xe_mmio_read32(>->mmio, GUC_PMTIMESTAMP); > - xe_force_wake_put(gt_to_fw(gt), XE_FW_GT); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > } > snapshot->ktime = ktime_get_boottime_ns(); > snapshot->level = log->level; > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c > index 2b654f820ae2..e8b9faeaef64 100644 > --- a/drivers/gpu/drm/xe/xe_guc_pc.c > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c > @@ -415,22 +415,24 @@ u32 xe_guc_pc_get_act_freq(struct xe_guc_pc *pc) > int xe_guc_pc_get_cur_freq(struct xe_guc_pc *pc, u32 *freq) > { > struct xe_gt *gt = pc_to_gt(pc); > - int ret; > + unsigned int fw_ref; > > /* > * GuC SLPC plays with cur freq request when GuCRC is enabled > * Block RC6 for a more reliable read. > */ > - ret = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > - if (ret) > - return ret; > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) { > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > + return -ETIMEDOUT; > + } > > *freq = xe_mmio_read32(>->mmio, RPNSWREQ); > > *freq = REG_FIELD_GET(REQ_RATIO_MASK, *freq); > *freq = decode_freq(*freq); > > - XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > return 0; > } > > @@ -480,6 +482,7 @@ u32 xe_guc_pc_get_rpn_freq(struct xe_guc_pc *pc) > int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq) > { > struct xe_gt *gt = pc_to_gt(pc); > + unsigned int fw_ref; > int ret; > > mutex_lock(&pc->freq_lock); > @@ -493,9 +496,11 @@ int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq) > * GuC SLPC plays with min freq request when GuCRC is enabled > * Block RC6 for a more reliable read. > */ > - ret = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > - if (ret) > - goto out; > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) { > + ret = -ETIMEDOUT; > + goto fw; > + } > > ret = pc_action_query_task_state(pc); > if (ret) > @@ -504,7 +509,7 @@ int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, u32 *freq) > *freq = pc_get_min_freq(pc); > > fw: > - XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > out: > mutex_unlock(&pc->freq_lock); > return ret; > @@ -855,6 +860,7 @@ int xe_guc_pc_gucrc_disable(struct xe_guc_pc *pc) > { > struct xe_device *xe = pc_to_xe(pc); > struct xe_gt *gt = pc_to_gt(pc); > + unsigned int fw_ref; > int ret = 0; > > if (xe->info.skip_guc_pc) > @@ -864,13 +870,15 @@ int xe_guc_pc_gucrc_disable(struct xe_guc_pc *pc) > if (ret) > return ret; > > - ret = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > - if (ret) > - return ret; > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) { > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > + return -ETIMEDOUT; > + } > > xe_gt_idle_disable_c6(gt); > > - XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > > return 0; > } > @@ -956,13 +964,16 @@ int xe_guc_pc_start(struct xe_guc_pc *pc) > struct xe_device *xe = pc_to_xe(pc); > struct xe_gt *gt = pc_to_gt(pc); > u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data)); > + unsigned int fw_ref; > int ret; > > xe_gt_assert(gt, xe_device_uc_enabled(xe)); > > - ret = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > - if (ret) > - return ret; > + fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) { > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > + return -ETIMEDOUT; > + } > > if (xe->info.skip_guc_pc) { > if (xe->info.platform != XE_PVC) > @@ -1005,7 +1016,7 @@ int xe_guc_pc_start(struct xe_guc_pc *pc) > ret = pc_action_setup_gucrc(pc, GUCRC_FIRMWARE_CONTROL); > > out: > - XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > + xe_force_wake_put(gt_to_fw(gt), fw_ref); > return ret; > } > > @@ -1037,18 +1048,19 @@ static void xe_guc_pc_fini_hw(void *arg) > { > struct xe_guc_pc *pc = arg; > struct xe_device *xe = pc_to_xe(pc); > + unsigned int fw_ref; > > if (xe_device_wedged(xe)) > return; > > - XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL)); > + fw_ref = xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL); > xe_guc_pc_gucrc_disable(pc); > XE_WARN_ON(xe_guc_pc_stop(pc)); > > /* Bind requested freq to mert_freq_cap before unload */ > pc_set_cur_freq(pc, min(pc_max_freq_cap(pc), pc->rpe_freq)); > > - xe_force_wake_put(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL); > + xe_force_wake_put(gt_to_fw(pc_to_gt(pc)), fw_ref); > } > > /** > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c > index 0e5649b394b6..fc8ababc79fb 100644 > --- a/drivers/gpu/drm/xe/xe_guc_submit.c > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c > @@ -1098,6 +1098,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job) > struct xe_guc *guc = exec_queue_to_guc(q); > const char *process_name = "no process"; > struct xe_device *xe = guc_to_xe(guc); > + unsigned int fw_ref; > int err = -ETIME; > pid_t pid = -1; > int i = 0; > @@ -1135,12 +1136,13 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job) > if (!exec_queue_killed(q) && !xe->devcoredump.captured && > !xe_guc_capture_get_matching_and_lock(job)) { > /* take force wake before engine register manual capture */ > - if (xe_force_wake_get(gt_to_fw(q->gt), XE_FORCEWAKE_ALL)) > + fw_ref = xe_force_wake_get(gt_to_fw(q->gt), XE_FORCEWAKE_ALL); > + if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) > xe_gt_info(q->gt, "failed to get forcewake for coredump capture\n"); > > xe_engine_snapshot_capture_for_job(job); > > - xe_force_wake_put(gt_to_fw(q->gt), XE_FORCEWAKE_ALL); > + xe_force_wake_put(gt_to_fw(q->gt), fw_ref); > } > > /*