[Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

John Harrison <john.c.harrison at intel.com> writes:

> On 11/1/2021 08:39, Jordan Justen wrote:
>> <John.C.Harrison at Intel.com> writes:
>>
>>> From: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>
>>> 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