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

[Jordan Justen]

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

> On 1/27/2022 16:48, 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.
>> 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.

That is not what I was told. That is apparently what someone is trying
to sell here.

Mesa would prefer to "hardcode" info rather than depend on the closed
source guc software.

>>
>>> 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.

No, I did not.

You said, "Sure. Can remove comments." to which, I replied, "Obviously
not what should be done, but apparently all i915 is willing to do."

So, i915 should document and stand behind this blob's format. But, if
they are not willing to, they shouldn't half-heartedly put some text in
a commit message.

>>
>> 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.

Heh.

Have you learned anything new about the status of it in the past 3
months?

> That doesn't mean it isn't going to happen.

It also doesn't mean it is going to happen either. Maybe you want to add
some text wherein Intel guarantees that it will be released in a spec by
some date?

> Also, just because a document is currently confidential and private
> doesn't mean that it doesn't exist.

Should we add "This is documented in a private spec, so it really does
exist!"?

-Jordan