| Message ID | 20210916184012.2642295-3-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for querying hw info that UMDs need | expand |
<John.C.Harrison@Intel.com> writes: > From: Rodrigo Vivi <rodrigo.vivi@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? 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? > The attribute ids are defined in a hardware spec. Which spec? -Jordan
On 11/1/2021 08:39, Jordan Justen wrote: > <John.C.Harrison@Intel.com> writes: > >> From: Rodrigo Vivi <rodrigo.vivi@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. > > 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. >> 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 :(. John. > > -Jordan
John Harrison <john.c.harrison@intel.com> writes: > On 11/1/2021 08:39, Jordan Justen wrote: >> <John.C.Harrison@Intel.com> writes: >> >>> From: Rodrigo Vivi <rodrigo.vivi@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
On 11/3/2021 14:38, Jordan Justen wrote: > John Harrison <john.c.harrison@intel.com> writes: > >> On 11/1/2021 08:39, Jordan Justen wrote: >>> <John.C.Harrison@Intel.com> writes: >>> >>>> From: Rodrigo Vivi <rodrigo.vivi@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... Actually, no. The table is not "coming from closed source software". The table is defined by hardware specs. It is a table of hardware specific values. It is not being invented by the GuC just for fun or as a way to subvert the universe into the realms of closed source software. As per KMD, GuC is merely passing the table through. The table is only supported on newer hardware platforms and all GuC does is provide a mechanism for the KMD to retrieve it because the KMD cannot access it directly. The table contents are defined by hardware architects same as all the other aspects of the hardware. > > It seems like these details should be dropped from the i915 patch commit > message since i915 wants nothing to do with it. Sure. Can remove comments. > > 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. Close source software is not allowed to change the format because closed source software has no say in defining the format. The format is officially defined as being fixed in the spec. New key values can be added to the key enumeration but existing values cannot be deprecated and re-purposed. The table must be stable across all OSs and all platforms. No software can arbitrarily decide to change it. > > 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.) See the corresponding IGT test that details all the currently defined keys. > >>> 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? See above - because the table is defined by hardware. No software, closed or open, has any say in the specification of the table. John. > >>>> 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
John Harrison <john.c.harrison@intel.com> writes: > On 11/3/2021 14:38, Jordan Justen wrote: >> 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... > Actually, no. The table is not "coming from closed source software". The > table is defined by hardware specs. It is a table of hardware specific > values. So, guc literally reads this info from the hardware verbatim? Then gives it to i915 so i915 can give it to UMDs? Otherwise, it seems like a table in software. Anyway... >> It seems like these details should be dropped from the i915 patch commit >> message since i915 wants nothing to do with it. > Sure. Can remove comments. Obviously not what should be done, but apparently all i915 is willing to do. >> 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. > Close source software is not allowed to change the format because closed > source software has no say in defining the format. So, why can't i915 define this extremely simple (apparently unchangeable) blob format, and thereby guarantee that it will have to insulate UMDs if the format changes by making a different query item? It ought to be made painful for everyone if this blob format changes. Hopefully the format will basically never change. (Even if new keys/values might be added.) Further, it seems there is an implication that the keys will always be backward compatible. Is that true, and if so, how can there be harm in i915 enumerating the "known" ones? >> 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.) > See the corresponding IGT test that details all the currently defined keys. i915 can't/won't say anything about it, but look at this unmerged IGT test? In the next sentence you'll say, but don't count on that because IGT really has no control over it. :) >>>>> 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? :) Apparently all the data for this spec is "available" (in an unmerged IGT patch), but am I correct in assuming that no actual spec timeframe is defined? -Jordan
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 5e2b909827f4..96989a37453c 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -477,12 +477,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 = &i915->gt; + 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 bde5860b3686..a1281f35b190 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2499,6 +2499,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 */ /**