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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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

A patch review should contain one of the following:
  - Acknowledge and accept patch: provide Reviewed-by tag
  - Request for changes or fixes – clear list of actionable items to be
    addressed before Reviewed-by tag is given
  - Reject due to fundamental issue with approach or conflict with
    other work – clear reasons must be provided, in the case of
    conflict, JIRA or BZ of conflicting work should be provided for
    tracking and to ensure requirements are captured

If you really have a fundamental issue here (it doesn't sound like it)
you need to be clear about it.

Thanks,
Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx