Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests
Date: Thu, 7 Jan 2016 16:53:00 +0000	[thread overview]
Message-ID: <20160107165300.GR652@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <568E9722.8010309@virtuousgeek.org>

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 Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx