Re: [PATCH v6 03/11] drm/i915/perf: allow for CS OA configs to be created lazily

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

On 01/07/2019 18:09, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-07-01 12:34:29)
>>   struct i915_oa_config {
>> +       struct drm_i915_private *i915;
>> +
>>          char uuid[UUID_STRING_LEN + 1];
>>          int id;
>>   
>> @@ -1110,6 +1112,10 @@ struct i915_oa_config {
>>          struct attribute *attrs[2];
>>          struct device_attribute sysfs_metric_id;
>>   
>> +       struct drm_i915_gem_object *obj;
>> +
>> +       struct list_head vma_link;
>> +
>>          atomic_t ref_count;
>>   };
>> -static void free_oa_config(struct drm_i915_private *dev_priv,
>> -                          struct i915_oa_config *oa_config)
>> +static void put_oa_config(struct i915_oa_config *oa_config)
>>   {
>> +       if (!atomic_dec_and_test(&oa_config->ref_count))
>> +               return;
> I strongly advise that ref_count be replaced by struct kref, just so that
> we get the benefit of debugging.
>
> put_oa_config -> kref_put(&oa_config->ref, free_oa_config)
> (free_oa_config takes kref as its arg and uses container_of())


This is done in "drm/i915: add a new perf configuration execbuf parameter"

I'll factor it in this commit.


>
>> +int i915_perf_get_oa_config(struct drm_i915_private *i915,
>> +                           int metrics_set,
>> +                           struct i915_oa_config **out_config,
>> +                           struct drm_i915_gem_object **out_obj)
>> +{
>> +       int ret = 0;
>> +       struct i915_oa_config *oa_config;
>> +
>> +       if (!i915->perf.initialized)
>> +               return -ENODEV;
>> +
>> +       ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
>>          if (ret)
>>                  return ret;
>>   
>> -       *out_config = idr_find(&dev_priv->perf.metrics_idr, metrics_set);
>> -       if (!*out_config)
>> -               ret = -EINVAL;
>> -       else
>> -               atomic_inc(&(*out_config)->ref_count);
>> +       if (metrics_set == 1) {
>> +               oa_config = &i915->perf.oa.test_config;
>> +       } else {
>> +               oa_config = idr_find(&i915->perf.metrics_idr, metrics_set);
> Why not have the builtin[1] inside the idr?


I think it was just a way to avoid removing it from the idr through 
userspace calls.


> -Chris
>

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