[PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

[PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

[Chris Wilson]

When setting up the overlay page, we pin it into the GGTT (when using
virtual addresses) and store the offset as overlay->flip_addr. Rather
than doing a lookup of the GGTT address everytime, we can use the known
address instead.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_overlay.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index bcc3b6a016d8..9746b9841c13 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -198,7 +198,7 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
 		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_wc(ggtt->mappable,
-					 i915_gem_obj_ggtt_offset(overlay->reg_bo));
+					 overlay->flip_addr);
 
 	return regs;
 }
@@ -1493,7 +1493,7 @@ intel_overlay_map_regs_atomic(struct intel_overlay *overlay)
 			overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_atomic_wc(ggtt->mappable,
-						i915_gem_obj_ggtt_offset(overlay->reg_bo));
+						overlay->flip_addr);
 
 	return regs;
 }
@@ -1523,10 +1523,7 @@ intel_overlay_capture_error_state(struct drm_device *dev)
 
 	error->dovsta = I915_READ(DOVSTA);
 	error->isr = I915_READ(ISR);
-	if (OVERLAY_NEEDS_PHYSICAL(overlay->dev))
-		error->base = (__force long)overlay->reg_bo->phys_handle->vaddr;
-	else
-		error->base = i915_gem_obj_ggtt_offset(overlay->reg_bo);
+	error->base = overlay->flip_addr;
 
 	regs = intel_overlay_map_regs_atomic(overlay);
 	if (!regs)
-- 
2.8.0.rc3

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

[PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()

[Chris Wilson]

The ioremap() hidden behind the io_mapping_map_wc() convenience helper
can be used for remapping multiple pages. Extend the helper so that
future callers can use it for larger ranges.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Yishai Hadas <yishaih@mellanox.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_overlay.c    |  3 ++-
 drivers/net/ethernet/mellanox/mlx4/pd.c |  4 +++-
 include/linux/io-mapping.h              | 10 +++++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 9746b9841c13..0d5a376878d3 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -198,7 +198,8 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
 		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_wc(ggtt->mappable,
-					 overlay->flip_addr);
+					 overlay->flip_addr,
+					 PAGE_SIZE);
 
 	return regs;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/pd.c b/drivers/net/ethernet/mellanox/mlx4/pd.c
index b3cc3ab63799..6fc156a3918d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/pd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/pd.c
@@ -205,7 +205,9 @@ int mlx4_bf_alloc(struct mlx4_dev *dev, struct mlx4_bf *bf, int node)
 			goto free_uar;
 		}
 
-		uar->bf_map = io_mapping_map_wc(priv->bf_mapping, uar->index << PAGE_SHIFT);
+		uar->bf_map = io_mapping_map_wc(priv->bf_mapping,
+						uar->index << PAGE_SHIFT,
+						PAGE_SIZE);
 		if (!uar->bf_map) {
 			err = -ENOMEM;
 			goto unamp_uar;
diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
index e399029b68c5..645ad06b5d52 100644
--- a/include/linux/io-mapping.h
+++ b/include/linux/io-mapping.h
@@ -100,14 +100,16 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 }
 
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	resource_size_t phys_addr;
 
 	BUG_ON(offset >= mapping->size);
 	phys_addr = mapping->base + offset;
 
-	return ioremap_wc(phys_addr, PAGE_SIZE);
+	return ioremap_wc(phys_addr, size);
 }
 
 static inline void
@@ -155,7 +157,9 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 
 /* Non-atomic map/unmap */
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	return ((char __force __iomem *) mapping) + offset;
 }
-- 
2.8.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()

[Chris Wilson]

On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> The ioremap() hidden behind the io_mapping_map_wc() convenience helper
> can be used for remapping multiple pages. Extend the helper so that
> future callers can use it for larger ranges.

Adding Luis Rodriguez as he has been active in the area of ioremap_*().

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Yishai Hadas <yishaih@mellanox.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: netdev@vger.kernel.org
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_overlay.c    |  3 ++-
>  drivers/net/ethernet/mellanox/mlx4/pd.c |  4 +++-
>  include/linux/io-mapping.h              | 10 +++++++---
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 9746b9841c13..0d5a376878d3 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -198,7 +198,8 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
>  		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
>  	else
>  		regs = io_mapping_map_wc(ggtt->mappable,
> -					 overlay->flip_addr);
> +					 overlay->flip_addr,
> +					 PAGE_SIZE);
>  
>  	return regs;
>  }
> diff --git a/drivers/net/ethernet/mellanox/mlx4/pd.c b/drivers/net/ethernet/mellanox/mlx4/pd.c
> index b3cc3ab63799..6fc156a3918d 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/pd.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/pd.c
> @@ -205,7 +205,9 @@ int mlx4_bf_alloc(struct mlx4_dev *dev, struct mlx4_bf *bf, int node)
>  			goto free_uar;
>  		}
>  
> -		uar->bf_map = io_mapping_map_wc(priv->bf_mapping, uar->index << PAGE_SHIFT);
> +		uar->bf_map = io_mapping_map_wc(priv->bf_mapping,
> +						uar->index << PAGE_SHIFT,
> +						PAGE_SIZE);
>  		if (!uar->bf_map) {
>  			err = -ENOMEM;
>  			goto unamp_uar;
> diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
> index e399029b68c5..645ad06b5d52 100644
> --- a/include/linux/io-mapping.h
> +++ b/include/linux/io-mapping.h
> @@ -100,14 +100,16 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
>  }
>  
>  static inline void __iomem *
> -io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
> +io_mapping_map_wc(struct io_mapping *mapping,
> +		  unsigned long offset,
> +		  unsigned long size)
>  {
>  	resource_size_t phys_addr;
>  
>  	BUG_ON(offset >= mapping->size);
>  	phys_addr = mapping->base + offset;
>  
> -	return ioremap_wc(phys_addr, PAGE_SIZE);
> +	return ioremap_wc(phys_addr, size);
>  }
>  
>  static inline void
> @@ -155,7 +157,9 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
>  
>  /* Non-atomic map/unmap */
>  static inline void __iomem *
> -io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
> +io_mapping_map_wc(struct io_mapping *mapping,
> +		  unsigned long offset,
> +		  unsigned long size)
>  {
>  	return ((char __force __iomem *) mapping) + offset;
>  }

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()

[Luis R. Rodriguez]

On Tue, Apr 19, 2016 at 01:02:25PM +0100, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> > The ioremap() hidden behind the io_mapping_map_wc() convenience helper
> > can be used for remapping multiple pages. Extend the helper so that
> > future callers can use it for larger ranges.
> 
> Adding Luis Rodriguez as he has been active in the area of ioremap_*().

Can someone bounce me a copy of the original e-mails? Can you also use
mcgrof@kernel.org ?

  Luis

Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()

[Chris Wilson]

On Tue, Apr 19, 2016 at 02:30:28PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 19, 2016 at 01:02:25PM +0100, Chris Wilson wrote:
> > On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> > > The ioremap() hidden behind the io_mapping_map_wc() convenience helper
> > > can be used for remapping multiple pages. Extend the helper so that
> > > future callers can use it for larger ranges.
> > 
> > Adding Luis Rodriguez as he has been active in the area of ioremap_*().
> 
> Can someone bounce me a copy of the original e-mails? Can you also use
> mcgrof@kernel.org ?

Hopefully done. Please ping me if they don't arrive, or have a look at
https://cgit.freedesktop.org/~ickle/linux-2.6/log/?h=iomap
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Luis R. Rodriguez]

On Tue, Apr 19, 2016 at 01:33:58PM +0100, Chris Wilson wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..9ef47329e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
>  					    old_write_domain);
>  }
>  
> +static void __i915_vma_iounmap(struct i915_vma *vma)
> +{
> +	if (vma->iomap == NULL)
> +		return;
> +
> +	io_mapping_unmap(vma->iomap);

The NULL check could just be done by io_mapping_unmap() then you
can avoid this in other drivers too.

> +	vma->iomap = NULL;

You added accounting here, by simple int and inc / dec'ing it.
I cannot confirm if it is correctly avoiding races, can you
confirm?

Also you added accounting for the custom vma pinning thing and do
GEM_BUG_ON(vma->pin_count == 0); when you unpin one instance but *you do not*
do something like GEM_BUG_ON(vma->pin_count != 0); when you do the final full
iounmap. That seems rather sloppy.

iomapping stuff has its own custom data structure, why not just use that data
structure instead of the struct i915_vma and generalize this ? Drivers can
be buggy and best if we avoid custom driver accounting and just do it in a neat
generic fashion.

Then other drivers could use this too.

> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>  	info->fbops = &intelfb_ops;
>  
> +	vma = i915_gem_obj_to_ggtt(obj);
> +
>  	/* setup aperture base/size for vesafb takeover */
>  	info->apertures->ranges[0].base = dev->mode_config.fb_base;
>  	info->apertures->ranges[0].size = ggtt->mappable_end;
>  
> -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> -	info->fix.smem_len = size;
> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> +	info->fix.smem_len = vma->node.size;
>  
> -	info->screen_base =
> -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -			   size);
> -	if (!info->screen_base) {
> +	vaddr = i915_vma_pin_iomap(vma);
> +	if (IS_ERR(vaddr)) {
>  		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(vaddr);
>  		goto out_destroy_fbi;
>  	}
> -	info->screen_size = size;
> +	info->screen_base = vaddr;
> +	info->screen_size = vma->node.size;

some framebuffer drivers tend to use a generic start address of
iinfo->fix.smem_start and a length of info->fix.smem_len, this
driver sets the smem_start above, but its different than what
gets ioremap for a start address:

+               ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+                                       vma->node.start,
+                                       vma->node.size);

fix.smem_start is :


> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;

The smem_len matches though. Can you clarify if its correct for
the io_mapping_map_wc() should not be using info->fix.smem_start
(which is dev->mode_config.fb_base + vma->node.start)?

Reason I ask is since I noticed a while ago a lot of drivers
were using info->fix.smem_start and info->fix.smem_len consistently
for their ioremap'd areas it might make sense instead to let the
internal framebuffer (register_framebuffer()) optionally manage the
ioremap_wc() for drivers, given that this is pretty generic stuff.

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

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

[Chris Wilson]

