Overview

The initial idea is to have a DRM_IOCTL_I915_PERF_ADD_CONFIG ioctl that would only be usable with root privileges, that can be given arrays of registers for a NOA/MUX config + Boolean logic config + possibly FLEX EU config (for Broadwell+) which would also be associated with a GUID for listing the config under /sys/class/drm/card0/metrics in the same way as built in metrics.

This will allow us to be able to work with experimental/unstable OA configurations or allow tools to distribute new configs without requiring developers to build a custom kernel.

Details

Add the ioctl

Referring to the patch that adds DRM_IOCTL_I915_PERF_OPEN that should provide an overview of how to add a new ioctl.

i915_perf.c:i915_perf_open_ioctl() is the start of the implementation for DRM_IOCTL_I915_PERF_OPEN and this will need another function with the same prototype something like i915_perf_add_config_ioctl()

Maybe the struct to pass the to ioctl will be something like:

{
    u8 uuid[16]; /* XXX: userspace should give a unique ID that will be used to advertise the config under /sys/class/drm/card0/metrics */

    u32 n_mux_regs;
    u64 mux_regs; /* XXX: note this is using a uint64 as a pointer to have a deterministic size, compare with the DRM_IOCTL_I915_PERF_OPEN ioctl doing a similar thing for properties and cast with to_user_ptr() macro */

    u32 n_boolean_regs;
    u64 boolean_regs;

    u32 n_flex_regs;
    u64 flex_regs;
}

mux_regs + boolean_regs would then probably be an array of (address, value) pairs (though probably just use a 1D array)

Validate user state

Early on the implementation should check that the user has root privileges (search for CAP_SYS_ADMIN in i915_perf.c to find an example)

To read the arrays of registers from userspace refer to read_properties_unlocked() as one example and look into get_user() as the kernel api for safely reading from the userspace.

The implementation will need to check for trying to add a config with a conflicting uuid. We'll have to think more careful about the best behaviour here (maybe we would trust userspace knows what it's doing and allow it to replace/override a config) but the simplest way to start will be to return an error like -EINVAL for a conflict.

Even though this interface will require root privileges, I think it will still be appropriate to white list what address ranges for these register configurations will be accepted, so that's one thing to validate early on in the implementation of this ioctl.

Track the new configs

So in the end we want to add a directory to /sys/class/drm/card0/metrics/ for a new config registered with this ioctl

After validating what userspace is giving the kernel, then we will need to store a record of this new config and that will probably go under dev_priv->perf

Take a look at struct drm_i915_private in i915-drv.h which contains a nested 'perf' structure.

I expect we can add a struct list_head dynamic_configs; here with a structure similar to the one passed to the ioctl, but also an embedded struct list_head link; member so that it can be linked into the dynamic_configs list.

For each config added to this list it will also need an ID.

Similar to how context IDs are allocated in i915-gem-context.c it could make sense to use the idr_alloc() utility api to allocate IDs. For that we will need to add a struct idr metrics_idr; member to dev_priv->perf too.

Currently it's the {hsw,bdw,chv,skl}_enable_metric_set() functions that take an ID from userspace to lookup the corresponding noa + boolean + flex registers that were pre-defined at build time. These functions will need to recognise a partitioning of the IDs userspace can give between the pre-defined IDs < I915_OA_METRIC_SET_MAX (ref: include/uapi/drm/i915_perf.h) and some dynamically allocated IDs for configs registered via this ioctl.

When initializing the struct idr, you'll want to make sure it doesn't allocate in the range of these pre-defined IDs.

Sysfs

Look at the sysfs related code that's e.g. generated in i915_oa_bdw.c to see how to add a new metrics/ directory.

One fiddly detail is going to be that unlike the autogenerated sysfs code the callbacks can't just use snprintf with a literal guid string or refer to the predefined IDs from include/uapi/drm/i915_drm.h.

E.g. considering the state to add the 3D config to sysfs:

static ssize_t
show_3d_id(struct device *kdev, struct device_attribute *attr, char *buf)
{
        return sprintf(buf, "%d\n", I915_OA_METRICS_SET_3D);
}

static struct device_attribute dev_attr_3d_id = {
        .attr = { .name = "id", .mode = S_IRUGO },
        .show = show_3d_id,
        .store = NULL,
};

static struct attribute *attrs_3d[] = {
        &dev_attr_3d_id.attr,
        NULL,
};

static struct attribute_group group_3d = {
        .name = "b541bd57-0e0f-4154-b4c0-5858010a2bf7",
        .attrs =  attrs_3d,
};

And see i915_perf_init_sysfs_bdw() where a new group is added to the metrics kobject that represents the 'metrics' directory in sysfs where it calls:

ret = sysfs_create_group(dev_priv->perf.metrics_kobj, &group_3d);

I imagine that it would work to embed a struct attribute_group and struct device_attribute into the same structure that will be linked into the dev_priv->perf.dynamic_configs list.

Lets say we add a struct i915_oa_config something like:

struct i915_oa_config {

    struct list_head link;

    char guid[40]; /* enough to fit a name like "b541bd57-0e0f-4154-b4c0-5858010a2bf7" that can be pointed to by attribute_group::name */
    u64 id; /* allocated via idr_alloc() */

    const struct i915_oa_reg *mux_regs;
    int mux_regs_len;
    const struct i915_oa_reg *b_counter_regs;
    int b_counter_regs_len;
    const struct i915_oa_reg *flex_regs;
    int flex_regs_len;

    struct attribute_group sysfs_metric;
    struct device_attribute sysfs_metric_id;
};

Then the sysfs members would be initialized similar to how they are initialized in the autogenerated code and added to sysfs via sysfs_create_group()

For the device attribute callback (like show_3d_id()) since it's passed a pointer to the struct device_attribute which we know is embedded within a struct i915_oa_config we can use the container_of() macro to map the attr pointer to an i915_oa_config pointer where we'll find the corresponding ID that should be written to buf.

Footnote

In general to start with you'll pretty much be able to ignore worrying about locking, and there will only be some minor cause for locking in the end considering the possibility that another process might try and open a stream at the same time that you are registering a stream.