On Mon, 18 Jan 2021 at 14:54, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>
> Quoting Matthew Auld (2021-01-18 14:17:31)
> > From: CQ Tang <cq.tang@xxxxxxxxx>
> >
> > The pool is shared and so we might find that there is a pool object with
> > an existing mapping, but is mapped with different underlying type, which
> > will result in -EBUSY.
> >
> > Signed-off-by: CQ Tang <cq.tang@xxxxxxxxx>
> > Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_object_blt.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
> > index 10cac9fac79b..c6db745900b3 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_blt.c
> > @@ -55,7 +55,7 @@ struct i915_vma *intel_emit_vma_fill_blt(struct intel_context *ce,
> > if (unlikely(err))
> > goto out_put;
> >
> > - cmd = i915_gem_object_pin_map(pool->obj, I915_MAP_WC);
> > + cmd = i915_gem_object_pin_map(pool->obj, I915_MAP_FORCE_WC);
> > if (IS_ERR(cmd)) {
> > err = PTR_ERR(cmd);
> > goto out_unpin;
> > @@ -277,7 +277,7 @@ struct i915_vma *intel_emit_vma_copy_blt(struct intel_context *ce,
> > if (unlikely(err))
> > goto out_put;
> >
> > - cmd = i915_gem_object_pin_map(pool->obj, I915_MAP_WC);
> > + cmd = i915_gem_object_pin_map(pool->obj, I915_MAP_FORCE_WC);
> > if (IS_ERR(cmd)) {
> > err = PTR_ERR(cmd);
> > goto out_unpin;
>
> FORCE is becoming meaningless.
>
> In this case we pin the pages upon acquiring from the pool, which then
> prevents us from changing the mapping type. The purpose of which was so
> that we could cache the mapping between users, and here we are saying
> that cache is made useless. The danger is that we are now thrashing the
> cache, hurting ourselves with the vmap overhead.
>
> Maybe we should move the mapping-type into the buffer-pool cache itself?
Yeah, makes sense I think. Maybe something simple like:
--- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
@@ -145,7 +145,8 @@ static void pool_retire(struct i915_active *ref)
}
static struct intel_gt_buffer_pool_node *
-node_create(struct intel_gt_buffer_pool *pool, size_t sz)
+node_create(struct intel_gt_buffer_pool *pool, size_t sz,
+ enum i915_map_type type)
{
struct intel_gt *gt = to_gt(pool);
struct intel_gt_buffer_pool_node *node;
@@ -169,12 +170,14 @@ node_create(struct intel_gt_buffer_pool *pool, size_t sz)
i915_gem_object_set_readonly(obj);
+ node->type = type;
node->obj = obj;
return node;
}
struct intel_gt_buffer_pool_node *
-intel_gt_get_buffer_pool(struct intel_gt *gt, size_t size)
+intel_gt_get_buffer_pool(struct intel_gt *gt, size_t size,
+ enum i915_map_type type)
{
struct intel_gt_buffer_pool *pool = >->buffer_pool;
struct intel_gt_buffer_pool_node *node;
@@ -191,6 +194,9 @@ intel_gt_get_buffer_pool(struct intel_gt *gt, size_t size)
if (node->obj->base.size < size)
continue;
+ if (node->type != type)
+ continue;
+
age = READ_ONCE(node->age);
if (!age)
continue;
@@ -205,7 +211,7 @@ intel_gt_get_buffer_pool(struct intel_gt *gt, size_t size)
rcu_read_unlock();
if (&node->link == list) {
- node = node_create(pool, size);
+ node = node_create(pool, size, type);
if (IS_ERR(node))
return node;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.h
b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.h
index 42cbac003e8a..6068f8f1762e 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.h
@@ -15,7 +15,8 @@ struct intel_gt;
struct i915_request;
struct intel_gt_buffer_pool_node *
-intel_gt_get_buffer_pool(struct intel_gt *gt, size_t size);
+intel_gt_get_buffer_pool(struct intel_gt *gt, size_t size,
+ enum i915_map_type type);
static inline int
intel_gt_buffer_pool_mark_active(struct intel_gt_buffer_pool_node *node,
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool_types.h
b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool_types.h
index bcf1658c9633..e8f7dba36b76 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool_types.h
@@ -31,6 +31,7 @@ struct intel_gt_buffer_pool_node {
struct rcu_head rcu;
};
unsigned long age;
+ enum i915_map_type type;
};
Or maybe it should be split over multiple lists or something, one for each type?
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
