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

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

On 22/12/15 09:08, Chris Wilson wrote:
> On Mon, Dec 21, 2015 at 04:04:42PM +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. For
>> such requests, we associate them with the default context for the engine
>> that the request will be submitted to.
>>
>> This patch provides a shorthand for doing such request allocations and
>> changes all such calls to use the new function.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>   drivers/gpu/drm/i915/i915_gem.c      | 36 ++++++++++++++++++++++++++++--------
>>   drivers/gpu/drm/i915/intel_display.c |  6 ++++--
>>   drivers/gpu/drm/i915/intel_overlay.c | 24 ++++++++++++------------
>>   4 files changed, 46 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 666d07c..4955db9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2270,6 +2270,8 @@ 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_anon(struct intel_engine_cs *ring);
>>   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,
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index be1f984..9f9c0c0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2751,6 +2751,21 @@ err:
>>   	return ret;
>>   }
>>
>> +/*
>> + * Allocate a request associated with the default context for the given
>> + * ring. This can be used where the driver needs a request for internal
>> + * purposes not directly related to a user batch submission.
>> + */
>> +struct drm_i915_gem_request *
>> +i915_gem_request_alloc_anon(struct intel_engine_cs *ring)
>> +{
>
> As demonstrated, no. Contexts need to be considered properly first.
>
>> +	struct drm_i915_gem_request *req;
>> +	int err;
>> +
>> +	err = i915_gem_request_alloc(ring, ring->default_context, &req);
>> +	return err ? ERR_PTR(err) : req;
>> +}
>> +
>>   void i915_gem_request_cancel(struct drm_i915_gem_request *req)
>>   {
>>   	intel_ring_reserved_space_cancel(req->ringbuf);
>> @@ -3168,9 +3183,13 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>>   			return 0;
>>
>>   		if (*to_req == NULL) {
>> -			ret = i915_gem_request_alloc(to, to->default_context, to_req);
>> -			if (ret)
>> -				return ret;
>> +			struct drm_i915_gem_request *req;
>> +
>> +			req = i915_gem_request_alloc_anon(to);
>
> Wrong context. Please see patches to fix this mess.
>
>> +			if (IS_ERR(req))
>> +				return PTR_ERR(req);
>> +
>> +			*to_req = req;
>>   		}
>>
>>   		trace_i915_gem_ring_sync_to(*to_req, from, from_req);
>> @@ -3370,9 +3389,9 @@ int i915_gpu_idle(struct drm_device *dev)
>>   		if (!i915.enable_execlists) {
>>   			struct drm_i915_gem_request *req;
>>
>> -			ret = i915_gem_request_alloc(ring, ring->default_context, &req);
>> -			if (ret)
>> -				return ret;
>> +			req = i915_gem_request_alloc_anon(ring);
>> +			if (IS_ERR(req))
>> +				return PTR_ERR(req);
>>
>>   			ret = i915_switch_context(req);
>>   			if (ret) {
>> @@ -4867,8 +4886,9 @@ i915_gem_init_hw(struct drm_device *dev)
>>   	for_each_ring(ring, dev_priv, i) {
>>   		struct drm_i915_gem_request *req;
>>
>> -		ret = i915_gem_request_alloc(ring, ring->default_context, &req);
>> -		if (ret) {
>> +		req = i915_gem_request_alloc_anon(ring);
>> +		if (IS_ERR(req)) {
>> +			ret = PTR_ERR(req);
>>   			i915_gem_cleanup_ringbuffer(dev);
>>   			goto out;
>>   		}
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index abd2d29..5716f4a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11662,9 +11662,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   					obj->last_write_req);
>>   	} else {
>>   		if (!request) {
>> -			ret = i915_gem_request_alloc(ring, ring->default_context, &request);
>> -			if (ret)
>
> Wrong context.
> -Chris

What do you mean, "wrong context". The line above is the way it 
*currently* works, which I'm replacing with one that *doesn't* refer to 
the default context.

This patch doesn't change any functionality, just simplifies it by 
reducing the number of instances of "default_context" so that patch 4/6 
can actually get rid of ring->default_context entirely. I thought that 
was what you were aiming for?

.Dave.

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