On 1/27/2022 16:48, Jordan Justen wrote:
John.C.Harrison@xxxxxxxxx writes:
From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
GuC contains a consolidated table with a bunch of information about the
current device.
Previously, this information was spread and hardcoded to all the components
including GuC, i915 and various UMDs. The goal here is to consolidate
the data into GuC in a way that all interested components can grab the
very latest and synchronized information using a simple query.
This "consolidate" goal is not what I was told for the purpose of this.
I don't think these paragraphs are the true.
The intention is to remove multiple hardcoded tables spread across a
bunch of different drivers and replace them with a single table
retrieved from the hardware itself. That sounds like consolidation to me.
As per most of the other queries, this one can be called twice.
Once with item.length=0 to determine the exact buffer size, then
allocate the user memory and call it again for to retrieve the
table data. For example:
struct drm_i915_query_item item = {
.query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
};
query.items_ptr = (int64_t) &item;
query.num_items = 1;
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
if (item.length <= 0)
return -ENOENT;
data = malloc(item.length);
item.data_ptr = (int64_t) &data;
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
// Parse the data as appropriate...
The returned array is a simple and flexible KLV (Key/Length/Value)
formatted table. For example, it could be just:
enum device_attr {
ATTR_SOME_VALUE = 0,
ATTR_SOME_MASK = 1,
};
static const u32 hwconfig[] = {
ATTR_SOME_VALUE,
1, // Value Length in DWords
8, // Value
ATTR_SOME_MASK,
3,
0x00FFFFFFFF, 0xFFFFFFFF, 0xFF000000,
};
You said on 03 Nov 2021 that you would remove the parts of this commit
message that document the format. Why? Because i915 will not make any
guarantees as to the format of what is returned. Thus, i915 should not
comment on the format.
And you replied that you would prefer to keep it.
Can you Cc me on future postings of this patch?
The attribute ids are defined in a hardware spec.
As this spec is not published, it's hard to verify or refute this claim.
Think this is a more accurate commit message for this patch:
In this interface i915 is returning a currently undocumented blob of
data which it receives from the closed source guc software. The
format of this blob *might* be defined in a hardware spec in the
future.
I'm sure you will prefer to replace "might" with "is planned to". I
think "might" is more accurate, but I suppose the other would be
acceptable.
-Jordan
Getting brand new spec documents published is not a fast process. That
doesn't mean it isn't going to happen. Also, just because a document is
currently confidential and private doesn't mean that it doesn't exist.
John.
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Cc: Kenneth Graunke <kenneth.w.graunke@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Slawomir Milczarek <slawomir.milczarek@xxxxxxxxx>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_query.c | 23 +++++++++++++++++++++++
include/uapi/drm/i915_drm.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..609e64d5f395 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private *i915,
return total_length;
}
+static int query_hwconfig_table(struct drm_i915_private *i915,
+ struct drm_i915_query_item *query_item)
+{
+ struct intel_gt *gt = to_gt(i915);
+ struct intel_guc_hwconfig *hwconfig = >->uc.guc.hwconfig;
+
+ if (!hwconfig->size || !hwconfig->ptr)
+ return -ENODEV;
+
+ if (query_item->length == 0)
+ return hwconfig->size;
+
+ if (query_item->length < hwconfig->size)
+ return -EINVAL;
+
+ if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+ hwconfig->ptr, hwconfig->size))
+ return -EFAULT;
+
+ return hwconfig->size;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) = {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+ query_hwconfig_table,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..132515199f27 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
#define DRM_I915_QUERY_ENGINE_INFO 2
#define DRM_I915_QUERY_PERF_CONFIG 3
#define DRM_I915_QUERY_MEMORY_REGIONS 4
+#define DRM_I915_QUERY_HWCONFIG_TABLE 5
/* Must be kept compact -- no holes and well documented */
/**
--
2.25.1
