[Intel-gfx] [PATCH 2/3] drm/i915: prefer FORCE_WC for the blitter routines

[Matthew Auld]

On Mon, 18 Jan 2021 at 14:54, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>
> Quoting Matthew Auld (2021-01-18 14:17:31)
> > From: CQ Tang <cq.tang at intel.com>
> >
> > 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 at intel.com>
> > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> > ---
> >  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 = &gt->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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx