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

[Dave Gordon <david.s.gordon@intel.com>]

On 07/01/16 11:58, 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.
> -Chris

I think you'll find that the version of i915_gem_request_alloc() I've 
implemented is equivalent to yours, with the *additional* (and better) 
semantic of not requiring the caller to specify (ring->default_param) as 
the context parameter (which is the main point, as far as I'm concerned; 
making the calling convention nicer was just incidental).

*MINE*:
diff --git a/drivers/gpu/drm/i915/i915_drv.h 
b/drivers/gpu/drm/i915/i915_drv.h
index c6dd4db..c2b000a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2260,9 +2260,9 @@ struct drm_i915_gem_request {

  };

-int i915_gem_request_alloc(struct intel_engine_cs *ring,
-			   struct intel_context *ctx,
-			   struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request * __must_check
+i915_gem_request_alloc(struct intel_engine_cs *engine,
+		       struct intel_context *ctx);
  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
  void i915_gem_request_free(struct kref *req_ref);
  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
...

*YOURS*:
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h 
b/drivers/gpu/drm/i915/i915_gem_request.h
index 0869505..2da9e0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -118,9 +118,9 @@ struct drm_i915_gem_request {
  	int elsp_submitted;
  };

-int i915_gem_request_alloc(struct intel_engine_cs *ring,
-			   struct intel_context *ctx,
-			   struct drm_i915_gem_request **req_out);
+struct drm_i915_gem_request *
+i915_gem_request_alloc(struct intel_engine_cs *ring,
+		       struct intel_context *ctx);
  void i915_gem_request_cancel(struct drm_i915_gem_request *req);
  int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
  				   struct drm_file *file);
...

You seem to have done EXACTLY THE SAME transformation of the function 
signature (except I remembered to add __must_check, and wrote some 
kerneldoc for it), so what's not to like?

I haven't done anything to {__}i915_gem_object_sync(), but that's a 
separate issue that you can fix on top of this.

.Dave.

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