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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-08-01 09:42:15)
> 
> 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.

It's not as if we are going to backport it... The bug is trivial to hit
when ring is alias, to hit it going the other way requires more mutex
evasion.
-Chris