[Intel-gfx] [PATCH 04/23] drm/i915: Push the ring creation flags to the backend

[Tvrtko Ursulin]

On 30/07/2019 10:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-29 13:59:22)
>>
>> On 26/07/2019 09:43, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-07-24 12:11:54)
>>>>
>>>> On 23/07/2019 19:38, Chris Wilson wrote:
>>>>> Push the ring creation flags from the outer GEM context to the inner
>>>>> intel_cotnext to avoid an unsightly back-reference from inside the
>>>>> backend.
>>>>
>>>> Sorry I find this quite ugly. Passing in integers in pointers filed and
>>>> casting back and forth.
>>>
>>> But better than a union, since once the intel_context is active, the
>>> ring is always a ring.
>>
>> Unless it is u64 size. I am not buying it. :)
> 
> We don't need u64 size? I don't understand what you mean.

I complained about very unobvious and questionable hack of passing the 
size in the pointer field and you said it is better than an union. For 
me union certainly rates way higher than the casing hack with a macro.

>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index 6d3911469801..e237bcecfa1f 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -328,10 +328,14 @@ static void print_context_stats(struct seq_file *m,
>>>>>     
>>>>>                 for_each_gem_engine(ce,
>>>>>                                     i915_gem_context_lock_engines(ctx), it) {
>>>>> -                     if (ce->state)
>>>>> -                             per_file_stats(0, ce->state->obj, &kstats);
>>>>> -                     if (ce->ring)
>>>>> +                     intel_context_lock_pinned(ce);
>>>>> +                     if (intel_context_is_pinned(ce)) {
>>>>
>>>> And these hunks do not seem to belong to this patch.
>>>
>>> Then you are mistaken. The bug is older, but it gets triggered by this
>>> patch.
>>
>> Gets triggered or gets fixed? Perhaps commit message needs improving
>> since it talks about avoiding an unsightly back-reference (and I argue
>> ce->ring = u64 size is at least equally unsightly), and not fixing any bugs.
> 
> The bug is a potential race condition inside the debug. What is hit here
> is that without the state of the pin known, the meaning of ce->ring is
> unknown (whereas the other bug is that condition can change during
> evaluation).

Commit doesn't say anything about fixing bugs. It talks about making the 
code prettier.

If here we need a pin, then it should be a separate patch which says so 
and does only one thing.

Regards,

Tvrtko