[136/190] drm/i915: Move ioremap_wc tracking onto VMA
diff mbox

Message ID 1452509174-16671-50-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Jan. 11, 2016, 10:45 a.m. UTC
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).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c     |  5 +++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h |  4 ++++
 drivers/gpu/drm/i915/intel_fbdev.c  |  8 +++-----
 4 files changed, 45 insertions(+), 5 deletions(-)

Comments

Tvrtko Ursulin Feb. 11, 2016, 1:20 p.m. UTC | #1
On 11/01/16 10:45, 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).

Between the lines and from some IRC discussion it seems the goal of this 
is to fix an address space memory leak with fbcon?

But I don't know fbdev so can't find who would do the unpin on the VMA 
to allow unbind to eventually unmap it?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     |  5 +++++
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |  4 ++++
>   drivers/gpu/drm/i915/intel_fbdev.c  |  8 +++-----
>   4 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0c4e8e1aeeff..5bb21b20c36a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2699,6 +2699,11 @@ int i915_vma_unbind(struct i915_vma *vma)
>   		if (ret)
>   			return ret;
>
> +		if (vma->iomap) {
> +			iounmap(vma->iomap);
> +			vma->iomap = NULL;
> +		}
> +
>   		vma->map_and_fenceable = false;
>   	}
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b8af904ad12c..3fcf2fd73453 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3575,3 +3575,36 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>
>   	return 0;
>   }
> +
> +void *i915_vma_iomap(struct drm_i915_private *dev_priv,
> +		     struct i915_vma *vma)
> +{
> +	if (WARN_ON(!vma->map_and_fenceable))
> +		return ERR_PTR(-ENODEV);
> +
> +	GEM_BUG_ON(!vma->is_ggtt);
> +	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> +
> +	if (vma->iomap == NULL) {
> +		u32 base = dev_priv->gtt.mappable_base + vma->node.start;
> +		void *ptr;
> +
> +		ptr = ioremap_wc(base, vma->size);
> +		if (ptr == NULL) {
> +			int ret;
> +
> +			/* Too many areas already allocated? */
> +			ret = i915_gem_evict_vm(vma->vm, true);
> +			if (ret)
> +				return ERR_PTR(ret);
> +
> +			ptr = ioremap_wc(base, vma->size);
> +			if (ptr == NULL)
> +				return ERR_PTR(-ENOMEM);
> +		}
> +
> +		vma->iomap = ptr;
> +	}
> +
> +	return vma->iomap;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 6b0f557982d5..0e0570e13a68 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -181,6 +181,7 @@ struct i915_vma {
>   	struct drm_mm_node node;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_address_space *vm;
> +	void *iomap;
>   	u64 size;
>
>   	struct i915_gem_active last_read[I915_NUM_RINGS];
> @@ -579,4 +580,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev);
>
>   int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
>   void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
> +
> +void *i915_vma_iomap(struct drm_i915_private *dev_priv,
> +		     struct i915_vma *vma);
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7decbca25dbb..8e7c341951fd 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -248,12 +248,10 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
>   	info->fix.smem_len = vma->node.size;
>
> -	info->screen_base =
> -		ioremap_wc(dev_priv->gtt.mappable_base + vma->node.start,
> -			   vma->node.size);
> -	if (!info->screen_base) {
> +	info->screen_base = i915_vma_iomap(dev_priv, vma);
> +	if (IS_ERR(info->screen_base)) {
>   		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(info->screen_base);
>   		goto out_destroy_fbi;
>   	}
>   	info->screen_size = vma->node.size;
>
Chris Wilson Feb. 11, 2016, 1:29 p.m. UTC | #2
On Thu, Feb 11, 2016 at 01:20:46PM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 11/01/16 10:45, 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).
> 
> Between the lines and from some IRC discussion it seems the goal of
> this is to fix an address space memory leak with fbcon?

The goal is to prevent an issue from hastily dropping iomappings (and
vmappings elsewhere) when unpinning contexts. That comes into play when
we track the ring->vma (not just track ring->vma as we do now, but can
rely on the vma being persistent). Fixing a leak on driver unload is an
interesting side-effect.
 
> But I don't know fbdev so can't find who would do the unpin on the
> VMA to allow unbind to eventually unmap it?

That actually gets fixed in another patch when we teach intel_fbdev to
actually track the VMA it allocates, as right now we deliberately leak
it to keep the code simple.
-Chris
Tvrtko Ursulin Feb. 11, 2016, 2:10 p.m. UTC | #3
On 11/02/16 13:29, Chris Wilson wrote:
> On Thu, Feb 11, 2016 at 01:20:46PM +0000, Tvrtko Ursulin wrote:
>>
>>
>> On 11/01/16 10:45, 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).
>>
>> Between the lines and from some IRC discussion it seems the goal of
>> this is to fix an address space memory leak with fbcon?
>
> The goal is to prevent an issue from hastily dropping iomappings (and
> vmappings elsewhere) when unpinning contexts. That comes into play when
> we track the ring->vma (not just track ring->vma as we do now, but can
> rely on the vma being persistent). Fixing a leak on driver unload is an
> interesting side-effect.
>
>> But I don't know fbdev so can't find who would do the unpin on the
>> VMA to allow unbind to eventually unmap it?
>
> That actually gets fixed in another patch when we teach intel_fbdev to
> actually track the VMA it allocates, as right now we deliberately leak
> it to keep the code simple.

