On 07/01/16 16:53, Chris Wilson wrote:
On Thu, Jan 07, 2016 at 08:49:38AM -0800, Jesse Barnes wrote:
On 01/07/2016 03:58 AM, Chris Wilson wrote:
On Thu, Jan 07, 2016 at 10:20:50AM +0000, Dave Gordon wrote:
There are a number of places where the driver needs a request, but isn't
working on behalf of any specific user or in a specific context. At
present, we associate them with the per-engine default context. A future
patch will abolish those per-engine context pointers; but we can already
eliminate a lot of the references to them, just by making the allocator
allow NULL as a shorthand for "an appropriate context for this ring",
which will mean that the callers don't need to know anything about how
the "appropriate context" is found (e.g. per-ring vs per-device, etc).
So this patch renames the existing i915_gem_request_alloc(), and makes
it local (static inline), and replaces it with a wrapper that provides
a default if the context is NULL, and also has a nicer calling
convention (doesn't require a pointer to an output parameter). Then we
change all callers to use the new convention:
OLD:
err = i915_gem_request_alloc(ring, user_ctx, &req);
if (err) ...
NEW:
req = i915_gem_request_alloc(ring, user_ctx);
if (IS_ERR(req)) ...
OLD:
err = i915_gem_request_alloc(ring, ring->default_context, &req);
if (err) ...
NEW:
req = i915_gem_request_alloc(ring, NULL);
if (IS_ERR(req)) ...
Nak. You haven't fixed i915_gem_request_alloc() at all.
http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=breadcrumbs&id=82c72e1a2b4385f0ab07dccee45acef38303e96f
is the patch I have been carrying ever since.
Can we stop with the "nak"? This patch wraps the request alloc
differently than yours, but you haven't given details as to why you
think it's incorrect (see Dave's reply).
I am annoyed that people do not review my patches and are duplicating
work I did last year and the year before, without attempting to fix
real bugs.
-Chris
Chris, this patchset is totally directed towards fixing a specific bug,
one which, moreover, arose a consequence of a patch YOU wrote:
b0366a5 drm/i915: intel_ring_initialized() must be simple and inline
(mea culpa too, obviously, since I was the one who rebased & pushed it).
Nick has a fix for the original bug, which involves reversing the
teardown order, but can't now use it since b0366a5, so the bug remains.
Nick's fix can be made to work if we replace the per-engine default
contexts with the global one, which you've already agreed is a good idea
(I think it was your idea in the first place!).
We can't take your patch because it doesn't apply to nightly. If you
provide a standalone version that's not entangled with 100 other patches
I'll happily review it. Or I might cherry-pick your existing one out of
the 190-element patchset and try to rebase it onto nightly, which is how
b0366a5 got in in the first place. I suspect it would look very much
like mine then ...
Maybe we should just revert b0366a5 instead? Even though it was quite
nice in itself ...
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
