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

[Lionel Landwerlin]

On 01/07/2019 16:06, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-07-01 12:34:29)
>> @@ -2535,9 +2635,21 @@ static int i915_perf_release(struct inode *inode, struct file *file)
>>   {
>>          struct i915_perf_stream *stream = file->private_data;
>>          struct drm_i915_private *dev_priv = stream->dev_priv;
>> +       struct i915_oa_config *oa_config, *next;
>>   
>>          mutex_lock(&dev_priv->perf.lock);
>> +
>>          i915_perf_destroy_locked(stream);
>> +
>> +       /* Dispose of all oa config batch buffers. */
>> +       mutex_lock(&dev_priv->perf.metrics_lock);
>> +       list_for_each_entry_safe(oa_config, next, &dev_priv->perf.metrics_buffers, vma_link) {
>> +               list_del(&oa_config->vma_link);
>> +               i915_gem_object_put(oa_config->obj);
>> +               oa_config->obj = NULL;
>> +       }
>> +       mutex_unlock(&dev_priv->perf.metrics_lock);
> What's the reference chain from the i915_perf fd to the i915_device?
> What's even keeping the module alive!
>
> Shouldn't be a drm_dev_get() in i915_perf_open_ioctl() and a
> drm_dev_put() here?


Aye!

Looks like a candidate for stable...


>
> So there may be more than one stream, sharing the same oa_config. If a
> stream closes, you let all the current streams keep their reference and
> the next gets a new object. Looks like there's some scope for
> duplication, but looks safe enough. My main worry was for zombie
> oa_config.


The goal of this loop is to garbage collect the config BOs once OA isn't 
used anymore.

Right now there is only one engine with OA support.

We could potentially put that list on the engine to be safe.


Thanks,


-Lionel


> -Chris
>