[Intel-gfx] [PATCH v6 3/3] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface

[Lionel Landwerlin]

On 18/07/17 19:34, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2017-07-18 17:50:42)
>>   static struct drm_driver driver = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 2b824f8875c4..607484737f3d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1917,6 +1917,9 @@ struct i915_oa_config {
>>          struct attribute_group sysfs_metric;
>>          struct attribute *attrs[2];
>>          struct device_attribute sysfs_metric_id;
>> +
>> +       /* Only updated while holding dev_priv->perf.metrics_lock. */
>> +       bool in_use;
> Definitely do not get warm fuzzy feeling about this.
>
> Would making this a regular refcount be difficult? It would for example
> allow removing whilst it is still in use by a stream (just removing it
> from the user table so new streams cannot be created with it). And stop
> me from making helpful suggestions about how in_use access is purely
> atomic...

Moved to atomic refcount.

>
>> +       oa_config = kzalloc(sizeof(*oa_config), GFP_KERNEL);
>> +       if (!oa_config) {
>> +               DRM_DEBUG("Failed to allocate memory for the OA config\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       err = strncpy_from_user(oa_config->uuid, u64_to_user_ptr(args->uuid),
>> +                               UUID_STRING_LEN);
> So you have a fixed size buffer, why have userspace pass a pointer
> rather than an array? Try char uuid[UUID_STRING_LEN] in
> struct drm_i915_perf_oa_config and see which you think is eaiser to use
> and, more importantly, harder to get wrong.

Done.

>
>> +       if (err < 0) {
>> +               DRM_DEBUG("Failed to copy uuid from OA config\n");
>> +               goto mux_err;
>> +       }
>> +
>> +       if (!uuid_is_valid(oa_config->uuid)) {
>> +               DRM_DEBUG("Invalid uuid format for OA config\n");
>> +               err = -EINVAL;
>> +               goto mux_err;
>> +       }
>> +       idr_for_each_entry(&dev_priv->perf.metrics_idr, tmp, id) {
> Comment that you don't expect many so this iteration shouldn't be too
> costly.

Done.

>
>> +               if (!strcmp(tmp->uuid, oa_config->uuid)) {
>> +                       DRM_DEBUG("OA config already exists with this uuid\n");
>> +                       err = -EADDRINUSE;
>> +                       goto sysfs_err;
>> +               }
>> +       }
>> +
>> +       err = create_dynamic_oa_sysfs_entry(dev_priv, oa_config);
>> +       if (err) {
>> +               DRM_DEBUG("Failed to create sysfs entry for OA config\n");
>> +               goto sysfs_err;
>> +       }
>> +
>> +       oa_config->id = idr_alloc(&dev_priv->perf.metrics_idr,
>> +                                 oa_config, 2,
> Comment on reserving 0 for invalid and 1 for default.

Done.

>
>> +}
>> +/**
>> + * Structure to upload perf dynamic configuration into the kernel.
>> + */
>> +struct drm_i915_perf_oa_config {
>> +       /** String formatted like "%08x-%04x-%04x-%04x-%012x" */
>> +       __u64 __user uuid;
> Not a pointer, so __user is meaningless (and should generate warnings
> from sparse). Equally it is nice to know in the name which of these are
> encoding user pointers.
>
>> +       __u32 n_mux_regs;
>> +       __u32 pad0;
>> +       __u64 __user mux_regs;
>> +
>> +       __u32 n_boolean_regs;
>> +       __u32 pad1;
>> +       __u64 __user boolean_regs;
>> +
>> +       __u32 n_flex_regs;
>> +       __u32 pad2;
>> +       __u64 __user flex_regs;
>
> With a little bit of rearranging you can reduce the padding.

Done.

> -Chris
>