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

[Chris Wilson]

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.

> >>> 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).
-Chris