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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