On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 19, 2016 at 01:33:58PM +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6ce2c31b9a81..9ef47329e8ae 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
> >  					    old_write_domain);
> >  }
> >  
> > +static void __i915_vma_iounmap(struct i915_vma *vma)
> > +{
> > +	if (vma->iomap == NULL)
> > +		return;
> > +
> > +	io_mapping_unmap(vma->iomap);
> 
> The NULL check could just be done by io_mapping_unmap() then you
> can avoid this in other drivers too.
> 
> > +	vma->iomap = NULL;
> 
> You added accounting here, by simple int and inc / dec'ing it.
> I cannot confirm if it is correctly avoiding races, can you
> confirm?

Yes, the vma->pin_count is guarded by the struct_mutex atm. (The
struct_mutex is our own BKL :(

> Also you added accounting for the custom vma pinning thing and do
> GEM_BUG_ON(vma->pin_count == 0); when you unpin one instance but *you do not*
> do something like GEM_BUG_ON(vma->pin_count != 0); when you do the final full
> iounmap. That seems rather sloppy.

It's placed next to the function where pin_count == 0 and only called
from it. Yes, I did think the same...
 
> iomapping stuff has its own custom data structure, why not just use that data
> structure instead of the struct i915_vma and generalize this ? Drivers can
> be buggy and best if we avoid custom driver accounting and just do it in a neat
> generic fashion.

Completely different tasks, as far as I am aware. The iomapping is about
providing CPU access to the IO region, dma-remapping about providing
device access to physical memory, and our own VMA is about how the
object sits in all the different views of both CPU and device address
spaces (of which there are many, and even the CPU accessible address
space is not the entirety of that particular address space). 
 
> Then other drivers could use this too.

drivers/gpu/drm/ttm (you didn't hear me say that...)

> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 79ac202f3870..93f54a10042f 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
> >  	info->fbops = &intelfb_ops;
> >  
> > +	vma = i915_gem_obj_to_ggtt(obj);
> > +
> >  	/* setup aperture base/size for vesafb takeover */
> >  	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> >  	info->apertures->ranges[0].size = ggtt->mappable_end;
> >  
> > -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> > -	info->fix.smem_len = size;
> > +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> > +	info->fix.smem_len = vma->node.size;
> >  
> > -	info->screen_base =
> > -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> > -			   size);
> > -	if (!info->screen_base) {
> > +	vaddr = i915_vma_pin_iomap(vma);
> > +	if (IS_ERR(vaddr)) {
> >  		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> > -		ret = -ENOSPC;
> > +		ret = PTR_ERR(vaddr);
> >  		goto out_destroy_fbi;
> >  	}
> > -	info->screen_size = size;
> > +	info->screen_base = vaddr;
> > +	info->screen_size = vma->node.size;
> 
> some framebuffer drivers tend to use a generic start address of
> iinfo->fix.smem_start and a length of info->fix.smem_len, this
> driver sets the smem_start above, but its different than what
> gets ioremap for a start address:
> 
> +               ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> +                                       vma->node.start,
> +                                       vma->node.size);
> 
> fix.smem_start is :
> 
> 
> > +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> 
> The smem_len matches though. Can you clarify if its correct for
> the io_mapping_map_wc() should not be using info->fix.smem_start
> (which is dev->mode_config.fb_base + vma->node.start)?

dev->mode_config.fb_base is the base address of the mappable region. It
is an inconsistently in naming that just hasn't annoyed me enough to
fix.
 
> Reason I ask is since I noticed a while ago a lot of drivers
> were using info->fix.smem_start and info->fix.smem_len consistently
> for their ioremap'd areas it might make sense instead to let the
> internal framebuffer (register_framebuffer()) optionally manage the
> ioremap_wc() for drivers, given that this is pretty generic stuff.

Apart from drivers like ours we would end up with multiple mappings to
the same region. It was just a little grevience that I think was worth
fixing. It does highlight how buggy our code is currently though as we
never relinquish that mapping when the driver is unloaded.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA

[Daniel Vetter]

On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> Reason I ask is since I noticed a while ago a lot of drivers
> were using info->fix.smem_start and info->fix.smem_len consistently
> for their ioremap'd areas it might make sense instead to let the
> internal framebuffer (register_framebuffer()) optionally manage the
> ioremap_wc() for drivers, given that this is pretty generic stuff.

All that legacy fbdev stuff is just for legacy support, and I prefer to
have that as dumb as possible. There's been some discussion even around
lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
to have a simple drm driver for e.g. uefi.

But I definitely don't want a legacy horror show like fbdev to
automagically take care of device mappings for drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA

[Luis R. Rodriguez]

On Wed, Apr 20, 2016 at 01:17:30PM +0200, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> > Reason I ask is since I noticed a while ago a lot of drivers
> > were using info->fix.smem_start and info->fix.smem_len consistently
> > for their ioremap'd areas it might make sense instead to let the
> > internal framebuffer (register_framebuffer()) optionally manage the
> > ioremap_wc() for drivers, given that this is pretty generic stuff.
> 
> All that legacy fbdev stuff is just for legacy support, and I prefer to
> have that as dumb as possible. There's been some discussion even around
> lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
> to have a simple drm driver for e.g. uefi.
> 
> But I definitely don't want a legacy horror show like fbdev to
> automagically take care of device mappings for drivers.

Makes sense, it also still begs the question if more modern APIs
could manage the ioremap for you. Evidence shows people get
sloppy and if things were done internally with helpers it may
be easier to later make adjustments.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Vetter]

On Wed, Apr 20, 2016 at 11:27:27PM +0200, Luis R. Rodriguez wrote:
> On Wed, Apr 20, 2016 at 01:17:30PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> > > Reason I ask is since I noticed a while ago a lot of drivers
> > > were using info->fix.smem_start and info->fix.smem_len consistently
> > > for their ioremap'd areas it might make sense instead to let the
> > > internal framebuffer (register_framebuffer()) optionally manage the
> > > ioremap_wc() for drivers, given that this is pretty generic stuff.
> > 
> > All that legacy fbdev stuff is just for legacy support, and I prefer to
> > have that as dumb as possible. There's been some discussion even around
> > lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
> > to have a simple drm driver for e.g. uefi.
> > 
> > But I definitely don't want a legacy horror show like fbdev to
> > automagically take care of device mappings for drivers.
> 
> Makes sense, it also still begs the question if more modern APIs
> could manage the ioremap for you. Evidence shows people get
> sloppy and if things were done internally with helpers it may
> be easier to later make adjustments.

Real gpus generally have so much mmio space that you want to ioremap them
on demand. At least if you still care about 32bit support. And on-die gpus
on socs or similar tend to not have an mmio range to access the gfx
remapping range at all, but instead expect that to be done with gpu
pagetables.

So at least with gpus I don't see a real demand for this, and the existing
users are mostly old fbdev drivers that really no one should be touching
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()

[Chris Wilson]

In a couple of places, we have an i915_address_space that we know is
really an i915_ggtt that we want to use. Create an inline helper to
convert from the i915_address_space subclass into its container.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9f165feb54ae..b3af2e808b49 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -93,6 +93,13 @@
  *
  */
 
+static inline struct i915_ggtt *
+i915_vm_to_ggtt(struct i915_address_space *vm)
+{
+	GEM_BUG_ON(!i915_is_ggtt(vm));
+	return container_of(vm, struct i915_ggtt, base);
+}
+
 static int
 i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
@@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 				     enum i915_cache_level level, u32 unused)
 {
 	struct drm_i915_private *dev_priv = to_i915(vm->dev);
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	unsigned first_entry = start >> PAGE_SHIFT;
 	gen8_pte_t __iomem *gtt_entries =
 		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
@@ -2437,7 +2444,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 				     enum i915_cache_level level, u32 flags)
 {
 	struct drm_i915_private *dev_priv = to_i915(vm->dev);
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	unsigned first_entry = start >> PAGE_SHIFT;
 	gen6_pte_t __iomem *gtt_entries =
 		(gen6_pte_t __iomem *)ggtt->gsm + first_entry;
@@ -2481,7 +2488,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 				  bool use_scratch)
 {
 	struct drm_i915_private *dev_priv = to_i915(vm->dev);
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	unsigned first_entry = start >> PAGE_SHIFT;
 	unsigned num_entries = length >> PAGE_SHIFT;
 	gen8_pte_t scratch_pte, __iomem *gtt_base =
@@ -2513,7 +2520,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 				  bool use_scratch)
 {
 	struct drm_i915_private *dev_priv = to_i915(vm->dev);
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	unsigned first_entry = start >> PAGE_SHIFT;
 	unsigned num_entries = length >> PAGE_SHIFT;
 	gen6_pte_t scratch_pte, __iomem *gtt_base =
-- 
2.8.0.rc3

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

Re: [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()

[Tvrtko Ursulin]

On 18/04/16 12:14, Chris Wilson wrote:
> In a couple of places, we have an i915_address_space that we know is
> really an i915_ggtt that we want to use. Create an inline helper to
> convert from the i915_address_space subclass into its container.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9f165feb54ae..b3af2e808b49 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -93,6 +93,13 @@
>    *
>    */
>
> +static inline struct i915_ggtt *
> +i915_vm_to_ggtt(struct i915_address_space *vm)
> +{
> +	GEM_BUG_ON(!i915_is_ggtt(vm));
> +	return container_of(vm, struct i915_ggtt, base);
> +}
> +
>   static int
>   i915_get_ggtt_vma_pages(struct i915_vma *vma);
>
> @@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>   				     enum i915_cache_level level, u32 unused)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>   	unsigned first_entry = start >> PAGE_SHIFT;
>   	gen8_pte_t __iomem *gtt_entries =
>   		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2437,7 +2444,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>   				     enum i915_cache_level level, u32 flags)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>   	unsigned first_entry = start >> PAGE_SHIFT;
>   	gen6_pte_t __iomem *gtt_entries =
>   		(gen6_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2481,7 +2488,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
>   				  bool use_scratch)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>   	unsigned first_entry = start >> PAGE_SHIFT;
>   	unsigned num_entries = length >> PAGE_SHIFT;
>   	gen8_pte_t scratch_pte, __iomem *gtt_base =
> @@ -2513,7 +2520,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
>   				  bool use_scratch)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>   	unsigned first_entry = start >> PAGE_SHIFT;
>   	unsigned num_entries = length >> PAGE_SHIFT;
>   	gen6_pte_t scratch_pte, __iomem *gtt_base =
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()

