Re: [PATCH 2/2] drm/i915: Perform object clflushing asynchronously

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 17 February 2017 at 14:07, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> Flushing the cachelines for an object is slow, can be as much as 100ms
> for a large framebuffer. We currently do this under the struct_mutex BKL
> on execution or on pageflip. But now with the ability to add fences to
> obj->resv for both flips and execbuf (and we naturally wait on the fence
> before CPU access), we can move the clflush operation to a workqueue and
> signal a fence for completion, thereby doing the work asynchronously and
> not blocking the driver or its clients.
>
> Suggested-by: Akash Goel <akash.goel@xxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_drv.h            |   8 +-
>  drivers/gpu/drm/i915/i915_gem.c            |  66 ++--------
>  drivers/gpu/drm/i915/i915_gem_clflush.c    | 194 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   8 +-
>  drivers/gpu/drm/i915/intel_display.c       |  49 ++++----
>  6 files changed, 240 insertions(+), 86 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem_clflush.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 1490d8622234..b1b580337c7a 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -29,6 +29,7 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o
>  # GEM code
>  i915-y += i915_cmd_parser.o \
>           i915_gem_batch_pool.o \
> +         i915_gem_clflush.o \
>           i915_gem_context.o \
>           i915_gem_dmabuf.o \
>           i915_gem_evict.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5005922f267b..97d3ecbcf8d4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3369,7 +3369,13 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv);
>  void i915_gem_reset(struct drm_i915_private *dev_priv);
>  void i915_gem_reset_finish(struct drm_i915_private *dev_priv);
>  void i915_gem_set_wedged(struct drm_i915_private *dev_priv);
> -void i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
> +
> +void i915_gem_clflush_init(struct drm_i915_private *i915);
> +void i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> +                            unsigned int flags);
> +#define I915_CLFLUSH_FORCE BIT(0)
> +#define I915_CLFLUSH_SYNC BIT(1)
> +
>  void i915_gem_init_mmio(struct drm_i915_private *i915);
>  int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
>  int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 96098a7e0bc3..fc11f5205e00 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1621,23 +1621,17 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  {
>         struct drm_i915_gem_sw_finish *args = data;
>         struct drm_i915_gem_object *obj;
> -       int err = 0;
>
>         obj = i915_gem_object_lookup(file, args->handle);
>         if (!obj)
>                 return -ENOENT;
>
>         /* Pinned buffers may be scanout, so flush the cache */
> -       if (READ_ONCE(obj->pin_display)) {
> -               err = i915_mutex_lock_interruptible(dev);
> -               if (!err) {
> -                       i915_gem_object_flush_cpu_write_domain(obj);
> -                       mutex_unlock(&dev->struct_mutex);
> -               }
> -       }
> +       if (READ_ONCE(obj->pin_display))
> +               i915_gem_clflush_object(obj, I915_CLFLUSH_FORCE);
>
>         i915_gem_object_put(obj);
> -       return err;
> +       return 0;
>  }
>
>  /**
> @@ -3155,46 +3149,6 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
>         return 0;
>  }
>
> -void i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> -                            bool force)
> -{
> -       /* If we don't have a page list set up, then we're not pinned
> -        * to GPU, and we can ignore the cache flush because it'll happen
> -        * again at bind time.
> -        */
> -       if (!obj->mm.pages) {
> -               GEM_BUG_ON(obj->base.write_domain != I915_GEM_DOMAIN_CPU);
> -               return;
> -       }
> -
> -       /*
> -        * Stolen memory is always coherent with the GPU as it is explicitly
> -        * marked as wc by the system, or the system is cache-coherent.
> -        * Similarly, we only access struct pages through the CPU cache, so
> -        * anything not backed by physical memory we consider to be always
> -        * coherent and not need clflushing.
> -        */
> -       if (!i915_gem_object_has_struct_page(obj))
> -               return;
> -
> -       /* If the GPU is snooping the contents of the CPU cache,
> -        * we do not need to manually clear the CPU cache lines.  However,
> -        * the caches are only snooped when the render cache is
> -        * flushed/invalidated.  As we always have to emit invalidations
> -        * and flushes when moving into and out of the RENDER domain, correct
> -        * snooping behaviour occurs naturally as the result of our domain
> -        * tracking.
> -        */
> -       if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> -               obj->cache_dirty = true;
> -               return;
> -       }
> -
> -       trace_i915_gem_object_clflush(obj);
> -       drm_clflush_sg(obj->mm.pages);
> -       obj->cache_dirty = false;
> -}
> -
>  /** Flushes the GTT write domain for the object if it's dirty. */
>  static void
>  i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
> @@ -3238,8 +3192,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
>         if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
>                 return;
>
> -       i915_gem_clflush_object(obj, obj->pin_display);
> -       intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> +       i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
>
>         obj->base.write_domain = 0;
>         trace_i915_gem_object_change_domain(obj,
> @@ -3610,10 +3563,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>         vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>
>         /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> -       if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU) {
> -               i915_gem_clflush_object(obj, true);
> -               intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
> -       }
> +       if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU)
> +               i915_gem_clflush_object(obj, I915_CLFLUSH_FORCE);
>
>         old_write_domain = obj->base.write_domain;
>         old_read_domains = obj->base.read_domains;
> @@ -3687,8 +3638,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
>
>         /* Flush the CPU cache if it's still invalid. */
>         if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
> -               i915_gem_clflush_object(obj, false);
> -
> +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
>                 obj->base.read_domains |= I915_GEM_DOMAIN_CPU;
>         }
>
> @@ -4560,6 +4510,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>
>         mutex_lock(&dev_priv->drm.struct_mutex);
>
> +       i915_gem_clflush_init(dev_priv);
> +
>         if (!i915.enable_execlists) {
>                 dev_priv->gt.resume = intel_legacy_submission_resume;
>                 dev_priv->gt.cleanup_engine = intel_engine_cleanup;
> diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
> new file mode 100644
> index 000000000000..1a8fc58aa925
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
> @@ -0,0 +1,194 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_frontbuffer.h"
> +
> +static DEFINE_SPINLOCK(clflush_lock);
> +static u64 clflush_context;
> +
> +struct clflushf {
> +       struct dma_fence dma;
> +       struct i915_sw_fence wait;
> +       struct work_struct work;
> +       struct drm_i915_gem_object *obj;
> +};
> +
> +static const char *i915_clflush_get_driver_name(struct dma_fence *fence)
> +{
> +       return "i915";
> +}
> +
> +static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
> +{
> +       return "clflush";
> +}
> +
> +static bool i915_clflush_enable_signaling(struct dma_fence *fence)
> +{
> +       return true;
> +}
> +
> +static void i915_clflush_release(struct dma_fence *fence)
> +{
> +       struct clflushf *f = container_of(fence, typeof(*f), dma);
> +
> +       i915_sw_fence_fini(&f->wait);
> +       dma_fence_free(&f->dma);
Don't we need a kfree(f) somewhere?

Also CI didn't look very happy, is this series meant to be standalone,
or is there something else waiting in the wings?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux