Re: [PATCH 4/4] drm/i915: expose EU topology through sysfs

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 20/11/17 16:13, Tvrtko Ursulin wrote:
>
> On 20/11/2017 12:23, Lionel Landwerlin wrote:
>> With the introduction of asymetric slices in CNL, we cannot rely on
>> the previous SUBSLICE_MASK getparam. 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 signals need to be accounted properly based on whether part of
>> the GPU has been fused off. The current aggregated numbers like
>> EU_TOTAL do not gives us sufficient information.
>>
>> Here is the sysfs layout on a Skylake GT4 :
>>
>> /sys/devices/pci0000:00/0000:00:02.0/drm/card0/gt/rcs/0/
>> ├── capabilities
>> ├── class
>> ├── id
>> ├── max_eus_per_subslice
>> ├── max_slices
>> ├── max_subslices_per_slice
>> ├── slice0
>> │   ├── subslice0
>> │   │   └── eus_enabled_mask
>> │   ├── subslice1
>> │   │   └── eus_enabled_mask
>> │   ├── subslice2
>> │   │   └── eus_enabled_mask
>> │   ├── subslice3
>> │   │   └── eus_enabled_mask
>> │   └── subslices_enabled_mask
>> ├── slice1
>> │   ├── subslice0
>> │   │   └── eus_enabled_mask
>> │   ├── subslice1
>> │   │   └── eus_enabled_mask
>> │   ├── subslice2
>> │   │   └── eus_enabled_mask
>> │   ├── subslice3
>> │   │   └── eus_enabled_mask
>> │   └── subslices_enabled_mask
>> ├── slice2
>> │   ├── subslice0
>> │   │   └── eus_enabled_mask
>> │   ├── subslice1
>> │   │   └── eus_enabled_mask
>> │   ├── subslice2
>> │   │   └── eus_enabled_mask
>> │   ├── subslice3
>> │   │   └── eus_enabled_mask
>> │   └── subslices_enabled_mask
>> └── slices_enabled_mask
>
> For the topology layout I don't feel like an expert. :(
>
> Is it more correct for it to be per engine instance, or per class?

I think you have a point here.
As far as I can tell this is only one sets of slices. So I guess it 
should be at the class level.

>
> Should it go under the topology subdir?
>
> Should there be a symlink from rcs/0/topology -> ../topology ?

Yeah, sounds good to me.

>
> Should the organization be with more subdirs like 
> topology/slice/0/subslice/0/eus_enabled_mask?

I would leave it slice0/subslice0/... for now and if it becomes more 
crowded, create slice/<id>/subslice/<id> with links for backward 
compatibility.
I don't think we're going to expose a lot of properties.

>
> Is (sub)slices_enabled_mask required/helpful or it can be inferred 
> from enumerating the slices?

For simplicity in the kernel & userspace, it's helpful.
It makes the reading of the structure simpler.

>
> Same for max_slices? Or perhaps it is a different meaning than the 
> enabled mask?

That is essentially to help out userspace.
Most application will use a flat structure to access this information 
with a couple of strides.
The max_* fields gives the strides.

>
> Sorry, this is not very constructive, more questions than answers... :(

An outside opinion always raises interesting questions :)
Thanks!