Ok in that case ack on the patch.

It will need to assert on VMA being pinned I think and some minor 
changes when you rebase it on top of nightly as standalone so I can 
properly review it then.

Regards,

Tvrtko
Chris Wilson Feb. 19, 2016, 3:11 p.m. UTC | #4
On Thu, Feb 11, 2016 at 02:10:19PM +0000, Tvrtko Ursulin wrote:
> 
> On 11/02/16 13:29, Chris Wilson wrote:
> >On Thu, Feb 11, 2016 at 01:20:46PM +0000, Tvrtko Ursulin wrote:
> >>
> >>
> >>On 11/01/16 10:45, 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).
> >>
> >>Between the lines and from some IRC discussion it seems the goal of
> >>this is to fix an address space memory leak with fbcon?
> >
> >The goal is to prevent an issue from hastily dropping iomappings (and
> >vmappings elsewhere) when unpinning contexts. That comes into play when
> >we track the ring->vma (not just track ring->vma as we do now, but can
> >rely on the vma being persistent). Fixing a leak on driver unload is an
> >interesting side-effect.
> >
> >>But I don't know fbdev so can't find who would do the unpin on the
> >>VMA to allow unbind to eventually unmap it?
> >
> >That actually gets fixed in another patch when we teach intel_fbdev to
> >actually track the VMA it allocates, as right now we deliberately leak
> >it to keep the code simple.
> 
> Ok in that case ack on the patch.
> 
> It will need to assert on VMA being pinned I think and some minor
> changes when you rebase it on top of nightly as standalone so I can
> properly review it then.

