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 192F9C27C79 for ; Wed, 12 Jun 2024 02:10:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B595910E1DA; Wed, 12 Jun 2024 02:10:47 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="cvrQ7vAl"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9448C10E1FA for ; Wed, 12 Jun 2024 02:10:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1718158244; x=1749694244; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=DlPYU8/1RYVDGyiX2nQOoaYdd6gx7S6kG3S8mKIHeEo=; b=cvrQ7vAlyxEstjgyUKiD34bJPg/lZmfHsSNeVF/zCIq/EYG+dclyGXS0 CQbFb4OgAunuNQH+1a6W9FVTa3+/jUnY/IE2QwG3wibzQpLY9bkYMU85F LIZqbdb+NphbPUmj9F4TN/ZLKsqdXAWdqVcSrLS/6iP3wHTKSmCitDpOB UDNNLX7WMRWarkfsGp+wS/p0suMUxfAGmaQhx5PxBChnvO5f/Ib9eRcE9 OiFXWzwLyLlxA6GK2KVGBCjogMhkCuTYzZtxTWL35ov2blumuwiGMBu3o RZvOKkYNHir+qMF5I0fVAP7y2U3pIPpUb7o64NjTGybC4tzFBg9IXJ7Cv w==; X-CSE-ConnectionGUID: JZKPKRl+Q8WXLYVavVCOeQ== X-CSE-MsgGUID: ADEy1dEjR36uzRklGW9edQ== X-IronPort-AV: E=McAfee;i="6600,9927,11100"; a="40306804" X-IronPort-AV: E=Sophos;i="6.08,231,1712646000"; d="scan'208";a="40306804" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2024 19:10:44 -0700 X-CSE-ConnectionGUID: Ri+Z9ug7QCWmcO8QILNsRg== X-CSE-MsgGUID: SIwyESAWRmapK4GDC4dhEw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,231,1712646000"; d="scan'208";a="39691343" Received: from jrglassb-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.125.0.65]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2024 19:10:43 -0700 Date: Tue, 11 Jun 2024 19:04:09 -0700 Message-ID: <87v82ezzl2.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Michal Wajdeczko Cc: Subject: Re: [PATCH 15/17] drm/xe/oa: Override GuC RC with OA on PVC In-Reply-To: <3facdf80-ca8a-41ee-9f5c-59b9c1e41beb@intel.com> References: <20240607204322.1966831-1-ashutosh.dixit@intel.com> <20240607204322.1966831-16-ashutosh.dixit@intel.com> <3facdf80-ca8a-41ee-9f5c-59b9c1e41beb@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII 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 Sat, 08 Jun 2024 04:45:15 -0700, Michal Wajdeczko wrote: > Hi Michal, > On 07.06.2024 22:43, Ashutosh Dixit wrote: > > On PVC, a w/a resets RCS/CCS before it goes into RC6. This breaks OA since > > OA does not expect engine resets during its use. Fix it by disabling RC6. > > > > Reviewed-by: Umesh Nerlige Ramappa > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/xe/xe_guc_pc.c | 57 ++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/xe/xe_guc_pc.h | 3 ++ > > drivers/gpu/drm/xe/xe_oa.c | 23 +++++++++++++ > > drivers/gpu/drm/xe/xe_oa_types.h | 3 ++ > > 4 files changed, 86 insertions(+) > > > > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c > > index 508f0d39b4ad..9f9a4132c399 100644 > > --- a/drivers/gpu/drm/xe/xe_guc_pc.c > > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c > > @@ -24,6 +24,7 @@ > > #include "xe_map.h" > > #include "xe_mmio.h" > > #include "xe_pcode.h" > > +#include "xe_pm.h" > > > > #define MCHBAR_MIRROR_BASE_SNB 0x140000 > > > > @@ -191,6 +192,27 @@ static int pc_action_set_param(struct xe_guc_pc *pc, u8 id, u32 value) > > return ret; > > } > > > > +static int pc_action_unset_param(struct xe_guc_pc *pc, u8 id) > > +{ > > + struct xe_guc_ct *ct = &pc_to_guc(pc)->ct; > > + int ret; > > Xe driver BKM is to keep var declarations in reversed xmas tree order Fixed. > > > + u32 action[] = { > > + GUC_ACTION_HOST2GUC_PC_SLPC_REQUEST, > > + SLPC_EVENT(SLPC_EVENT_PARAMETER_UNSET, 1), > > + id, > > + }; > > + > > + if (wait_for_pc_state(pc, SLPC_GLOBAL_STATE_RUNNING)) > > + return -EAGAIN; > > + > > + ret = xe_guc_ct_send(ct, action, ARRAY_SIZE(action), 0, 0); > > + if (ret) > > + drm_err(&pc_to_xe(pc)->drm, "GuC PC unset param failed: %pe", > > + ERR_PTR(ret)); > > please use xe_gt_err(gt, ...) Done. > > hmm, but I can see right now that this is a common mistake in this file, > likely we need to fix that first Looks like you already fixed that :) > > > + > > + return ret; > > +} > > + > > static int pc_action_setup_gucrc(struct xe_guc_pc *pc, u32 mode) > > { > > struct xe_guc_ct *ct = &pc_to_guc(pc)->ct; > > @@ -773,6 +795,41 @@ int xe_guc_pc_gucrc_disable(struct xe_guc_pc *pc) > > return 0; > > } > > > > +/** > > + * xe_guc_pc_override_gucrc_mode - override GUCRC mode > > + * @pc: Xe_GuC_PC instance > > + * @mode: new value of the mode. > > + * > > + * Return: 0 on success, negative error code on error > > + */ > > +int xe_guc_pc_override_gucrc_mode(struct xe_guc_pc *pc, enum slpc_gucrc_mode mode) > > +{ > > + int ret; > > + > > + xe_pm_runtime_get(pc_to_xe(pc)); > > + ret = pc_action_set_param(pc, SLPC_PARAM_PWRGATE_RC_MODE, mode); > > + xe_pm_runtime_put(pc_to_xe(pc)); > > + > > + return ret; > > +} > > + > > +/** > > + * xe_guc_pc_unset_gucrc_mode - unset GUCRC mode override > > + * @pc: Xe_GuC_PC instance > > + * > > + * Return: 0 on success, negative error code on error > > + */ > > +int xe_guc_pc_unset_gucrc_mode(struct xe_guc_pc *pc) > > +{ > > + int ret; > > + > > + xe_pm_runtime_get(pc_to_xe(pc)); > > + ret = pc_action_unset_param(pc, SLPC_PARAM_PWRGATE_RC_MODE); > > + xe_pm_runtime_put(pc_to_xe(pc)); > > + > > + return ret; > > +} > > + > > static void pc_init_pcode_freq(struct xe_guc_pc *pc) > > { > > u32 min = DIV_ROUND_CLOSEST(pc->rpn_freq, GT_FREQUENCY_MULTIPLIER); > > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h b/drivers/gpu/drm/xe/xe_guc_pc.h > > index 532cac985a6d..eb5bba9f0736 100644 > > --- a/drivers/gpu/drm/xe/xe_guc_pc.h > > +++ b/drivers/gpu/drm/xe/xe_guc_pc.h > > @@ -9,11 +9,14 @@ > > #include > > > > struct xe_guc_pc; > > +#include "abi/guc_actions_slpc_abi.h" > > bad order (all #includes shall be before forward decls) but... > > that's unfortunate that you need ABI definitions here at all (as slpc > gucrc modes were defined as enum not plain values) but maybe it would be > sufficient to just use forward declaration of that enum: > > enum slpc_gucrc_mode; Done. > > > > > int xe_guc_pc_init(struct xe_guc_pc *pc); > > int xe_guc_pc_start(struct xe_guc_pc *pc); > > int xe_guc_pc_stop(struct xe_guc_pc *pc); > > int xe_guc_pc_gucrc_disable(struct xe_guc_pc *pc); > > +int xe_guc_pc_override_gucrc_mode(struct xe_guc_pc *pc, enum slpc_gucrc_mode mode); > > +int xe_guc_pc_unset_gucrc_mode(struct xe_guc_pc *pc); > > > > 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); > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c > > index 5cb0cbb58a1d..c89b11f5e01e 100644 > > --- a/drivers/gpu/drm/xe/xe_oa.c > > +++ b/drivers/gpu/drm/xe/xe_oa.c > > @@ -24,6 +24,7 @@ > > #include "xe_force_wake.h" > > #include "xe_gt.h" > > #include "xe_gt_mcr.h" > > +#include "xe_guc_pc.h" > > #include "xe_lrc.h" > > #include "xe_macros.h" > > #include "xe_mmio.h" > > @@ -815,6 +816,10 @@ static void xe_oa_stream_destroy(struct xe_oa_stream *stream) > > XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > > xe_pm_runtime_put(stream->oa->xe); > > > > + /* Wa_1509372804:pvc: Unset the override of GUCRC mode to enable rc6 */ > > + if (stream->override_gucrc) > > + XE_WARN_ON(xe_guc_pc_unset_gucrc_mode(>->uc.guc.pc)); > > that doesn't look like a good pattern, more clear way would be: > > if (...) { > err = xe_guc_pc_unset_gucrc_mode(>->uc.guc.pc); > xe_gt_WARN(gt, err, "explain what just happen"); > } Changed to xe_gt_WARN_ON, that should be sufficient. > > > + > > xe_oa_free_configs(stream); > > } > > > > @@ -1303,6 +1308,21 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream, > > goto exit; > > } > > > > + /* > > + * Wa_1509372804:pvc > > + * > > + * GuC reset of engines causes OA to lose configuration > > + * state. Prevent this by overriding GUCRC mode. > > + */ > > + if (stream->oa->xe->info.platform == XE_PVC) { > > + ret = xe_guc_pc_override_gucrc_mode(>->uc.guc.pc, > > + SLPC_GUCRC_MODE_GUCRC_NO_RC6); > > + if (ret) > > + goto err_free_configs; > > + > > + stream->override_gucrc = true; > > + } > > + > > /* Take runtime pm ref and forcewake to disable RC6 */ > > xe_pm_runtime_get(stream->oa->xe); > > XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > > @@ -1349,6 +1369,9 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream, > > err_fw_put: > > XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL)); > > xe_pm_runtime_put(stream->oa->xe); > > + if (stream->override_gucrc) > > + XE_WARN_ON(xe_guc_pc_unset_gucrc_mode(>->uc.guc.pc)); > > ditto Changed to xe_gt_WARN_ON. > > > +err_free_configs: > > xe_oa_free_configs(stream); > > exit: > > return ret; > > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h > > index 7f7c84e4b3a6..7775fe91616f 100644 > > --- a/drivers/gpu/drm/xe/xe_oa_types.h > > +++ b/drivers/gpu/drm/xe/xe_oa_types.h > > @@ -220,6 +220,9 @@ struct xe_oa_stream { > > /** @poll_period_ns: hrtimer period for checking OA buffer for available data */ > > u64 poll_period_ns; > > > > + /** @override_gucrc: GuC RC has been overridden for the OA stream */ > > + bool override_gucrc; > > + > > /** @oa_status: temporary storage for oa_status register value */ > > u32 oa_status; > > }; Thanks. -- Ashutosh