[Intel-gfx] [PATCH 3/6] drm/i915: Track pages pinned due to swizzling quirk
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Nov 1 08:39:14 UTC 2016
On 31/10/2016 10:26, Chris Wilson wrote:
> If we have a tiled object and an unknown CPU swizzle pattern, we pin the
> pages to prevent the object from being swapped out (and us corrupting
> the contents as we do not know the access pattern and so cannot convert
> it to linear and back to tiled on reuse). This requires us to remember
> to drop the extra pinning when freeing the object, or else we trigger
> warnings about the pin leak. In commit fbbd37b36fa5 ("drm/i915: Move
> object release to a freelist + worker"), the object free path was
> deferred to a work, but the unpinning of the quirk, along with marking
> the object as reclaimable, was left on the immediate path (so that if
> required we could reclaim the pages under memory pressure as early as
> possible). However, this split introduced a bug where the pages we no
> longer being unpinned if they were marked as unneeded.
Last sentence is broken.
>
> [ 231.800401] WARNING: CPU: 1 PID: 90 at drivers/gpu/drm/i915/i915_gem.c:4275 __i915_gem_free_objects+0x326/0x3c0 [i915]
> [ 231.800403] WARN_ON(i915_gem_object_has_pinned_pages(obj))
> [ 231.800405] Modules linked in:
> [ 231.800406] snd_hda_intel i915 snd_hda_codec_generic mei_me snd_hda_codec coretemp snd_hwdep mei lpc_ich snd_hda_core snd_pcm e1000e ptp pps_core [last unloaded: i915]
> [ 231.800426] CPU: 1 PID: 90 Comm: kworker/1:4 Tainted: G U 4.9.0-rc2-CI-CI_DRM_1780+ #1
> [ 231.800428] Hardware name: LENOVO 7465CTO/7465CTO, BIOS 6DET44WW (2.08 ) 04/22/2009
> [ 231.800456] Workqueue: events __i915_gem_free_work [i915]
> [ 231.800459] ffffc9000034fc80 ffffffff8142dd65 ffffc9000034fcd0 0000000000000000
> [ 231.800465] ffffc9000034fcc0 ffffffff8107e4e6 000010b300000001 0000000000001000
> [ 231.800469] ffff88011d3db740 ffff880130ef0000 0000000000000000 ffff880130ef5ea0
> [ 231.800474] Call Trace:
> [ 231.800479] [<ffffffff8142dd65>] dump_stack+0x67/0x92
> [ 231.800484] [<ffffffff8107e4e6>] __warn+0xc6/0xe0
> [ 231.800487] [<ffffffff8107e54a>] warn_slowpath_fmt+0x4a/0x50
> [ 231.800491] [<ffffffff811d12ac>] ? kmem_cache_free+0x2dc/0x340
> [ 231.800520] [<ffffffffa009ef36>] __i915_gem_free_objects+0x326/0x3c0 [i915]
> [ 231.800548] [<ffffffffa009effe>] __i915_gem_free_work+0x2e/0x50 [i915]
> [ 231.800552] [<ffffffff8109c27c>] process_one_work+0x1ec/0x6b0
> [ 231.800555] [<ffffffff8109c1f6>] ? process_one_work+0x166/0x6b0
> [ 231.800558] [<ffffffff8109c789>] worker_thread+0x49/0x490
> [ 231.800561] [<ffffffff8109c740>] ? process_one_work+0x6b0/0x6b0
> [ 231.800563] [<ffffffff8109c740>] ? process_one_work+0x6b0/0x6b0
> [ 231.800566] [<ffffffff810a2aab>] kthread+0xeb/0x110
> [ 231.800569] [<ffffffff810a29c0>] ? kthread_park+0x60/0x60
> [ 231.800573] [<ffffffff818164a7>] ret_from_fork+0x27/0x40
>
> Fixes: fbbd37b36fa5 ("drm/i915: Move object release to a freelist + worker")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 ++++++
> drivers/gpu/drm/i915/i915_gem.c | 21 +++++++++++++--------
> drivers/gpu/drm/i915/i915_gem_tiling.c | 9 +++++++--
> 3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 42a499681966..7a18bf66f797 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2304,6 +2304,12 @@ struct drm_i915_gem_object {
> * pages were last acquired.
> */
> bool dirty:1;
> +
> + /**
> + * This is set if the object has been pinned due to unknown
> + * swizzling.
> + */
> + bool quirked:1;
> } mm;
>
> /** Breadcrumb of last rendering to the buffer.
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b9f540b16a45..c58b7cabe87b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2324,8 +2324,10 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> i915_gem_object_do_bit_17_swizzle(obj, st);
>
> if (i915_gem_object_is_tiled(obj) &&
> - dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES)
> + dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
> __i915_gem_object_pin_pages(obj);
> + obj->mm.quirked = true;
> + }
>
> return st;
>
> @@ -4091,10 +4093,15 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
> if (obj->mm.pages &&
> i915_gem_object_is_tiled(obj) &&
> dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
> - if (obj->mm.madv == I915_MADV_WILLNEED)
> + if (obj->mm.madv == I915_MADV_WILLNEED) {
> + GEM_BUG_ON(!obj->mm.quirked);
> __i915_gem_object_unpin_pages(obj);
> - if (args->madv == I915_MADV_WILLNEED)
> + obj->mm.quirked = false;
> + }
> + if (args->madv == I915_MADV_WILLNEED) {
> __i915_gem_object_pin_pages(obj);
> + obj->mm.quirked = true;
> + }
> }
>
> if (obj->mm.madv != __I915_MADV_PURGED)
> @@ -4335,14 +4342,12 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> {
> struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
>
> + if (obj->mm.quirked)
> + __i915_gem_object_unpin_pages(obj);
> +
> if (discard_backing_storage(obj))
> obj->mm.madv = I915_MADV_DONTNEED;
>
> - if (obj->mm.pages && obj->mm.madv == I915_MADV_WILLNEED &&
> - to_i915(obj->base.dev)->quirks & QUIRK_PIN_SWIZZLED_PAGES &&
> - i915_gem_object_is_tiled(obj))
> - __i915_gem_object_unpin_pages(obj);
> -
This reordering would not have been enough to fix this?
> /* Before we free the object, make sure any pure RCU-only
> * read-side critical sections are complete, e.g.
> * i915_gem_busy_ioctl(). For the corresponding synchronized
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 6395e62bd9e4..1577e7810cd6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -263,10 +263,15 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
> if (obj->mm.pages &&
> obj->mm.madv == I915_MADV_WILLNEED &&
> dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
> - if (args->tiling_mode == I915_TILING_NONE)
> + if (args->tiling_mode == I915_TILING_NONE) {
> + GEM_BUG_ON(!obj->mm.quirked);
> __i915_gem_object_unpin_pages(obj);
> - if (!i915_gem_object_is_tiled(obj))
> + obj->mm.quirked = false;
> + }
> + if (!i915_gem_object_is_tiled(obj)) {
> __i915_gem_object_pin_pages(obj);
> + obj->mm.quirked = true;
> + }
> }
> mutex_unlock(&obj->mm.lock);
>
>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list