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

[Nick Hoath]

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?

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