[Joonas Lahtinen]

On ma, 2016-04-18 at 12:14 +0100, Chris Wilson wrote:
> In a couple of places, we have an i915_address_space that we know is
> really an i915_ggtt that we want to use. Create an inline helper to
> convert from the i915_address_space subclass into its container.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Comment below.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9f165feb54ae..b3af2e808b49 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -93,6 +93,13 @@
>   *
>   */
>  
> +static inline struct i915_ggtt *
> +i915_vm_to_ggtt(struct i915_address_space *vm)
> +{
> +	GEM_BUG_ON(!i915_is_ggtt(vm));
> +	return container_of(vm, struct i915_ggtt, base);
> +}
> +
>  static int
>  i915_get_ggtt_vma_pages(struct i915_vma *vma);
>  
> @@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>  				     enum i915_cache_level level, u32 unused)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);

I do not see this as a very necessary change, because we are going to
have only one Global XYZ per device by definiton of "global".

Especially after the rename it's going to be;

struct i915_ggtt *ggtt = &i915->ggtt;

But regardless my R-b.

Regards, Joonas

>  	unsigned first_entry = start >> PAGE_SHIFT;
>  	gen8_pte_t __iomem *gtt_entries =
>  		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2437,7 +2444,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>  				     enum i915_cache_level level, u32 flags)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	unsigned first_entry = start >> PAGE_SHIFT;
>  	gen6_pte_t __iomem *gtt_entries =
>  		(gen6_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2481,7 +2488,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
>  				  bool use_scratch)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	unsigned first_entry = start >> PAGE_SHIFT;
>  	unsigned num_entries = length >> PAGE_SHIFT;
>  	gen8_pte_t scratch_pte, __iomem *gtt_base =
> @@ -2513,7 +2520,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
>  				  bool use_scratch)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>  	unsigned first_entry = start >> PAGE_SHIFT;
>  	unsigned num_entries = length >> PAGE_SHIFT;
>  	gen6_pte_t scratch_pte, __iomem *gtt_base =
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()

[Chris Wilson]

On Mon, Apr 18, 2016 at 04:37:12PM +0300, Joonas Lahtinen wrote:
> On ma, 2016-04-18 at 12:14 +0100, Chris Wilson wrote:
> > In a couple of places, we have an i915_address_space that we know is
> > really an i915_ggtt that we want to use. Create an inline helper to
> > convert from the i915_address_space subclass into its container.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Comment below.
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 9f165feb54ae..b3af2e808b49 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -93,6 +93,13 @@
> >   *
> >   */
> >  
> > +static inline struct i915_ggtt *
> > +i915_vm_to_ggtt(struct i915_address_space *vm)
> > +{
> > +	GEM_BUG_ON(!i915_is_ggtt(vm));
> > +	return container_of(vm, struct i915_ggtt, base);
> > +}
> > +
> >  static int
> >  i915_get_ggtt_vma_pages(struct i915_vma *vma);
> >  
> > @@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> >  				     enum i915_cache_level level, u32 unused)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> > -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > +	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> 
> I do not see this as a very necessary change, because we are going to
> have only one Global XYZ per device by definiton of "global".