Had some more fun recently where the iomap was showing up again, and
realised that for 64b systems we can keep a pointer to inside our
dev_priv->gtt.mappable iomapping. Makes the patch a little more ugly
(requires #ifdeffry) but eliminates all of the runtime iomapping
overhead!

However, that wasn't enough for the test case as the limitation to the
mappable aperture was too severe...
-Chris
Tvrtko Ursulin Feb. 22, 2016, 3:29 p.m. UTC | #5
On 19/02/16 15:11, Chris Wilson wrote:
> On Thu, Feb 11, 2016 at 02:10:19PM +0000, Tvrtko Ursulin wrote:
>>
>> On 11/02/16 13:29, Chris Wilson wrote:
>>> On Thu, Feb 11, 2016 at 01:20:46PM +0000, Tvrtko Ursulin wrote:
>>>>
>>>>
>>>> On 11/01/16 10:45, 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).
>>>>
>>>> Between the lines and from some IRC discussion it seems the goal of
>>>> this is to fix an address space memory leak with fbcon?
>>>
>>> The goal is to prevent an issue from hastily dropping iomappings (and
>>> vmappings elsewhere) when unpinning contexts. That comes into play when
>>> we track the ring->vma (not just track ring->vma as we do now, but can
>>> rely on the vma being persistent). Fixing a leak on driver unload is an
>>> interesting side-effect.
>>>
>>>> But I don't know fbdev so can't find who would do the unpin on the
>>>> VMA to allow unbind to eventually unmap it?
>>>
>>> That actually gets fixed in another patch when we teach intel_fbdev to
>>> actually track the VMA it allocates, as right now we deliberately leak
>>> it to keep the code simple.
>>
>> Ok in that case ack on the patch.
>>
>> It will need to assert on VMA being pinned I think and some minor
>> changes when you rebase it on top of nightly as standalone so I can
>> properly review it then.
>
> Had some more fun recently where the iomap was showing up again, and
> realised that for 64b systems we can keep a pointer to inside our
> dev_priv->gtt.mappable iomapping. Makes the patch a little more ugly
> (requires #ifdeffry) but eliminates all of the runtime iomapping
> overhead!
>
> However, that wasn't enough for the test case as the limitation to the
> mappable aperture was too severe...

Could use kmap then and not go through the aperture? I had a patch of 
similar semantics to your vma->iomap somewhere which, to start with, 
adds ability to kmap one page. Or it could do the whole objects for 
simplicity which for LRCs should be OK.

Regards,

Tvrtko
Chris Wilson Feb. 23, 2016, 10:21 a.m. UTC | #6
On Mon, Feb 22, 2016 at 03:29:57PM +0000, Tvrtko Ursulin wrote:
> 
> On 19/02/16 15:11, Chris Wilson wrote:
> >On Thu, Feb 11, 2016 at 02:10:19PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 11/02/16 13:29, Chris Wilson wrote:
> >>>On Thu, Feb 11, 2016 at 01:20:46PM +0000, Tvrtko Ursulin wrote:
> >>>>
> >>>>
> >>>>On 11/01/16 10:45, 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).
> >>>>
> >>>>Between the lines and from some IRC discussion it seems the goal of
> >>>>this is to fix an address space memory leak with fbcon?
> >>>
> >>>The goal is to prevent an issue from hastily dropping iomappings (and
> >>>vmappings elsewhere) when unpinning contexts. That comes into play when
> >>>we track the ring->vma (not just track ring->vma as we do now, but can
> >>>rely on the vma being persistent). Fixing a leak on driver unload is an
> >>>interesting side-effect.
> >>>
> >>>>But I don't know fbdev so can't find who would do the unpin on the
> >>>>VMA to allow unbind to eventually unmap it?
> >>>
> >>>That actually gets fixed in another patch when we teach intel_fbdev to
> >>>actually track the VMA it allocates, as right now we deliberately leak
> >>>it to keep the code simple.
> >>
> >>Ok in that case ack on the patch.
> >>
> >>It will need to assert on VMA being pinned I think and some minor
> >>changes when you rebase it on top of nightly as standalone so I can
> >>properly review it then.
> >
> >Had some more fun recently where the iomap was showing up again, and
> >realised that for 64b systems we can keep a pointer to inside our
> >dev_priv->gtt.mappable iomapping. Makes the patch a little more ugly
> >(requires #ifdeffry) but eliminates all of the runtime iomapping
> >overhead!
> >
> >However, that wasn't enough for the test case as the limitation to the
> >mappable aperture was too severe...
> 
> Could use kmap then and not go through the aperture? I had a patch
> of similar semantics to your vma->iomap somewhere which, to start
> with, adds ability to kmap one page. Or it could do the whole
> objects for simplicity which for LRCs should be OK.

Actually I used vmap to avoid the limitation. But yes, for LRC we really
don't need to keep the whole state object mapped (and we don't, we just
kmap the registers), but we would either want to reduce the ringbuffer to
a single page, avoid commands spanning the page boundaries, or just use
vmap.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0c4e8e1aeeff..5bb21b20c36a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2699,6 +2699,11 @@  int i915_vma_unbind(struct i915_vma *vma)
 		if (ret)
 			return ret;
 
+		if (vma->iomap) {
+			iounmap(vma->iomap);
+			vma->iomap = NULL;
+		}
+
 		vma->map_and_fenceable = false;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b8af904ad12c..3fcf2fd73453 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3575,3 +3575,36 @@  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 
 	return 0;
 }
+
+void *i915_vma_iomap(struct drm_i915_private *dev_priv,
+		     struct i915_vma *vma)
+{
+	if (WARN_ON(!vma->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	GEM_BUG_ON(!vma->is_ggtt);
+	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	if (vma->iomap == NULL) {
+		u32 base = dev_priv->gtt.mappable_base + vma->node.start;
+		void *ptr;
+
+		ptr = ioremap_wc(base, vma->size);
+		if (ptr == NULL) {
+			int ret;
+
+			/* Too many areas already allocated? */
+			ret = i915_gem_evict_vm(vma->vm, true);
+			if (ret)
+				return ERR_PTR(ret);
+
+			ptr = ioremap_wc(base, vma->size);
+			if (ptr == NULL)
+				return ERR_PTR(-ENOMEM);
+		}
+
+		vma->iomap = ptr;
+	}
+
+	return vma->iomap;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 6b0f557982d5..0e0570e13a68 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -181,6 +181,7 @@  struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void *iomap;
 	u64 size;
 
 	struct i915_gem_active last_read[I915_NUM_RINGS];
@@ -579,4 +580,7 @@  void i915_gem_restore_gtt_mappings(struct drm_device *dev);
 
 int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
 void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
+
+void *i915_vma_iomap(struct drm_i915_private *dev_priv,
+		     struct i915_vma *vma);
 #endif
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 7decbca25dbb..8e7c341951fd 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -248,12 +248,10 @@  static int intelfb_create(struct drm_fb_helper *helper,
 	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
 	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(dev_priv->gtt.mappable_base + vma->node.start,
-			   vma->node.size);
-	if (!info->screen_base) {
+	info->screen_base = i915_vma_iomap(dev_priv, vma);
+	if (IS_ERR(info->screen_base)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(info->screen_base);
 		goto out_destroy_fbi;
 	}
 	info->screen_size = vma->node.size;