[Intel-gfx] [PATCH v10] drm/i915: Extend LRC pinning to cover GPU context writeback

[Nick Hoath]

On 14/01/2016 12:37, Nick Hoath wrote:
> On 14/01/2016 12:31, Chris Wilson wrote:
>> On Thu, Jan 14, 2016 at 11:56:07AM +0000, Nick Hoath wrote:
>>> On 14/01/2016 11:36, Chris Wilson wrote:
>>>> On Wed, Jan 13, 2016 at 04:19:45PM +0000, Nick Hoath wrote:
>>>>> +	if (ctx->engine[ring->id].dirty) {
>>>>> +		struct drm_i915_gem_request *req = NULL;
>>>>> +
>>>>> +		/**
>>>>> +		 * If there is already a request pending on
>>>>> +		 * this ring, wait for that to complete,
>>>>> +		 * otherwise create a switch to idle request
>>>>> +		 */
>>>>> +		if (list_empty(&ring->request_list)) {
>>>>> +			int ret;
>>>>> +
>>>>> +			ret = i915_gem_request_alloc(
>>>>> +					ring,
>>>>> +					ring->default_context,
>>>>> +					&req);
>>>>> +			if (!ret)
>>>>> +				i915_add_request(req);
>>>>> +			else
>>>>> +				DRM_DEBUG("Failed to ensure context saved");
>>>>> +		} else {
>>>>> +			req = list_first_entry(
>>>>> +					&ring->request_list,
>>>>> +					typeof(*req), list);
>>>>> +		}
>>>>> +		if (req) {
>>>>> +			ret = i915_wait_request(req);
>>>>> +			if (ret != 0) {
>>>>> +				/**
>>>>> +				 * If we get here, there's probably been a ring
>>>>> +				 * reset, so we just clean up the dirty flag.&
>>>>> +				 * pin count.
>>>>> +				 */
>>>>> +				ctx->engine[ring->id].dirty = false;
>>>>> +				__intel_lr_context_unpin(
>>>>> +					ring,
>>>>> +					ctx);
>>>>> +			}
>>>>> +		}
>>>>
>>>> If you were to take a lr_context_pin on the last_context, and only
>>>> release that pin when you change to a new context, you do not need to
>>>
>>> That what this patch does.
>>>
>>>> introduce a blocking context-close, nor do you need to introduce the
>>>> usage of default_context.
>>>
>>> The use of default_context here is to stop a context hanging around
>>> after it is no longer needed.
>>
>> By blocking, which is not acceptable. Also we can eliminate the
>> default_context and so pinning that opposed to the last_context serves
>> no purpose other than by chance having a more preferrable position when
>> it comes to defragmentation. But you don't enable that anyway and we
>
> Enabling the shrinker on execlists is something I'm working on which is
> predicated on this patch. Also why is blocking on closing a context not
> acceptable?
>

As a clarification: Without rewriting the execlist code to not submit or 
cleanup from an interrupt handler, we can't use refcounting to allow non 
blocking closing.

>> have alternative strategies now that avoid the issue with fragmentation
>> of the mappable aperture.
>>
>>>> (lr_context_pin should take a reference on the ctx to prevent early
>>>> freeeing ofc).
>>>
>>> You can't clear the reference on the ctx in an interrupt context.
>>
>> The execlists submission should moved out of the interrupt context, for
>> the very simple reason that it is causing machine panics. userspace
>> submits a workload, machine lockups....
>
> Create a jira, and I'm sure we'll look at making that change.
>
>> -Chris
>>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>