I ultimately want to remove the dev_priv usage here. But it is also
useful for consistency with a couple of other ggtt PTE routines that do
not need the drm_i915_private pointer at all, that aren't yet upstream.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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)
+{
+	void *ptr;
+
+	if (vma->iomap)
+		return vma->iomap;
+
+	if (WARN_ON(!vma->obj->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	GEM_BUG_ON(!vma->is_ggtt);
+	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+				vma->node.start,
+				vma->node.size);
+	if (ptr == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	vma->iomap = ptr;
+	return ptr;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..d95190ddf2d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+#include <linux/io-mapping.h>
+
 struct drm_i915_file_private;
 
 typedef uint32_t gen6_pte_t;
@@ -175,6 +177,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void *iomap;
 
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
@@ -559,4 +562,39 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+/**
+ * i915_vma_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
+ * @vma: VMA to iomap
+ *
+ * The passed in VMA has to be pinned in the global GTT mappable region. Or in
+ * other words callers are responsible for managing the VMA pinned lifetime and
+ * ensuring it covers the use of the returned mapping.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void *i915_vma_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_iounmap - unmaps the mapping returned from i915_vma_iomap
+ * @dev_priv: i915 private pointer
+ * @vma: VMA to unmap
+ *
+ * Unmaps the previously iomapped VMA using iounmap.
+ *
+ * Users of i915_vma_iomap should not manually unmap by calling this function
+ * if they want to take advantage of the mapping getting cached in the VMA.
+ *
+ * Callers must hold the struct_mutex.
+ */
+static inline void i915_vma_iounmap(struct i915_vma *vma)
+{
+	if (vma->iomap == NULL)
+		return;
+
+	io_mapping_unmap(vma->iomap);
+	vma->iomap = NULL;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..6156ee96f429 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct drm_i915_private *dev_priv =
 		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
 	struct shrinker_lock_uninterruptible slu;
-	unsigned long freed_pages;
+	struct i915_vma *vma, *next;
+	unsigned long freed_pages = 0;
+	int ret;
 
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
-	freed_pages = i915_gem_shrink(dev_priv, -1UL,
-				      I915_SHRINK_BOUND |
-				      I915_SHRINK_UNBOUND |
-				      I915_SHRINK_ACTIVE |
-				      I915_SHRINK_VMAPS);
+	/* Force everything onto the inactive lists */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		goto out;
+
+	freed_pages += i915_gem_shrink(dev_priv, -1UL,
+				       I915_SHRINK_BOUND |
+				       I915_SHRINK_UNBOUND |
+				       I915_SHRINK_ACTIVE |
+				       I915_SHRINK_VMAPS);
+
+	/* We also want to clear any cached iomaps as they wrap vmap */
+	list_for_each_entry_safe(vma, next,
+				 &dev_priv->ggtt.base.inactive_list, vm_link) {
+		unsigned long count = vma->node.size >> PAGE_SHIFT;
+		if (vma->iomap && i915_vma_unbind(vma) == 0)
+			freed_pages += count;
+	}
 
+out:
 	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
 
 	*(unsigned long *)ptr += freed_pages;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202f3870..3f3c97a30418 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
+	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
-	int size, ret;
 	bool prealloc = false;
+	void *vaddr;
+	int ret;
 
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
@@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 
 	obj = intel_fb->obj;
-	size = obj->base.size;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &intelfb_ops;
 
+	vma = i915_gem_obj_to_ggtt(obj);
+
 	/* setup aperture base/size for vesafb takeover */
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = ggtt->mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
-	info->fix.smem_len = size;
+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
+	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
-	if (!info->screen_base) {
+	vaddr = i915_vma_iomap(vma);
+	if (IS_ERR(vaddr)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(vaddr);
 		goto out_destroy_fbi;
 	}
-	info->screen_size = size;
+	info->screen_base = vaddr;
+	info->screen_size = vma->node.size;
 
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
-- 
2.8.0.rc3

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

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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

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

[Chris Wilson]

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
v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
sections and ensure the VMA cannot be reaped whilst mapped.

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      | 26 ++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h      | 58 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++----
 drivers/gpu/drm/i915/intel_fbdev.c       | 22 ++++++------
 5 files changed, 120 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..e185dff358e1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		return obj->base.size;
 	}
 }
+
+void *i915_vma_pin_iomap(struct i915_vma *vma)
+{
+	void *ptr;
+
+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
+	if (WARN_ON(!vma->obj->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	GEM_BUG_ON(!vma->is_ggtt);
+	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	ptr = vma->iomap;
+	if (ptr == NULL) {
+		ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+					vma->node.start,
+					vma->node.size);
+		if (ptr == NULL)
+			return ERR_PTR(-ENOMEM);
+
+		vma->iomap = ptr;
+	}
+
+	vma->pin_count++;
+	return ptr;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..a01d78bc9f24 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+#include <linux/io-mapping.h>
+
 struct drm_i915_file_private;
 
 typedef uint32_t gen6_pte_t;
@@ -175,6 +177,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void *iomap;
 
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
@@ -559,4 +562,59 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+/**
+ * i915_vma_pin_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
+ * @vma: VMA to iomap
+ *
+ * The passed in VMA has to be pinned in the global GTT mappable region.
+ * An extra pinning of the VMA is acquired for the return iomapping,
+ * the caller must call i915_vma_unpin_iomap to relinquish the pinning
+ * after the iomapping is no longer required.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void *i915_vma_pin_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_iounmap - unmaps the VMA
+ * @vma: VMA to unmap
+ *
+ * Unmaps the previously iomapped VMA using iounmap.
+ *
+ * Users of i915_vma_pin_iomap() should not manually unmap by calling this
+ * function but should call i915_vma_unpin_iomap() instead.
+ *
+ * Callers must hold the struct_mutex.
+ */
+static inline void i915_vma_iounmap(struct i915_vma *vma)
+{
+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
+	GEM_BUG_ON(vma->pin_count);
+
+	if (vma->iomap == NULL)
+		return;
+
+	io_mapping_unmap(vma->iomap);
+	vma->iomap = NULL;
+}
+
+/**
+ * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
+ * @vma: VMA to unpin
+ *
+ * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
+ *
+ * Callers must hold the struct_mutex. This function is only valid to be
+ * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
+ */
+static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
+{
+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
+	GEM_BUG_ON(vma->pin_count == 0);
+	GEM_BUG_ON(vma->iomap == NULL);
+	vma->pin_count--;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..6156ee96f429 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct drm_i915_private *dev_priv =
 		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
 	struct shrinker_lock_uninterruptible slu;
-	unsigned long freed_pages;
+	struct i915_vma *vma, *next;
+	unsigned long freed_pages = 0;
+	int ret;
 
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
-	freed_pages = i915_gem_shrink(dev_priv, -1UL,
-				      I915_SHRINK_BOUND |
-				      I915_SHRINK_UNBOUND |
-				      I915_SHRINK_ACTIVE |
-				      I915_SHRINK_VMAPS);
+	/* Force everything onto the inactive lists */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		goto out;
+
+	freed_pages += i915_gem_shrink(dev_priv, -1UL,
+				       I915_SHRINK_BOUND |
+				       I915_SHRINK_UNBOUND |
+				       I915_SHRINK_ACTIVE |
+				       I915_SHRINK_VMAPS);
+
+	/* We also want to clear any cached iomaps as they wrap vmap */
+	list_for_each_entry_safe(vma, next,
+				 &dev_priv->ggtt.base.inactive_list, vm_link) {
+		unsigned long count = vma->node.size >> PAGE_SHIFT;
+		if (vma->iomap && i915_vma_unbind(vma) == 0)
+			freed_pages += count;
+	}
 
+out:
 	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
 
 	*(unsigned long *)ptr += freed_pages;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202f3870..93f54a10042f 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
+	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
-	int size, ret;
 	bool prealloc = false;
+	void *vaddr;
+	int ret;
 
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
@@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 
 	obj = intel_fb->obj;
-	size = obj->base.size;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &intelfb_ops;
 
+	vma = i915_gem_obj_to_ggtt(obj);
+
 	/* setup aperture base/size for vesafb takeover */
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = ggtt->mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
-	info->fix.smem_len = size;
+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
+	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
-	if (!info->screen_base) {
+	vaddr = i915_vma_pin_iomap(vma);
+	if (IS_ERR(vaddr)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(vaddr);
 		goto out_destroy_fbi;
 	}
-	info->screen_size = size;
+	info->screen_base = vaddr;
+	info->screen_size = vma->node.size;
 
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
-- 
2.8.0.rc3

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

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

[Tvrtko Ursulin]

On 18/04/16 15:54, 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
> v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
> sections and ensure the VMA cannot be reaped whilst mapped.
>
> 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      | 26 ++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h      | 58 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++----
>   drivers/gpu/drm/i915/intel_fbdev.c       | 22 ++++++------
>   5 files changed, 120 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..e185dff358e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		return obj->base.size;
>   	}
>   }
> +
> +void *i915_vma_pin_iomap(struct i915_vma *vma)
> +{
> +	void *ptr;

Just realized we should mark the pointers with __iomem here.

> +
> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
> +	if (WARN_ON(!vma->obj->map_and_fenceable))
> +		return ERR_PTR(-ENODEV);
> +
> +	GEM_BUG_ON(!vma->is_ggtt);
> +	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> +
> +	ptr = vma->iomap;
> +	if (ptr == NULL) {
> +		ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> +					vma->node.start,
> +					vma->node.size);
> +		if (ptr == NULL)
> +			return ERR_PTR(-ENOMEM);
> +
> +		vma->iomap = ptr;
> +	}
> +
> +	vma->pin_count++;
> +	return ptr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..a01d78bc9f24 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -34,6 +34,8 @@
>   #ifndef __I915_GEM_GTT_H__
>   #define __I915_GEM_GTT_H__
>
> +#include <linux/io-mapping.h>
> +
>   struct drm_i915_file_private;
>
>   typedef uint32_t gen6_pte_t;
> @@ -175,6 +177,7 @@ struct i915_vma {
>   	struct drm_mm_node node;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_address_space *vm;
> +	void *iomap;
>
>   	/** Flags and address space this VMA is bound to */
>   #define GLOBAL_BIND	(1<<0)
> @@ -559,4 +562,59 @@ size_t
>   i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		    const struct i915_ggtt_view *view);
>
> +/**
> + * i915_vma_pin_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
> + * @vma: VMA to iomap
> + *
> + * The passed in VMA has to be pinned in the global GTT mappable region.
> + * An extra pinning of the VMA is acquired for the return iomapping,
> + * the caller must call i915_vma_unpin_iomap to relinquish the pinning
> + * after the iomapping is no longer required.
> + *
> + * Callers must hold the struct_mutex.
> + *
> + * Returns a valid iomapped pointer or ERR_PTR.
> + */
> +void *i915_vma_pin_iomap(struct i915_vma *vma);
> +
> +/**
> + * i915_vma_iounmap - unmaps the VMA
> + * @vma: VMA to unmap
> + *
> + * Unmaps the previously iomapped VMA using iounmap.
> + *
> + * Users of i915_vma_pin_iomap() should not manually unmap by calling this
> + * function but should call i915_vma_unpin_iomap() instead.
> + *
> + * Callers must hold the struct_mutex.
> + */
> +static inline void i915_vma_iounmap(struct i915_vma *vma)
> +{
> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
> +	GEM_BUG_ON(vma->pin_count);
> +
> +	if (vma->iomap == NULL)
> +		return;
> +
> +	io_mapping_unmap(vma->iomap);
> +	vma->iomap = NULL;
> +}

It would be best to hide this near the unbind code. Alternatively maybe 
put a louder warning in the kerneldoc? Just so it is a bit more 
distinguishable from the GEM exported API for this feature.

> +
> +/**
> + * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
> + * @vma: VMA to unpin
> + *
> + * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
> + *
> + * Callers must hold the struct_mutex. This function is only valid to be
> + * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
> + */
> +static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
> +{
> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
> +	GEM_BUG_ON(vma->pin_count == 0);
> +	GEM_BUG_ON(vma->iomap == NULL);
> +	vma->pin_count--;
> +}
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d46388f25e04..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>   	struct drm_i915_private *dev_priv =
>   		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
>   	struct shrinker_lock_uninterruptible slu;
> -	unsigned long freed_pages;
> +	struct i915_vma *vma, *next;
> +	unsigned long freed_pages = 0;
> +	int ret;
>
>   	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
>   		return NOTIFY_DONE;
>
> -	freed_pages = i915_gem_shrink(dev_priv, -1UL,
> -				      I915_SHRINK_BOUND |
> -				      I915_SHRINK_UNBOUND |
> -				      I915_SHRINK_ACTIVE |
> -				      I915_SHRINK_VMAPS);
> +	/* Force everything onto the inactive lists */
> +	ret = i915_gpu_idle(dev_priv->dev);
> +	if (ret)
> +		goto out;
> +
> +	freed_pages += i915_gem_shrink(dev_priv, -1UL,
> +				       I915_SHRINK_BOUND |
> +				       I915_SHRINK_UNBOUND |
> +				       I915_SHRINK_ACTIVE |
> +				       I915_SHRINK_VMAPS);
> +
> +	/* We also want to clear any cached iomaps as they wrap vmap */
> +	list_for_each_entry_safe(vma, next,
> +				 &dev_priv->ggtt.base.inactive_list, vm_link) {
> +		unsigned long count = vma->node.size >> PAGE_SHIFT;
> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> +			freed_pages += count;
> +	}
>
> +out:
>   	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
>
>   	*(unsigned long *)ptr += freed_pages;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	struct fb_info *info;
>   	struct drm_framebuffer *fb;
> +	struct i915_vma *vma;
>   	struct drm_i915_gem_object *obj;
> -	int size, ret;
>   	bool prealloc = false;
> +	void *vaddr;
> +	int ret;
>
>   	if (intel_fb &&
>   	    (sizes->fb_width > intel_fb->base.width ||
> @@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	}
>
>   	obj = intel_fb->obj;
> -	size = obj->base.size;
>
>   	mutex_lock(&dev->struct_mutex);
>
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>   	info->fbops = &intelfb_ops;
>
> +	vma = i915_gem_obj_to_ggtt(obj);
> +
>   	/* setup aperture base/size for vesafb takeover */
>   	info->apertures->ranges[0].base = dev->mode_config.fb_base;
>   	info->apertures->ranges[0].size = ggtt->mappable_end;
>
> -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> -	info->fix.smem_len = size;
> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> +	info->fix.smem_len = vma->node.size;
>
> -	info->screen_base =
> -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -			   size);
> -	if (!info->screen_base) {
> +	vaddr = i915_vma_pin_iomap(vma);
> +	if (IS_ERR(vaddr)) {
>   		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(vaddr);
>   		goto out_destroy_fbi;
>   	}
> -	info->screen_size = size;
> +	info->screen_base = vaddr;
> +	info->screen_size = vma->node.size;
>
>   	/* This driver doesn't need a VT switch to restore the mode on resume */
>   	info->skip_vt_switch = true;
>

Regards,

Tvrtko

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

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

[Chris Wilson]

On Mon, Apr 18, 2016 at 04:08:55PM +0100, Tvrtko Ursulin wrote:
> On 18/04/16 15:54, Chris Wilson wrote:
> >@@ -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..e185dff358e1 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> >  		return obj->base.size;
> >  	}
> >  }
> >+
> >+void *i915_vma_pin_iomap(struct i915_vma *vma)
> >+{
> >+	void *ptr;
> 
> Just realized we should mark the pointers with __iomem here.

The problem is that we need to lose the annotation at some point as we
mix access via ring->virtual_start. For correctness, we should report
__iomem here and discard it later.

> >+/**
> >+ * i915_vma_iounmap - unmaps the VMA
> >+ * @vma: VMA to unmap
> >+ *
> >+ * Unmaps the previously iomapped VMA using iounmap.
> >+ *
> >+ * Users of i915_vma_pin_iomap() should not manually unmap by calling this
> >+ * function but should call i915_vma_unpin_iomap() instead.
> >+ *
> >+ * Callers must hold the struct_mutex.
> >+ */
> >+static inline void i915_vma_iounmap(struct i915_vma *vma)
> >+{
> >+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
> >+	GEM_BUG_ON(vma->pin_count);
> >+
> >+	if (vma->iomap == NULL)
> >+		return;
> >+
> >+	io_mapping_unmap(vma->iomap);
> >+	vma->iomap = NULL;
> >+}
> 
> It would be best to hide this near the unbind code. Alternatively
> maybe put a louder warning in the kerneldoc? Just so it is a bit
> more distinguishable from the GEM exported API for this feature.

I had the same thought, then decided to stick with the
i915_vma_iounmap() call in i915_vma_unbind(). Maybe __i915_vma_iounmap?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 18/04/16 16:22, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 04:08:55PM +0100, Tvrtko Ursulin wrote:
>> On 18/04/16 15:54, Chris Wilson wrote:
>>> @@ -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..e185dff358e1 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>>>   		return obj->base.size;
>>>   	}
>>>   }
>>> +
>>> +void *i915_vma_pin_iomap(struct i915_vma *vma)
>>> +{
>>> +	void *ptr;
>>
>> Just realized we should mark the pointers with __iomem here.
>
> The problem is that we need to lose the annotation at some point as we
> mix access via ring->virtual_start. For correctness, we should report
> __iomem here and discard it later.
>
>>> +/**
>>> + * i915_vma_iounmap - unmaps the VMA
>>> + * @vma: VMA to unmap
>>> + *
>>> + * Unmaps the previously iomapped VMA using iounmap.
>>> + *
>>> + * Users of i915_vma_pin_iomap() should not manually unmap by calling this
>>> + * function but should call i915_vma_unpin_iomap() instead.
>>> + *
>>> + * Callers must hold the struct_mutex.
>>> + */
>>> +static inline void i915_vma_iounmap(struct i915_vma *vma)
>>> +{
>>> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
>>> +	GEM_BUG_ON(vma->pin_count);
>>> +
>>> +	if (vma->iomap == NULL)
>>> +		return;
>>> +
>>> +	io_mapping_unmap(vma->iomap);
>>> +	vma->iomap = NULL;
>>> +}
>>
>> It would be best to hide this near the unbind code. Alternatively
>> maybe put a louder warning in the kerneldoc? Just so it is a bit
>> more distinguishable from the GEM exported API for this feature.
>
> I had the same thought, then decided to stick with the
> i915_vma_iounmap() call in i915_vma_unbind(). Maybe __i915_vma_iounmap?

__i915_vma_ionmap in i915_gem_gtt.h or in i915_gem.c ?

Regards,

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

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

[Chris Wilson]

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
v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
sections and ensure the VMA cannot be reaped whilst mapped.
v7: Move i915_vma_iounmap so that consumers of the API are not tempted,
and add iomem annotations

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          | 11 ++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c      | 26 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h      | 35 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++++++++++------
 drivers/gpu/drm/i915/intel_fbdev.c       | 22 +++++++++++---------
 5 files changed, 106 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ce2c31b9a81..9ef47329e8ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
 					    old_write_domain);
 }
 
+static void __i915_vma_iounmap(struct i915_vma *vma)
+{
+	if (vma->iomap == NULL)
+		return;
+
+	io_mapping_unmap(vma->iomap);
+	vma->iomap = NULL;
+}
+
 static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
 {
 	struct drm_i915_gem_object *obj = vma->obj;
@@ -3378,6 +3387,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..9d4293f247fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		return obj->base.size;
 	}
 }
+
+void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
+{
+	void __iomem *ptr;
+
+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
+	if (WARN_ON(!vma->obj->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	GEM_BUG_ON(!vma->is_ggtt);
+	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	ptr = vma->iomap;
+	if (ptr == NULL) {
+		ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+					vma->node.start,
+					vma->node.size);
+		if (ptr == NULL)
+			return ERR_PTR(-ENOMEM);
+
+		vma->iomap = ptr;
+	}
+
+	vma->pin_count++;
+	return ptr;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..b0ae6632c01a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+#include <linux/io-mapping.h>
+
 struct drm_i915_file_private;
 
 typedef uint32_t gen6_pte_t;
@@ -175,6 +177,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void __iomem *iomap;
 
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
@@ -559,4 +562,36 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+/**
+ * i915_vma_pin_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
+ * @vma: VMA to iomap
+ *
+ * The passed in VMA has to be pinned in the global GTT mappable region.
+ * An extra pinning of the VMA is acquired for the return iomapping,
+ * the caller must call i915_vma_unpin_iomap to relinquish the pinning
+ * after the iomapping is no longer required.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void __iomem *i915_vma_pin_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
+ * @vma: VMA to unpin
+ *
+ * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
+ *
+ * Callers must hold the struct_mutex. This function is only valid to be
+ * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
+ */
+static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
+{
+	lockdep_assert_held(&vma->vm->dev->struct_mutex);
+	GEM_BUG_ON(vma->pin_count == 0);
+	GEM_BUG_ON(vma->iomap == NULL);
+	vma->pin_count--;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..6156ee96f429 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct drm_i915_private *dev_priv =
 		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
 	struct shrinker_lock_uninterruptible slu;
-	unsigned long freed_pages;
+	struct i915_vma *vma, *next;
+	unsigned long freed_pages = 0;
+	int ret;
 
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
-	freed_pages = i915_gem_shrink(dev_priv, -1UL,
-				      I915_SHRINK_BOUND |
-				      I915_SHRINK_UNBOUND |
-				      I915_SHRINK_ACTIVE |
-				      I915_SHRINK_VMAPS);
+	/* Force everything onto the inactive lists */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		goto out;
+
+	freed_pages += i915_gem_shrink(dev_priv, -1UL,
+				       I915_SHRINK_BOUND |
+				       I915_SHRINK_UNBOUND |
+				       I915_SHRINK_ACTIVE |
+				       I915_SHRINK_VMAPS);
+
+	/* We also want to clear any cached iomaps as they wrap vmap */
+	list_for_each_entry_safe(vma, next,
+				 &dev_priv->ggtt.base.inactive_list, vm_link) {
+		unsigned long count = vma->node.size >> PAGE_SHIFT;
+		if (vma->iomap && i915_vma_unbind(vma) == 0)
+			freed_pages += count;
+	}
 
+out:
 	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
 
 	*(unsigned long *)ptr += freed_pages;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202f3870..93f54a10042f 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
+	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
-	int size, ret;
 	bool prealloc = false;
+	void *vaddr;
+	int ret;
 
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
@@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 
 	obj = intel_fb->obj;
-	size = obj->base.size;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &intelfb_ops;
 
+	vma = i915_gem_obj_to_ggtt(obj);
+
 	/* setup aperture base/size for vesafb takeover */
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = ggtt->mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
-	info->fix.smem_len = size;
+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
+	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
-	if (!info->screen_base) {
+	vaddr = i915_vma_pin_iomap(vma);
+	if (IS_ERR(vaddr)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(vaddr);
 		goto out_destroy_fbi;
 	}
-	info->screen_size = size;
+	info->screen_base = vaddr;
+	info->screen_size = vma->node.size;
 
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
-- 
2.8.0.rc3

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

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

[Tvrtko Ursulin]

On 19/04/16 12:06, 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
> v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
> sections and ensure the VMA cannot be reaped whilst mapped.
> v7: Move i915_vma_iounmap so that consumers of the API are not tempted,
> and add iomem annotations
>
> 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          | 11 ++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.c      | 26 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h      | 35 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++++++++++------
>   drivers/gpu/drm/i915/intel_fbdev.c       | 22 +++++++++++---------
>   5 files changed, 106 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..9ef47329e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
>   					    old_write_domain);
>   }
>
> +static void __i915_vma_iounmap(struct i915_vma *vma)
> +{
> +	if (vma->iomap == NULL)
> +		return;
> +
> +	io_mapping_unmap(vma->iomap);
> +	vma->iomap = NULL;
> +}
> +
>   static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>   {
>   	struct drm_i915_gem_object *obj = vma->obj;
> @@ -3378,6 +3387,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..9d4293f247fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		return obj->base.size;
>   	}
>   }
> +
> +void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
> +{
> +	void __iomem *ptr;
> +
> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
> +	if (WARN_ON(!vma->obj->map_and_fenceable))
> +		return ERR_PTR(-ENODEV);
> +
> +	GEM_BUG_ON(!vma->is_ggtt);
> +	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> +
> +	ptr = vma->iomap;
> +	if (ptr == NULL) {
> +		ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> +					vma->node.start,
> +					vma->node.size);
> +		if (ptr == NULL)
> +			return ERR_PTR(-ENOMEM);
> +
> +		vma->iomap = ptr;
> +	}
> +
> +	vma->pin_count++;
> +	return ptr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..b0ae6632c01a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -34,6 +34,8 @@
>   #ifndef __I915_GEM_GTT_H__
>   #define __I915_GEM_GTT_H__
>
> +#include <linux/io-mapping.h>
> +
>   struct drm_i915_file_private;
>
>   typedef uint32_t gen6_pte_t;
> @@ -175,6 +177,7 @@ struct i915_vma {
>   	struct drm_mm_node node;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_address_space *vm;
> +	void __iomem *iomap;
>
>   	/** Flags and address space this VMA is bound to */
>   #define GLOBAL_BIND	(1<<0)
> @@ -559,4 +562,36 @@ size_t
>   i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		    const struct i915_ggtt_view *view);
>
> +/**
> + * i915_vma_pin_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
> + * @vma: VMA to iomap
> + *
> + * The passed in VMA has to be pinned in the global GTT mappable region.
> + * An extra pinning of the VMA is acquired for the return iomapping,
> + * the caller must call i915_vma_unpin_iomap to relinquish the pinning
> + * after the iomapping is no longer required.
> + *
> + * Callers must hold the struct_mutex.
> + *
> + * Returns a valid iomapped pointer or ERR_PTR.
> + */
> +void __iomem *i915_vma_pin_iomap(struct i915_vma *vma);
> +
> +/**
> + * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
> + * @vma: VMA to unpin
> + *
> + * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
> + *
> + * Callers must hold the struct_mutex. This function is only valid to be
> + * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
> + */
> +static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
> +{
> +	lockdep_assert_held(&vma->vm->dev->struct_mutex);
> +	GEM_BUG_ON(vma->pin_count == 0);
> +	GEM_BUG_ON(vma->iomap == NULL);
> +	vma->pin_count--;
> +}
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d46388f25e04..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>   	struct drm_i915_private *dev_priv =
>   		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
>   	struct shrinker_lock_uninterruptible slu;
> -	unsigned long freed_pages;
> +	struct i915_vma *vma, *next;
> +	unsigned long freed_pages = 0;
> +	int ret;
>
>   	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
>   		return NOTIFY_DONE;
>
> -	freed_pages = i915_gem_shrink(dev_priv, -1UL,
> -				      I915_SHRINK_BOUND |
> -				      I915_SHRINK_UNBOUND |
> -				      I915_SHRINK_ACTIVE |
> -				      I915_SHRINK_VMAPS);
> +	/* Force everything onto the inactive lists */
> +	ret = i915_gpu_idle(dev_priv->dev);
> +	if (ret)
> +		goto out;
> +
> +	freed_pages += i915_gem_shrink(dev_priv, -1UL,
> +				       I915_SHRINK_BOUND |
> +				       I915_SHRINK_UNBOUND |
> +				       I915_SHRINK_ACTIVE |
> +				       I915_SHRINK_VMAPS);
> +
> +	/* We also want to clear any cached iomaps as they wrap vmap */
> +	list_for_each_entry_safe(vma, next,
> +				 &dev_priv->ggtt.base.inactive_list, vm_link) {
> +		unsigned long count = vma->node.size >> PAGE_SHIFT;
> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> +			freed_pages += count;
> +	}
>
> +out:
>   	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
>
>   	*(unsigned long *)ptr += freed_pages;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	struct fb_info *info;
>   	struct drm_framebuffer *fb;
> +	struct i915_vma *vma;
>   	struct drm_i915_gem_object *obj;
> -	int size, ret;
>   	bool prealloc = false;
> +	void *vaddr;
> +	int ret;
>
>   	if (intel_fb &&
>   	    (sizes->fb_width > intel_fb->base.width ||
> @@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	}
>
>   	obj = intel_fb->obj;
> -	size = obj->base.size;
>
>   	mutex_lock(&dev->struct_mutex);
>
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>   	info->fbops = &intelfb_ops;
>
> +	vma = i915_gem_obj_to_ggtt(obj);
> +
>   	/* setup aperture base/size for vesafb takeover */
>   	info->apertures->ranges[0].base = dev->mode_config.fb_base;
>   	info->apertures->ranges[0].size = ggtt->mappable_end;
>
> -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> -	info->fix.smem_len = size;
> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> +	info->fix.smem_len = vma->node.size;
>
> -	info->screen_base =
> -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -			   size);
> -	if (!info->screen_base) {
> +	vaddr = i915_vma_pin_iomap(vma);
> +	if (IS_ERR(vaddr)) {
>   		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(vaddr);
>   		goto out_destroy_fbi;
>   	}
> -	info->screen_size = size;
> +	info->screen_base = vaddr;
> +	info->screen_size = vma->node.size;
>
>   	/* This driver doesn't need a VT switch to restore the mode on resume */
>   	info->skip_vt_switch = true;
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

Hi,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160419]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Move-ioremap_wc-tracking-onto-VMA/20160419-190843
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem_gtt.c: In function 'i915_vma_pin_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:3: error: implicit declaration of function 'i915_vm_to_ggtt' [-Werror=implicit-function-declaration]
      ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
      ^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:51: error: invalid type argument of '->' (have 'int')
      ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
                                                      ^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:9: error: too many arguments to function 'io_mapping_map_wc'
      ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
            ^
   In file included from drivers/gpu/drm/i915/i915_drv.h:36:0,
                    from drivers/gpu/drm/i915/i915_gem_gtt.c:30:
   include/linux/io-mapping.h:158:1: note: declared here
    io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
    ^
   cc1: some warnings being treated as errors

vim +/i915_vm_to_ggtt +3644 drivers/gpu/drm/i915/i915_gem_gtt.c

  3638	
  3639		GEM_BUG_ON(!vma->is_ggtt);
  3640		GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
  3641	
  3642		ptr = vma->iomap;
  3643		if (ptr == NULL) {
> 3644			ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
  3645						vma->node.start,
  3646						vma->node.size);
  3647			if (ptr == NULL)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36435 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

Hi Chris,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160418]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Move-ioremap_wc-tracking-onto-VMA/20160418-225730
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201616 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem_gtt.c: In function 'i915_vma_pin_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:27: error: implicit declaration of function 'i915_vm_to_ggtt' [-Werror=implicit-function-declaration]
      ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
                              ^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:51: error: invalid type argument of '->' (have 'int')
      ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
                                                      ^
   drivers/gpu/drm/i915/i915_gem_gtt.c:3644:9: error: too many arguments to function 'io_mapping_map_wc'
      ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
            ^
   In file included from drivers/gpu/drm/i915/i915_drv.h:36:0,
                    from drivers/gpu/drm/i915/i915_gem_gtt.c:30:
   include/linux/io-mapping.h:158:1: note: declared here
    io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
    ^
   cc1: some warnings being treated as errors

vim +/i915_vm_to_ggtt +3644 drivers/gpu/drm/i915/i915_gem_gtt.c

  3638	
  3639		GEM_BUG_ON(!vma->is_ggtt);
  3640		GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
  3641	
  3642		ptr = vma->iomap;
  3643		if (ptr == NULL) {
> 3644			ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
  3645						vma->node.start,
  3646						vma->node.size);
  3647			if (ptr == NULL)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25849 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

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

[Tvrtko Ursulin]

On 18/04/16 12:14, 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(-)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 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)
> +{
> +	void *ptr;
> +
> +	if (vma->iomap)
> +		return vma->iomap;
> +
> +	if (WARN_ON(!vma->obj->map_and_fenceable))
> +		return ERR_PTR(-ENODEV);
> +
> +	GEM_BUG_ON(!vma->is_ggtt);
> +	GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> +
> +	ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> +				vma->node.start,
> +				vma->node.size);
> +	if (ptr == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	vma->iomap = ptr;
> +	return ptr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..d95190ddf2d6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -34,6 +34,8 @@
>   #ifndef __I915_GEM_GTT_H__
>   #define __I915_GEM_GTT_H__
>
> +#include <linux/io-mapping.h>
> +
>   struct drm_i915_file_private;
>
>   typedef uint32_t gen6_pte_t;
> @@ -175,6 +177,7 @@ struct i915_vma {
>   	struct drm_mm_node node;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_address_space *vm;
> +	void *iomap;
>
>   	/** Flags and address space this VMA is bound to */
>   #define GLOBAL_BIND	(1<<0)
> @@ -559,4 +562,39 @@ size_t
>   i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		    const struct i915_ggtt_view *view);
>
> +/**
> + * i915_vma_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
> + * @vma: VMA to iomap
> + *
> + * The passed in VMA has to be pinned in the global GTT mappable region. Or in
> + * other words callers are responsible for managing the VMA pinned lifetime and
> + * ensuring it covers the use of the returned mapping.
> + *
> + * Callers must hold the struct_mutex.
> + *
> + * Returns a valid iomapped pointer or ERR_PTR.
> + */
> +void *i915_vma_iomap(struct i915_vma *vma);
> +
> +/**
> + * i915_vma_iounmap - unmaps the mapping returned from i915_vma_iomap
> + * @dev_priv: i915 private pointer
> + * @vma: VMA to unmap
> + *
> + * Unmaps the previously iomapped VMA using iounmap.
> + *
> + * Users of i915_vma_iomap should not manually unmap by calling this function
> + * if they want to take advantage of the mapping getting cached in the VMA.
> + *
> + * Callers must hold the struct_mutex.
> + */
> +static inline void i915_vma_iounmap(struct i915_vma *vma)
> +{
> +	if (vma->iomap == NULL)
> +		return;
> +
> +	io_mapping_unmap(vma->iomap);
> +	vma->iomap = NULL;
> +}
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d46388f25e04..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
>   	struct drm_i915_private *dev_priv =
>   		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
>   	struct shrinker_lock_uninterruptible slu;
> -	unsigned long freed_pages;
> +	struct i915_vma *vma, *next;
> +	unsigned long freed_pages = 0;
> +	int ret;
>
>   	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
>   		return NOTIFY_DONE;
>
> -	freed_pages = i915_gem_shrink(dev_priv, -1UL,
> -				      I915_SHRINK_BOUND |
> -				      I915_SHRINK_UNBOUND |
> -				      I915_SHRINK_ACTIVE |
> -				      I915_SHRINK_VMAPS);
> +	/* Force everything onto the inactive lists */
> +	ret = i915_gpu_idle(dev_priv->dev);
> +	if (ret)
> +		goto out;
> +
> +	freed_pages += i915_gem_shrink(dev_priv, -1UL,
> +				       I915_SHRINK_BOUND |
> +				       I915_SHRINK_UNBOUND |
> +				       I915_SHRINK_ACTIVE |
> +				       I915_SHRINK_VMAPS);
> +
> +	/* We also want to clear any cached iomaps as they wrap vmap */
> +	list_for_each_entry_safe(vma, next,
> +				 &dev_priv->ggtt.base.inactive_list, vm_link) {
> +		unsigned long count = vma->node.size >> PAGE_SHIFT;
> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> +			freed_pages += count;
> +	}
>
> +out:
>   	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
>
>   	*(unsigned long *)ptr += freed_pages;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..3f3c97a30418 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	struct fb_info *info;
>   	struct drm_framebuffer *fb;
> +	struct i915_vma *vma;
>   	struct drm_i915_gem_object *obj;
> -	int size, ret;
>   	bool prealloc = false;
> +	void *vaddr;
> +	int ret;
>
>   	if (intel_fb &&
>   	    (sizes->fb_width > intel_fb->base.width ||
> @@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	}
>
>   	obj = intel_fb->obj;
> -	size = obj->base.size;
>
>   	mutex_lock(&dev->struct_mutex);
>
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>   	info->fbops = &intelfb_ops;
>
> +	vma = i915_gem_obj_to_ggtt(obj);
> +
>   	/* setup aperture base/size for vesafb takeover */
>   	info->apertures->ranges[0].base = dev->mode_config.fb_base;
>   	info->apertures->ranges[0].size = ggtt->mappable_end;
>
> -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> -	info->fix.smem_len = size;
> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> +	info->fix.smem_len = vma->node.size;
>
> -	info->screen_base =
> -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -			   size);
> -	if (!info->screen_base) {
> +	vaddr = i915_vma_iomap(vma);
> +	if (IS_ERR(vaddr)) {
>   		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(vaddr);
>   		goto out_destroy_fbi;
>   	}
> -	info->screen_size = size;
> +	info->screen_base = vaddr;
> +	info->screen_size = vma->node.size;
>
>   	/* This driver doesn't need a VT switch to restore the mode on resume */
>   	info->skip_vt_switch = true;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr
URL   : https://patchwork.freedesktop.org/series/5865/
State : warning

== Summary ==

Series 5865v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5865/revisions/1/mbox/

Test gem_exec_whisper:
        Subgroup basic:
                incomplete -> PASS       (bdw-nuci7)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (snb-dellxps)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:163  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:203  pass:135  dwarn:0   dfail:0   fail:0   skip:68 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:164  dwarn:1   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:165  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1926/

78673b24b60c1a884c947ee5a45ad860ca618418 drm-intel-nightly: 2016y-04m-18d-12h-32m-33s UTC integration manifest
3888a6c drm/i915: Move ioremap_wc tracking onto VMA
1714d0a drm/i915: Introduce i915_vm_to_ggtt()
959fab4 io-mapping: Specify mapping size for io_mapping_map_wc()
ec6a39c drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

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

✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2)
URL   : https://patchwork.freedesktop.org/series/5865/
State : warning

== Summary ==

Series 5865v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5865/revisions/2/mbox/

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (skl-nuci5)
Test gem_busy:
        Subgroup basic-blt:
                pass       -> SKIP       (bsw-nuc-2)
Test gem_exec_whisper:
        Subgroup basic:
                incomplete -> PASS       (bdw-nuci7)
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> SKIP       (snb-x220t)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (ivb-t430s)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (skl-nuci5)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:162  dwarn:0   dfail:0   fail:0   skip:40 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19 
ivb-t430s        total:203  pass:174  dwarn:1   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:190  dwarn:2   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:165  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:164  dwarn:0   dfail:0   fail:1   skip:38 

Results at /archive/results/CI_IGT_test/Patchwork_1929/

78673b24b60c1a884c947ee5a45ad860ca618418 drm-intel-nightly: 2016y-04m-18d-12h-32m-33s UTC integration manifest
d352727 drm/i915: Move ioremap_wc tracking onto VMA
0c66e48 drm/i915: Introduce i915_vm_to_ggtt()
e3f5de7 io-mapping: Specify mapping size for io_mapping_map_wc()
96da749 drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

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

✗ Fi.CI.BAT: failure for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3)
URL   : https://patchwork.freedesktop.org/series/5865/
State : failure

== Summary ==

Series 5865v3 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5865/revisions/3/mbox/

Test gem_busy:
        Subgroup basic-blt:
                skip       -> PASS       (bsw-nuc-2)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                pass       -> SKIP       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                pass       -> INCOMPLETE (snb-dellxps)
        Subgroup read-crc-pipe-a-frame-sequence:
                fail       -> PASS       (ivb-t430s)

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:127  pass:105  dwarn:0   dfail:0   fail:0   skip:22 
bsw-nuc-2        total:191  pass:152  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:191  pass:153  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:192  pass:168  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:192  pass:173  dwarn:0   dfail:0   fail:0   skip:19 
ivb-t430s        total:192  pass:163  dwarn:0   dfail:0   fail:0   skip:29 
skl-i7k-2        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:11   pass:7    dwarn:0   dfail:0   fail:0   skip:3  
snb-x220t        total:192  pass:154  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1939/

c438a31de88e7dc6bfe17ed0e094f9a1b08bcb94 drm-intel-nightly: 2016y-04m-19d-09h-36m-00s UTC integration manifest
c0bc8687 drm/i915: Move ioremap_wc tracking onto VMA
7b43c8e drm/i915: Introduce i915_vm_to_ggtt()
870b4c31 io-mapping: Specify mapping size for io_mapping_map_wc()
4c789fb drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

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

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

