John Harrison <john.c.harrison@xxxxxxxxx> writes:
> On 11/1/2021 08:39, 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.
>>>
>>> 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,
>>> };
>> Seems simple enough, so why doesn't i915 define the format of the
>> returned hwconfig blob in i915_drm.h?
> Because the definition is nothing to do with i915. This table comes from
> the hardware spec. It is not defined by the KMD and it is not currently
> used by the KMD. So there is no reason for the KMD to be creating
> structures for it in the same way that the KMD does not document,
> define, struct, etc. every other feature of the hardware that the UMDs
> might use.
So, i915 wants to wash it's hands completely of the format? There is
obviously a difference between hardware features and a blob coming from
closed source software. (Which i915 just happens to be passing along.)
The hardware is a lot more difficult to change...
It seems like these details should be dropped from the i915 patch commit
message since i915 wants nothing to do with it.
I would think it'd be preferable for i915 to stand behind the basic blob
format as is (even if the keys/values can't be defined), and make a new
query item if the closed source software changes the format.
Of course, it'd be even better if i915 could define some keys/values as
well. (Or if a spec could be released to help document / tie down the
format.)
>>
>> struct drm_i915_hwconfig {
>> uint32_t key;
>> uint32_t length;
>> uint32_t values[];
>> };
>>
>> It sounds like the kernel depends on the closed source guc being loaded
>> to return this information. Is that right? Will i915 also become
>> dependent on some of this data such that it won't be able to initialize
>> without the firmware being loaded?
> At the moment, the KMD does not use the table at all. We merely provide
> a mechanism for the UMDs to retrieve it from the hardware.
>
> In terms of future direction, that is something you need to take up with
> the hardware architects.
>
Why do you keep saying hardware, when only software is involved?
>
>>> The attribute ids are defined in a hardware spec.
>> Which spec?
>
> Unfortunately, it is not one that is currently public. We are pushing
> the relevant people to get it included in the public bspec / HRM. It is
> a slow process :(.
>
Right. I take it no progress has been made in the 1.5 months since you
posted this, so it'll probably finally be documented 6~12 months after
hardware is available? :)
-Jordan
