Re: [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Subject: Re: [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA
Date: Mon, 18 Apr 2016 13:08:45 +0100	[thread overview]
Message-ID: <5714CE4D.7020101@linux.intel.com> (raw)
In-Reply-To: <20160418115633.GD18964@nuc-i3427.alporthouse.com>

On 18/04/16 12:56, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 12:14:02PM +0100, Chris Wilson wrote:
>> By tracking the iomapping on the VMA itself, we can share that area
>> between multiple users. Also by only revoking the iomapping upon
>> unbinding from the mappable portion of the GGTT, we can keep that iomap
>> across multiple invocations (e.g. execlists context pinning).
>>
>> Note that by moving the iounnmap tracking to the VMA, we actually end up
>> fixing a leak of the iomapping in intel_fbdev.
>>
>> v1.5: Rebase prompted by Tvrtko
>> v2: Drop dev_priv parameter, we can recover the i915_ggtt from the vma.
>> v3: Move handling of ioremap space exhaustion to vmap_purge and also
>> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
>> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
>> v5: Back to i915_vm_to_ggtt
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c          |  2 ++
>>   drivers/gpu/drm/i915/i915_gem_gtt.c      | 23 +++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_gem_gtt.h      | 38 ++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 ++++++++++++++++++-----
>>   drivers/gpu/drm/i915/intel_fbdev.c       | 22 +++++++++---------
>>   5 files changed, 97 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 6ce2c31b9a81..82d0af3f1692 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3378,6 +3378,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>>   		ret = i915_gem_object_put_fence(obj);
>>   		if (ret)
>>   			return ret;
>> +
>> +		i915_vma_iounmap(vma);
>>   	}
>>
>>   	trace_i915_vma_unbind(vma);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index b3af2e808b49..c894af7e0ea1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3634,3 +3634,26 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>>   		return obj->base.size;
>>   	}
>>   }
>> +
>> +void *i915_vma_iomap(struct i915_vma *vma)
>
> I've been arguing with myself as to whether to make this
> i915_vma_pin_iomap (and so acquire the vma->pin_count as well).
>
> There will still be the requirement that the vma is already usuable, but
> it will mean that the returned iomap cannot be reaped for the critical
> section of its user.

I don't have a strong feeling either way. vma_pin_iomap would make the 
name (and usage model) more consistent with obj_pin_map and taking the 
pin count could potentially make it a little bit safer for the users 
where it would leak rather than crash. So maybe on the basis of those it 
would be a tiny bit better.

Regards,

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