<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 09-04-2024 21:41, Michal Wajdeczko wrote: </div> <blockquote type="cite" cite="mid:3970f70e-db7a-405b-a027-ea5195056639@intel.com"> <pre class="moz-quote-pre" wrap=""> On 09.04.2024 17:45, Ghimiray, Himal Prasad wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> On 09-04-2024 15:05, Michal Wajdeczko wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> On 08.04.2024 22:23, Ghimiray, Himal Prasad wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">On 08-04-2024 23:44, Michal Wajdeczko wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Many of the GuC actions use KLVs to pass additional parameters or configuration data. Add few helper functions for better reporting any information related to KLVs. Signed-off-by: Michal Wajdeczko<a class="moz-txt-link-rfc2396E" href="mailto:michal.wajdeczko@intel.com"><michal.wajdeczko@intel.com></a> --- drivers/gpu/drm/xe/Makefile | 1 + drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 133 ++++++++++++++++++++++++ drivers/gpu/drm/xe/xe_guc_klv_helpers.h | 51 +++++++++ 3 files changed, 185 insertions(+) create mode 100644 drivers/gpu/drm/xe/xe_guc_klv_helpers.c create mode 100644 drivers/gpu/drm/xe/xe_guc_klv_helpers.h diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index e5b1715f721e..ec0d1fb49b1e 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -98,6 +98,7 @@ xe-y += xe_bb.o \ xe_guc_debugfs.o \ xe_guc_hwconfig.o \ xe_guc_id_mgr.o \ + xe_guc_klv_helpers.o \ xe_guc_log.o \ xe_guc_pc.o \ xe_guc_submit.o \ diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c new file mode 100644 index 000000000000..c39fda08afcb --- /dev/null +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include <linux/bitfield.h> +#include <drm/drm_print.h> + +#include "abi/guc_klvs_abi.h" +#include "xe_guc_klv_helpers.h" + +#define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo))) </pre> </blockquote> <pre class="moz-quote-pre" wrap="">is this macro necessary, it's being called in one place ? ((u64)hi << 32) | lo should be enough. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">IMO it is better to have a macro (with name) than a magic formula besides, this macro here is (I hope) just temporary as either: - it will be promoted to xe_macros.h [1], or - something similar will be available in wordpart.h [2] [1]<a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/129854/">https://patchwork.freedesktop.org/series/129854/</a> [2] <a class="moz-txt-link-freetext" href="https://lore.kernel.org/lkml/20240214214654.1700-1-michal.wajdeczko@intel.com/">https://lore.kernel.org/lkml/20240214214654.1700-1-michal.wajdeczko@intel.com/</a> </pre> </blockquote> <pre class="moz-quote-pre" wrap="">Thanks for the info. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ +/** + * xe_guc_klv_key_to_string - Convert KLV key into friendly name. + * @key: the `GuC KLV`_ key + * + * Return: name of the KLV key. + */ +const char *xe_guc_klv_key_to_string(u16 key) +{ + switch (key) { + /* VGT POLICY keys */ + case GUC_KLV_VGT_POLICY_SCHED_IF_IDLE_KEY: + return "sched_if_idle"; + case GUC_KLV_VGT_POLICY_ADVERSE_SAMPLE_PERIOD_KEY: + return "sample_period"; + case GUC_KLV_VGT_POLICY_RESET_AFTER_VF_SWITCH_KEY: + return "reset_engine"; + /* VF CFG keys */ + case GUC_KLV_VF_CFG_GGTT_START_KEY: + return "ggtt_start"; + case GUC_KLV_VF_CFG_GGTT_SIZE_KEY: + return "ggtt_size"; + case GUC_KLV_VF_CFG_LMEM_SIZE_KEY: + return "lmem_size"; + case GUC_KLV_VF_CFG_NUM_CONTEXTS_KEY: + return "num_contexts"; + case GUC_KLV_VF_CFG_TILE_MASK_KEY: + return "tile_mask"; + case GUC_KLV_VF_CFG_NUM_DOORBELLS_KEY: + return "num_doorbells"; + case GUC_KLV_VF_CFG_EXEC_QUANTUM_KEY: + return "exec_quantum"; + case GUC_KLV_VF_CFG_PREEMPT_TIMEOUT_KEY: + return "preempt_timeout"; + case GUC_KLV_VF_CFG_BEGIN_DOORBELL_ID_KEY: + return "begin_db_id"; + case GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_KEY: + return "begin_ctx_id"; </pre> </blockquote> <pre class="moz-quote-pre" wrap="">default: return "(unknown)"; Have seen warning/errors if default is not defined in switch-case. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">likely compiler was complaining because of enum being used in switch statement, but here it is u16, so there shouldn't be any warnings but can add 'default' case if you consider that as a blocker </pre> </blockquote> <pre class="moz-quote-pre" wrap="">I have no strong objection on this, if compiler is not complaining. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ } + return "(unknown)"; </pre> </blockquote> <pre class="moz-quote-pre" wrap="">remove. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+} + +/** + * xe_guc_klv_print - Print content of the buffer with `GuC KLV`_. + * @klvs: the buffer with KLVs + * @num_dwords: number of dwords (u32) available in the buffer + * @p: the &drm_printer + * + * The buffer may contain more than one KLV. + */ +void xe_guc_klv_print(const u32 *klvs, u32 num_dwords, struct drm_printer *p) +{ + while (num_dwords >= GUC_KLV_LEN_MIN) { + u32 key = FIELD_GET(GUC_KLV_0_KEY, klvs[0]); + u32 len = FIELD_GET(GUC_KLV_0_LEN, klvs[0]); + + klvs += GUC_KLV_LEN_MIN; + num_dwords -= GUC_KLV_LEN_MIN; + + if (num_dwords < len) { + drm_printf(p, "{ key %#06x : truncated %zu of %zu bytes %*ph } # %s\n", + key, num_dwords * sizeof(u32), len * sizeof(u32), + (int)(num_dwords * sizeof(u32)), klvs, + xe_guc_klv_key_to_string(key)); + return; + } + + switch (len) { + case 0: + drm_printf(p, "{ key %#06x : no value } # %s\n", + key, xe_guc_klv_key_to_string(key)); + break; + case 1: + drm_printf(p, "{ key %#06x : 32b value %u } # %s\n", + key, klvs[0], xe_guc_klv_key_to_string(key)); + break; + case 2: + drm_printf(p, "{ key %#06x : 64b value %#llx } # %s\n", + key, make_u64(klvs[1], klvs[0]), + xe_guc_klv_key_to_string(key)); + break; + default: + drm_printf(p, "{ key %#06x : %zu bytes %*ph } # %s\n", + key, len * sizeof(u32), (int)(len * sizeof(u32)), + klvs, xe_guc_klv_key_to_string(key)); + break; + } + + klvs += len; + num_dwords -= len; + } + + /* we don't expect any leftovers, fix if KLV header is ever changed */ + BUILD_BUG_ON(GUC_KLV_LEN_MIN > 1); +} + +/** + * xe_guc_klv_count - Count KLVs present in the buffer. + * @klvs: the buffer with KLVs + * @num_dwords: number of dwords (u32) in the buffer + * + * Return: number of recognized KLVs or + * a negative error code if KLV buffer is truncated. </pre> </blockquote> </blockquote> <pre class="moz-quote-pre" wrap="">see here ^^^ </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ */ +int xe_guc_klv_count(const u32 *klvs, u32 num_dwords) +{ + int num_klvs = 0; + + while (num_dwords >= GUC_KLV_LEN_MIN) { + u32 len = FIELD_GET(GUC_KLV_0_LEN, klvs[0]); + + if (num_dwords < len + GUC_KLV_LEN_MIN) + return -ENODATA; + + klvs += GUC_KLV_LEN_MIN + len; + num_dwords -= GUC_KLV_LEN_MIN + len; + num_klvs++; + } + + return num_dwords ? -ENODATA : num_klvs; </pre> </blockquote> <pre class="moz-quote-pre" wrap="">while loop can be terminated only if num_dwords < len + GUC_KLV_LEN_MIN or num_dwords is 0. Therefore return num_klvs; should be fine. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">but then we won't catch the case with truncated buffer (which indicates bug somewhere in code or firmware) and it is one of the function goal and return value </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> I fail to understand in current implementation, even if there is bug somewhere in code or firmware how return num_dwords ? -ENODATA : num_klvs; can return -ENODATA ? num_dwords is u32 so always >= 0 and while loop keeps running unless num_dwords < len + GUC_KLV_LEN_MIN </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> loop keeps running unless (num_dwords < GUC_KLV_LEN_MIN) there is no 'len' in this condition </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> or num_dwords is 0. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> that's true but only because you've looked at the value of GUC_KLV_LEN_MIN which happen to be 1 </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">In case of num_dwords < len + GUC_KLV_LEN_MIN it will return -ENODATA in above loop itself. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> this early return inside the loop was for cases where the buffer was truncated in such a way that it contains the KLV header but doesn't hold full value, while the final return was to catch the case where the buffer was truncated at the KLV header itself so yes, with GUC_KLV_LEN_MIN=1 the num_dwords outside the loop will be always 0, making this final condition questionable, but that requires knowledge of the GUC_KLV_LEN_MIN</pre> </blockquote> Yes , my comments are on the basis of GUC_KLV_LEN_MIN value defined in guc_klvs_abi.h <blockquote type="cite" cite="mid:3970f70e-db7a-405b-a027-ea5195056639@intel.com"> <pre class="moz-quote-pre" wrap=""> maybe best option to have generic solution would be to just break the loop on (num_dwords < len + GUC_KLV_LEN_MIN) instead doing early return</pre> </blockquote> <span style="color: rgb(13, 13, 13); font-family: Söhne, ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Ubuntu, Cantarell, "Noto Sans", sans-serif, "Helvetica Neue", Arial, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; font-size: 16px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: pre-wrap; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">This approach separates out the condition expression and return at the expense of extra comparison. I think that's ok. <blockquote type="cite" cite="mid:3970f70e-db7a-405b-a027-ea5195056639@intel.com"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+} diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h new file mode 100644 index 000000000000..b835e0ebe6db --- /dev/null +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2024 Intel Corporation + */ + +#ifndef _XE_GUC_KLV_HELPERS_H_ +#define _XE_GUC_KLV_HELPERS_H_ + +#include <linux/types.h> + +struct drm_printer; + +const char *xe_guc_klv_key_to_string(u16 key); + +void xe_guc_klv_print(const u32 *klvs, u32 num_dwords, struct drm_printer *p); +int xe_guc_klv_count(const u32 *klvs, u32 num_dwords); + +/** + * PREP_GUC_KLV - Prepare KLV header value based on provided key and len. + * @key: KLV key + * @len: KLV length + * + * Return: value of the KLV header (u32). + */ +#define PREP_GUC_KLV(key, len) \ + (FIELD_PREP(GUC_KLV_0_KEY, (key)) | \ + FIELD_PREP(GUC_KLV_0_LEN, (len))) + +/** + * PREP_GUC_KLV_CONST - Prepare KLV header value based on const key and len. + * @key: const KLV key + * @len: const KLV length + * + * Return: value of the KLV header (u32). + */ +#define PREP_GUC_KLV_CONST(key, len) \ + (FIELD_PREP_CONST(GUC_KLV_0_KEY, (key)) | \ + FIELD_PREP_CONST(GUC_KLV_0_LEN, (len))) + +/** + * PREP_GUC_KLV_TAG - Prepare KLV header value based on unique KLV definition tag. + * @TAG: unique tag of the KLV definition + * + * Combine separate KEY and LEN definitions of the KLV identified by the TAG. + * + * Return: value of the KLV header (u32). + */ +#define PREP_GUC_KLV_TAG(TAG) \ + PREP_GUC_KLV_CONST(GUC_KLV_##TAG##_KEY, GUC_KLV_##TAG##_LEN) + +#endif </pre> </blockquote> </blockquote> </blockquote> </blockquote> </blockquote> </body> </html>