| Message ID | 20210124135726.1795-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove guard page insertion around unevictable nodes | expand |
On Sun, 24 Jan 2021 at 13:57, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Assume that unevictable nodes are not in the GTT and so we can ignore > page boundary concerns, and so allow regular nodes to abutt against > irregular unevictable nodes. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 -- > drivers/gpu/drm/i915/i915_gem_evict.c | 6 ++++-- > drivers/gpu/drm/i915/i915_vma.h | 10 +++++++++- > drivers/gpu/drm/i915/i915_vma_types.h | 2 ++ > 4 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 99cf861df92d..69c5a185ecff 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -357,8 +357,6 @@ enum i915_cache_level { > I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ > }; > > -#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ > - > struct intel_fbc { > /* This is always the inner lock when overlapping with struct_mutex and > * it's the outer lock when overlapping with stolen_lock. */ > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > index 4d2d59a9942b..aef88fdb9f66 100644 > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > @@ -313,11 +313,13 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, > */ > if (i915_vm_has_cache_coloring(vm)) { > if (node->start + node->size == target->start) { > - if (node->color == target->color) > + if (i915_node_color_matches(node, > + target->color)) > continue; > } > if (node->start == target->start + target->size) { > - if (node->color == target->color) > + if (i915_node_color_matches(node, > + target->color)) > continue; > } > } Since we bail early on seeing COLOR_UNEVICTABLE, and since we have to enlarge our search space by a page on both ends, do we need something like: @@ -291,18 +291,22 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, /* Always look at the page afterwards to avoid the end-of-GTT */ end += I915_GTT_PAGE_SIZE; + + /* + * The special head node is marked as I915_COLOR_UNEVICTABLE to + * prevent normal vma from using the last page, in order to + * prevent prefetching over the boundary. If we are dealing with + * a normal vma and we abutt with the head node we will always + * get -ENOSPC as per the color adjust later, so just bail + * early??? + */ + if (target->color != I915_COLOR_UNEVICTABLE && end >= vm->total) + return -ENOSPC; } GEM_BUG_ON(start >= end); drm_mm_for_each_node_in_range(node, &vm->mm, start, end) { - /* If we find any non-objects (!vma), we cannot evict them */ - if (node->color == I915_COLOR_UNEVICTABLE) { - ret = -ENOSPC; - break; - } - GEM_BUG_ON(!drm_mm_node_allocated(node)); - vma = container_of(node, typeof(*vma), node); /* * If we are using coloring to insert guard pages between @@ -310,6 +314,9 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, * to check whether the objects on either side of our range * abutt and conflict. If they are in conflict, then we evict * those as well to make room for our guard pages. + * + * Also consider I915_COLOR_UNEVICTABLE nodes, since we consider + * these special and permit abutting on either end. */ if (i915_vm_has_cache_coloring(vm)) { if (node->start + node->size == target->start) { @@ -324,6 +331,14 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, } } + /* If we find any non-objects (!vma), we cannot evict them */ + if (node->color == I915_COLOR_UNEVICTABLE) { + ret = -ENOSPC; + break; + } + + vma = container_of(node, typeof(*vma), node); + if (i915_vma_is_pinned(vma)) { ret = -ENOSPC; break; > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h > index a64adc8c883b..dac953815118 100644 > --- a/drivers/gpu/drm/i915/i915_vma.h > +++ b/drivers/gpu/drm/i915/i915_vma.h > @@ -283,10 +283,18 @@ static inline bool i915_vma_is_bound(const struct i915_vma *vma, > return atomic_read(&vma->flags) & where; > } > > +static inline bool i915_node_color_matches(const struct drm_mm_node *node, > + unsigned long color) > +{ > + return (node->color | color) == I915_COLOR_UNEVICTABLE || > + node->color == color; > +} > + > static inline bool i915_node_color_differs(const struct drm_mm_node *node, > unsigned long color) > { > - return drm_mm_node_allocated(node) && node->color != color; > + return drm_mm_node_allocated(node) && > + !i915_node_color_matches(node, color); > } > > /** > diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h > index f5cb848b7a7e..e72a07692a64 100644 > --- a/drivers/gpu/drm/i915/i915_vma_types.h > +++ b/drivers/gpu/drm/i915/i915_vma_types.h > @@ -95,6 +95,8 @@ enum i915_cache_level; > * > */ > > +#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ > + > struct intel_remapped_plane_info { > /* in gtt pages */ > unsigned int width, height, stride, offset; > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Matthew Auld (2021-01-25 11:16:13) > On Sun, 24 Jan 2021 at 13:57, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Assume that unevictable nodes are not in the GTT and so we can ignore > > page boundary concerns, and so allow regular nodes to abutt against > > irregular unevictable nodes. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 -- > > drivers/gpu/drm/i915/i915_gem_evict.c | 6 ++++-- > > drivers/gpu/drm/i915/i915_vma.h | 10 +++++++++- > > drivers/gpu/drm/i915/i915_vma_types.h | 2 ++ > > 4 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 99cf861df92d..69c5a185ecff 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -357,8 +357,6 @@ enum i915_cache_level { > > I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ > > }; > > > > -#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ > > - > > struct intel_fbc { > > /* This is always the inner lock when overlapping with struct_mutex and > > * it's the outer lock when overlapping with stolen_lock. */ > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > > index 4d2d59a9942b..aef88fdb9f66 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > @@ -313,11 +313,13 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, > > */ > > if (i915_vm_has_cache_coloring(vm)) { > > if (node->start + node->size == target->start) { > > - if (node->color == target->color) > > + if (i915_node_color_matches(node, > > + target->color)) > > continue; > > } > > if (node->start == target->start + target->size) { > > - if (node->color == target->color) > > + if (i915_node_color_matches(node, > > + target->color)) > > continue; > > } > > } > > Since we bail early on seeing COLOR_UNEVICTABLE, and since we have to > enlarge our search space by a page on both ends, do we need something > like: Are we not doing the opposite here, and skipping the evict if either node/target is unevictable? So we always expand the search by a page if the vm has coloring enabled, but then skip the guard page eviction if either side says no. -Chris
Quoting Chris Wilson (2021-01-25 11:24:22) > Quoting Matthew Auld (2021-01-25 11:16:13) > > On Sun, 24 Jan 2021 at 13:57, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > Assume that unevictable nodes are not in the GTT and so we can ignore > > > page boundary concerns, and so allow regular nodes to abutt against > > > irregular unevictable nodes. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 2 -- > > > drivers/gpu/drm/i915/i915_gem_evict.c | 6 ++++-- > > > drivers/gpu/drm/i915/i915_vma.h | 10 +++++++++- > > > drivers/gpu/drm/i915/i915_vma_types.h | 2 ++ > > > 4 files changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 99cf861df92d..69c5a185ecff 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -357,8 +357,6 @@ enum i915_cache_level { > > > I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ > > > }; > > > > > > -#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ > > > - > > > struct intel_fbc { > > > /* This is always the inner lock when overlapping with struct_mutex and > > > * it's the outer lock when overlapping with stolen_lock. */ > > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > > > index 4d2d59a9942b..aef88fdb9f66 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > > @@ -313,11 +313,13 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, > > > */ > > > if (i915_vm_has_cache_coloring(vm)) { > > > if (node->start + node->size == target->start) { > > > - if (node->color == target->color) > > > + if (i915_node_color_matches(node, > > > + target->color)) > > > continue; > > > } > > > if (node->start == target->start + target->size) { > > > - if (node->color == target->color) > > > + if (i915_node_color_matches(node, > > > + target->color)) > > > continue; > > > } > > > } > > > > Since we bail early on seeing COLOR_UNEVICTABLE, and since we have to > > enlarge our search space by a page on both ends, do we need something > > like: > > Are we not doing the opposite here, and skipping the evict if either > node/target is unevictable? Oh, you mean the earlier abort if we try to evict an unevictable node inside the target range. -Chris
On Mon, 25 Jan 2021 at 11:28, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Chris Wilson (2021-01-25 11:24:22) > > Quoting Matthew Auld (2021-01-25 11:16:13) > > > On Sun, 24 Jan 2021 at 13:57, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > > > Assume that unevictable nodes are not in the GTT and so we can ignore > > > > page boundary concerns, and so allow regular nodes to abutt against > > > > irregular unevictable nodes. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.h | 2 -- > > > > drivers/gpu/drm/i915/i915_gem_evict.c | 6 ++++-- > > > > drivers/gpu/drm/i915/i915_vma.h | 10 +++++++++- > > > > drivers/gpu/drm/i915/i915_vma_types.h | 2 ++ > > > > 4 files changed, 15 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > > index 99cf861df92d..69c5a185ecff 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -357,8 +357,6 @@ enum i915_cache_level { > > > > I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ > > > > }; > > > > > > > > -#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ > > > > - > > > > struct intel_fbc { > > > > /* This is always the inner lock when overlapping with struct_mutex and > > > > * it's the outer lock when overlapping with stolen_lock. */ > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > > > > index 4d2d59a9942b..aef88fdb9f66 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > > > @@ -313,11 +313,13 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, > > > > */ > > > > if (i915_vm_has_cache_coloring(vm)) { > > > > if (node->start + node->size == target->start) { > > > > - if (node->color == target->color) > > > > + if (i915_node_color_matches(node, > > > > + target->color)) > > > > continue; > > > > } > > > > if (node->start == target->start + target->size) { > > > > - if (node->color == target->color) > > > > + if (i915_node_color_matches(node, > > > > + target->color)) > > > > continue; > > > > } > > > > } > > > > > > Since we bail early on seeing COLOR_UNEVICTABLE, and since we have to > > > enlarge our search space by a page on both ends, do we need something > > > like: > > > > Are we not doing the opposite here, and skipping the evict if either > > node/target is unevictable? > > Oh, you mean the earlier abort if we try to evict an unevictable node > inside the target range. Yeah, if it only abuts and is COLOR_UNEVICTABLE we can ignore the node now, but if it's actually within our range then we abort like before. And then there is some strangeness with the head node. > -Chris
Quoting Matthew Auld (2021-01-25 11:35:22) > On Mon, 25 Jan 2021 at 11:28, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Chris Wilson (2021-01-25 11:24:22) > > > Quoting Matthew Auld (2021-01-25 11:16:13) > > > > On Sun, 24 Jan 2021 at 13:57, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > > > > > Assume that unevictable nodes are not in the GTT and so we can ignore > > > > > page boundary concerns, and so allow regular nodes to abutt against > > > > > irregular unevictable nodes. > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 -- > > > > > drivers/gpu/drm/i915/i915_gem_evict.c | 6 ++++-- > > > > > drivers/gpu/drm/i915/i915_vma.h | 10 +++++++++- > > > > > drivers/gpu/drm/i915/i915_vma_types.h | 2 ++ > > > > > 4 files changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > > > index 99cf861df92d..69c5a185ecff 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > > @@ -357,8 +357,6 @@ enum i915_cache_level { > > > > > I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ > > > > > }; > > > > > > > > > > -#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ > > > > > - > > > > > struct intel_fbc { > > > > > /* This is always the inner lock when overlapping with struct_mutex and > > > > > * it's the outer lock when overlapping with stolen_lock. */ > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > > > > > index 4d2d59a9942b..aef88fdb9f66 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > > > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > > > > @@ -313,11 +313,13 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, > > > > > */ > > > > > if (i915_vm_has_cache_coloring(vm)) { > > > > > if (node->start + node->size == target->start) { > > > > > - if (node->color == target->color) > > > > > + if (i915_node_color_matches(node, > > > > > + target->color)) > > > > > continue; > > > > > } > > > > > if (node->start == target->start + target->size) { > > > > > - if (node->color == target->color) > > > > > + if (i915_node_color_matches(node, > > > > > + target->color)) > > > > > continue; > > > > > } > > > > > } > > > > > > > > Since we bail early on seeing COLOR_UNEVICTABLE, and since we have to > > > > enlarge our search space by a page on both ends, do we need something > > > > like: > > > > > > Are we not doing the opposite here, and skipping the evict if either > > > node/target is unevictable? > > > > Oh, you mean the earlier abort if we try to evict an unevictable node > > inside the target range. > > Yeah, if it only abuts and is COLOR_UNEVICTABLE we can ignore the node > now, but if it's actually within our range then we abort like before. > And then there is some strangeness with the head node. Hmm. On second thought, the reservation is using the direct reserve now and not entering i915_gem_evict_for_now() so for the moment we don't have to worry about any changes here. We can ponder whether we can remove guard pages around foreign nodes later. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 99cf861df92d..69c5a185ecff 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -357,8 +357,6 @@ enum i915_cache_level { I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ }; -#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ - struct intel_fbc { /* This is always the inner lock when overlapping with struct_mutex and * it's the outer lock when overlapping with stolen_lock. */ diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 4d2d59a9942b..aef88fdb9f66 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -313,11 +313,13 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, */ if (i915_vm_has_cache_coloring(vm)) { if (node->start + node->size == target->start) { - if (node->color == target->color) + if (i915_node_color_matches(node, + target->color)) continue; } if (node->start == target->start + target->size) { - if (node->color == target->color) + if (i915_node_color_matches(node, + target->color)) continue; } } diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index a64adc8c883b..dac953815118 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -283,10 +283,18 @@ static inline bool i915_vma_is_bound(const struct i915_vma *vma, return atomic_read(&vma->flags) & where; } +static inline bool i915_node_color_matches(const struct drm_mm_node *node, + unsigned long color) +{ + return (node->color | color) == I915_COLOR_UNEVICTABLE || + node->color == color; +} + static inline bool i915_node_color_differs(const struct drm_mm_node *node, unsigned long color) { - return drm_mm_node_allocated(node) && node->color != color; + return drm_mm_node_allocated(node) && + !i915_node_color_matches(node, color); } /** diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h index f5cb848b7a7e..e72a07692a64 100644 --- a/drivers/gpu/drm/i915/i915_vma_types.h +++ b/drivers/gpu/drm/i915/i915_vma_types.h @@ -95,6 +95,8 @@ enum i915_cache_level; * */ +#define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ + struct intel_remapped_plane_info { /* in gtt pages */ unsigned int width, height, stride, offset;
Assume that unevictable nodes are not in the GTT and so we can ignore page boundary concerns, and so allow regular nodes to abutt against irregular unevictable nodes. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 2 -- drivers/gpu/drm/i915/i915_gem_evict.c | 6 ++++-- drivers/gpu/drm/i915/i915_vma.h | 10 +++++++++- drivers/gpu/drm/i915/i915_vma_types.h | 2 ++ 4 files changed, 15 insertions(+), 5 deletions(-)