| Message ID | 20180117153902.9638-7-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/01/18 15:39, Lionel Landwerlin wrote: > With the introduction of asymmetric slices in CNL, we cannot rely on > the previous SUBSLICE_MASK getparam to tell userspace what subslices > are available. Here we introduce a more detailed way of querying the > Gen's GPU topology that doesn't aggregate numbers. > > This is essential for monitoring parts of the GPU with the OA unit, > because counters need to be normalized to the number of > EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do > not gives us sufficient information. > > As a bonus we can draw representations of the GPU : > > https://imgur.com/a/vuqpa > > v2: Rename uapi struct s/_mask/_info/ (Tvrtko) > Report max_slice/subslice/eus_per_subslice rather than strides (Tvrtko) > Add uapi macros to read data from *_info structs (Tvrtko) > > v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom shifts (Tvrtko) > > v4: factorize query item writting (Tvrtko) > tweak uapi struct/define names (Tvrtko) > > v5: Replace ALIGN() macro (Chris) > > v6: Updated uapi comments (Tvrtko) > Moved flags != 0 checks into vfuncs (Tvrtko) > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_query.c | 105 ++++++++++++++++++++++++++++++++++++++ > include/uapi/drm/i915_drm.h | 68 ++++++++++++++++++++++++ > 2 files changed, 173 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 5aa886313cf9..5d1386db8e7b 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -25,8 +25,113 @@ > #include "i915_drv.h" > #include <uapi/drm/i915_drm.h> > > +static int copy_query_data(struct drm_i915_query_item *query_item, > + const void *item_ptr, u32 item_length, > + const void *data_ptr, u32 data_length) > +{ > + u32 total_length = item_length + data_length; > + > + BUG_ON(add_overflows(item_length, data_length)); > + > + if (query_item->length == 0) > + return total_length; > + > + if (query_item->length < total_length) > + return -EINVAL; > + > + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), > + item_ptr, item_length)) > + return -EFAULT; > + > + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + item_length), > + data_ptr, data_length)) > + return -EFAULT; > + Dammit... Forgot some overflow checking here.
On 17/01/18 15:50, Lionel Landwerlin wrote: > On 17/01/18 15:39, Lionel Landwerlin wrote: >> With the introduction of asymmetric slices in CNL, we cannot rely on >> the previous SUBSLICE_MASK getparam to tell userspace what subslices >> are available. Here we introduce a more detailed way of querying the >> Gen's GPU topology that doesn't aggregate numbers. >> >> This is essential for monitoring parts of the GPU with the OA unit, >> because counters need to be normalized to the number of >> EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do >> not gives us sufficient information. >> >> As a bonus we can draw representations of the GPU : >> >> https://imgur.com/a/vuqpa >> >> v2: Rename uapi struct s/_mask/_info/ (Tvrtko) >> Report max_slice/subslice/eus_per_subslice rather than strides >> (Tvrtko) >> Add uapi macros to read data from *_info structs (Tvrtko) >> >> v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom >> shifts (Tvrtko) >> >> v4: factorize query item writting (Tvrtko) >> tweak uapi struct/define names (Tvrtko) >> >> v5: Replace ALIGN() macro (Chris) >> >> v6: Updated uapi comments (Tvrtko) >> Moved flags != 0 checks into vfuncs (Tvrtko) >> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_query.c | 105 >> ++++++++++++++++++++++++++++++++++++++ >> include/uapi/drm/i915_drm.h | 68 ++++++++++++++++++++++++ >> 2 files changed, 173 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_query.c >> b/drivers/gpu/drm/i915/i915_query.c >> index 5aa886313cf9..5d1386db8e7b 100644 >> --- a/drivers/gpu/drm/i915/i915_query.c >> +++ b/drivers/gpu/drm/i915/i915_query.c >> @@ -25,8 +25,113 @@ >> #include "i915_drv.h" >> #include <uapi/drm/i915_drm.h> >> +static int copy_query_data(struct drm_i915_query_item *query_item, >> + const void *item_ptr, u32 item_length, >> + const void *data_ptr, u32 data_length) >> +{ >> + u32 total_length = item_length + data_length; >> + >> + BUG_ON(add_overflows(item_length, data_length)); >> + >> + if (query_item->length == 0) >> + return total_length; >> + >> + if (query_item->length < total_length) >> + return -EINVAL; >> + >> + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), >> + item_ptr, item_length)) >> + return -EFAULT; >> + >> + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + >> item_length), >> + data_ptr, data_length)) >> + return -EFAULT; >> + > > Dammit... Forgot some overflow checking here. Fixed locally.
On 17/01/2018 15:39, Lionel Landwerlin wrote: > With the introduction of asymmetric slices in CNL, we cannot rely on > the previous SUBSLICE_MASK getparam to tell userspace what subslices > are available. Here we introduce a more detailed way of querying the > Gen's GPU topology that doesn't aggregate numbers. > > This is essential for monitoring parts of the GPU with the OA unit, > because counters need to be normalized to the number of > EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do > not gives us sufficient information. > > As a bonus we can draw representations of the GPU : > > https://imgur.com/a/vuqpa > > v2: Rename uapi struct s/_mask/_info/ (Tvrtko) > Report max_slice/subslice/eus_per_subslice rather than strides (Tvrtko) > Add uapi macros to read data from *_info structs (Tvrtko) > > v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom shifts (Tvrtko) > > v4: factorize query item writting (Tvrtko) > tweak uapi struct/define names (Tvrtko) > > v5: Replace ALIGN() macro (Chris) > > v6: Updated uapi comments (Tvrtko) > Moved flags != 0 checks into vfuncs (Tvrtko) > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_query.c | 105 ++++++++++++++++++++++++++++++++++++++ > include/uapi/drm/i915_drm.h | 68 ++++++++++++++++++++++++ > 2 files changed, 173 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 5aa886313cf9..5d1386db8e7b 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -25,8 +25,113 @@ > #include "i915_drv.h" > #include <uapi/drm/i915_drm.h> > > +static int copy_query_data(struct drm_i915_query_item *query_item, > + const void *item_ptr, u32 item_length, > + const void *data_ptr, u32 data_length) > +{ > + u32 total_length = item_length + data_length; > + > + BUG_ON(add_overflows(item_length, data_length)); Please change to if (WARN_ON_ONCE(...)) return -EINVAL;. Or even GEM_WARN_ON. This can only happen during development so is enough. > + > + if (query_item->length == 0) > + return total_length; > + > + if (query_item->length < total_length) > + return -EINVAL; Did we talk about whether this check should be != or < yet? != would sound safer to me, or you want to allow extending the queries in the future and keeping the same id? > + > + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), > + item_ptr, item_length)) > + return -EFAULT; > + > + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + item_length), > + data_ptr, data_length)) > + return -EFAULT; > + > + return total_length; > +} > + > +static int query_slice_info(struct drm_i915_private *dev_priv, > + struct drm_i915_query_item *query_item) > +{ > + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; > + struct drm_i915_query_slice_info slice_info; > + > + if (query_item->flags != 0) > + return -EINVAL; > + > + if (sseu->max_slices == 0) > + return -ENODEV; > + > + /* > + * If we ever change the internal slice mask data type, we'll need to > + * update this function. > + */ > + BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask)); > + > + memset(&slice_info, 0, sizeof(slice_info)); > + slice_info.max_slices = sseu->max_slices; > + > + return copy_query_data(query_item, &slice_info, sizeof(slice_info), > + &sseu->slice_mask, sizeof(sseu->slice_mask)); > +} > + > +static int query_subslice_info(struct drm_i915_private *dev_priv, > + struct drm_i915_query_item *query_item) > +{ > + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; > + struct drm_i915_query_subslice_info subslice_info; > + u32 data_length; > + > + if (query_item->flags != 0) > + return -EINVAL; > + > + if (sseu->max_slices == 0) > + return -ENODEV; > + > + memset(&subslice_info, 0, sizeof(subslice_info)); > + subslice_info.max_slices = sseu->max_slices; > + subslice_info.max_subslices = sseu->max_subslices; > + > + data_length = subslice_info.max_slices * > + DIV_ROUND_UP(subslice_info.max_subslices, > + sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE); > + > + return copy_query_data(query_item, > + &subslice_info, sizeof(subslice_info), > + sseu->subslice_mask, data_length); > +} > + > +static int query_eu_info(struct drm_i915_private *dev_priv, > + struct drm_i915_query_item *query_item) > +{ > + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; > + struct drm_i915_query_eu_info eu_info; > + u32 data_length; > + > + if (query_item->flags != 0) > + return -EINVAL; > + > + if (sseu->max_slices == 0) > + return -ENODEV; > + > + memset(&eu_info, 0, sizeof(eu_info)); > + eu_info.max_slices = sseu->max_slices; > + eu_info.max_subslices = sseu->max_subslices; > + eu_info.max_eus_per_subslice = sseu->max_eus_per_subslice; > + > + data_length = eu_info.max_slices * eu_info.max_subslices * > + DIV_ROUND_UP(eu_info.max_eus_per_subslice, BITS_PER_BYTE); > + > + return copy_query_data(query_item, > + &eu_info, sizeof(eu_info), > + sseu->eu_mask, data_length); > +} > + > static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, > struct drm_i915_query_item *query_item) = { > + query_slice_info, > + query_subslice_info, > + query_eu_info, > }; > > 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 d94a00380bf6..bcbdd8deb841 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config { > > struct drm_i915_query_item { > __u64 query_id; > +#define DRM_I915_QUERY_SLICE_INFO 0x01 > +#define DRM_I915_QUERY_SUBSLICE_INFO 0x02 > +#define DRM_I915_QUERY_EU_INFO 0x03 > > /* > * When set to zero by userspace, this is filled with the size of the > @@ -1650,6 +1653,71 @@ struct drm_i915_query { > __u64 items_ptr; > }; > > +#define DRM_I915_BIT(bit) ((__u32)1 << (bit)) > +#define DRM_I915_DIV_ROUND_UP(val, div) (((val) + (div) - 1) / (div)) > + > +/* Data written by the kernel with query DRM_I915_QUERY_SLICE_INFO : > + * > + * data: each bit indicates whether a slice is available (1) or fused off (0). > + * Formula to tell if slice X is available : > + * > + * (data[X / 8] >> (X % 8)) & 1 > + * > + * Alternatively, use DRM_I915_QUERY_SLICE_AVAILABLE() to query a > + * given subslice's availability. > + */ > +struct drm_i915_query_slice_info { > + __u32 max_slices; > + > +#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \ > + !!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8)) > + __u8 data[]; > +}; > + > +/* Data written by the kernel with query DRM_I915_QUERY_SUBSLICE_INFO : > + * > + * data: each bit indicates whether a subslice is available (1) or fused off > + * (0). Formula to tell if slice X subslice Y is available : > + * > + * (data[(X * max_subslices) + Y / 8] >> (Y % 8)) & 1 > + * > + * Alternatively, use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to query a > + * given subslice's availability. > + */ > +struct drm_i915_query_subslice_info { > + __u32 max_slices; > + __u32 max_subslices; > + > +#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \ > + !!((info)->data[(slice) * DRM_I915_DIV_ROUND_UP((info)->max_subslices, 8) + \ > + (subslice) / 8] & DRM_I915_BIT((subslice) % 8)) > + __u8 data[]; > +}; > + > +/* Data written by the kernel with query DRM_I915_QUERY_EU_INFO : > + * > + * data: Each bit indicates whether an EU is available (1) or fused off (0). > + * Formula to tell if slice X subslice Y eu Z is available : > + * > + * (data[X * max_subslices * ALIGN(max_eus_per_subslice, 8) / 8 + > + * Y * ALIGN(max_eus_per_subslice, 8) / 8 + > + * Z / 8] >> (Z % 8)) & 1 > + * > + * Alternatively, use DRM_I915_QUERY_EU_AVAILABLE() to query a given > + * EU's availability. > + */ > +struct drm_i915_query_eu_info { > + __u32 max_slices; > + __u32 max_subslices; > + __u32 max_eus_per_subslice; > + > +#define DRM_I915_QUERY_EU_AVAILABLE(info, slice, subslice, eu) \ > + !!((info)->data[(slice) * DRM_I915_DIV_ROUND_UP((info)->max_eus_per_subslice, 8) * (info)->max_subslices + \ > + (subslice) * DRM_I915_DIV_ROUND_UP((info)->max_eus_per_subslice, 8) + \ > + (eu) / 8] & DRM_I915_BIT((eu) % 8)) > + __u8 data[]; > +}; > + > #if defined(__cplusplus) > } > #endif > Rest looks OK to me. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-01-18 12:46:55) > > On 17/01/2018 15:39, Lionel Landwerlin wrote: > > + > > + if (query_item->length == 0) > > + return total_length; > > + > > + if (query_item->length < total_length) > > + return -EINVAL; > > Did we talk about whether this check should be != or < yet? != would > sound safer to me, or you want to allow extending the queries in the > future and keeping the same id? This is me, because I like read(buf, len) style behaviour that lets me pass in a preallocated buffer that should be big enough without having to worry and do a 2-pass approach to find out the length of the buffer to allocate first. '!=' style I have found much more painful to handle in userspace. -Chris
On 18/01/2018 13:04, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-01-18 12:46:55) >> >> On 17/01/2018 15:39, Lionel Landwerlin wrote: >>> + >>> + if (query_item->length == 0) >>> + return total_length; >>> + >>> + if (query_item->length < total_length) >>> + return -EINVAL; >> >> Did we talk about whether this check should be != or < yet? != would >> sound safer to me, or you want to allow extending the queries in the >> future and keeping the same id? > > This is me, because I like read(buf, len) style behaviour that lets me > pass in a preallocated buffer that should be big enough without having > to worry and do a 2-pass approach to find out the length of the buffer > to allocate first. '!=' style I have found much more painful to handle in > userspace. Yeah that's fine by me. Doesn't preclude kernel returning you some new fields you don't know about either. Regards, Tvrtko
On 18/01/18 12:46, Tvrtko Ursulin wrote: > > On 17/01/2018 15:39, Lionel Landwerlin wrote: >> With the introduction of asymmetric slices in CNL, we cannot rely on >> the previous SUBSLICE_MASK getparam to tell userspace what subslices >> are available. Here we introduce a more detailed way of querying the >> Gen's GPU topology that doesn't aggregate numbers. >> >> This is essential for monitoring parts of the GPU with the OA unit, >> because counters need to be normalized to the number of >> EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do >> not gives us sufficient information. >> >> As a bonus we can draw representations of the GPU : >> >> https://imgur.com/a/vuqpa >> >> v2: Rename uapi struct s/_mask/_info/ (Tvrtko) >> Report max_slice/subslice/eus_per_subslice rather than strides >> (Tvrtko) >> Add uapi macros to read data from *_info structs (Tvrtko) >> >> v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom >> shifts (Tvrtko) >> >> v4: factorize query item writting (Tvrtko) >> tweak uapi struct/define names (Tvrtko) >> >> v5: Replace ALIGN() macro (Chris) >> >> v6: Updated uapi comments (Tvrtko) >> Moved flags != 0 checks into vfuncs (Tvrtko) >> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_query.c | 105 >> ++++++++++++++++++++++++++++++++++++++ >> include/uapi/drm/i915_drm.h | 68 ++++++++++++++++++++++++ >> 2 files changed, 173 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_query.c >> b/drivers/gpu/drm/i915/i915_query.c >> index 5aa886313cf9..5d1386db8e7b 100644 >> --- a/drivers/gpu/drm/i915/i915_query.c >> +++ b/drivers/gpu/drm/i915/i915_query.c >> @@ -25,8 +25,113 @@ >> #include "i915_drv.h" >> #include <uapi/drm/i915_drm.h> >> +static int copy_query_data(struct drm_i915_query_item *query_item, >> + const void *item_ptr, u32 item_length, >> + const void *data_ptr, u32 data_length) >> +{ >> + u32 total_length = item_length + data_length; >> + >> + BUG_ON(add_overflows(item_length, data_length)); > > Please change to if (WARN_ON_ONCE(...)) return -EINVAL;. Or even > GEM_WARN_ON. This can only happen during development so is enough. Thanks, done. > >> + >> + if (query_item->length == 0) >> + return total_length; >> + >> + if (query_item->length < total_length) >> + return -EINVAL; > > Did we talk about whether this check should be != or < yet? != would > sound safer to me, or you want to allow extending the queries in the > future and keeping the same id? Like Chris said :) > >> + >> + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), >> + item_ptr, item_length)) >> + return -EFAULT; >> + >> + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + >> item_length), >> + data_ptr, data_length)) >> + return -EFAULT; >> + >> + return total_length; >> +} >> + >> +static int query_slice_info(struct drm_i915_private *dev_priv, >> + struct drm_i915_query_item *query_item) >> +{ >> + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; >> + struct drm_i915_query_slice_info slice_info; >> + >> + if (query_item->flags != 0) >> + return -EINVAL; >> + >> + if (sseu->max_slices == 0) >> + return -ENODEV; >> + >> + /* >> + * If we ever change the internal slice mask data type, we'll >> need to >> + * update this function. >> + */ >> + BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask)); >> + >> + memset(&slice_info, 0, sizeof(slice_info)); >> + slice_info.max_slices = sseu->max_slices; >> + >> + return copy_query_data(query_item, &slice_info, sizeof(slice_info), >> + &sseu->slice_mask, sizeof(sseu->slice_mask)); >> +} >> + >> +static int query_subslice_info(struct drm_i915_private *dev_priv, >> + struct drm_i915_query_item *query_item) >> +{ >> + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; >> + struct drm_i915_query_subslice_info subslice_info; >> + u32 data_length; >> + >> + if (query_item->flags != 0) >> + return -EINVAL; >> + >> + if (sseu->max_slices == 0) >> + return -ENODEV; >> + >> + memset(&subslice_info, 0, sizeof(subslice_info)); >> + subslice_info.max_slices = sseu->max_slices; >> + subslice_info.max_subslices = sseu->max_subslices; >> + >> + data_length = subslice_info.max_slices * >> + DIV_ROUND_UP(subslice_info.max_subslices, >> + sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE); >> + >> + return copy_query_data(query_item, >> + &subslice_info, sizeof(subslice_info), >> + sseu->subslice_mask, data_length); >> +} >> + >> +static int query_eu_info(struct drm_i915_private *dev_priv, >> + struct drm_i915_query_item *query_item) >> +{ >> + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; >> + struct drm_i915_query_eu_info eu_info; >> + u32 data_length; >> + >> + if (query_item->flags != 0) >> + return -EINVAL; >> + >> + if (sseu->max_slices == 0) >> + return -ENODEV; >> + >> + memset(&eu_info, 0, sizeof(eu_info)); >> + eu_info.max_slices = sseu->max_slices; >> + eu_info.max_subslices = sseu->max_subslices; >> + eu_info.max_eus_per_subslice = sseu->max_eus_per_subslice; >> + >> + data_length = eu_info.max_slices * eu_info.max_subslices * >> + DIV_ROUND_UP(eu_info.max_eus_per_subslice, BITS_PER_BYTE); >> + >> + return copy_query_data(query_item, >> + &eu_info, sizeof(eu_info), >> + sseu->eu_mask, data_length); >> +} >> + >> static int (* const i915_query_funcs[])(struct drm_i915_private >> *dev_priv, >> struct drm_i915_query_item *query_item) = { >> + query_slice_info, >> + query_subslice_info, >> + query_eu_info, >> }; >> 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 d94a00380bf6..bcbdd8deb841 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config { >> struct drm_i915_query_item { >> __u64 query_id; >> +#define DRM_I915_QUERY_SLICE_INFO 0x01 >> +#define DRM_I915_QUERY_SUBSLICE_INFO 0x02 >> +#define DRM_I915_QUERY_EU_INFO 0x03 >> /* >> * When set to zero by userspace, this is filled with the size >> of the >> @@ -1650,6 +1653,71 @@ struct drm_i915_query { >> __u64 items_ptr; >> }; >> +#define DRM_I915_BIT(bit) ((__u32)1 << (bit)) >> +#define DRM_I915_DIV_ROUND_UP(val, div) (((val) + (div) - 1) / (div)) >> + >> +/* Data written by the kernel with query DRM_I915_QUERY_SLICE_INFO : >> + * >> + * data: each bit indicates whether a slice is available (1) or >> fused off (0). >> + * Formula to tell if slice X is available : >> + * >> + * (data[X / 8] >> (X % 8)) & 1 >> + * >> + * Alternatively, use DRM_I915_QUERY_SLICE_AVAILABLE() to query a >> + * given subslice's availability. >> + */ >> +struct drm_i915_query_slice_info { >> + __u32 max_slices; >> + >> +#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \ >> + !!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8)) >> + __u8 data[]; >> +}; >> + >> +/* Data written by the kernel with query DRM_I915_QUERY_SUBSLICE_INFO : >> + * >> + * data: each bit indicates whether a subslice is available (1) or >> fused off >> + * (0). Formula to tell if slice X subslice Y is available : >> + * >> + * (data[(X * max_subslices) + Y / 8] >> (Y % 8)) & 1 >> + * >> + * Alternatively, use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to >> query a >> + * given subslice's availability. >> + */ >> +struct drm_i915_query_subslice_info { >> + __u32 max_slices; >> + __u32 max_subslices; >> + >> +#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \ >> + !!((info)->data[(slice) * >> DRM_I915_DIV_ROUND_UP((info)->max_subslices, 8) + \ >> + (subslice) / 8] & DRM_I915_BIT((subslice) % 8)) >> + __u8 data[]; >> +}; >> + >> +/* Data written by the kernel with query DRM_I915_QUERY_EU_INFO : >> + * >> + * data: Each bit indicates whether an EU is available (1) or fused >> off (0). >> + * Formula to tell if slice X subslice Y eu Z is available : >> + * >> + * (data[X * max_subslices * ALIGN(max_eus_per_subslice, >> 8) / 8 + >> + * Y * ALIGN(max_eus_per_subslice, 8) / 8 + >> + * Z / 8] >> (Z % 8)) & 1 >> + * >> + * Alternatively, use DRM_I915_QUERY_EU_AVAILABLE() to query a >> given >> + * EU's availability. >> + */ >> +struct drm_i915_query_eu_info { >> + __u32 max_slices; >> + __u32 max_subslices; >> + __u32 max_eus_per_subslice; >> + >> +#define DRM_I915_QUERY_EU_AVAILABLE(info, slice, subslice, eu) \ >> + !!((info)->data[(slice) * >> DRM_I915_DIV_ROUND_UP((info)->max_eus_per_subslice, 8) * >> (info)->max_subslices + \ >> + (subslice) * >> DRM_I915_DIV_ROUND_UP((info)->max_eus_per_subslice, 8) + \ >> + (eu) / 8] & DRM_I915_BIT((eu) % 8)) >> + __u8 data[]; >> +}; >> + >> #if defined(__cplusplus) >> } >> #endif >> > > Rest looks OK to me. > > Regards, > > Tvrtko >
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 5aa886313cf9..5d1386db8e7b 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -25,8 +25,113 @@ #include "i915_drv.h" #include <uapi/drm/i915_drm.h> +static int copy_query_data(struct drm_i915_query_item *query_item, + const void *item_ptr, u32 item_length, + const void *data_ptr, u32 data_length) +{ + u32 total_length = item_length + data_length; + + BUG_ON(add_overflows(item_length, data_length)); + + if (query_item->length == 0) + return total_length; + + if (query_item->length < total_length) + return -EINVAL; + + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), + item_ptr, item_length)) + return -EFAULT; + + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + item_length), + data_ptr, data_length)) + return -EFAULT; + + return total_length; +} + +static int query_slice_info(struct drm_i915_private *dev_priv, + struct drm_i915_query_item *query_item) +{ + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; + struct drm_i915_query_slice_info slice_info; + + if (query_item->flags != 0) + return -EINVAL; + + if (sseu->max_slices == 0) + return -ENODEV; + + /* + * If we ever change the internal slice mask data type, we'll need to + * update this function. + */ + BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask)); + + memset(&slice_info, 0, sizeof(slice_info)); + slice_info.max_slices = sseu->max_slices; + + return copy_query_data(query_item, &slice_info, sizeof(slice_info), + &sseu->slice_mask, sizeof(sseu->slice_mask)); +} + +static int query_subslice_info(struct drm_i915_private *dev_priv, + struct drm_i915_query_item *query_item) +{ + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; + struct drm_i915_query_subslice_info subslice_info; + u32 data_length; + + if (query_item->flags != 0) + return -EINVAL; + + if (sseu->max_slices == 0) + return -ENODEV; + + memset(&subslice_info, 0, sizeof(subslice_info)); + subslice_info.max_slices = sseu->max_slices; + subslice_info.max_subslices = sseu->max_subslices; + + data_length = subslice_info.max_slices * + DIV_ROUND_UP(subslice_info.max_subslices, + sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE); + + return copy_query_data(query_item, + &subslice_info, sizeof(subslice_info), + sseu->subslice_mask, data_length); +} + +static int query_eu_info(struct drm_i915_private *dev_priv, + struct drm_i915_query_item *query_item) +{ + const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu; + struct drm_i915_query_eu_info eu_info; + u32 data_length; + + if (query_item->flags != 0) + return -EINVAL; + + if (sseu->max_slices == 0) + return -ENODEV; + + memset(&eu_info, 0, sizeof(eu_info)); + eu_info.max_slices = sseu->max_slices; + eu_info.max_subslices = sseu->max_subslices; + eu_info.max_eus_per_subslice = sseu->max_eus_per_subslice; + + data_length = eu_info.max_slices * eu_info.max_subslices * + DIV_ROUND_UP(eu_info.max_eus_per_subslice, BITS_PER_BYTE); + + return copy_query_data(query_item, + &eu_info, sizeof(eu_info), + sseu->eu_mask, data_length); +} + static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, struct drm_i915_query_item *query_item) = { + query_slice_info, + query_subslice_info, + query_eu_info, }; 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 d94a00380bf6..bcbdd8deb841 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config { struct drm_i915_query_item { __u64 query_id; +#define DRM_I915_QUERY_SLICE_INFO 0x01 +#define DRM_I915_QUERY_SUBSLICE_INFO 0x02 +#define DRM_I915_QUERY_EU_INFO 0x03 /* * When set to zero by userspace, this is filled with the size of the @@ -1650,6 +1653,71 @@ struct drm_i915_query { __u64 items_ptr; }; +#define DRM_I915_BIT(bit) ((__u32)1 << (bit)) +#define DRM_I915_DIV_ROUND_UP(val, div) (((val) + (div) - 1) / (div)) + +/* Data written by the kernel with query DRM_I915_QUERY_SLICE_INFO : + * + * data: each bit indicates whether a slice is available (1) or fused off (0). + * Formula to tell if slice X is available : + * + * (data[X / 8] >> (X % 8)) & 1 + * + * Alternatively, use DRM_I915_QUERY_SLICE_AVAILABLE() to query a + * given subslice's availability. + */ +struct drm_i915_query_slice_info { + __u32 max_slices; + +#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \ + !!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8)) + __u8 data[]; +}; + +/* Data written by the kernel with query DRM_I915_QUERY_SUBSLICE_INFO : + * + * data: each bit indicates whether a subslice is available (1) or fused off + * (0). Formula to tell if slice X subslice Y is available : + * + * (data[(X * max_subslices) + Y / 8] >> (Y % 8)) & 1 + * + * Alternatively, use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to query a + * given subslice's availability. + */ +struct drm_i915_query_subslice_info { + __u32 max_slices; + __u32 max_subslices; + +#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \ + !!((info)->data[(slice) * DRM_I915_DIV_ROUND_UP((info)->max_subslices, 8) + \ + (subslice) / 8] & DRM_I915_BIT((subslice) % 8)) + __u8 data[]; +}; + +/* Data written by the kernel with query DRM_I915_QUERY_EU_INFO : + * + * data: Each bit indicates whether an EU is available (1) or fused off (0). + * Formula to tell if slice X subslice Y eu Z is available : + * + * (data[X * max_subslices * ALIGN(max_eus_per_subslice, 8) / 8 + + * Y * ALIGN(max_eus_per_subslice, 8) / 8 + + * Z / 8] >> (Z % 8)) & 1 + * + * Alternatively, use DRM_I915_QUERY_EU_AVAILABLE() to query a given + * EU's availability. + */ +struct drm_i915_query_eu_info { + __u32 max_slices; + __u32 max_subslices; + __u32 max_eus_per_subslice; + +#define DRM_I915_QUERY_EU_AVAILABLE(info, slice, subslice, eu) \ + !!((info)->data[(slice) * DRM_I915_DIV_ROUND_UP((info)->max_eus_per_subslice, 8) * (info)->max_subslices + \ + (subslice) * DRM_I915_DIV_ROUND_UP((info)->max_eus_per_subslice, 8) + \ + (eu) / 8] & DRM_I915_BIT((eu) % 8)) + __u8 data[]; +}; + #if defined(__cplusplus) } #endif
With the introduction of asymmetric slices in CNL, we cannot rely on the previous SUBSLICE_MASK getparam to tell userspace what subslices are available. Here we introduce a more detailed way of querying the Gen's GPU topology that doesn't aggregate numbers. This is essential for monitoring parts of the GPU with the OA unit, because counters need to be normalized to the number of EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do not gives us sufficient information. As a bonus we can draw representations of the GPU : https://imgur.com/a/vuqpa v2: Rename uapi struct s/_mask/_info/ (Tvrtko) Report max_slice/subslice/eus_per_subslice rather than strides (Tvrtko) Add uapi macros to read data from *_info structs (Tvrtko) v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom shifts (Tvrtko) v4: factorize query item writting (Tvrtko) tweak uapi struct/define names (Tvrtko) v5: Replace ALIGN() macro (Chris) v6: Updated uapi comments (Tvrtko) Moved flags != 0 checks into vfuncs (Tvrtko) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/i915/i915_query.c | 105 ++++++++++++++++++++++++++++++++++++++ include/uapi/drm/i915_drm.h | 68 ++++++++++++++++++++++++ 2 files changed, 173 insertions(+)