Re: [PATCH] drm/i915/perf: Refactor oa object to better manage resources

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

On 14/05/2019 19:14, Umesh Nerlige Ramappa wrote:
> On Tue, May 14, 2019 at 10:34:49AM +0100, Lionel Landwerlin wrote:
>> Hi Umesh,
>>
>> I just noticed this different between v1 & v2.
>> My understanding is that if destroy() is called, stream should be the 
>> same as dev_priv->perf.exclusive_stream.
>> If it's not it sounds like a bug. So why change this?
>>
> v2 fixes only checkpatch warnings. it warned on use of BUG_ON. BUG_ON 
> is intended to crash the system in severe cases where the 
> driver/kernel is unusable. In this case, the mismatch between user 
> passed information and exclusive_stream may not require a crash.


This is called from i915_perf_release() which is attached to the 
i915-perf file descriptor only in i915_perf.c.

If we managed to reach that function it must be because the file 
descriptor given by userspace is associated to the i915-perf stream.

Having stream != dev_priv->perf.exclusive_stream means that we probably 
screwed up the locking somewhere in this file.

So I would argue this is a kernel issue, not a user issue and that using 
BUG_ON() is justified.


Thanks,


-Lionel


>> -Lionel
>>
>> On 03/05/2019 00:13, Umesh Nerlige Ramappa wrote:
>>>  static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
>>>  {
>>>      struct drm_i915_private *dev_priv = stream->dev_priv;
>>> -    BUG_ON(stream != dev_priv->perf.oa.exclusive_stream);
>>> +    if (stream != dev_priv->perf.exclusive_stream) {
>>> +        WARN_ON_ONCE(stream != dev_priv->perf.exclusive_stream);
>>> +        return;
>>> +    }
>>>      /*
>>
>>
>

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