>
> Regards,
>
> Tvrtko
>
>>
>> Each enabled_mask file gives us a mask of the enabled units :
>>
>> $ cat 
>> /sys/devices/pci0000\:00/0000\:00\:02.0/drm/card0/gt/rcs/0/slices_enabled_mask
>> 0x7
>>
>> $ cat 
>> /sys/devices/pci0000\:00/0000\:00\:02.0/drm/card0/gt/rcs/0/slice0/subslice2/eus_enabled_mask
>> 0xff
>>
>> v2: Move topology below rcs engine (Chris)
>>      Add max_eus_per_subslice/max_slices/max_subslices_per_slice 
>> (Lionel)
>>
>> v3: Rename enabled_mask (Lionel)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h   |  26 ++++++
>>   drivers/gpu/drm/i915/i915_sysfs.c | 188 
>> ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 214 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index db550322207c..1ac0a191e8fc 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2259,6 +2259,24 @@ struct intel_cdclk_state {
>>       u8 voltage_level;
>>   };
>>   +struct intel_topology_kobject {
>> +    struct kobject kobj;
>> +    struct drm_i915_private *dev_priv;
>> +};
>> +
>> +struct intel_slice_kobject {
>> +    struct kobject kobj;
>> +    struct drm_i915_private *dev_priv;
>> +    u8 slice_index;
>> +};
>> +
>> +struct intel_subslice_kobject {
>> +    struct kobject kobj;
>> +    struct drm_i915_private *dev_priv;
>> +    u8 slice_index;
>> +    u8 subslice_index;
>> +};
>> +
>>   struct drm_i915_private {
>>       struct drm_device drm;
>>   @@ -2732,6 +2750,14 @@ struct drm_i915_private {
>>       struct {
>>           struct kobject kobj;
>>           struct kobject classes_kobjs[MAX_ENGINE_CLASS];
>> +
>> +        struct sysfs_slice {
>> +            struct intel_slice_kobject kobj;
>> +
>> +            struct sysfs_subslice {
>> +                struct intel_subslice_kobject kobj;
>> +            } subslices[GEN_MAX_SUBSLICES];
>> +        } slices[GEN_MAX_SLICES];
>>       } gt_topology;
>>         /* Abstract the submission mechanism (legacy ringbuffer or 
>> execlists) away */
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c 
>> b/drivers/gpu/drm/i915/i915_sysfs.c
>> index fd04d0b93eaf..df9d8fdbcb0a 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -559,6 +559,174 @@ static void i915_setup_error_capture(struct 
>> device *kdev) {}
>>   static void i915_teardown_error_capture(struct device *kdev) {}
>>   #endif
>>   +static struct attribute slices_enabled_mask_attr = {
>> +    .name = "slices_enabled_mask",
>> +    .mode = 0444,
>> +};
>> +
>> +static struct attribute subslices_enabled_mask_attr = {
>> +    .name = "subslices_enabled_mask",
>> +    .mode = 0444,
>> +};
>> +
>> +static struct attribute eus_enabled_mask_attr = {
>> +    .name = "eus_enabled_mask",
>> +    .mode = 0444,
>> +};
>> +
>> +static struct attribute max_slices_attr = {
>> +    .name = "max_slices",
>> +    .mode = 0444,
>> +};
>> +
>> +static struct attribute max_subslices_per_slice_attr = {
>> +    .name = "max_subslices_per_slice",
>> +    .mode = 0444,
>> +};
>> +
>> +static struct attribute max_eus_per_subslice_attr = {
>> +    .name = "max_eus_per_subslice",
>> +    .mode = 0444,
>> +};
>> +
>> +static ssize_t
>> +show_slice_attr(struct kobject *kobj, struct attribute *attr, char 
>> *buf)
>> +{
>> +    struct intel_slice_kobject *kobj_wrapper =
>> +        container_of(kobj, struct intel_slice_kobject, kobj);
>> +    struct drm_i915_private *dev_priv = kobj_wrapper->dev_priv;
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +
>> +    if (attr == &subslices_enabled_mask_attr) {
>> +        return sprintf(buf, "0x%hhx\n",
>> + sseu->subslices_mask[kobj_wrapper->slice_index]);
>> +    }
>> +
>> +    return sprintf(buf, "0x0\n");
>> +}
>> +
>> +static const struct sysfs_ops slice_ops = {
>> +    .show = show_slice_attr,
>> +};
>> +
>> +static struct kobj_type slice_type = {
>> +    .sysfs_ops = &slice_ops,
>> +};
>> +
>> +static ssize_t
>> +show_subslice_attr(struct kobject *kobj, struct attribute *attr, 
>> char *buf)
>> +{
>> +    struct intel_subslice_kobject *kobj_wrapper =
>> +        container_of(kobj, struct intel_subslice_kobject, kobj);
>> +    struct drm_i915_private *dev_priv = kobj_wrapper->dev_priv;
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    int subslice_stride = ALIGN(sseu->max_eus_per_subslice, 8) / 8;
>> +    int slice_stride = sseu->max_subslices * subslice_stride;
>> +
>> +    if (attr == &eus_enabled_mask_attr)
>> +        return sprintf(buf, "0x%hhx\n",
>> + sseu->eu_mask[kobj_wrapper->slice_index * slice_stride +
>> +                         kobj_wrapper->subslice_index * 
>> subslice_stride]);
>> +    return sprintf(buf, "0x0\n");
>> +}
>> +
>> +static const struct sysfs_ops subslice_ops = {
>> +    .show = show_subslice_attr,
>> +};
>> +
>> +static struct kobj_type subslice_type = {
>> +    .sysfs_ops = &subslice_ops,
>> +};
>> +
>> +static int i915_setup_rcs_topology_sysfs(struct drm_i915_private 
>> *dev_priv,
>> +                     struct kobject *engine_kobj)
>> +{
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    int ret, s, ss;
>> +
>> +    ret = sysfs_create_file(engine_kobj, &slices_enabled_mask_attr);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = sysfs_create_file(engine_kobj, &max_slices_attr);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = sysfs_create_file(engine_kobj, 
>> &max_subslices_per_slice_attr);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = sysfs_create_file(engine_kobj, &max_eus_per_subslice_attr);
>> +    if (ret)
>> +        return ret;
>> +
>> +    for (s = 0; s < sseu->max_slices; s++) {
>> +        struct intel_slice_kobject *slice_kobj =
>> +            &dev_priv->gt_topology.slices[s].kobj;
>> +
>> +        slice_kobj->dev_priv = dev_priv;
>> +        slice_kobj->slice_index = s;
>> +        ret = kobject_init_and_add(&slice_kobj->kobj, &slice_type,
>> +                       engine_kobj, "slice%i", s);
>> +        if (ret)
>> +            return ret;
>> +
>> +        ret = sysfs_create_file(&slice_kobj->kobj,
>> +                    &subslices_enabled_mask_attr);
>> +        if (ret)
>> +            return ret;
>> +
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>> +            struct intel_subslice_kobject *subslice_kobj =
>> + &dev_priv->gt_topology.slices[s].subslices[ss].kobj;
>> +
>> +            subslice_kobj->dev_priv = dev_priv;
>> +            subslice_kobj->slice_index = s;
>> +            subslice_kobj->subslice_index = ss;
>> +            ret = kobject_init_and_add(&subslice_kobj->kobj,
>> +                           &subslice_type,
>> +                           &slice_kobj->kobj,
>> +                           "subslice%i", ss);
>> +            if (ret)
>> +                return ret;
>> +
>> +            ret = sysfs_create_file(&subslice_kobj->kobj,
>> +                        &eus_enabled_mask_attr);
>> +            if (ret)
>> +                return ret;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void i915_teardown_rcs_topology_sysfs(struct drm_i915_private 
>> *dev_priv,
>> +                         struct kobject *engine_kobj)
>> +{
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +    int s, ss;
>> +
>> +    for (s = 0; s < sseu->max_slices; s++) {
>> +        struct intel_slice_kobject *slice_kobj =
>> +            &dev_priv->gt_topology.slices[s].kobj;
>> +
>> +        for (ss = 0; ss < sseu->max_subslices; ss++) {
>> +            struct intel_subslice_kobject *subslice_kobj =
>> + &dev_priv->gt_topology.slices[s].subslices[ss].kobj;
>> +
>> +            sysfs_remove_file(&subslice_kobj->kobj,
>> +                      &eus_enabled_mask_attr);
>> +        }
>> +
>> +        sysfs_remove_file(&slice_kobj->kobj,
>> +                  &subslices_enabled_mask_attr);
>> +    }
>> +    sysfs_remove_file(engine_kobj, &slices_enabled_mask_attr);
>> +    sysfs_remove_file(engine_kobj, &max_eus_per_subslice_attr);
>> +    sysfs_remove_file(engine_kobj, &max_subslices_per_slice_attr);
>> +    sysfs_remove_file(engine_kobj, &max_slices_attr);
>> +}
>> +
>>   static struct attribute engine_id_attr = {
>>       .name = "id",
>>       .mode = 0444,
>> @@ -574,11 +742,20 @@ show_engine_attr(struct kobject *kobj, struct 
>> attribute *attr, char *buf)
>>   {
>>       struct intel_engine_cs *engine =
>>           container_of(kobj, struct intel_engine_cs, instance_kobj);
>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(engine->i915)->sseu;
>>         if (attr == &engine_id_attr)
>>           return sprintf(buf, "%hhu\n", engine->uabi_id);
>>       if (attr == &engine_class_attr)
>>           return sprintf(buf, "%hhu\n", engine->uabi_class);
>> +    if (attr == &slices_enabled_mask_attr)
>> +        return sprintf(buf, "0x%hhx\n", sseu->slice_mask);
>> +    if (attr == &max_eus_per_subslice_attr)
>> +        return sprintf(buf, "%hhd\n", sseu->max_eus_per_subslice);
>> +    if (attr == &max_subslices_per_slice_attr)
>> +        return sprintf(buf, "%hhd\n", sseu->max_subslices);
>> +    if (attr == &max_slices_attr)
>> +        return sprintf(buf, "%hhd\n", sseu->max_slices);
>>       return sprintf(buf, "\n");
>>   }
>>   @@ -671,6 +848,12 @@ static int i915_setup_engines_sysfs(struct 
>> drm_i915_private *dev_priv,
>>                   if (ret)
>>                       return ret;
>>               }
>> +            if (engine->id == RCS) {
>> +                ret = i915_setup_rcs_topology_sysfs(dev_priv,
>> + &engine->instance_kobj);
>> +                if (ret)
>> +                    return ret;
>> +            }
>>           }
>>       }
>>   @@ -683,6 +866,11 @@ static void i915_teardown_engines_sysfs(struct 
>> drm_i915_private *dev_priv)
>>       enum intel_engine_id id;
>>         for_each_engine(engine, dev_priv, id) {
>> +        if (id == RCS) {
>> +            i915_teardown_rcs_topology_sysfs(dev_priv,
>> +                             &engine->instance_kobj);
>> +        }
>> +
>>           sysfs_remove_file(&engine->instance_kobj, &engine_id_attr);
>>           sysfs_remove_file(&engine->instance_kobj, &engine_class_attr);
>>           sysfs_remove_file(&engine->capabilities_kobj,
>>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx