* [PATCH 1/5] [v3] drm/i915: Move gtt and ppgtt under address space umbrella
@ 2013-07-16 23:50 Ben Widawsky
2013-07-16 23:50 ` [PATCH 2/5] [v2] drm/i915: Put the mm in the parent address space Ben Widawsky
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Ben Widawsky @ 2013-07-16 23:50 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
The GTT and PPGTT can be thought of more generally as GPU address
spaces. Many of their actions (insert entries), state (LRU lists), and
many of their characteristics (size) can be shared. Do that.
The change itself doesn't actually impact most of the VMA/VM rework
coming up, it just fits in with the grand scheme of abstracting the GPU
VM operations. GGTT will usually be a special case where we either know
an object must be in the GGTT (dislay engine, workarounds, etc.).
The scratch page is left as part of the VM (even though it's currently
shared with the ppgtt code) because in the future when we have Full
PPGTT, I intend to create a separate scratch page for each.
v2: Drop usage of i915_gtt_vm (Daniel)
Make cleanup also part of the parent class (Ben)
Modified commit msg
Rebased
v3: Properly share scratch page (Imre)
Finish commit message (Daniel, Imre)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_dma.c | 4 +-
drivers/gpu/drm/i915/i915_drv.h | 57 ++++++-------
drivers/gpu/drm/i915/i915_gem.c | 4 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 165 ++++++++++++++++++++----------------
5 files changed, 123 insertions(+), 111 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8819f85..1c697c0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -276,8 +276,8 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
count, size);
seq_printf(m, "%zu [%lu] gtt total\n",
- dev_priv->gtt.total,
- dev_priv->gtt.mappable_end - dev_priv->gtt.start);
+ dev_priv->gtt.base.total,
+ dev_priv->gtt.mappable_end - dev_priv->gtt.base.start);
seq_putc(m, '\n');
list_for_each_entry_reverse(file, &dev->filelist, lhead) {
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a1d04b2..c5fab2f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1673,7 +1673,7 @@ out_gem_unload:
out_mtrrfree:
arch_phys_wc_del(dev_priv->gtt.mtrr);
io_mapping_free(dev_priv->gtt.mappable);
- dev_priv->gtt.gtt_remove(dev);
+ dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
out_rmmap:
pci_iounmap(dev->pdev, dev_priv->regs);
put_bridge:
@@ -1768,7 +1768,7 @@ int i915_driver_unload(struct drm_device *dev)
destroy_workqueue(dev_priv->wq);
pm_qos_remove_request(&dev_priv->pm_qos);
- dev_priv->gtt.gtt_remove(dev);
+ dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
if (dev_priv->slab)
kmem_cache_destroy(dev_priv->slab);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fdc8e3..4a465e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -446,6 +446,29 @@ enum i915_cache_level {
typedef uint32_t gen6_gtt_pte_t;
+struct i915_address_space {
+ struct drm_device *dev;
+ unsigned long start; /* Start offset always 0 for dri2 */
+ size_t total; /* size addr space maps (ex. 2GB for ggtt) */
+
+ struct {
+ dma_addr_t addr;
+ struct page *page;
+ } scratch;
+
+ /* FIXME: Need a more generic return type */
+ gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
+ enum i915_cache_level level);
+ void (*clear_range)(struct i915_address_space *vm,
+ unsigned int first_entry,
+ unsigned int num_entries);
+ void (*insert_entries)(struct i915_address_space *vm,
+ struct sg_table *st,
+ unsigned int first_entry,
+ enum i915_cache_level cache_level);
+ void (*cleanup)(struct i915_address_space *vm);
+};
+
/* The Graphics Translation Table is the way in which GEN hardware translates a
* Graphics Virtual Address into a Physical Address. In addition to the normal
* collateral associated with any va->pa translations GEN hardware also has a
@@ -454,8 +477,7 @@ typedef uint32_t gen6_gtt_pte_t;
* the spec.
*/
struct i915_gtt {
- unsigned long start; /* Start offset of used GTT */
- size_t total; /* Total size GTT can map */
+ struct i915_address_space base;
size_t stolen_size; /* Total size of stolen memory */
unsigned long mappable_end; /* End offset that we can CPU map */
@@ -466,10 +488,6 @@ struct i915_gtt {
void __iomem *gsm;
bool do_idle_maps;
- struct {
- dma_addr_t addr;
- struct page *page;
- } scratch;
int mtrr;
@@ -477,38 +495,17 @@ struct i915_gtt {
int (*gtt_probe)(struct drm_device *dev, size_t *gtt_total,
size_t *stolen, phys_addr_t *mappable_base,
unsigned long *mappable_end);
- void (*gtt_remove)(struct drm_device *dev);
- void (*gtt_clear_range)(struct drm_device *dev,
- unsigned int first_entry,
- unsigned int num_entries);
- void (*gtt_insert_entries)(struct drm_device *dev,
- struct sg_table *st,
- unsigned int pg_start,
- enum i915_cache_level cache_level);
- gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
- enum i915_cache_level level);
};
-#define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
+#define gtt_total_entries(gtt) ((gtt).base.total >> PAGE_SHIFT)
struct i915_hw_ppgtt {
- struct drm_device *dev;
+ struct i915_address_space base;
unsigned num_pd_entries;
struct page **pt_pages;
uint32_t pd_offset;
dma_addr_t *pt_dma_addr;
- /* pte functions, mirroring the interface of the global gtt. */
- void (*clear_range)(struct i915_hw_ppgtt *ppgtt,
- unsigned int first_entry,
- unsigned int num_entries);
- void (*insert_entries)(struct i915_hw_ppgtt *ppgtt,
- struct sg_table *st,
- unsigned int pg_start,
- enum i915_cache_level cache_level);
- gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
- enum i915_cache_level level);
int (*enable)(struct drm_device *dev);
- void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
};
struct i915_ctx_hang_stats {
@@ -1124,7 +1121,7 @@ typedef struct drm_i915_private {
enum modeset_restore modeset_restore;
struct mutex modeset_restore_lock;
- struct i915_gtt gtt;
+ struct i915_gtt gtt; /* VMA representing the global address space */
struct i915_gem_mm mm;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c9d9d20..f29cf23 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -181,7 +181,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
pinned += i915_gem_obj_ggtt_size(obj);
mutex_unlock(&dev->struct_mutex);
- args->aper_size = dev_priv->gtt.total;
+ args->aper_size = dev_priv->gtt.base.total;
args->aper_available_size = args->aper_size - pinned;
return 0;
@@ -3051,7 +3051,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
u32 size, fence_size, fence_alignment, unfenced_alignment;
bool mappable, fenceable;
size_t gtt_max = map_and_fenceable ?
- dev_priv->gtt.mappable_end : dev_priv->gtt.total;
+ dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
int ret;
fence_size = i915_gem_get_gtt_size(dev,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4222733..f982bf0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -124,7 +124,7 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
static void gen6_write_pdes(struct i915_hw_ppgtt *ppgtt)
{
- struct drm_i915_private *dev_priv = ppgtt->dev->dev_private;
+ struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private;
gen6_gtt_pte_t __iomem *pd_addr;
uint32_t pd_entry;
int i;
@@ -203,18 +203,18 @@ static int gen6_ppgtt_enable(struct drm_device *dev)
}
/* PPGTT support for Sandybdrige/Gen6 and later */
-static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
+static void gen6_ppgtt_clear_range(struct i915_address_space *vm,
unsigned first_entry,
unsigned num_entries)
{
- struct drm_i915_private *dev_priv = ppgtt->dev->dev_private;
+ struct i915_hw_ppgtt *ppgtt =
+ container_of(vm, struct i915_hw_ppgtt, base);
gen6_gtt_pte_t *pt_vaddr, scratch_pte;
unsigned act_pt = first_entry / I915_PPGTT_PT_ENTRIES;
unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
unsigned last_pte, i;
- scratch_pte = ppgtt->pte_encode(dev_priv->gtt.scratch.addr,
- I915_CACHE_LLC);
+ scratch_pte = vm->pte_encode(vm->scratch.addr, I915_CACHE_LLC);
while (num_entries) {
last_pte = first_pte + num_entries;
@@ -234,11 +234,13 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
}
}
-static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
+static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
struct sg_table *pages,
unsigned first_entry,
enum i915_cache_level cache_level)
{
+ struct i915_hw_ppgtt *ppgtt =
+ container_of(vm, struct i915_hw_ppgtt, base);
gen6_gtt_pte_t *pt_vaddr;
unsigned act_pt = first_entry / I915_PPGTT_PT_ENTRIES;
unsigned act_pte = first_entry % I915_PPGTT_PT_ENTRIES;
@@ -249,7 +251,7 @@ static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
dma_addr_t page_addr;
page_addr = sg_page_iter_dma_address(&sg_iter);
- pt_vaddr[act_pte] = ppgtt->pte_encode(page_addr, cache_level);
+ pt_vaddr[act_pte] = vm->pte_encode(page_addr, cache_level);
if (++act_pte == I915_PPGTT_PT_ENTRIES) {
kunmap_atomic(pt_vaddr);
act_pt++;
@@ -261,13 +263,15 @@ static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
kunmap_atomic(pt_vaddr);
}
-static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
+static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
{
+ struct i915_hw_ppgtt *ppgtt =
+ container_of(vm, struct i915_hw_ppgtt, base);
int i;
if (ppgtt->pt_dma_addr) {
for (i = 0; i < ppgtt->num_pd_entries; i++)
- pci_unmap_page(ppgtt->dev->pdev,
+ pci_unmap_page(ppgtt->base.dev->pdev,
ppgtt->pt_dma_addr[i],
4096, PCI_DMA_BIDIRECTIONAL);
}
@@ -281,7 +285,7 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
{
- struct drm_device *dev = ppgtt->dev;
+ struct drm_device *dev = ppgtt->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
unsigned first_pd_entry_in_global_pt;
int i;
@@ -293,17 +297,18 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt);
if (IS_HASWELL(dev)) {
- ppgtt->pte_encode = hsw_pte_encode;
+ ppgtt->base.pte_encode = hsw_pte_encode;
} else if (IS_VALLEYVIEW(dev)) {
- ppgtt->pte_encode = byt_pte_encode;
+ ppgtt->base.pte_encode = byt_pte_encode;
} else {
- ppgtt->pte_encode = gen6_pte_encode;
+ ppgtt->base.pte_encode = gen6_pte_encode;
}
ppgtt->num_pd_entries = GEN6_PPGTT_PD_ENTRIES;
ppgtt->enable = gen6_ppgtt_enable;
- ppgtt->clear_range = gen6_ppgtt_clear_range;
- ppgtt->insert_entries = gen6_ppgtt_insert_entries;
- ppgtt->cleanup = gen6_ppgtt_cleanup;
+ ppgtt->base.clear_range = gen6_ppgtt_clear_range;
+ ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
+ ppgtt->base.cleanup = gen6_ppgtt_cleanup;
+ ppgtt->base.scratch = dev_priv->gtt.base.scratch;
ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
GFP_KERNEL);
if (!ppgtt->pt_pages)
@@ -334,8 +339,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
ppgtt->pt_dma_addr[i] = pt_addr;
}
- ppgtt->clear_range(ppgtt, 0,
- ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES);
+ ppgtt->base.clear_range(&ppgtt->base, 0,
+ ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES);
ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gen6_gtt_pte_t);
@@ -368,7 +373,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
if (!ppgtt)
return -ENOMEM;
- ppgtt->dev = dev;
+ ppgtt->base.dev = dev;
if (INTEL_INFO(dev)->gen < 8)
ret = gen6_ppgtt_init(ppgtt);
@@ -391,7 +396,7 @@ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev)
if (!ppgtt)
return;
- ppgtt->cleanup(ppgtt);
+ ppgtt->base.cleanup(&ppgtt->base);
dev_priv->mm.aliasing_ppgtt = NULL;
}
@@ -399,17 +404,17 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
struct drm_i915_gem_object *obj,
enum i915_cache_level cache_level)
{
- ppgtt->insert_entries(ppgtt, obj->pages,
- i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
- cache_level);
+ ppgtt->base.insert_entries(&ppgtt->base, obj->pages,
+ i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
+ cache_level);
}
void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
struct drm_i915_gem_object *obj)
{
- ppgtt->clear_range(ppgtt,
- i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
- obj->base.size >> PAGE_SHIFT);
+ ppgtt->base.clear_range(&ppgtt->base,
+ i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
+ obj->base.size >> PAGE_SHIFT);
}
extern int intel_iommu_gfx_mapped;
@@ -456,8 +461,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
struct drm_i915_gem_object *obj;
/* First fill our portion of the GTT with scratch pages */
- dev_priv->gtt.gtt_clear_range(dev, dev_priv->gtt.start / PAGE_SIZE,
- dev_priv->gtt.total / PAGE_SIZE);
+ dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
+ dev_priv->gtt.base.start / PAGE_SIZE,
+ dev_priv->gtt.base.total / PAGE_SIZE);
list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
i915_gem_clflush_object(obj);
@@ -486,12 +492,12 @@ int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
* within the global GTT as well as accessible by the GPU through the GMADR
* mapped BAR (dev_priv->mm.gtt->gtt).
*/
-static void gen6_ggtt_insert_entries(struct drm_device *dev,
+static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
struct sg_table *st,
unsigned int first_entry,
enum i915_cache_level level)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_private *dev_priv = vm->dev->dev_private;
gen6_gtt_pte_t __iomem *gtt_entries =
(gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry;
int i = 0;
@@ -500,8 +506,7 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
addr = sg_page_iter_dma_address(&sg_iter);
- iowrite32(dev_priv->gtt.pte_encode(addr, level),
- >t_entries[i]);
+ iowrite32(vm->pte_encode(addr, level), >t_entries[i]);
i++;
}
@@ -512,8 +517,8 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
* hardware should work, we must keep this posting read for paranoia.
*/
if (i != 0)
- WARN_ON(readl(>t_entries[i-1])
- != dev_priv->gtt.pte_encode(addr, level));
+ WARN_ON(readl(>t_entries[i-1]) !=
+ vm->pte_encode(addr, level));
/* This next bit makes the above posting read even more important. We
* want to flush the TLBs only after we're certain all the PTE updates
@@ -523,11 +528,11 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
POSTING_READ(GFX_FLSH_CNTL_GEN6);
}
-static void gen6_ggtt_clear_range(struct drm_device *dev,
+static void gen6_ggtt_clear_range(struct i915_address_space *vm,
unsigned int first_entry,
unsigned int num_entries)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_private *dev_priv = vm->dev->dev_private;
gen6_gtt_pte_t scratch_pte, __iomem *gtt_base =
(gen6_gtt_pte_t __iomem *) dev_priv->gtt.gsm + first_entry;
const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
@@ -538,15 +543,14 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
first_entry, num_entries, max_entries))
num_entries = max_entries;
- scratch_pte = dev_priv->gtt.pte_encode(dev_priv->gtt.scratch.addr,
- I915_CACHE_LLC);
+ scratch_pte = vm->pte_encode(vm->scratch.addr, I915_CACHE_LLC);
for (i = 0; i < num_entries; i++)
iowrite32(scratch_pte, >t_base[i]);
readl(gtt_base);
}
-static void i915_ggtt_insert_entries(struct drm_device *dev,
+static void i915_ggtt_insert_entries(struct i915_address_space *vm,
struct sg_table *st,
unsigned int pg_start,
enum i915_cache_level cache_level)
@@ -558,7 +562,7 @@ static void i915_ggtt_insert_entries(struct drm_device *dev,
}
-static void i915_ggtt_clear_range(struct drm_device *dev,
+static void i915_ggtt_clear_range(struct i915_address_space *vm,
unsigned int first_entry,
unsigned int num_entries)
{
@@ -571,10 +575,11 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
{
struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ const unsigned long entry = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT;
- dev_priv->gtt.gtt_insert_entries(dev, obj->pages,
- i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
- cache_level);
+ dev_priv->gtt.base.insert_entries(&dev_priv->gtt.base, obj->pages,
+ entry,
+ cache_level);
obj->has_global_gtt_mapping = 1;
}
@@ -583,10 +588,11 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
{
struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ const unsigned long entry = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT;
- dev_priv->gtt.gtt_clear_range(obj->base.dev,
- i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT,
- obj->base.size >> PAGE_SHIFT);
+ dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
+ entry,
+ obj->base.size >> PAGE_SHIFT);
obj->has_global_gtt_mapping = 0;
}
@@ -663,20 +669,23 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
obj->has_global_gtt_mapping = 1;
}
- dev_priv->gtt.start = start;
- dev_priv->gtt.total = end - start;
+ dev_priv->gtt.base.start = start;
+ dev_priv->gtt.base.total = end - start;
/* Clear any non-preallocated blocks */
drm_mm_for_each_hole(entry, &dev_priv->mm.gtt_space,
hole_start, hole_end) {
+ const unsigned long count = (hole_end - hole_start) / PAGE_SIZE;
DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
hole_start, hole_end);
- dev_priv->gtt.gtt_clear_range(dev, hole_start / PAGE_SIZE,
- (hole_end-hole_start) / PAGE_SIZE);
+ dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
+ hole_start / PAGE_SIZE,
+ count);
}
/* And finally clear the reserved guard page */
- dev_priv->gtt.gtt_clear_range(dev, end / PAGE_SIZE - 1, 1);
+ dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
+ end / PAGE_SIZE - 1, 1);
}
static bool
@@ -699,7 +708,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev->dev_private;
unsigned long gtt_size, mappable_size;
- gtt_size = dev_priv->gtt.total;
+ gtt_size = dev_priv->gtt.base.total;
mappable_size = dev_priv->gtt.mappable_end;
if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
@@ -744,8 +753,8 @@ static int setup_scratch_page(struct drm_device *dev)
#else
dma_addr = page_to_phys(page);
#endif
- dev_priv->gtt.scratch.page = page;
- dev_priv->gtt.scratch.addr = dma_addr;
+ dev_priv->gtt.base.scratch.page = page;
+ dev_priv->gtt.base.scratch.addr = dma_addr;
return 0;
}
@@ -753,11 +762,13 @@ static int setup_scratch_page(struct drm_device *dev)
static void teardown_scratch_page(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- set_pages_wb(dev_priv->gtt.scratch.page, 1);
- pci_unmap_page(dev->pdev, dev_priv->gtt.scratch.addr,
+ struct page *page = dev_priv->gtt.base.scratch.page;
+
+ set_pages_wb(page, 1);
+ pci_unmap_page(dev->pdev, dev_priv->gtt.base.scratch.addr,
PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
- put_page(dev_priv->gtt.scratch.page);
- __free_page(dev_priv->gtt.scratch.page);
+ put_page(page);
+ __free_page(page);
}
static inline unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl)
@@ -820,17 +831,18 @@ static int gen6_gmch_probe(struct drm_device *dev,
if (ret)
DRM_ERROR("Scratch setup failed\n");
- dev_priv->gtt.gtt_clear_range = gen6_ggtt_clear_range;
- dev_priv->gtt.gtt_insert_entries = gen6_ggtt_insert_entries;
+ dev_priv->gtt.base.clear_range = gen6_ggtt_clear_range;
+ dev_priv->gtt.base.insert_entries = gen6_ggtt_insert_entries;
return ret;
}
-static void gen6_gmch_remove(struct drm_device *dev)
+static void gen6_gmch_remove(struct i915_address_space *vm)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- iounmap(dev_priv->gtt.gsm);
- teardown_scratch_page(dev_priv->dev);
+
+ struct i915_gtt *gtt = container_of(vm, struct i915_gtt, base);
+ iounmap(gtt->gsm);
+ teardown_scratch_page(vm->dev);
}
static int i915_gmch_probe(struct drm_device *dev,
@@ -851,13 +863,13 @@ static int i915_gmch_probe(struct drm_device *dev,
intel_gtt_get(gtt_total, stolen, mappable_base, mappable_end);
dev_priv->gtt.do_idle_maps = needs_idle_maps(dev_priv->dev);
- dev_priv->gtt.gtt_clear_range = i915_ggtt_clear_range;
- dev_priv->gtt.gtt_insert_entries = i915_ggtt_insert_entries;
+ dev_priv->gtt.base.clear_range = i915_ggtt_clear_range;
+ dev_priv->gtt.base.insert_entries = i915_ggtt_insert_entries;
return 0;
}
-static void i915_gmch_remove(struct drm_device *dev)
+static void i915_gmch_remove(struct i915_address_space *vm)
{
intel_gmch_remove();
}
@@ -870,27 +882,30 @@ int i915_gem_gtt_init(struct drm_device *dev)
if (INTEL_INFO(dev)->gen <= 5) {
gtt->gtt_probe = i915_gmch_probe;
- gtt->gtt_remove = i915_gmch_remove;
+ gtt->base.cleanup = i915_gmch_remove;
} else {
gtt->gtt_probe = gen6_gmch_probe;
- gtt->gtt_remove = gen6_gmch_remove;
+ gtt->base.cleanup = gen6_gmch_remove;
if (IS_HASWELL(dev) && dev_priv->ellc_size)
- gtt->pte_encode = iris_pte_encode;
+ gtt->base.pte_encode = iris_pte_encode;
else if (IS_HASWELL(dev))
- gtt->pte_encode = hsw_pte_encode;
+ gtt->base.pte_encode = hsw_pte_encode;
else if (IS_VALLEYVIEW(dev))
- gtt->pte_encode = byt_pte_encode;
+ gtt->base.pte_encode = byt_pte_encode;
else
- gtt->pte_encode = gen6_pte_encode;
+ gtt->base.pte_encode = gen6_pte_encode;
}
- ret = gtt->gtt_probe(dev, >t->total, >t->stolen_size,
+ ret = gtt->gtt_probe(dev, >t->base.total, >t->stolen_size,
>t->mappable_base, >t->mappable_end);
if (ret)
return ret;
+ gtt->base.dev = dev;
+
/* GMADR is the PCI mmio aperture into the global GTT. */
- DRM_INFO("Memory usable by graphics device = %zdM\n", gtt->total >> 20);
+ DRM_INFO("Memory usable by graphics device = %zdM\n",
+ gtt->base.total >> 20);
DRM_DEBUG_DRIVER("GMADR size = %ldM\n", gtt->mappable_end >> 20);
DRM_DEBUG_DRIVER("GTT stolen size = %zdM\n", gtt->stolen_size >> 20);
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/5] [v2] drm/i915: Put the mm in the parent address space
2013-07-16 23:50 [PATCH 1/5] [v3] drm/i915: Move gtt and ppgtt under address space umbrella Ben Widawsky
@ 2013-07-16 23:50 ` Ben Widawsky
2013-07-16 23:50 ` [PATCH 3/5] [v2] drm/i915: Create a global list of vms Ben Widawsky
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Ben Widawsky @ 2013-07-16 23:50 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
Every address space should support object allocation. It therefore makes
sense to have the allocator be part of the "superclass" which GGTT and
PPGTT will derive.
Since our maximum address space size is only 2GB we're not yet able to
avoid doing allocation/eviction; but we'd hope one day this becomes
almost irrelvant.
v2: Rebased
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_dma.c | 4 ++--
drivers/gpu/drm/i915/i915_drv.h | 3 +--
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_evict.c | 10 +++++-----
drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++------
drivers/gpu/drm/i915/i915_gem_stolen.c | 4 ++--
6 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5fab2f..5dd4fa5 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1358,7 +1358,7 @@ cleanup_gem:
i915_gem_context_fini(dev);
mutex_unlock(&dev->struct_mutex);
i915_gem_cleanup_aliasing_ppgtt(dev);
- drm_mm_takedown(&dev_priv->mm.gtt_space);
+ drm_mm_takedown(&dev_priv->gtt.base.mm);
cleanup_irq:
drm_irq_uninstall(dev);
cleanup_gem_stolen:
@@ -1758,7 +1758,7 @@ int i915_driver_unload(struct drm_device *dev)
i915_free_hws(dev);
}
- drm_mm_takedown(&dev_priv->mm.gtt_space);
+ drm_mm_takedown(&dev_priv->gtt.base.mm);
if (dev_priv->regs != NULL)
pci_iounmap(dev->pdev, dev_priv->regs);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4a465e1..680eac7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -447,6 +447,7 @@ enum i915_cache_level {
typedef uint32_t gen6_gtt_pte_t;
struct i915_address_space {
+ struct drm_mm mm;
struct drm_device *dev;
unsigned long start; /* Start offset always 0 for dri2 */
size_t total; /* size addr space maps (ex. 2GB for ggtt) */
@@ -831,8 +832,6 @@ struct intel_l3_parity {
struct i915_gem_mm {
/** Memory allocator for GTT stolen memory */
struct drm_mm stolen;
- /** Memory allocator for GTT */
- struct drm_mm gtt_space;
/** List of all objects in gtt_space. Used to restore gtt
* mappings on resume */
struct list_head bound_list;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f29cf23..093fa76 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3093,7 +3093,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
i915_gem_object_pin_pages(obj);
search_free:
- ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space,
+ ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
&obj->gtt_space,
size, alignment,
obj->cache_level, 0, gtt_max);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 5f8afc4..f1c9ab0 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -78,12 +78,12 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
INIT_LIST_HEAD(&unwind_list);
if (mappable)
- drm_mm_init_scan_with_range(&dev_priv->mm.gtt_space,
- min_size, alignment, cache_level,
- 0, dev_priv->gtt.mappable_end);
+ drm_mm_init_scan_with_range(&dev_priv->gtt.base.mm, min_size,
+ alignment, cache_level, 0,
+ dev_priv->gtt.mappable_end);
else
- drm_mm_init_scan(&dev_priv->mm.gtt_space,
- min_size, alignment, cache_level);
+ drm_mm_init_scan(&dev_priv->gtt.base.mm, min_size, alignment,
+ cache_level);
/* First see if there is a large enough contiguous idle region... */
list_for_each_entry(obj, &dev_priv->mm.inactive_list, mm_list) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f982bf0..999ecfe 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -269,6 +269,8 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
container_of(vm, struct i915_hw_ppgtt, base);
int i;
+ drm_mm_takedown(&ppgtt->base.mm);
+
if (ppgtt->pt_dma_addr) {
for (i = 0; i < ppgtt->num_pd_entries; i++)
pci_unmap_page(ppgtt->base.dev->pdev,
@@ -382,8 +384,11 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
if (ret)
kfree(ppgtt);
- else
+ else {
dev_priv->mm.aliasing_ppgtt = ppgtt;
+ drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
+ ppgtt->base.total);
+ }
return ret;
}
@@ -651,9 +656,9 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
BUG_ON(mappable_end > end);
/* Subtract the guard page ... */
- drm_mm_init(&dev_priv->mm.gtt_space, start, end - start - PAGE_SIZE);
+ drm_mm_init(&dev_priv->gtt.base.mm, start, end - start - PAGE_SIZE);
if (!HAS_LLC(dev))
- dev_priv->mm.gtt_space.color_adjust = i915_gtt_color_adjust;
+ dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
/* Mark any preallocated objects as occupied */
list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
@@ -662,7 +667,7 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
i915_gem_obj_ggtt_offset(obj), obj->base.size);
WARN_ON(i915_gem_obj_ggtt_bound(obj));
- ret = drm_mm_reserve_node(&dev_priv->mm.gtt_space,
+ ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
&obj->gtt_space);
if (ret)
DRM_DEBUG_KMS("Reservation failed\n");
@@ -673,7 +678,7 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
dev_priv->gtt.base.total = end - start;
/* Clear any non-preallocated blocks */
- drm_mm_for_each_hole(entry, &dev_priv->mm.gtt_space,
+ drm_mm_for_each_hole(entry, &dev_priv->gtt.base.mm,
hole_start, hole_end) {
const unsigned long count = (hole_end - hole_start) / PAGE_SIZE;
DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
@@ -727,7 +732,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
return;
DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
- drm_mm_takedown(&dev_priv->mm.gtt_space);
+ drm_mm_takedown(&dev_priv->gtt.base.mm);
gtt_size += GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE;
}
i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 5c1a535..ede8c41 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -399,8 +399,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
*/
obj->gtt_space.start = gtt_offset;
obj->gtt_space.size = size;
- if (drm_mm_initialized(&dev_priv->mm.gtt_space)) {
- ret = drm_mm_reserve_node(&dev_priv->mm.gtt_space,
+ if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
+ ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
&obj->gtt_space);
if (ret) {
DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/5] [v2] drm/i915: Create a global list of vms
2013-07-16 23:50 [PATCH 1/5] [v3] drm/i915: Move gtt and ppgtt under address space umbrella Ben Widawsky
2013-07-16 23:50 ` [PATCH 2/5] [v2] drm/i915: Put the mm in the parent address space Ben Widawsky
@ 2013-07-16 23:50 ` Ben Widawsky
2013-07-16 23:50 ` [PATCH 4/5] [v2] drm/i915: Move active/inactive lists to new mm Ben Widawsky
2013-07-16 23:50 ` [PATCH 5/5] [v5] drm/i915: Create VMAs Ben Widawsky
3 siblings, 0 replies; 15+ messages in thread
From: Ben Widawsky @ 2013-07-16 23:50 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
After we plumb our code to support multiple address spaces (VMs), there
are a few situations where we want to be able to traverse the list of
all address spaces in the system. Cases like eviction, or error state
collection are obvious example.
v2: Delete the global link instead of the list head. While this in and
of itself shouldn't be really be a problem, doing this allows us to WARN
on an non-empty list, which is a problem. (Daniel)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_dma.c | 6 ++++++
drivers/gpu/drm/i915/i915_drv.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5dd4fa5..fd52de7 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1492,6 +1492,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
i915_dump_device_info(dev_priv);
+ INIT_LIST_HEAD(&dev_priv->vm_list);
+ INIT_LIST_HEAD(&dev_priv->gtt.base.global_link);
+ list_add(&dev_priv->gtt.base.global_link, &dev_priv->vm_list);
+
if (i915_get_bridge_dev(dev)) {
ret = -EIO;
goto free_priv;
@@ -1758,6 +1762,8 @@ int i915_driver_unload(struct drm_device *dev)
i915_free_hws(dev);
}
+ list_del(&dev_priv->gtt.base.global_link);
+ WARN_ON(!list_empty(&dev_priv->vm_list));
drm_mm_takedown(&dev_priv->gtt.base.mm);
if (dev_priv->regs != NULL)
pci_iounmap(dev->pdev, dev_priv->regs);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 680eac7..c44e6c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -449,6 +449,7 @@ typedef uint32_t gen6_gtt_pte_t;
struct i915_address_space {
struct drm_mm mm;
struct drm_device *dev;
+ struct list_head global_link;
unsigned long start; /* Start offset always 0 for dri2 */
size_t total; /* size addr space maps (ex. 2GB for ggtt) */
@@ -1120,6 +1121,7 @@ typedef struct drm_i915_private {
enum modeset_restore modeset_restore;
struct mutex modeset_restore_lock;
+ struct list_head vm_list; /* Global list of all address spaces */
struct i915_gtt gtt; /* VMA representing the global address space */
struct i915_gem_mm mm;
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/5] [v2] drm/i915: Move active/inactive lists to new mm
2013-07-16 23:50 [PATCH 1/5] [v3] drm/i915: Move gtt and ppgtt under address space umbrella Ben Widawsky
2013-07-16 23:50 ` [PATCH 2/5] [v2] drm/i915: Put the mm in the parent address space Ben Widawsky
2013-07-16 23:50 ` [PATCH 3/5] [v2] drm/i915: Create a global list of vms Ben Widawsky
@ 2013-07-16 23:50 ` Ben Widawsky
2013-07-16 23:50 ` [PATCH 5/5] [v5] drm/i915: Create VMAs Ben Widawsky
3 siblings, 0 replies; 15+ messages in thread
From: Ben Widawsky @ 2013-07-16 23:50 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
Shamelessly manipulated out of Daniel :-)
"When moving the lists around explain that the active/inactive stuff is
used by eviction when we run out of address space, so needs to be
per-vma and per-address space. Bound/unbound otoh is used by the
shrinker which only cares about the amount of memory used and not one
bit about in which address space this memory is all used in. Of course
to actual kick out an object we need to unbind it from every address
space, but for that we have the per-object list of vmas."
v2: Leave the bound list as a global one. (Chris, indirectly)
v3: Rebased with no i915_gtt_vm. In most places I added a new *vm local,
since it will eventually be replaces by a vm argument.
Put comment back inline, since it no longer makes sense to do otherwise.
v4: Rebased on hangcheck/error state movement
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_debugfs.c | 16 +++++++-----
drivers/gpu/drm/i915/i915_drv.h | 46 +++++++++++++++++-----------------
drivers/gpu/drm/i915/i915_gem.c | 33 ++++++++++++------------
drivers/gpu/drm/i915/i915_gem_debug.c | 2 +-
drivers/gpu/drm/i915/i915_gem_evict.c | 18 ++++++-------
drivers/gpu/drm/i915/i915_gem_stolen.c | 3 ++-
drivers/gpu/drm/i915/i915_gpu_error.c | 8 +++---
7 files changed, 67 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1c697c0..a9246e9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -135,7 +135,8 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data)
uintptr_t list = (uintptr_t) node->info_ent->data;
struct list_head *head;
struct drm_device *dev = node->minor->dev;
- drm_i915_private_t *dev_priv = dev->dev_private;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
size_t total_obj_size, total_gtt_size;
int count, ret;
@@ -147,11 +148,11 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data)
switch (list) {
case ACTIVE_LIST:
seq_puts(m, "Active:\n");
- head = &dev_priv->mm.active_list;
+ head = &vm->active_list;
break;
case INACTIVE_LIST:
seq_puts(m, "Inactive:\n");
- head = &dev_priv->mm.inactive_list;
+ head = &vm->inactive_list;
break;
default:
mutex_unlock(&dev->struct_mutex);
@@ -219,6 +220,7 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
u32 count, mappable_count, purgeable_count;
size_t size, mappable_size, purgeable_size;
struct drm_i915_gem_object *obj;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_file *file;
int ret;
@@ -236,12 +238,12 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
count, mappable_count, size, mappable_size);
size = count = mappable_size = mappable_count = 0;
- count_objects(&dev_priv->mm.active_list, mm_list);
+ count_objects(&vm->active_list, mm_list);
seq_printf(m, " %u [%u] active objects, %zu [%zu] bytes\n",
count, mappable_count, size, mappable_size);
size = count = mappable_size = mappable_count = 0;
- count_objects(&dev_priv->mm.inactive_list, mm_list);
+ count_objects(&vm->inactive_list, mm_list);
seq_printf(m, " %u [%u] inactive objects, %zu [%zu] bytes\n",
count, mappable_count, size, mappable_size);
@@ -1625,6 +1627,7 @@ i915_drop_caches_set(void *data, u64 val)
struct drm_device *dev = data;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_gem_object *obj, *next;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
int ret;
DRM_DEBUG_DRIVER("Dropping caches: 0x%08llx\n", val);
@@ -1645,7 +1648,8 @@ i915_drop_caches_set(void *data, u64 val)
i915_gem_retire_requests(dev);
if (val & DROP_BOUND) {
- list_for_each_entry_safe(obj, next, &dev_priv->mm.inactive_list, mm_list)
+ list_for_each_entry_safe(obj, next, &vm->inactive_list,
+ mm_list)
if (obj->pin_count == 0) {
ret = i915_gem_object_unbind(obj);
if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c44e6c0..b3ba428 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -458,6 +458,29 @@ struct i915_address_space {
struct page *page;
} scratch;
+ /**
+ * List of objects currently involved in rendering.
+ *
+ * Includes buffers having the contents of their GPU caches
+ * flushed, not necessarily primitives. last_rendering_seqno
+ * represents when the rendering involved will be completed.
+ *
+ * A reference is held on the buffer while on this list.
+ */
+ struct list_head active_list;
+
+ /**
+ * LRU list of objects which are not in the ringbuffer and
+ * are ready to unbind, but are still in the GTT.
+ *
+ * last_rendering_seqno is 0 while an object is in this list.
+ *
+ * A reference is not held on the buffer while on this list,
+ * as merely being GTT-bound shouldn't prevent its being
+ * freed, and we'll pull it off the list in the free path.
+ */
+ struct list_head inactive_list;
+
/* FIXME: Need a more generic return type */
gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
enum i915_cache_level level);
@@ -852,29 +875,6 @@ struct i915_gem_mm {
struct shrinker inactive_shrinker;
bool shrinker_no_lock_stealing;
- /**
- * List of objects currently involved in rendering.
- *
- * Includes buffers having the contents of their GPU caches
- * flushed, not necessarily primitives. last_rendering_seqno
- * represents when the rendering involved will be completed.
- *
- * A reference is held on the buffer while on this list.
- */
- struct list_head active_list;
-
- /**
- * LRU list of objects which are not in the ringbuffer and
- * are ready to unbind, but are still in the GTT.
- *
- * last_rendering_seqno is 0 while an object is in this list.
- *
- * A reference is not held on the buffer while on this list,
- * as merely being GTT-bound shouldn't prevent its being
- * freed, and we'll pull it off the list in the free path.
- */
- struct list_head inactive_list;
-
/** LRU list of objects with fence regs on them. */
struct list_head fence_list;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 093fa76..812275a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1692,6 +1692,7 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
bool purgeable_only)
{
struct drm_i915_gem_object *obj, *next;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
long count = 0;
list_for_each_entry_safe(obj, next,
@@ -1705,9 +1706,7 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
}
}
- list_for_each_entry_safe(obj, next,
- &dev_priv->mm.inactive_list,
- mm_list) {
+ list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) {
if ((i915_gem_object_is_purgeable(obj) || !purgeable_only) &&
i915_gem_object_unbind(obj) == 0 &&
i915_gem_object_put_pages(obj) == 0) {
@@ -1878,6 +1877,7 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
{
struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
u32 seqno = intel_ring_get_seqno(ring);
BUG_ON(ring == NULL);
@@ -1894,7 +1894,7 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
}
/* Move from whatever list we were on to the tail of execution. */
- list_move_tail(&obj->mm_list, &dev_priv->mm.active_list);
+ list_move_tail(&obj->mm_list, &vm->active_list);
list_move_tail(&obj->ring_list, &ring->active_list);
obj->last_read_seqno = seqno;
@@ -1918,11 +1918,12 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
{
struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
BUG_ON(!obj->active);
- list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
+ list_move_tail(&obj->mm_list, &vm->inactive_list);
list_del_init(&obj->ring_list);
obj->ring = NULL;
@@ -2264,6 +2265,7 @@ void i915_gem_restore_fences(struct drm_device *dev)
void i915_gem_reset(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
struct intel_ring_buffer *ring;
int i;
@@ -2274,12 +2276,8 @@ void i915_gem_reset(struct drm_device *dev)
/* Move everything out of the GPU domains to ensure we do any
* necessary invalidation upon reuse.
*/
- list_for_each_entry(obj,
- &dev_priv->mm.inactive_list,
- mm_list)
- {
+ list_for_each_entry(obj, &vm->inactive_list, mm_list)
obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
- }
i915_gem_restore_fences(dev);
}
@@ -3048,6 +3046,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
{
struct drm_device *dev = obj->base.dev;
drm_i915_private_t *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
u32 size, fence_size, fence_alignment, unfenced_alignment;
bool mappable, fenceable;
size_t gtt_max = map_and_fenceable ?
@@ -3123,7 +3122,7 @@ search_free:
}
list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
- list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
+ list_add_tail(&obj->mm_list, &vm->inactive_list);
fenceable =
i915_gem_obj_ggtt_size(obj) == fence_size &&
@@ -3271,7 +3270,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
/* And bump the LRU for this access */
if (i915_gem_object_is_inactive(obj))
- list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
+ list_move_tail(&obj->mm_list,
+ &dev_priv->gtt.base.inactive_list);
return 0;
}
@@ -4212,7 +4212,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
return ret;
}
- BUG_ON(!list_empty(&dev_priv->mm.active_list));
+ BUG_ON(!list_empty(&dev_priv->gtt.base.active_list));
mutex_unlock(&dev->struct_mutex);
ret = drm_irq_install(dev);
@@ -4290,8 +4290,8 @@ i915_gem_load(struct drm_device *dev)
SLAB_HWCACHE_ALIGN,
NULL);
- INIT_LIST_HEAD(&dev_priv->mm.active_list);
- INIT_LIST_HEAD(&dev_priv->mm.inactive_list);
+ INIT_LIST_HEAD(&dev_priv->gtt.base.active_list);
+ INIT_LIST_HEAD(&dev_priv->gtt.base.inactive_list);
INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
INIT_LIST_HEAD(&dev_priv->mm.bound_list);
INIT_LIST_HEAD(&dev_priv->mm.fence_list);
@@ -4562,6 +4562,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
struct drm_i915_private,
mm.inactive_shrinker);
struct drm_device *dev = dev_priv->dev;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
int nr_to_scan = sc->nr_to_scan;
bool unlock = true;
@@ -4590,7 +4591,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
if (obj->pages_pin_count == 0)
cnt += obj->base.size >> PAGE_SHIFT;
- list_for_each_entry(obj, &dev_priv->mm.inactive_list, mm_list)
+ list_for_each_entry(obj, &vm->inactive_list, mm_list)
if (obj->pin_count == 0 && obj->pages_pin_count == 0)
cnt += obj->base.size >> PAGE_SHIFT;
diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c b/drivers/gpu/drm/i915/i915_gem_debug.c
index 582e6a5..bf945a3 100644
--- a/drivers/gpu/drm/i915/i915_gem_debug.c
+++ b/drivers/gpu/drm/i915/i915_gem_debug.c
@@ -97,7 +97,7 @@ i915_verify_lists(struct drm_device *dev)
}
}
- list_for_each_entry(obj, &dev_priv->mm.inactive_list, list) {
+ list_for_each_entry(obj, &i915_gtt_vm->inactive_list, list) {
if (obj->base.dev != dev ||
!atomic_read(&obj->base.refcount.refcount)) {
DRM_ERROR("freed inactive %p\n", obj);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index f1c9ab0..43b8235 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -47,6 +47,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
bool mappable, bool nonblocking)
{
drm_i915_private_t *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct list_head eviction_list, unwind_list;
struct drm_i915_gem_object *obj;
int ret = 0;
@@ -78,15 +79,14 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
INIT_LIST_HEAD(&unwind_list);
if (mappable)
- drm_mm_init_scan_with_range(&dev_priv->gtt.base.mm, min_size,
+ drm_mm_init_scan_with_range(&vm->mm, min_size,
alignment, cache_level, 0,
dev_priv->gtt.mappable_end);
else
- drm_mm_init_scan(&dev_priv->gtt.base.mm, min_size, alignment,
- cache_level);
+ drm_mm_init_scan(&vm->mm, min_size, alignment, cache_level);
/* First see if there is a large enough contiguous idle region... */
- list_for_each_entry(obj, &dev_priv->mm.inactive_list, mm_list) {
+ list_for_each_entry(obj, &vm->inactive_list, mm_list) {
if (mark_free(obj, &unwind_list))
goto found;
}
@@ -95,7 +95,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
goto none;
/* Now merge in the soon-to-be-expired objects... */
- list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) {
+ list_for_each_entry(obj, &vm->active_list, mm_list) {
if (mark_free(obj, &unwind_list))
goto found;
}
@@ -154,12 +154,13 @@ int
i915_gem_evict_everything(struct drm_device *dev)
{
drm_i915_private_t *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj, *next;
bool lists_empty;
int ret;
- lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
- list_empty(&dev_priv->mm.active_list));
+ lists_empty = (list_empty(&vm->inactive_list) &&
+ list_empty(&vm->active_list));
if (lists_empty)
return -ENOSPC;
@@ -176,8 +177,7 @@ i915_gem_evict_everything(struct drm_device *dev)
i915_gem_retire_requests(dev);
/* Having flushed everything, unbind() should never raise an error */
- list_for_each_entry_safe(obj, next,
- &dev_priv->mm.inactive_list, mm_list)
+ list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list)
if (obj->pin_count == 0)
WARN_ON(i915_gem_object_unbind(obj));
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index ede8c41..46a9715 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -351,6 +351,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
u32 size)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
struct drm_mm_node *stolen;
int ret;
@@ -411,7 +412,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
obj->has_global_gtt_mapping = 1;
list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
- list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
+ list_add_tail(&obj->mm_list, &vm->inactive_list);
return obj;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 58386ce..d970d84 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -622,6 +622,7 @@ static struct drm_i915_error_object *
i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
struct intel_ring_buffer *ring)
{
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
u32 seqno;
@@ -641,7 +642,7 @@ i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
}
seqno = ring->get_seqno(ring, false);
- list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list) {
+ list_for_each_entry(obj, &vm->active_list, mm_list) {
if (obj->ring != ring)
continue;
@@ -773,11 +774,12 @@ static void i915_gem_record_rings(struct drm_device *dev,
static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
struct drm_i915_error_state *error)
{
+ struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
int i;
i = 0;
- list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list)
+ list_for_each_entry(obj, &vm->active_list, mm_list)
i++;
error->active_bo_count = i;
list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
@@ -797,7 +799,7 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
error->active_bo_count =
capture_active_bo(error->active_bo,
error->active_bo_count,
- &dev_priv->mm.active_list);
+ &vm->active_list);
if (error->pinned_bo)
error->pinned_bo_count =
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/5] [v5] drm/i915: Create VMAs
2013-07-16 23:50 [PATCH 1/5] [v3] drm/i915: Move gtt and ppgtt under address space umbrella Ben Widawsky
` (2 preceding siblings ...)
2013-07-16 23:50 ` [PATCH 4/5] [v2] drm/i915: Move active/inactive lists to new mm Ben Widawsky
@ 2013-07-16 23:50 ` Ben Widawsky
2013-07-17 14:43 ` Imre Deak
2013-07-17 19:19 ` [PATCH 5/6] drm/i915: Free stolen node on failed preallocation Ben Widawsky
3 siblings, 2 replies; 15+ messages in thread
From: Ben Widawsky @ 2013-07-16 23:50 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
Formerly: "drm/i915: Create VMAs (part 1)"
In a previous patch, the notion of a VM was introduced. A VMA describes
an area of part of the VM address space. A VMA is similar to the concept
in the linux mm. However, instead of representing regular memory, a VMA
is backed by a GEM BO. There may be many VMAs for a given object, one
for each VM the object is to be used in. This may occur through flink,
dma-buf, or a number of other transient states.
Currently the code depends on only 1 VMA per object, for the global GTT
(and aliasing PPGTT). The following patches will address this and make
the rest of the infrastructure more suited
v2: s/i915_obj/i915_gem_obj (Chris)
v3: Only move an object to the now global unbound list if there are no
more VMAs for the object which are bound into a VM (ie. the list is
empty).
v4: killed obj->gtt_space
some reworks due to rebase
v5: Free vma on error path (Imre)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_drv.h | 48 ++++++++++++++++++-----
drivers/gpu/drm/i915/i915_gem.c | 71 +++++++++++++++++++++++++++-------
drivers/gpu/drm/i915/i915_gem_evict.c | 12 ++++--
drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++-
drivers/gpu/drm/i915/i915_gem_stolen.c | 14 +++++--
5 files changed, 118 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b3ba428..1a32412 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
int (*enable)(struct drm_device *dev);
};
+/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime
+ * will always be <= an objects lifetime. So object refcounting should cover us.
+ */
+struct i915_vma {
+ struct drm_mm_node node;
+ struct drm_i915_gem_object *obj;
+ struct i915_address_space *vm;
+
+ struct list_head vma_link; /* Link in the object's VMA list */
+};
+
struct i915_ctx_hang_stats {
/* This context had batch pending when hang was declared */
unsigned batch_pending;
@@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
const struct drm_i915_gem_object_ops *ops;
- /** Current space allocated to this object in the GTT, if any. */
- struct drm_mm_node gtt_space;
+ /** List of VMAs backed by this object */
+ struct list_head vma_list;
+
/** Stolen memory for this object, instead of being backed by shmem. */
struct drm_mm_node *stolen;
struct list_head global_list;
@@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
-/* Offset of the first PTE pointing to this object */
-static inline unsigned long
-i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
+/* This is a temporary define to help transition us to real VMAs. If you see
+ * this, you're either reviewing code, or bisecting it. */
+static inline struct i915_vma *
+__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
{
- return o->gtt_space.start;
+ if (list_empty(&obj->vma_list))
+ return NULL;
+ return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
}
/* Whether or not this object is currently mapped by the translation tables */
static inline bool
i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
{
- return drm_mm_node_allocated(&o->gtt_space);
+ struct i915_vma *vma = __i915_gem_obj_to_vma(o);
+ if (vma == NULL)
+ return false;
+ return drm_mm_node_allocated(&vma->node);
+}
+
+/* Offset of the first PTE pointing to this object */
+static inline unsigned long
+i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
+{
+ BUG_ON(list_empty(&o->vma_list));
+ return __i915_gem_obj_to_vma(o)->node.start;
}
/* The size used in the translation tables may be larger than the actual size of
@@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
static inline unsigned long
i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
{
- return o->gtt_space.size;
+ BUG_ON(list_empty(&o->vma_list));
+ return __i915_gem_obj_to_vma(o)->node.size;
}
static inline void
i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
enum i915_cache_level color)
{
- o->gtt_space.color = color;
+ __i915_gem_obj_to_vma(o)->node.color = color;
}
/**
@@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
size_t size);
void i915_gem_free_object(struct drm_gem_object *obj);
+struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm);
+void i915_gem_vma_destroy(struct i915_vma *vma);
int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
uint32_t alignment,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 812275a..3eb12ff 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2580,6 +2580,7 @@ int
i915_gem_object_unbind(struct drm_i915_gem_object *obj)
{
drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
+ struct i915_vma *vma;
int ret;
if (!i915_gem_obj_ggtt_bound(obj))
@@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
i915_gem_object_unpin_pages(obj);
list_del(&obj->mm_list);
- list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
/* Avoid an unnecessary call to unbind on rebind. */
obj->map_and_fenceable = true;
- drm_mm_remove_node(&obj->gtt_space);
+ vma = __i915_gem_obj_to_vma(obj);
+ list_del(&vma->vma_link);
+ drm_mm_remove_node(&vma->node);
+ i915_gem_vma_destroy(vma);
+
+ /* Since the unbound list is global, only move to that list if
+ * no more VMAs exist.
+ * NB: Until we have real VMAs there will only ever be one */
+ WARN_ON(!list_empty(&obj->vma_list));
+ if (list_empty(&obj->vma_list))
+ list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
return 0;
}
@@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
bool mappable, fenceable;
size_t gtt_max = map_and_fenceable ?
dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
+ struct i915_vma *vma;
int ret;
+ if (WARN_ON(!list_empty(&obj->vma_list)))
+ return -EBUSY;
+
fence_size = i915_gem_get_gtt_size(dev,
obj->base.size,
obj->tiling_mode);
@@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
i915_gem_object_pin_pages(obj);
+ vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
+ if (vma == NULL) {
+ i915_gem_object_unpin_pages(obj);
+ return -ENOMEM;
+ }
+
search_free:
ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
- &obj->gtt_space,
+ &vma->node,
size, alignment,
obj->cache_level, 0, gtt_max);
if (ret) {
@@ -3107,22 +3127,19 @@ search_free:
i915_gem_object_unpin_pages(obj);
return ret;
}
- if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space,
+ if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
obj->cache_level))) {
- i915_gem_object_unpin_pages(obj);
- drm_mm_remove_node(&obj->gtt_space);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_out;
}
ret = i915_gem_gtt_prepare_object(obj);
- if (ret) {
- i915_gem_object_unpin_pages(obj);
- drm_mm_remove_node(&obj->gtt_space);
- return ret;
- }
+ if (ret)
+ goto err_out;
list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
list_add_tail(&obj->mm_list, &vm->inactive_list);
+ list_add(&vma->vma_link, &obj->vma_list);
fenceable =
i915_gem_obj_ggtt_size(obj) == fence_size &&
@@ -3136,6 +3153,12 @@ search_free:
trace_i915_gem_object_bind(obj, map_and_fenceable);
i915_gem_verify_gtt(dev);
return 0;
+
+err_out:
+ i915_gem_vma_destroy(vma);
+ i915_gem_object_unpin_pages(obj);
+ drm_mm_remove_node(&vma->node);
+ return ret;
}
void
@@ -3281,6 +3304,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
{
struct drm_device *dev = obj->base.dev;
drm_i915_private_t *dev_priv = dev->dev_private;
+ struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
int ret;
if (obj->cache_level == cache_level)
@@ -3291,7 +3315,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
return -EBUSY;
}
- if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) {
+ if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
ret = i915_gem_object_unbind(obj);
if (ret)
return ret;
@@ -3836,6 +3860,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
INIT_LIST_HEAD(&obj->global_list);
INIT_LIST_HEAD(&obj->ring_list);
INIT_LIST_HEAD(&obj->exec_list);
+ INIT_LIST_HEAD(&obj->vma_list);
obj->ops = ops;
@@ -3956,6 +3981,26 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
i915_gem_object_free(obj);
}
+struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm)
+{
+ struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
+ if (vma == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&vma->vma_link);
+ vma->vm = vm;
+ vma->obj = obj;
+
+ return vma;
+}
+
+void i915_gem_vma_destroy(struct i915_vma *vma)
+{
+ WARN_ON(vma->node.allocated);
+ kfree(vma);
+}
+
int
i915_gem_idle(struct drm_device *dev)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 43b8235..df61f33 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -34,11 +34,13 @@
static bool
mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
{
+ struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
+
if (obj->pin_count)
return false;
list_add(&obj->exec_list, unwind);
- return drm_mm_scan_add_block(&obj->gtt_space);
+ return drm_mm_scan_add_block(&vma->node);
}
int
@@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
drm_i915_private_t *dev_priv = dev->dev_private;
struct i915_address_space *vm = &dev_priv->gtt.base;
struct list_head eviction_list, unwind_list;
+ struct i915_vma *vma;
struct drm_i915_gem_object *obj;
int ret = 0;
@@ -106,8 +109,8 @@ none:
obj = list_first_entry(&unwind_list,
struct drm_i915_gem_object,
exec_list);
-
- ret = drm_mm_scan_remove_block(&obj->gtt_space);
+ vma = __i915_gem_obj_to_vma(obj);
+ ret = drm_mm_scan_remove_block(&vma->node);
BUG_ON(ret);
list_del_init(&obj->exec_list);
@@ -127,7 +130,8 @@ found:
obj = list_first_entry(&unwind_list,
struct drm_i915_gem_object,
exec_list);
- if (drm_mm_scan_remove_block(&obj->gtt_space)) {
+ vma = __i915_gem_obj_to_vma(obj);
+ if (drm_mm_scan_remove_block(&vma->node)) {
list_move(&obj->exec_list, &eviction_list);
drm_gem_object_reference(&obj->base);
continue;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 999ecfe..3b639a9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
/* Mark any preallocated objects as occupied */
list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+ struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
int ret;
DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
i915_gem_obj_ggtt_offset(obj), obj->base.size);
WARN_ON(i915_gem_obj_ggtt_bound(obj));
- ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
- &obj->gtt_space);
+ ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
if (ret)
DRM_DEBUG_KMS("Reservation failed\n");
obj->has_global_gtt_mapping = 1;
+ list_add(&vma->vma_link, &obj->vma_list);
}
dev_priv->gtt.base.start = start;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 46a9715..a1f2308 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
struct drm_mm_node *stolen;
+ struct i915_vma *vma;
int ret;
if (!drm_mm_initialized(&dev_priv->mm.stolen))
@@ -393,16 +394,21 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
if (gtt_offset == I915_GTT_OFFSET_NONE)
return obj;
+ vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
+ if (!vma) {
+ drm_gem_object_unreference(&obj->base);
+ return NULL;
+ }
+
/* To simplify the initialisation sequence between KMS and GTT,
* we allow construction of the stolen object prior to
* setting up the GTT space. The actual reservation will occur
* later.
*/
- obj->gtt_space.start = gtt_offset;
- obj->gtt_space.size = size;
+ vma->node.start = gtt_offset;
+ vma->node.size = size;
if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
- ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
- &obj->gtt_space);
+ ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
if (ret) {
DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
goto unref_out;
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] [v5] drm/i915: Create VMAs
2013-07-16 23:50 ` [PATCH 5/5] [v5] drm/i915: Create VMAs Ben Widawsky
@ 2013-07-17 14:43 ` Imre Deak
2013-07-17 19:19 ` [PATCH 5/6] drm/i915: Free stolen node on failed preallocation Ben Widawsky
1 sibling, 0 replies; 15+ messages in thread
From: Imre Deak @ 2013-07-17 14:43 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX
[-- Attachment #1.1: Type: text/plain, Size: 14765 bytes --]
On Tue, 2013-07-16 at 16:50 -0700, Ben Widawsky wrote:
> Formerly: "drm/i915: Create VMAs (part 1)"
>
> In a previous patch, the notion of a VM was introduced. A VMA describes
> an area of part of the VM address space. A VMA is similar to the concept
> in the linux mm. However, instead of representing regular memory, a VMA
> is backed by a GEM BO. There may be many VMAs for a given object, one
> for each VM the object is to be used in. This may occur through flink,
> dma-buf, or a number of other transient states.
>
> Currently the code depends on only 1 VMA per object, for the global GTT
> (and aliasing PPGTT). The following patches will address this and make
> the rest of the infrastructure more suited
>
> v2: s/i915_obj/i915_gem_obj (Chris)
>
> v3: Only move an object to the now global unbound list if there are no
> more VMAs for the object which are bound into a VM (ie. the list is
> empty).
>
> v4: killed obj->gtt_space
> some reworks due to rebase
>
> v5: Free vma on error path (Imre)
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 48 ++++++++++++++++++-----
> drivers/gpu/drm/i915/i915_gem.c | 71 +++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/i915_gem_evict.c | 12 ++++--
> drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++-
> drivers/gpu/drm/i915/i915_gem_stolen.c | 14 +++++--
> 5 files changed, 118 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3ba428..1a32412 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
> int (*enable)(struct drm_device *dev);
> };
>
> +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime
> + * will always be <= an objects lifetime. So object refcounting should cover us.
> + */
> +struct i915_vma {
> + struct drm_mm_node node;
> + struct drm_i915_gem_object *obj;
> + struct i915_address_space *vm;
> +
> + struct list_head vma_link; /* Link in the object's VMA list */
> +};
> +
> struct i915_ctx_hang_stats {
> /* This context had batch pending when hang was declared */
> unsigned batch_pending;
> @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
>
> const struct drm_i915_gem_object_ops *ops;
>
> - /** Current space allocated to this object in the GTT, if any. */
> - struct drm_mm_node gtt_space;
> + /** List of VMAs backed by this object */
> + struct list_head vma_list;
> +
> /** Stolen memory for this object, instead of being backed by shmem. */
> struct drm_mm_node *stolen;
> struct list_head global_list;
> @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
>
> #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
>
> -/* Offset of the first PTE pointing to this object */
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +/* This is a temporary define to help transition us to real VMAs. If you see
> + * this, you're either reviewing code, or bisecting it. */
> +static inline struct i915_vma *
> +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
> {
> - return o->gtt_space.start;
> + if (list_empty(&obj->vma_list))
> + return NULL;
> + return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
> }
>
> /* Whether or not this object is currently mapped by the translation tables */
> static inline bool
> i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> {
> - return drm_mm_node_allocated(&o->gtt_space);
> + struct i915_vma *vma = __i915_gem_obj_to_vma(o);
> + if (vma == NULL)
> + return false;
> + return drm_mm_node_allocated(&vma->node);
> +}
> +
> +/* Offset of the first PTE pointing to this object */
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +{
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.start;
> }
>
> /* The size used in the translation tables may be larger than the actual size of
> @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> static inline unsigned long
> i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
> {
> - return o->gtt_space.size;
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.size;
> }
>
> static inline void
> i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
> enum i915_cache_level color)
> {
> - o->gtt_space.color = color;
> + __i915_gem_obj_to_vma(o)->node.color = color;
> }
>
> /**
> @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> size_t size);
> void i915_gem_free_object(struct drm_gem_object *obj);
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +void i915_gem_vma_destroy(struct i915_vma *vma);
>
> int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> uint32_t alignment,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 812275a..3eb12ff 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2580,6 +2580,7 @@ int
> i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> {
> drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
> + struct i915_vma *vma;
> int ret;
>
> if (!i915_gem_obj_ggtt_bound(obj))
> @@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> i915_gem_object_unpin_pages(obj);
>
> list_del(&obj->mm_list);
> - list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> /* Avoid an unnecessary call to unbind on rebind. */
> obj->map_and_fenceable = true;
>
> - drm_mm_remove_node(&obj->gtt_space);
> + vma = __i915_gem_obj_to_vma(obj);
> + list_del(&vma->vma_link);
> + drm_mm_remove_node(&vma->node);
> + i915_gem_vma_destroy(vma);
> +
> + /* Since the unbound list is global, only move to that list if
> + * no more VMAs exist.
> + * NB: Until we have real VMAs there will only ever be one */
> + WARN_ON(!list_empty(&obj->vma_list));
> + if (list_empty(&obj->vma_list))
> + list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>
> return 0;
> }
> @@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> bool mappable, fenceable;
> size_t gtt_max = map_and_fenceable ?
> dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
> + struct i915_vma *vma;
> int ret;
>
> + if (WARN_ON(!list_empty(&obj->vma_list)))
> + return -EBUSY;
> +
> fence_size = i915_gem_get_gtt_size(dev,
> obj->base.size,
> obj->tiling_mode);
> @@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>
> i915_gem_object_pin_pages(obj);
>
> + vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> + if (vma == NULL) {
> + i915_gem_object_unpin_pages(obj);
> + return -ENOMEM;
> + }
> +
> search_free:
> ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
> - &obj->gtt_space,
> + &vma->node,
> size, alignment,
> obj->cache_level, 0, gtt_max);
> if (ret) {
> @@ -3107,22 +3127,19 @@ search_free:
Freeing vma is missing here.
> i915_gem_object_unpin_pages(obj);
> return ret;
> }
> - if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space,
> + if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
> obj->cache_level))) {
> - i915_gem_object_unpin_pages(obj);
> - drm_mm_remove_node(&obj->gtt_space);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_out;
> }
>
> ret = i915_gem_gtt_prepare_object(obj);
> - if (ret) {
> - i915_gem_object_unpin_pages(obj);
> - drm_mm_remove_node(&obj->gtt_space);
> - return ret;
> - }
> + if (ret)
> + goto err_out;
>
> list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
> list_add_tail(&obj->mm_list, &vm->inactive_list);
> + list_add(&vma->vma_link, &obj->vma_list);
>
> fenceable =
> i915_gem_obj_ggtt_size(obj) == fence_size &&
> @@ -3136,6 +3153,12 @@ search_free:
> trace_i915_gem_object_bind(obj, map_and_fenceable);
> i915_gem_verify_gtt(dev);
> return 0;
> +
> +err_out:
> + i915_gem_vma_destroy(vma);
> + i915_gem_object_unpin_pages(obj);
> + drm_mm_remove_node(&vma->node);
> + return ret;
> }
>
> void
> @@ -3281,6 +3304,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> {
> struct drm_device *dev = obj->base.dev;
> drm_i915_private_t *dev_priv = dev->dev_private;
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> int ret;
>
> if (obj->cache_level == cache_level)
> @@ -3291,7 +3315,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> return -EBUSY;
> }
>
> - if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) {
> + if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> ret = i915_gem_object_unbind(obj);
> if (ret)
> return ret;
> @@ -3836,6 +3860,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> INIT_LIST_HEAD(&obj->global_list);
> INIT_LIST_HEAD(&obj->ring_list);
> INIT_LIST_HEAD(&obj->exec_list);
> + INIT_LIST_HEAD(&obj->vma_list);
>
> obj->ops = ops;
>
> @@ -3956,6 +3981,26 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> i915_gem_object_free(obj);
> }
>
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> +{
> + struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> + if (vma == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&vma->vma_link);
> + vma->vm = vm;
> + vma->obj = obj;
> +
> + return vma;
> +}
> +
> +void i915_gem_vma_destroy(struct i915_vma *vma)
> +{
> + WARN_ON(vma->node.allocated);
> + kfree(vma);
> +}
> +
> int
> i915_gem_idle(struct drm_device *dev)
> {
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 43b8235..df61f33 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -34,11 +34,13 @@
> static bool
> mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
> {
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> +
> if (obj->pin_count)
> return false;
>
> list_add(&obj->exec_list, unwind);
> - return drm_mm_scan_add_block(&obj->gtt_space);
> + return drm_mm_scan_add_block(&vma->node);
> }
>
> int
> @@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> drm_i915_private_t *dev_priv = dev->dev_private;
> struct i915_address_space *vm = &dev_priv->gtt.base;
> struct list_head eviction_list, unwind_list;
> + struct i915_vma *vma;
> struct drm_i915_gem_object *obj;
> int ret = 0;
>
> @@ -106,8 +109,8 @@ none:
> obj = list_first_entry(&unwind_list,
> struct drm_i915_gem_object,
> exec_list);
> -
> - ret = drm_mm_scan_remove_block(&obj->gtt_space);
> + vma = __i915_gem_obj_to_vma(obj);
> + ret = drm_mm_scan_remove_block(&vma->node);
> BUG_ON(ret);
>
> list_del_init(&obj->exec_list);
> @@ -127,7 +130,8 @@ found:
> obj = list_first_entry(&unwind_list,
> struct drm_i915_gem_object,
> exec_list);
> - if (drm_mm_scan_remove_block(&obj->gtt_space)) {
> + vma = __i915_gem_obj_to_vma(obj);
> + if (drm_mm_scan_remove_block(&vma->node)) {
> list_move(&obj->exec_list, &eviction_list);
> drm_gem_object_reference(&obj->base);
> continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 999ecfe..3b639a9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
>
> /* Mark any preallocated objects as occupied */
> list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> int ret;
> DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> i915_gem_obj_ggtt_offset(obj), obj->base.size);
>
> WARN_ON(i915_gem_obj_ggtt_bound(obj));
> - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> - &obj->gtt_space);
> + ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> if (ret)
> DRM_DEBUG_KMS("Reservation failed\n");
> obj->has_global_gtt_mapping = 1;
> + list_add(&vma->vma_link, &obj->vma_list);
> }
>
> dev_priv->gtt.base.start = start;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 46a9715..a1f2308 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> struct i915_address_space *vm = &dev_priv->gtt.base;
> struct drm_i915_gem_object *obj;
> struct drm_mm_node *stolen;
> + struct i915_vma *vma;
> int ret;
>
> if (!drm_mm_initialized(&dev_priv->mm.stolen))
> @@ -393,16 +394,21 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> if (gtt_offset == I915_GTT_OFFSET_NONE)
> return obj;
>
> + vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> + if (!vma) {
> + drm_gem_object_unreference(&obj->base);
> + return NULL;
> + }
> +
> /* To simplify the initialisation sequence between KMS and GTT,
> * we allow construction of the stolen object prior to
> * setting up the GTT space. The actual reservation will occur
> * later.
> */
> - obj->gtt_space.start = gtt_offset;
> - obj->gtt_space.size = size;
> + vma->node.start = gtt_offset;
> + vma->node.size = size;
> if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
> - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> - &obj->gtt_space);
> + ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> if (ret) {
> DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
Haven't noticed last time around, but freeing vma is missing here. A
separate issue, but the error path in this function needs to be fixed in
other places too.
With these fixed, on the series:
Reviewed-by: Imre Deak <imre.deak@intel.com>
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/6] drm/i915: Free stolen node on failed preallocation
2013-07-16 23:50 ` [PATCH 5/5] [v5] drm/i915: Create VMAs Ben Widawsky
2013-07-17 14:43 ` Imre Deak
@ 2013-07-17 19:19 ` Ben Widawsky
2013-07-17 19:19 ` [PATCH 6/6] drm/i915: Create VMAs Ben Widawsky
1 sibling, 1 reply; 15+ messages in thread
From: Ben Widawsky @ 2013-07-17 19:19 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
The odds of this happening are *extremely* unlikely.
Reported-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 46a9715..a893834 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -405,7 +405,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
&obj->gtt_space);
if (ret) {
DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
- goto unref_out;
+ goto err_out;
}
}
@@ -416,7 +416,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
return obj;
-unref_out:
+err_out:
+ drm_mm_put_block(stolen);
drm_gem_object_unreference(&obj->base);
return NULL;
}
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 6/6] drm/i915: Create VMAs
2013-07-17 19:19 ` [PATCH 5/6] drm/i915: Free stolen node on failed preallocation Ben Widawsky
@ 2013-07-17 19:19 ` Ben Widawsky
2013-07-17 20:27 ` Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Ben Widawsky @ 2013-07-17 19:19 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
Formerly: "drm/i915: Create VMAs (part 1)"
In a previous patch, the notion of a VM was introduced. A VMA describes
an area of part of the VM address space. A VMA is similar to the concept
in the linux mm. However, instead of representing regular memory, a VMA
is backed by a GEM BO. There may be many VMAs for a given object, one
for each VM the object is to be used in. This may occur through flink,
dma-buf, or a number of other transient states.
Currently the code depends on only 1 VMA per object, for the global GTT
(and aliasing PPGTT). The following patches will address this and make
the rest of the infrastructure more suited
v2: s/i915_obj/i915_gem_obj (Chris)
v3: Only move an object to the now global unbound list if there are no
more VMAs for the object which are bound into a VM (ie. the list is
empty).
v4: killed obj->gtt_space
some reworks due to rebase
v5: Free vma on error path (Imre)
v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
(Imre)
Fixed vma freeing in stolen preallocation (Imre)
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_drv.h | 48 +++++++++++++++++-----
drivers/gpu/drm/i915/i915_gem.c | 74 +++++++++++++++++++++++++++-------
drivers/gpu/drm/i915/i915_gem_evict.c | 12 ++++--
drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++-
drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +++++--
5 files changed, 120 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b3ba428..1a32412 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
int (*enable)(struct drm_device *dev);
};
+/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime
+ * will always be <= an objects lifetime. So object refcounting should cover us.
+ */
+struct i915_vma {
+ struct drm_mm_node node;
+ struct drm_i915_gem_object *obj;
+ struct i915_address_space *vm;
+
+ struct list_head vma_link; /* Link in the object's VMA list */
+};
+
struct i915_ctx_hang_stats {
/* This context had batch pending when hang was declared */
unsigned batch_pending;
@@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
const struct drm_i915_gem_object_ops *ops;
- /** Current space allocated to this object in the GTT, if any. */
- struct drm_mm_node gtt_space;
+ /** List of VMAs backed by this object */
+ struct list_head vma_list;
+
/** Stolen memory for this object, instead of being backed by shmem. */
struct drm_mm_node *stolen;
struct list_head global_list;
@@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
-/* Offset of the first PTE pointing to this object */
-static inline unsigned long
-i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
+/* This is a temporary define to help transition us to real VMAs. If you see
+ * this, you're either reviewing code, or bisecting it. */
+static inline struct i915_vma *
+__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
{
- return o->gtt_space.start;
+ if (list_empty(&obj->vma_list))
+ return NULL;
+ return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
}
/* Whether or not this object is currently mapped by the translation tables */
static inline bool
i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
{
- return drm_mm_node_allocated(&o->gtt_space);
+ struct i915_vma *vma = __i915_gem_obj_to_vma(o);
+ if (vma == NULL)
+ return false;
+ return drm_mm_node_allocated(&vma->node);
+}
+
+/* Offset of the first PTE pointing to this object */
+static inline unsigned long
+i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
+{
+ BUG_ON(list_empty(&o->vma_list));
+ return __i915_gem_obj_to_vma(o)->node.start;
}
/* The size used in the translation tables may be larger than the actual size of
@@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
static inline unsigned long
i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
{
- return o->gtt_space.size;
+ BUG_ON(list_empty(&o->vma_list));
+ return __i915_gem_obj_to_vma(o)->node.size;
}
static inline void
i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
enum i915_cache_level color)
{
- o->gtt_space.color = color;
+ __i915_gem_obj_to_vma(o)->node.color = color;
}
/**
@@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
size_t size);
void i915_gem_free_object(struct drm_gem_object *obj);
+struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm);
+void i915_gem_vma_destroy(struct i915_vma *vma);
int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
uint32_t alignment,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 812275a..fe7ee32 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2580,6 +2580,7 @@ int
i915_gem_object_unbind(struct drm_i915_gem_object *obj)
{
drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
+ struct i915_vma *vma;
int ret;
if (!i915_gem_obj_ggtt_bound(obj))
@@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
i915_gem_object_unpin_pages(obj);
list_del(&obj->mm_list);
- list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
/* Avoid an unnecessary call to unbind on rebind. */
obj->map_and_fenceable = true;
- drm_mm_remove_node(&obj->gtt_space);
+ vma = __i915_gem_obj_to_vma(obj);
+ list_del(&vma->vma_link);
+ drm_mm_remove_node(&vma->node);
+ i915_gem_vma_destroy(vma);
+
+ /* Since the unbound list is global, only move to that list if
+ * no more VMAs exist.
+ * NB: Until we have real VMAs there will only ever be one */
+ WARN_ON(!list_empty(&obj->vma_list));
+ if (list_empty(&obj->vma_list))
+ list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
return 0;
}
@@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
bool mappable, fenceable;
size_t gtt_max = map_and_fenceable ?
dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
+ struct i915_vma *vma;
int ret;
+ if (WARN_ON(!list_empty(&obj->vma_list)))
+ return -EBUSY;
+
fence_size = i915_gem_get_gtt_size(dev,
obj->base.size,
obj->tiling_mode);
@@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
i915_gem_object_pin_pages(obj);
+ vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
+ if (vma == NULL) {
+ i915_gem_object_unpin_pages(obj);
+ return -ENOMEM;
+ }
+
search_free:
ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
- &obj->gtt_space,
+ &vma->node,
size, alignment,
obj->cache_level, 0, gtt_max);
if (ret) {
@@ -3104,25 +3124,21 @@ search_free:
if (ret == 0)
goto search_free;
- i915_gem_object_unpin_pages(obj);
- return ret;
+ goto err_out;
}
- if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space,
+ if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
obj->cache_level))) {
- i915_gem_object_unpin_pages(obj);
- drm_mm_remove_node(&obj->gtt_space);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_out;
}
ret = i915_gem_gtt_prepare_object(obj);
- if (ret) {
- i915_gem_object_unpin_pages(obj);
- drm_mm_remove_node(&obj->gtt_space);
- return ret;
- }
+ if (ret)
+ goto err_out;
list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
list_add_tail(&obj->mm_list, &vm->inactive_list);
+ list_add(&vma->vma_link, &obj->vma_list);
fenceable =
i915_gem_obj_ggtt_size(obj) == fence_size &&
@@ -3136,6 +3152,12 @@ search_free:
trace_i915_gem_object_bind(obj, map_and_fenceable);
i915_gem_verify_gtt(dev);
return 0;
+
+err_out:
+ i915_gem_vma_destroy(vma);
+ i915_gem_object_unpin_pages(obj);
+ drm_mm_remove_node(&vma->node);
+ return ret;
}
void
@@ -3281,6 +3303,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
{
struct drm_device *dev = obj->base.dev;
drm_i915_private_t *dev_priv = dev->dev_private;
+ struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
int ret;
if (obj->cache_level == cache_level)
@@ -3291,7 +3314,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
return -EBUSY;
}
- if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) {
+ if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
ret = i915_gem_object_unbind(obj);
if (ret)
return ret;
@@ -3836,6 +3859,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
INIT_LIST_HEAD(&obj->global_list);
INIT_LIST_HEAD(&obj->ring_list);
INIT_LIST_HEAD(&obj->exec_list);
+ INIT_LIST_HEAD(&obj->vma_list);
obj->ops = ops;
@@ -3956,6 +3980,26 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
i915_gem_object_free(obj);
}
+struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm)
+{
+ struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
+ if (vma == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&vma->vma_link);
+ vma->vm = vm;
+ vma->obj = obj;
+
+ return vma;
+}
+
+void i915_gem_vma_destroy(struct i915_vma *vma)
+{
+ WARN_ON(vma->node.allocated);
+ kfree(vma);
+}
+
int
i915_gem_idle(struct drm_device *dev)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 43b8235..df61f33 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -34,11 +34,13 @@
static bool
mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
{
+ struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
+
if (obj->pin_count)
return false;
list_add(&obj->exec_list, unwind);
- return drm_mm_scan_add_block(&obj->gtt_space);
+ return drm_mm_scan_add_block(&vma->node);
}
int
@@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
drm_i915_private_t *dev_priv = dev->dev_private;
struct i915_address_space *vm = &dev_priv->gtt.base;
struct list_head eviction_list, unwind_list;
+ struct i915_vma *vma;
struct drm_i915_gem_object *obj;
int ret = 0;
@@ -106,8 +109,8 @@ none:
obj = list_first_entry(&unwind_list,
struct drm_i915_gem_object,
exec_list);
-
- ret = drm_mm_scan_remove_block(&obj->gtt_space);
+ vma = __i915_gem_obj_to_vma(obj);
+ ret = drm_mm_scan_remove_block(&vma->node);
BUG_ON(ret);
list_del_init(&obj->exec_list);
@@ -127,7 +130,8 @@ found:
obj = list_first_entry(&unwind_list,
struct drm_i915_gem_object,
exec_list);
- if (drm_mm_scan_remove_block(&obj->gtt_space)) {
+ vma = __i915_gem_obj_to_vma(obj);
+ if (drm_mm_scan_remove_block(&vma->node)) {
list_move(&obj->exec_list, &eviction_list);
drm_gem_object_reference(&obj->base);
continue;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 999ecfe..3b639a9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
/* Mark any preallocated objects as occupied */
list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+ struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
int ret;
DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
i915_gem_obj_ggtt_offset(obj), obj->base.size);
WARN_ON(i915_gem_obj_ggtt_bound(obj));
- ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
- &obj->gtt_space);
+ ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
if (ret)
DRM_DEBUG_KMS("Reservation failed\n");
obj->has_global_gtt_mapping = 1;
+ list_add(&vma->vma_link, &obj->vma_list);
}
dev_priv->gtt.base.start = start;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a893834..f526136 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
struct i915_address_space *vm = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
struct drm_mm_node *stolen;
+ struct i915_vma *vma;
int ret;
if (!drm_mm_initialized(&dev_priv->mm.stolen))
@@ -393,18 +394,24 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
if (gtt_offset == I915_GTT_OFFSET_NONE)
return obj;
+ vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
+ if (!vma) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
/* To simplify the initialisation sequence between KMS and GTT,
* we allow construction of the stolen object prior to
* setting up the GTT space. The actual reservation will occur
* later.
*/
- obj->gtt_space.start = gtt_offset;
- obj->gtt_space.size = size;
+ vma->node.start = gtt_offset;
+ vma->node.size = size;
if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
- ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
- &obj->gtt_space);
+ ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
if (ret) {
DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
+ i915_gem_vma_destroy(vma);
goto err_out;
}
}
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 6/6] drm/i915: Create VMAs
2013-07-17 19:19 ` [PATCH 6/6] drm/i915: Create VMAs Ben Widawsky
@ 2013-07-17 20:27 ` Daniel Vetter
2013-07-18 0:12 ` Chris Wilson
2013-07-18 12:08 ` [PATCH 6/6] drm/i915: Create VMAs Imre Deak
2 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2013-07-17 20:27 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX
On Wed, Jul 17, 2013 at 12:19:03PM -0700, Ben Widawsky wrote:
> Formerly: "drm/i915: Create VMAs (part 1)"
>
> In a previous patch, the notion of a VM was introduced. A VMA describes
> an area of part of the VM address space. A VMA is similar to the concept
> in the linux mm. However, instead of representing regular memory, a VMA
> is backed by a GEM BO. There may be many VMAs for a given object, one
> for each VM the object is to be used in. This may occur through flink,
> dma-buf, or a number of other transient states.
>
> Currently the code depends on only 1 VMA per object, for the global GTT
> (and aliasing PPGTT). The following patches will address this and make
> the rest of the infrastructure more suited
>
> v2: s/i915_obj/i915_gem_obj (Chris)
>
> v3: Only move an object to the now global unbound list if there are no
> more VMAs for the object which are bound into a VM (ie. the list is
> empty).
>
> v4: killed obj->gtt_space
> some reworks due to rebase
>
> v5: Free vma on error path (Imre)
>
> v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> (Imre)
> Fixed vma freeing in stolen preallocation (Imre)
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Entire series merged to dinq, thanks a lot for the patches and review. On
to the next step in this journey then!
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 48 +++++++++++++++++-----
> drivers/gpu/drm/i915/i915_gem.c | 74 +++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/i915_gem_evict.c | 12 ++++--
> drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++-
> drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +++++--
> 5 files changed, 120 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3ba428..1a32412 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
> int (*enable)(struct drm_device *dev);
> };
>
> +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime
> + * will always be <= an objects lifetime. So object refcounting should cover us.
> + */
> +struct i915_vma {
> + struct drm_mm_node node;
> + struct drm_i915_gem_object *obj;
> + struct i915_address_space *vm;
> +
> + struct list_head vma_link; /* Link in the object's VMA list */
> +};
> +
> struct i915_ctx_hang_stats {
> /* This context had batch pending when hang was declared */
> unsigned batch_pending;
> @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
>
> const struct drm_i915_gem_object_ops *ops;
>
> - /** Current space allocated to this object in the GTT, if any. */
> - struct drm_mm_node gtt_space;
> + /** List of VMAs backed by this object */
> + struct list_head vma_list;
> +
> /** Stolen memory for this object, instead of being backed by shmem. */
> struct drm_mm_node *stolen;
> struct list_head global_list;
> @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
>
> #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
>
> -/* Offset of the first PTE pointing to this object */
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +/* This is a temporary define to help transition us to real VMAs. If you see
> + * this, you're either reviewing code, or bisecting it. */
> +static inline struct i915_vma *
> +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
> {
> - return o->gtt_space.start;
> + if (list_empty(&obj->vma_list))
> + return NULL;
> + return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
> }
>
> /* Whether or not this object is currently mapped by the translation tables */
> static inline bool
> i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> {
> - return drm_mm_node_allocated(&o->gtt_space);
> + struct i915_vma *vma = __i915_gem_obj_to_vma(o);
> + if (vma == NULL)
> + return false;
> + return drm_mm_node_allocated(&vma->node);
> +}
> +
> +/* Offset of the first PTE pointing to this object */
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +{
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.start;
> }
>
> /* The size used in the translation tables may be larger than the actual size of
> @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> static inline unsigned long
> i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
> {
> - return o->gtt_space.size;
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.size;
> }
>
> static inline void
> i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
> enum i915_cache_level color)
> {
> - o->gtt_space.color = color;
> + __i915_gem_obj_to_vma(o)->node.color = color;
> }
>
> /**
> @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> size_t size);
> void i915_gem_free_object(struct drm_gem_object *obj);
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +void i915_gem_vma_destroy(struct i915_vma *vma);
>
> int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> uint32_t alignment,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 812275a..fe7ee32 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2580,6 +2580,7 @@ int
> i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> {
> drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
> + struct i915_vma *vma;
> int ret;
>
> if (!i915_gem_obj_ggtt_bound(obj))
> @@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> i915_gem_object_unpin_pages(obj);
>
> list_del(&obj->mm_list);
> - list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> /* Avoid an unnecessary call to unbind on rebind. */
> obj->map_and_fenceable = true;
>
> - drm_mm_remove_node(&obj->gtt_space);
> + vma = __i915_gem_obj_to_vma(obj);
> + list_del(&vma->vma_link);
> + drm_mm_remove_node(&vma->node);
> + i915_gem_vma_destroy(vma);
> +
> + /* Since the unbound list is global, only move to that list if
> + * no more VMAs exist.
> + * NB: Until we have real VMAs there will only ever be one */
> + WARN_ON(!list_empty(&obj->vma_list));
> + if (list_empty(&obj->vma_list))
> + list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>
> return 0;
> }
> @@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> bool mappable, fenceable;
> size_t gtt_max = map_and_fenceable ?
> dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
> + struct i915_vma *vma;
> int ret;
>
> + if (WARN_ON(!list_empty(&obj->vma_list)))
> + return -EBUSY;
> +
> fence_size = i915_gem_get_gtt_size(dev,
> obj->base.size,
> obj->tiling_mode);
> @@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>
> i915_gem_object_pin_pages(obj);
>
> + vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> + if (vma == NULL) {
> + i915_gem_object_unpin_pages(obj);
> + return -ENOMEM;
> + }
> +
> search_free:
> ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
> - &obj->gtt_space,
> + &vma->node,
> size, alignment,
> obj->cache_level, 0, gtt_max);
> if (ret) {
> @@ -3104,25 +3124,21 @@ search_free:
> if (ret == 0)
> goto search_free;
>
> - i915_gem_object_unpin_pages(obj);
> - return ret;
> + goto err_out;
> }
> - if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space,
> + if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
> obj->cache_level))) {
> - i915_gem_object_unpin_pages(obj);
> - drm_mm_remove_node(&obj->gtt_space);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_out;
> }
>
> ret = i915_gem_gtt_prepare_object(obj);
> - if (ret) {
> - i915_gem_object_unpin_pages(obj);
> - drm_mm_remove_node(&obj->gtt_space);
> - return ret;
> - }
> + if (ret)
> + goto err_out;
>
> list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
> list_add_tail(&obj->mm_list, &vm->inactive_list);
> + list_add(&vma->vma_link, &obj->vma_list);
>
> fenceable =
> i915_gem_obj_ggtt_size(obj) == fence_size &&
> @@ -3136,6 +3152,12 @@ search_free:
> trace_i915_gem_object_bind(obj, map_and_fenceable);
> i915_gem_verify_gtt(dev);
> return 0;
> +
> +err_out:
> + i915_gem_vma_destroy(vma);
> + i915_gem_object_unpin_pages(obj);
> + drm_mm_remove_node(&vma->node);
> + return ret;
> }
>
> void
> @@ -3281,6 +3303,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> {
> struct drm_device *dev = obj->base.dev;
> drm_i915_private_t *dev_priv = dev->dev_private;
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> int ret;
>
> if (obj->cache_level == cache_level)
> @@ -3291,7 +3314,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> return -EBUSY;
> }
>
> - if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) {
> + if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> ret = i915_gem_object_unbind(obj);
> if (ret)
> return ret;
> @@ -3836,6 +3859,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> INIT_LIST_HEAD(&obj->global_list);
> INIT_LIST_HEAD(&obj->ring_list);
> INIT_LIST_HEAD(&obj->exec_list);
> + INIT_LIST_HEAD(&obj->vma_list);
>
> obj->ops = ops;
>
> @@ -3956,6 +3980,26 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> i915_gem_object_free(obj);
> }
>
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> +{
> + struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> + if (vma == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&vma->vma_link);
> + vma->vm = vm;
> + vma->obj = obj;
> +
> + return vma;
> +}
> +
> +void i915_gem_vma_destroy(struct i915_vma *vma)
> +{
> + WARN_ON(vma->node.allocated);
> + kfree(vma);
> +}
> +
> int
> i915_gem_idle(struct drm_device *dev)
> {
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 43b8235..df61f33 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -34,11 +34,13 @@
> static bool
> mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
> {
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> +
> if (obj->pin_count)
> return false;
>
> list_add(&obj->exec_list, unwind);
> - return drm_mm_scan_add_block(&obj->gtt_space);
> + return drm_mm_scan_add_block(&vma->node);
> }
>
> int
> @@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> drm_i915_private_t *dev_priv = dev->dev_private;
> struct i915_address_space *vm = &dev_priv->gtt.base;
> struct list_head eviction_list, unwind_list;
> + struct i915_vma *vma;
> struct drm_i915_gem_object *obj;
> int ret = 0;
>
> @@ -106,8 +109,8 @@ none:
> obj = list_first_entry(&unwind_list,
> struct drm_i915_gem_object,
> exec_list);
> -
> - ret = drm_mm_scan_remove_block(&obj->gtt_space);
> + vma = __i915_gem_obj_to_vma(obj);
> + ret = drm_mm_scan_remove_block(&vma->node);
> BUG_ON(ret);
>
> list_del_init(&obj->exec_list);
> @@ -127,7 +130,8 @@ found:
> obj = list_first_entry(&unwind_list,
> struct drm_i915_gem_object,
> exec_list);
> - if (drm_mm_scan_remove_block(&obj->gtt_space)) {
> + vma = __i915_gem_obj_to_vma(obj);
> + if (drm_mm_scan_remove_block(&vma->node)) {
> list_move(&obj->exec_list, &eviction_list);
> drm_gem_object_reference(&obj->base);
> continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 999ecfe..3b639a9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
>
> /* Mark any preallocated objects as occupied */
> list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> int ret;
> DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> i915_gem_obj_ggtt_offset(obj), obj->base.size);
>
> WARN_ON(i915_gem_obj_ggtt_bound(obj));
> - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> - &obj->gtt_space);
> + ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> if (ret)
> DRM_DEBUG_KMS("Reservation failed\n");
> obj->has_global_gtt_mapping = 1;
> + list_add(&vma->vma_link, &obj->vma_list);
> }
>
> dev_priv->gtt.base.start = start;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index a893834..f526136 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> struct i915_address_space *vm = &dev_priv->gtt.base;
> struct drm_i915_gem_object *obj;
> struct drm_mm_node *stolen;
> + struct i915_vma *vma;
> int ret;
>
> if (!drm_mm_initialized(&dev_priv->mm.stolen))
> @@ -393,18 +394,24 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> if (gtt_offset == I915_GTT_OFFSET_NONE)
> return obj;
>
> + vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> + if (!vma) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> /* To simplify the initialisation sequence between KMS and GTT,
> * we allow construction of the stolen object prior to
> * setting up the GTT space. The actual reservation will occur
> * later.
> */
> - obj->gtt_space.start = gtt_offset;
> - obj->gtt_space.size = size;
> + vma->node.start = gtt_offset;
> + vma->node.size = size;
> if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
> - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> - &obj->gtt_space);
> + ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> if (ret) {
> DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
> + i915_gem_vma_destroy(vma);
> goto err_out;
> }
> }
> --
> 1.8.3.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 6/6] drm/i915: Create VMAs
2013-07-17 19:19 ` [PATCH 6/6] drm/i915: Create VMAs Ben Widawsky
2013-07-17 20:27 ` Daniel Vetter
@ 2013-07-18 0:12 ` Chris Wilson
2013-07-18 2:31 ` Ben Widawsky
2013-07-18 2:32 ` [PATCH] drm/i915: Don't try to deref an unbound VMA Ben Widawsky
2013-07-18 12:08 ` [PATCH 6/6] drm/i915: Create VMAs Imre Deak
2 siblings, 2 replies; 15+ messages in thread
From: Chris Wilson @ 2013-07-18 0:12 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX
On Wed, Jul 17, 2013 at 12:19:03PM -0700, Ben Widawsky wrote:
> Formerly: "drm/i915: Create VMAs (part 1)"
>
> In a previous patch, the notion of a VM was introduced. A VMA describes
> an area of part of the VM address space. A VMA is similar to the concept
> in the linux mm. However, instead of representing regular memory, a VMA
> is backed by a GEM BO. There may be many VMAs for a given object, one
> for each VM the object is to be used in. This may occur through flink,
> dma-buf, or a number of other transient states.
>
> Currently the code depends on only 1 VMA per object, for the global GTT
> (and aliasing PPGTT). The following patches will address this and make
> the rest of the infrastructure more suited
>
> v2: s/i915_obj/i915_gem_obj (Chris)
>
> v3: Only move an object to the now global unbound list if there are no
> more VMAs for the object which are bound into a VM (ie. the list is
> empty).
>
> v4: killed obj->gtt_space
> some reworks due to rebase
>
> v5: Free vma on error path (Imre)
>
> v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> (Imre)
> Fixed vma freeing in stolen preallocation (Imre)
Big-bada-boom; set-cache-level needs to iterate over vma, and in
particular should not dereference a non-existent one. Or if we decided
that set-cache-level was a ggtt only property, just not explode if there
is no global vma.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 6/6] drm/i915: Create VMAs
2013-07-18 0:12 ` Chris Wilson
@ 2013-07-18 2:31 ` Ben Widawsky
2013-07-18 8:19 ` Chris Wilson
2013-07-18 2:32 ` [PATCH] drm/i915: Don't try to deref an unbound VMA Ben Widawsky
1 sibling, 1 reply; 15+ messages in thread
From: Ben Widawsky @ 2013-07-18 2:31 UTC (permalink / raw)
To: Chris Wilson, Intel GFX
On Thu, Jul 18, 2013 at 01:12:17AM +0100, Chris Wilson wrote:
> On Wed, Jul 17, 2013 at 12:19:03PM -0700, Ben Widawsky wrote:
> > Formerly: "drm/i915: Create VMAs (part 1)"
> >
> > In a previous patch, the notion of a VM was introduced. A VMA describes
> > an area of part of the VM address space. A VMA is similar to the concept
> > in the linux mm. However, instead of representing regular memory, a VMA
> > is backed by a GEM BO. There may be many VMAs for a given object, one
> > for each VM the object is to be used in. This may occur through flink,
> > dma-buf, or a number of other transient states.
> >
> > Currently the code depends on only 1 VMA per object, for the global GTT
> > (and aliasing PPGTT). The following patches will address this and make
> > the rest of the infrastructure more suited
> >
> > v2: s/i915_obj/i915_gem_obj (Chris)
> >
> > v3: Only move an object to the now global unbound list if there are no
> > more VMAs for the object which are bound into a VM (ie. the list is
> > empty).
> >
> > v4: killed obj->gtt_space
> > some reworks due to rebase
> >
> > v5: Free vma on error path (Imre)
> >
> > v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> > (Imre)
> > Fixed vma freeing in stolen preallocation (Imre)
>
> Big-bada-boom;
Just from looking at the code I think I see a bug. A bug which didn't
exist in the original version of the code, and doesn't exist after the
very next patch in the overall series.
Now I am terribly curious - why in the world (if that's indeed the bug)
can I not seem to hit this locally on my machine? I'll send the patch
for the fix now, but I'd really like to know what's different in our
setup. I've tried, UXA, SNA, and the igt test suite...
> set-cache-level needs to iterate over vma, and in
> particular should not dereference a non-existent one. Or if we decided
> that set-cache-level was a ggtt only property, just not explode if there
> is no global vma.
> -Chris
The current state is that cache level is still per object, but this
function itself will iterate over all VMAs. As I said, it happens in the
very next patch in the series. In the original patch series cover
letter, I made cache levels per VMA an optional TODO. For the time
being, I don't see much benefit.
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] drm/i915: Don't try to deref an unbound VMA
2013-07-18 0:12 ` Chris Wilson
2013-07-18 2:31 ` Ben Widawsky
@ 2013-07-18 2:32 ` Ben Widawsky
2013-07-18 6:47 ` Daniel Vetter
1 sibling, 1 reply; 15+ messages in thread
From: Ben Widawsky @ 2013-07-18 2:32 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky
This was actually correct in the original series, and is also fixed
later in the patch series, but was broken in this middle state.
I'm not really certain how I didn't hit it sooner.
This patch should be squashed into:
commit 8f588cfc349bbbd8ae62a13679b9efba41645064
Author: Ben Widawsky <ben@bwidawsk.net>
Date: Wed Jul 17 12:19:03 2013 -0700
drm/i915: Create VMAs
CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a9fa2bb..c9487bb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3314,7 +3314,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
return -EBUSY;
}
- if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
+ if (vma && !i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
ret = i915_gem_object_unbind(obj);
if (ret)
return ret;
--
1.8.3.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Don't try to deref an unbound VMA
2013-07-18 2:32 ` [PATCH] drm/i915: Don't try to deref an unbound VMA Ben Widawsky
@ 2013-07-18 6:47 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2013-07-18 6:47 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX
On Wed, Jul 17, 2013 at 07:32:33PM -0700, Ben Widawsky wrote:
> This was actually correct in the original series, and is also fixed
> later in the patch series, but was broken in this middle state.
>
> I'm not really certain how I didn't hit it sooner.
>
> This patch should be squashed into:
> commit 8f588cfc349bbbd8ae62a13679b9efba41645064
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date: Wed Jul 17 12:19:03 2013 -0700
>
> drm/i915: Create VMAs
>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Squashed in, thanks for the quick fixup.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a9fa2bb..c9487bb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3314,7 +3314,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> return -EBUSY;
> }
>
> - if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> + if (vma && !i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> ret = i915_gem_object_unbind(obj);
> if (ret)
> return ret;
> --
> 1.8.3.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 6/6] drm/i915: Create VMAs
2013-07-18 2:31 ` Ben Widawsky
@ 2013-07-18 8:19 ` Chris Wilson
0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2013-07-18 8:19 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX
On Wed, Jul 17, 2013 at 07:31:37PM -0700, Ben Widawsky wrote:
> On Thu, Jul 18, 2013 at 01:12:17AM +0100, Chris Wilson wrote:
> > Big-bada-boom;
>
> Just from looking at the code I think I see a bug. A bug which didn't
> exist in the original version of the code, and doesn't exist after the
> very next patch in the overall series.
Indeed, that was the bug.
> Now I am terribly curious - why in the world (if that's indeed the bug)
> can I not seem to hit this locally on my machine? I'll send the patch
> for the fix now, but I'd really like to know what's different in our
> setup. I've tried, UXA, SNA, and the igt test suite...
Just requires testing on the forgotten non-LLC generations. Or Baytrail.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 6/6] drm/i915: Create VMAs
2013-07-17 19:19 ` [PATCH 6/6] drm/i915: Create VMAs Ben Widawsky
2013-07-17 20:27 ` Daniel Vetter
2013-07-18 0:12 ` Chris Wilson
@ 2013-07-18 12:08 ` Imre Deak
2 siblings, 0 replies; 15+ messages in thread
From: Imre Deak @ 2013-07-18 12:08 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX
On Wed, 2013-07-17 at 12:19 -0700, Ben Widawsky wrote:
> Formerly: "drm/i915: Create VMAs (part 1)"
>
> In a previous patch, the notion of a VM was introduced. A VMA describes
> an area of part of the VM address space. A VMA is similar to the concept
> in the linux mm. However, instead of representing regular memory, a VMA
> is backed by a GEM BO. There may be many VMAs for a given object, one
> for each VM the object is to be used in. This may occur through flink,
> dma-buf, or a number of other transient states.
>
> Currently the code depends on only 1 VMA per object, for the global GTT
> (and aliasing PPGTT). The following patches will address this and make
> the rest of the infrastructure more suited
>
> v2: s/i915_obj/i915_gem_obj (Chris)
>
> v3: Only move an object to the now global unbound list if there are no
> more VMAs for the object which are bound into a VM (ie. the list is
> empty).
>
> v4: killed obj->gtt_space
> some reworks due to rebase
>
> v5: Free vma on error path (Imre)
>
> v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> (Imre)
> Fixed vma freeing in stolen preallocation (Imre)
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Looks ok, so on patches 5-6:
Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 48 +++++++++++++++++-----
> drivers/gpu/drm/i915/i915_gem.c | 74 +++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/i915_gem_evict.c | 12 ++++--
> drivers/gpu/drm/i915/i915_gem_gtt.c | 5 ++-
> drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +++++--
> 5 files changed, 120 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3ba428..1a32412 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
> int (*enable)(struct drm_device *dev);
> };
>
> +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime
> + * will always be <= an objects lifetime. So object refcounting should cover us.
> + */
> +struct i915_vma {
> + struct drm_mm_node node;
> + struct drm_i915_gem_object *obj;
> + struct i915_address_space *vm;
> +
> + struct list_head vma_link; /* Link in the object's VMA list */
> +};
> +
> struct i915_ctx_hang_stats {
> /* This context had batch pending when hang was declared */
> unsigned batch_pending;
> @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
>
> const struct drm_i915_gem_object_ops *ops;
>
> - /** Current space allocated to this object in the GTT, if any. */
> - struct drm_mm_node gtt_space;
> + /** List of VMAs backed by this object */
> + struct list_head vma_list;
> +
> /** Stolen memory for this object, instead of being backed by shmem. */
> struct drm_mm_node *stolen;
> struct list_head global_list;
> @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
>
> #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
>
> -/* Offset of the first PTE pointing to this object */
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +/* This is a temporary define to help transition us to real VMAs. If you see
> + * this, you're either reviewing code, or bisecting it. */
> +static inline struct i915_vma *
> +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
> {
> - return o->gtt_space.start;
> + if (list_empty(&obj->vma_list))
> + return NULL;
> + return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
> }
>
> /* Whether or not this object is currently mapped by the translation tables */
> static inline bool
> i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> {
> - return drm_mm_node_allocated(&o->gtt_space);
> + struct i915_vma *vma = __i915_gem_obj_to_vma(o);
> + if (vma == NULL)
> + return false;
> + return drm_mm_node_allocated(&vma->node);
> +}
> +
> +/* Offset of the first PTE pointing to this object */
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +{
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.start;
> }
>
> /* The size used in the translation tables may be larger than the actual size of
> @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> static inline unsigned long
> i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
> {
> - return o->gtt_space.size;
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.size;
> }
>
> static inline void
> i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
> enum i915_cache_level color)
> {
> - o->gtt_space.color = color;
> + __i915_gem_obj_to_vma(o)->node.color = color;
> }
>
> /**
> @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> size_t size);
> void i915_gem_free_object(struct drm_gem_object *obj);
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +void i915_gem_vma_destroy(struct i915_vma *vma);
>
> int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> uint32_t alignment,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 812275a..fe7ee32 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2580,6 +2580,7 @@ int
> i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> {
> drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
> + struct i915_vma *vma;
> int ret;
>
> if (!i915_gem_obj_ggtt_bound(obj))
> @@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
> i915_gem_object_unpin_pages(obj);
>
> list_del(&obj->mm_list);
> - list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> /* Avoid an unnecessary call to unbind on rebind. */
> obj->map_and_fenceable = true;
>
> - drm_mm_remove_node(&obj->gtt_space);
> + vma = __i915_gem_obj_to_vma(obj);
> + list_del(&vma->vma_link);
> + drm_mm_remove_node(&vma->node);
> + i915_gem_vma_destroy(vma);
> +
> + /* Since the unbound list is global, only move to that list if
> + * no more VMAs exist.
> + * NB: Until we have real VMAs there will only ever be one */
> + WARN_ON(!list_empty(&obj->vma_list));
> + if (list_empty(&obj->vma_list))
> + list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>
> return 0;
> }
> @@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> bool mappable, fenceable;
> size_t gtt_max = map_and_fenceable ?
> dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
> + struct i915_vma *vma;
> int ret;
>
> + if (WARN_ON(!list_empty(&obj->vma_list)))
> + return -EBUSY;
> +
> fence_size = i915_gem_get_gtt_size(dev,
> obj->base.size,
> obj->tiling_mode);
> @@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>
> i915_gem_object_pin_pages(obj);
>
> + vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> + if (vma == NULL) {
> + i915_gem_object_unpin_pages(obj);
> + return -ENOMEM;
> + }
> +
> search_free:
> ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
> - &obj->gtt_space,
> + &vma->node,
> size, alignment,
> obj->cache_level, 0, gtt_max);
> if (ret) {
> @@ -3104,25 +3124,21 @@ search_free:
> if (ret == 0)
> goto search_free;
>
> - i915_gem_object_unpin_pages(obj);
> - return ret;
> + goto err_out;
> }
> - if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space,
> + if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
> obj->cache_level))) {
> - i915_gem_object_unpin_pages(obj);
> - drm_mm_remove_node(&obj->gtt_space);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_out;
> }
>
> ret = i915_gem_gtt_prepare_object(obj);
> - if (ret) {
> - i915_gem_object_unpin_pages(obj);
> - drm_mm_remove_node(&obj->gtt_space);
> - return ret;
> - }
> + if (ret)
> + goto err_out;
>
> list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
> list_add_tail(&obj->mm_list, &vm->inactive_list);
> + list_add(&vma->vma_link, &obj->vma_list);
>
> fenceable =
> i915_gem_obj_ggtt_size(obj) == fence_size &&
> @@ -3136,6 +3152,12 @@ search_free:
> trace_i915_gem_object_bind(obj, map_and_fenceable);
> i915_gem_verify_gtt(dev);
> return 0;
> +
> +err_out:
> + i915_gem_vma_destroy(vma);
> + i915_gem_object_unpin_pages(obj);
> + drm_mm_remove_node(&vma->node);
> + return ret;
> }
>
> void
> @@ -3281,6 +3303,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> {
> struct drm_device *dev = obj->base.dev;
> drm_i915_private_t *dev_priv = dev->dev_private;
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> int ret;
>
> if (obj->cache_level == cache_level)
> @@ -3291,7 +3314,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> return -EBUSY;
> }
>
> - if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) {
> + if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
> ret = i915_gem_object_unbind(obj);
> if (ret)
> return ret;
> @@ -3836,6 +3859,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> INIT_LIST_HEAD(&obj->global_list);
> INIT_LIST_HEAD(&obj->ring_list);
> INIT_LIST_HEAD(&obj->exec_list);
> + INIT_LIST_HEAD(&obj->vma_list);
>
> obj->ops = ops;
>
> @@ -3956,6 +3980,26 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> i915_gem_object_free(obj);
> }
>
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> +{
> + struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> + if (vma == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + INIT_LIST_HEAD(&vma->vma_link);
> + vma->vm = vm;
> + vma->obj = obj;
> +
> + return vma;
> +}
> +
> +void i915_gem_vma_destroy(struct i915_vma *vma)
> +{
> + WARN_ON(vma->node.allocated);
> + kfree(vma);
> +}
> +
> int
> i915_gem_idle(struct drm_device *dev)
> {
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 43b8235..df61f33 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -34,11 +34,13 @@
> static bool
> mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
> {
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> +
> if (obj->pin_count)
> return false;
>
> list_add(&obj->exec_list, unwind);
> - return drm_mm_scan_add_block(&obj->gtt_space);
> + return drm_mm_scan_add_block(&vma->node);
> }
>
> int
> @@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> drm_i915_private_t *dev_priv = dev->dev_private;
> struct i915_address_space *vm = &dev_priv->gtt.base;
> struct list_head eviction_list, unwind_list;
> + struct i915_vma *vma;
> struct drm_i915_gem_object *obj;
> int ret = 0;
>
> @@ -106,8 +109,8 @@ none:
> obj = list_first_entry(&unwind_list,
> struct drm_i915_gem_object,
> exec_list);
> -
> - ret = drm_mm_scan_remove_block(&obj->gtt_space);
> + vma = __i915_gem_obj_to_vma(obj);
> + ret = drm_mm_scan_remove_block(&vma->node);
> BUG_ON(ret);
>
> list_del_init(&obj->exec_list);
> @@ -127,7 +130,8 @@ found:
> obj = list_first_entry(&unwind_list,
> struct drm_i915_gem_object,
> exec_list);
> - if (drm_mm_scan_remove_block(&obj->gtt_space)) {
> + vma = __i915_gem_obj_to_vma(obj);
> + if (drm_mm_scan_remove_block(&vma->node)) {
> list_move(&obj->exec_list, &eviction_list);
> drm_gem_object_reference(&obj->base);
> continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 999ecfe..3b639a9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
>
> /* Mark any preallocated objects as occupied */
> list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> + struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> int ret;
> DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
> i915_gem_obj_ggtt_offset(obj), obj->base.size);
>
> WARN_ON(i915_gem_obj_ggtt_bound(obj));
> - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> - &obj->gtt_space);
> + ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> if (ret)
> DRM_DEBUG_KMS("Reservation failed\n");
> obj->has_global_gtt_mapping = 1;
> + list_add(&vma->vma_link, &obj->vma_list);
> }
>
> dev_priv->gtt.base.start = start;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index a893834..f526136 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> struct i915_address_space *vm = &dev_priv->gtt.base;
> struct drm_i915_gem_object *obj;
> struct drm_mm_node *stolen;
> + struct i915_vma *vma;
> int ret;
>
> if (!drm_mm_initialized(&dev_priv->mm.stolen))
> @@ -393,18 +394,24 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> if (gtt_offset == I915_GTT_OFFSET_NONE)
> return obj;
>
> + vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> + if (!vma) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> /* To simplify the initialisation sequence between KMS and GTT,
> * we allow construction of the stolen object prior to
> * setting up the GTT space. The actual reservation will occur
> * later.
> */
> - obj->gtt_space.start = gtt_offset;
> - obj->gtt_space.size = size;
> + vma->node.start = gtt_offset;
> + vma->node.size = size;
> if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
> - ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> - &obj->gtt_space);
> + ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
> if (ret) {
> DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
> + i915_gem_vma_destroy(vma);
> goto err_out;
> }
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-07-18 12:08 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-16 23:50 [PATCH 1/5] [v3] drm/i915: Move gtt and ppgtt under address space umbrella Ben Widawsky
2013-07-16 23:50 ` [PATCH 2/5] [v2] drm/i915: Put the mm in the parent address space Ben Widawsky
2013-07-16 23:50 ` [PATCH 3/5] [v2] drm/i915: Create a global list of vms Ben Widawsky
2013-07-16 23:50 ` [PATCH 4/5] [v2] drm/i915: Move active/inactive lists to new mm Ben Widawsky
2013-07-16 23:50 ` [PATCH 5/5] [v5] drm/i915: Create VMAs Ben Widawsky
2013-07-17 14:43 ` Imre Deak
2013-07-17 19:19 ` [PATCH 5/6] drm/i915: Free stolen node on failed preallocation Ben Widawsky
2013-07-17 19:19 ` [PATCH 6/6] drm/i915: Create VMAs Ben Widawsky
2013-07-17 20:27 ` Daniel Vetter
2013-07-18 0:12 ` Chris Wilson
2013-07-18 2:31 ` Ben Widawsky
2013-07-18 8:19 ` Chris Wilson
2013-07-18 2:32 ` [PATCH] drm/i915: Don't try to deref an unbound VMA Ben Widawsky
2013-07-18 6:47 ` Daniel Vetter
2013-07-18 12:08 ` [PATCH 6/6] drm/i915: Create VMAs Imre Deak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.