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

[John Harrison]

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.

>
>> 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 at linux.intel.com>
>> Cc: Kenneth Graunke <kenneth.w.graunke at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Slawomir Milczarek <slawomir.milczarek at intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> Reviewed-by: Matthew Brost <matthew.brost at intel.com>
>> ---
>>   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 = &gt->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