[Chris Wilson]

On Wed, Apr 13, 2016 at 01:30:39PM +0100, Tvrtko Ursulin wrote:
> As long as it remains hidden in here, otherwise is a bit heavy and
> rude (BUG_ON), or weak as API (if converted to GEM_BUG_ON and return
> pointer undocumented). And if it stays here BUG_ON is redundant.
> Hm.. leaning towards the direct container_of below to bypass all
> these considerations.

Reasonable. I do value the shorthand to_ggtt() though. Ok, the shorthand
can wait until we have more use cases.

> >  static int
> >  i915_get_ggtt_vma_pages(struct i915_vma *vma);
> >
> >@@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> >  		return obj->base.size;
> >  	}
> >  }
> >+
> >+void *i915_vma_iomap(struct i915_vma *vma)
> 
> Kerneldoc! :)

Sorry. Should have double checked with your patch.

> >+{
> >+	if (WARN_ON(!vma->obj->map_and_fenceable))
> >+		return ERR_PTR(-ENODEV);
> >+
> >+	BUG_ON(!vma->is_ggtt);
> >+	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> 
> Maybe aggregate the two into a single WARN_ON and return -EINVAL ?
> Since we easily can sounds better.
> 
> >+
> >+	if (vma->iomap == NULL) {
> >+		struct i915_ggtt *ggtt = to_ggtt(vma->vm);
> >+		void *ptr;
> >+
> >+		ptr = io_mapping_map_wc(ggtt->mappable,
> >+					vma->node.start,
> >+					vma->node.size);
> >+		if (ptr == NULL) {
> >+			int ret;
> >+
> >+			/* Too many areas already allocated? */
> >+			ret = i915_gem_evict_vm(vma->vm, true);
> 
> I don't know much about the iomapping bussiness. Can we exhaust the
> address space similar to vmap and would then interfere with our own
> (or other?) call sites doing plain io_mapping_map* calls?

It's vmap underneath. My thinking here was that if we couldn't allocate
a new ioremap_wc, the obvious candidates were to reap everything inside
the GGTT i.e. inactive ioremap_wc.

But the question is, do we want to reap vma->iomaps upon vmap_purge.
Yes, we probably should - simplest will be to do a similar forced GGTT
eviction from vmap_purge and refine from there.

> >+	vma = i915_gem_obj_to_ggtt(obj);
> >+
> >  	/* setup aperture base/size for vesafb takeover */
> >  	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> >  	info->apertures->ranges[0].size = ggtt->mappable_end;
> >
> >-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> >-	info->fix.smem_len = size;
> >+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> >+	info->fix.smem_len = vma->node.size;
> >
> >-	info->screen_base =
> >-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> >-			   size);
> >-	if (!info->screen_base) {
> >+	info->screen_base = i915_vma_iomap(vma);
> 
> We are slowly establishing that ERR_PTR should not be stored in
> structure members but I suppose here we can allow it since the
> structure is freed if it fails.

I know, I tell everyone else off for doing it like this as well.
Practice what you preach!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c          |  2 ++
 drivers/gpu/drm/i915/i915_gem_gtt.c      | 25 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h      | 38 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++++++++++++++++-----
 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 b37ffea8b458..6a485630595e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3393,6 +3393,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 c5cb04907525..53e55aead512 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3626,3 +3626,28 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		return obj->base.size;
 	}
 }
+
+void *i915_vma_iomap(struct i915_vma *vma)
+{
+	if (WARN_ON(!vma->obj->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	BUG_ON(!vma->is_ggtt);
+	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	if (vma->iomap == NULL) {
+		struct i915_ggtt *ggtt =
+			container_of(vma->vm, struct i915_ggtt, base);
+		void *ptr;
+
+		ptr = io_mapping_map_wc(ggtt->mappable,
+					vma->node.start,
+					vma->node.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 d7dd3d8a8758..d95190ddf2d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+#include <linux/io-mapping.h>
+
 struct drm_i915_file_private;
 
 typedef uint32_t gen6_pte_t;
@@ -175,6 +177,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void *iomap;
 
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
@@ -559,4 +562,39 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+/**
+ * i915_vma_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
+ * @vma: VMA to iomap
+ *
+ * The passed in VMA has to be pinned in the global GTT mappable region. Or in
+ * other words callers are responsible for managing the VMA pinned lifetime and
+ * ensuring it covers the use of the returned mapping.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void *i915_vma_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_iounmap - unmaps the mapping returned from i915_vma_iomap
+ * @dev_priv: i915 private pointer
+ * @vma: VMA to unmap
+ *
+ * Unmaps the previously iomapped VMA using iounmap.
+ *
+ * Users of i915_vma_iomap should not manually unmap by calling this function
+ * if they want to take advantage of the mapping getting cached in the VMA.
+ *
+ * Callers must hold the struct_mutex.
+ */
+static inline void i915_vma_iounmap(struct i915_vma *vma)
+{
+	if (vma->iomap == NULL)
+		return;
+
+	io_mapping_unmap(vma->iomap);
+	vma->iomap = NULL;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..908c083a39f1 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,31 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct drm_i915_private *dev_priv =
 		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
 	struct shrinker_lock_uninterruptible slu;
-	unsigned long freed_pages;
+	struct i915_vma *vma, *next;
+	unsigned long freed_pages = 0;
+	int ret;
 
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
-	freed_pages = i915_gem_shrink(dev_priv, -1UL,
-				      I915_SHRINK_BOUND |
-				      I915_SHRINK_UNBOUND |
-				      I915_SHRINK_ACTIVE |
-				      I915_SHRINK_VMAPS);
+	/* Force everything onto the inactive lists */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		goto out;
 
+	freed_pages += i915_gem_shrink(dev_priv, -1UL,
+				       I915_SHRINK_BOUND |
+				       I915_SHRINK_UNBOUND |
+				       I915_SHRINK_ACTIVE |
+				       I915_SHRINK_VMAPS);
+
+	/* We also want to clear any cached iomaps as they wrap vmap */
+	list_for_each_entry_safe(vma, next,
+				 &dev_priv->ggtt.base.inactive_list, vm_link)
+		if (vma->iomap && i915_vma_unbind(vma) == 0)
+			freed_pages += vma->node.size >> PAGE_SHIFT;
+
+out:
 	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
 
 	*(unsigned long *)ptr += freed_pages;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202f3870..3f3c97a30418 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
+	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
-	int size, ret;
 	bool prealloc = false;
+	void *vaddr;
+	int ret;
 
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
@@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 
 	obj = intel_fb->obj;
-	size = obj->base.size;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &intelfb_ops;
 
+	vma = i915_gem_obj_to_ggtt(obj);
+
 	/* setup aperture base/size for vesafb takeover */
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = ggtt->mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
-	info->fix.smem_len = size;
+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
+	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
-	if (!info->screen_base) {
+	vaddr = i915_vma_iomap(vma);
+	if (IS_ERR(vaddr)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(vaddr);
 		goto out_destroy_fbi;
 	}
-	info->screen_size = size;
+	info->screen_base = vaddr;
+	info->screen_size = vma->node.size;
 
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
-- 
2.8.0.rc3

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

Hi Chris,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160413]
[cannot apply to v4.6-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Move-ioremap_wc-tracking-onto-VMA/20160413-225200
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201615 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem_gtt.c: In function 'i915_vma_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3643:9: error: too many arguments to function 'io_mapping_map_wc'
      ptr = io_mapping_map_wc(ggtt->mappable,
            ^
   In file included from drivers/gpu/drm/i915/i915_gem_gtt.h:37:0,
                    from drivers/gpu/drm/i915/i915_drv.h:42,
                    from drivers/gpu/drm/i915/i915_gem_gtt.c:30:
   include/linux/io-mapping.h:158:1: note: declared here
    io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
    ^

vim +/io_mapping_map_wc +3643 drivers/gpu/drm/i915/i915_gem_gtt.c

  3637	
  3638		if (vma->iomap == NULL) {
  3639			struct i915_ggtt *ggtt =
  3640				container_of(vma->vm, struct i915_ggtt, base);
  3641			void *ptr;
  3642	
> 3643			ptr = io_mapping_map_wc(ggtt->mappable,
  3644						vma->node.start,
  3645						vma->node.size);
  3646			if (ptr == NULL)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 32481 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

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

[Chris Wilson]

On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> +	/* We also want to clear any cached iomaps as they wrap vmap */
> +	list_for_each_entry_safe(vma, next,
> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> +			freed_pages += vma->node.size >> PAGE_SHIFT;

Use after free. I need to store the page count in a local before calling
unbind.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 13/04/16 16:12, Chris Wilson wrote:
> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
>> +	/* We also want to clear any cached iomaps as they wrap vmap */
>> +	list_for_each_entry_safe(vma, next,
>> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
>> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
>> +			freed_pages += vma->node.size >> PAGE_SHIFT;
>
> Use after free. I need to store the page count in a local before calling
> unbind.

Waiting for respin. :)

Also, shouldn't the patch which adds the size argument to 
io_mapping_map_wc be in this series?

Regards,

Tvrtko

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

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

[Chris Wilson]

On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
> 
> On 13/04/16 16:12, Chris Wilson wrote:
> >On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> >>+	/* We also want to clear any cached iomaps as they wrap vmap */
> >>+	list_for_each_entry_safe(vma, next,
> >>+				 &dev_priv->ggtt.base.inactive_list, vm_link)
> >>+		if (vma->iomap && i915_vma_unbind(vma) == 0)
> >>+			freed_pages += vma->node.size >> PAGE_SHIFT;
> >
> >Use after free. I need to store the page count in a local before calling
> >unbind.
> 
> Waiting for respin. :)
> 
> Also, shouldn't the patch which adds the size argument to
> io_mapping_map_wc be in this series?

It was, this was just an update to patch 2. The delta here is just
unsigned long count = vma->node.size >> PAGE_SHIFT;
if (vma->iomap && i915_vma_unbind(vma) == 0)
	freed_pages += count;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 15/04/16 11:00, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
>>
>> On 13/04/16 16:12, Chris Wilson wrote:
>>> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
>>>> +	/* We also want to clear any cached iomaps as they wrap vmap */
>>>> +	list_for_each_entry_safe(vma, next,
>>>> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
>>>> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
>>>> +			freed_pages += vma->node.size >> PAGE_SHIFT;
>>>
>>> Use after free. I need to store the page count in a local before calling
>>> unbind.
>>
>> Waiting for respin. :)
>>
>> Also, shouldn't the patch which adds the size argument to
>> io_mapping_map_wc be in this series?
>
> It was, this was just an update to patch 2. The delta here is just
> unsigned long count = vma->node.size >> PAGE_SHIFT;
> if (vma->iomap && i915_vma_unbind(vma) == 0)
> 	freed_pages += count;

My bad, I got lost in the threads.. :(

Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs now 
that is in?

Maybe also add a quick return at the top, as a micro-opt:

	if (vma->iomap)
		reutrn vma->iomap;

Followed by WARNs, GEM_BUG_ONs and rest?

Shrinker fix mandatory and with or without my above comments r-b from me.

Regards,

Tvrtko

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

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

[Chris Wilson]

On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote:
> On 15/04/16 11:00, Chris Wilson wrote:
> >On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 13/04/16 16:12, Chris Wilson wrote:
> >>>On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> >>>>+	/* We also want to clear any cached iomaps as they wrap vmap */
> >>>>+	list_for_each_entry_safe(vma, next,
> >>>>+				 &dev_priv->ggtt.base.inactive_list, vm_link)
> >>>>+		if (vma->iomap && i915_vma_unbind(vma) == 0)
> >>>>+			freed_pages += vma->node.size >> PAGE_SHIFT;
> >>>
> >>>Use after free. I need to store the page count in a local before calling
> >>>unbind.
> >>
> >>Waiting for respin. :)
> >>
> >>Also, shouldn't the patch which adds the size argument to
> >>io_mapping_map_wc be in this series?
> >
> >It was, this was just an update to patch 2. The delta here is just
> >unsigned long count = vma->node.size >> PAGE_SHIFT;
> >if (vma->iomap && i915_vma_unbind(vma) == 0)
> >	freed_pages += count;
> 
> My bad, I got lost in the threads.. :(
> 
> Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs
> now that is in?
> 
> Maybe also add a quick return at the top, as a micro-opt:
> 
> 	if (vma->iomap)
> 		reutrn vma->iomap;
> 
> Followed by WARNs, GEM_BUG_ONs and rest?

Like so?

void *i915_vma_iomap(struct i915_vma *vma)
{
        struct i915_ggtt *ggtt;
        void *ptr;

        if (vma->iomap)
                return vma->iomap;

        if (WARN_ON(!vma->obj->map_and_fenceable))
                return ERR_PTR(-ENODEV);

        GEM_BUG_ON(!vma->is_ggtt);
        GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);

        ggtt = container_of(vma->vm, struct i915_ggtt, base);
        ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size);
        if (ptr == NULL)
                return ERR_PTR(-ENOMEM);

        vma->iomap = ptr;
        return ptr;
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 15/04/16 11:38, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote:
>> On 15/04/16 11:00, Chris Wilson wrote:
>>> On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 13/04/16 16:12, Chris Wilson wrote:
>>>>> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
>>>>>> +	/* We also want to clear any cached iomaps as they wrap vmap */
>>>>>> +	list_for_each_entry_safe(vma, next,
>>>>>> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
>>>>>> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
>>>>>> +			freed_pages += vma->node.size >> PAGE_SHIFT;
>>>>>
>>>>> Use after free. I need to store the page count in a local before calling
>>>>> unbind.
>>>>
>>>> Waiting for respin. :)
>>>>
>>>> Also, shouldn't the patch which adds the size argument to
>>>> io_mapping_map_wc be in this series?
>>>
>>> It was, this was just an update to patch 2. The delta here is just
>>> unsigned long count = vma->node.size >> PAGE_SHIFT;
>>> if (vma->iomap && i915_vma_unbind(vma) == 0)
>>> 	freed_pages += count;
>>
>> My bad, I got lost in the threads.. :(
>>
>> Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs
>> now that is in?
>>
>> Maybe also add a quick return at the top, as a micro-opt:
>>
>> 	if (vma->iomap)
>> 		reutrn vma->iomap;
>>
>> Followed by WARNs, GEM_BUG_ONs and rest?
>
> Like so?
>
> void *i915_vma_iomap(struct i915_vma *vma)
> {
>          struct i915_ggtt *ggtt;
>          void *ptr;
>
>          if (vma->iomap)
>                  return vma->iomap;
>
>          if (WARN_ON(!vma->obj->map_and_fenceable))
>                  return ERR_PTR(-ENODEV);
>
>          GEM_BUG_ON(!vma->is_ggtt);
>          GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
>
>          ggtt = container_of(vma->vm, struct i915_ggtt, base);
>          ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size);
>          if (ptr == NULL)
>                  return ERR_PTR(-ENOMEM);
>
>          vma->iomap = ptr;
>          return ptr;
> }

Yes, that's what I had in mind. You can put my r-b in the repost.

Regards,

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

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

[Joonas Lahtinen]

On pe, 2016-04-15 at 11:38 +0100, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 15/04/16 11:00, Chris Wilson wrote:
> > > 
> > > On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
> > > > 
> > > > 
> > > > On 13/04/16 16:12, Chris Wilson wrote:
> > > > > 
> > > > > On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> > > > > > 
> > > > > > +	/* We also want to clear any cached iomaps as they wrap vmap */
> > > > > > +	list_for_each_entry_safe(vma, next,
> > > > > > +				 &dev_priv->ggtt.base.inactive_list, vm_link)
> > > > > > +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> > > > > > +			freed_pages += vma->node.size >> PAGE_SHIFT;
> > > > > Use after free. I need to store the page count in a local before calling
> > > > > unbind.
> > > > Waiting for respin. :)
> > > > 
> > > > Also, shouldn't the patch which adds the size argument to
> > > > io_mapping_map_wc be in this series?
> > > It was, this was just an update to patch 2. The delta here is just
> > > unsigned long count = vma->node.size >> PAGE_SHIFT;
> > > if (vma->iomap && i915_vma_unbind(vma) == 0)
> > > 	freed_pages += count;
> > My bad, I got lost in the threads.. :(
> > 
> > Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs
> > now that is in?
> > 
> > Maybe also add a quick return at the top, as a micro-opt:
> > 
> > 	if (vma->iomap)
> > 		reutrn vma->iomap;
> > 
> > Followed by WARNs, GEM_BUG_ONs and rest?
> Like so?
> 
> void *i915_vma_iomap(struct i915_vma *vma)
> {
>         struct i915_ggtt *ggtt;
>         void *ptr;
> 
>         if (vma->iomap)
>                 return vma->iomap;
> 
>         if (WARN_ON(!vma->obj->map_and_fenceable))
>                 return ERR_PTR(-ENODEV);
> 
>         GEM_BUG_ON(!vma->is_ggtt);
>         GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> 
>         ggtt = container_of(vma->vm, struct i915_ggtt, base);

We have:

static inline struct i915_hw_ppgtt *
i915_vm_to_ppgtt(struct i915_address_space *vm)

So rather make a function just like it.


>         ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size);
>         if (ptr == NULL)
>                 return ERR_PTR(-ENOMEM);
> 
>         vma->iomap = ptr;
>         return ptr;
> }
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx