Re: [PATCH v2 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown sequence

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

On 25/01/16 10:56, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 11:10:10PM +0000, Dave Gordon wrote:
>> In LRC mode, the HWSP is part of the default context object, and
>> therefore does not exist independently. Worse, it doesn't contribute
>> to the refcount on the default context object either.
>>
>> Currently, the default context is deallocated in intel_lr_context_free(),
>> but the HWSP kmapping is not torn down until the subsequent call to
>> intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj
>> continues to point to the (now non-existent) default context object,
>> and the kernel mapping likewise points to a page which is now free.
>>
>> The solution is to dispose of the HWSP kmapping and pointer before the
>> object itself is freed, so this patch moves the kmap teardown code from
>> intel_lr_context_free() to intel_logical_ring_cleanup().
>>
>> This code was introduced in
>>
>>      48d8238 drm/i915/bdw: Generic logical ring init and cleanup
>>
>> i.e. it has been there ever since LRC mode was first implemented.
>>
>> v2:
>>      Split from "handle teardown of HWSP when context is freed".
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c | 26 +++++++++++++++++++-------
>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index a3ab2b4..c702fc2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1986,13 +1986,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>>   	i915_cmd_parser_fini_ring(ring);
>>   	i915_gem_batch_pool_fini(&ring->batch_pool);
>>
>> -	if (ring->status_page.obj) {
>> -		struct page *page;
>> -
>> -		page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN);
>> -		kunmap(page);
>> -		ring->status_page.obj = NULL;
>> -	}
>> +	/* Status page should have gone already */
>> +	WARN_ON(ring->status_page.page_addr);
>> +	WARN_ON(ring->status_page.obj);
>>
>>   	ring->disable_lite_restore_wa = false;
>>   	ring->ctx_desc_template = 0;
>> @@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx)
>>   			continue;
>>
>>   		if (ctx == ctx->i915->kernel_context) {
>> +			struct intel_engine_cs *ring = ringbuf->ring;
>> +
>> +			/*
>> +			 * The HWSP is part of the kernel context
>> +			 * object in LRC mode, so tear it down now.
>> +			 */
>> +			if (ring->status_page.obj) {
>> +				struct page *page;
>> +
>> +				page = i915_gem_object_get_page(
>> +						ring->status_page.obj,
>> +						LRC_PPHWSP_PN);
>> +				kunmap(page);
>> +				ring->status_page.obj = NULL;
>> +			}
>
> No. I see no reason why we need to add special code to the context-free.
> -Chris

The special-case-kernel-context test /is already there/.

I'm just making it do the right thing by moving this block out of 
intel_logical_ring_cleanup() and into the /existing/ special case.

Getting rid of the special case entirely is a project for another day, 
whereas this is a bug fix (NOT unmapping it here leaves a dangling 
reference to an object that has been freed).

OTOH I can change this to use:
	kunmap(kmap_to_page(ring->status_page.page_addr));
per the previous reply, which will make it a bit neater.

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