* [PATCH v2 1/4] drm/i915/clflush: fixup handling of cache_dirty
@ 2021-10-27 16:18 Matthew Auld
2021-10-27 16:18 ` [PATCH v2 2/4] drm/i915/clflush: disallow on discrete Matthew Auld
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Matthew Auld @ 2021-10-27 16:18 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, Thomas Hellström
In theory if clflush_work_create() somehow fails here, and we don't yet
have mm.pages populated then we end up resetting cache_dirty, which is
likely wrong, since that will potentially skip the flush-on-acquire, if
it was needed.
It looks like intel_user_framebuffer_dirty() can arrive here before the
pages are populated.
v2(Thomas):
- Move setting cache_dirty out of the async portion, also add a
comment for why that should still be safe.
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
index f0435c6feb68..47586a8a1b73 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
@@ -109,12 +109,20 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj,
I915_FENCE_GFP);
dma_resv_add_excl_fence(obj->base.resv, &clflush->base.dma);
dma_fence_work_commit(&clflush->base);
+ /*
+ * We must have successfully populated the pages(since we are
+ * holding a pin on the pages as per the flush worker) to reach
+ * this point, which must mean we have already done the required
+ * flush-on-acquire, hence resetting cache_dirty here should be
+ * safe.
+ */
+ obj->cache_dirty = false;
} else if (obj->mm.pages) {
__do_clflush(obj);
+ obj->cache_dirty = false;
} else {
GEM_BUG_ON(obj->write_domain != I915_GEM_DOMAIN_CPU);
}
- obj->cache_dirty = false;
return true;
}
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 2/4] drm/i915/clflush: disallow on discrete
2021-10-27 16:18 [PATCH v2 1/4] drm/i915/clflush: fixup handling of cache_dirty Matthew Auld
@ 2021-10-27 16:18 ` Matthew Auld
2021-10-27 16:18 ` [PATCH v2 3/4] drm/i915: move cpu_write_needs_clflush Matthew Auld
2021-10-27 16:18 ` [PATCH v2 4/4] drm/i915: stop setting cache_dirty on discrete Matthew Auld
2 siblings, 0 replies; 4+ messages in thread
From: Matthew Auld @ 2021-10-27 16:18 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, Thomas Hellström
We seem to have an unfortunate issue where we arrive from:
i915_gem_object_flush_if_display+0x86/0xd0 [i915]
intel_user_framebuffer_dirty+0x1a/0x50 [i915]
drm_mode_dirtyfb_ioctl+0xfb/0x1b0
which can be before the pages are populated(and pinned for display), and
so i915_gem_object_has_struct_page() might still return true, as per the
ttm backend. We could re-order the later get_pages() call here, but
since on discrete everything should already be coherent, with the
exception of the display engine, and even there display surfaces must be
allocated in device local-memory anyway, so there should in theory be no
conceivable reason to ever call i915_gem_clflush_object() on discrete.
References: https://gitlab.freedesktop.org/drm/intel/-/issues/4320
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
index 47586a8a1b73..aa91252c9324 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
@@ -69,6 +69,8 @@ static struct clflush *clflush_work_create(struct drm_i915_gem_object *obj)
bool i915_gem_clflush_object(struct drm_i915_gem_object *obj,
unsigned int flags)
{
+
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct clflush *clflush;
assert_object_held(obj);
@@ -80,7 +82,7 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj,
* anything not backed by physical memory we consider to be always
* coherent and not need clflushing.
*/
- if (!i915_gem_object_has_struct_page(obj)) {
+ if (!i915_gem_object_has_struct_page(obj) || IS_DGFX(i915)) {
obj->cache_dirty = false;
return false;
}
@@ -105,7 +107,7 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj,
if (clflush) {
i915_sw_fence_await_reservation(&clflush->base.chain,
obj->base.resv, NULL, true,
- i915_fence_timeout(to_i915(obj->base.dev)),
+ i915_fence_timeout(i915),
I915_FENCE_GFP);
dma_resv_add_excl_fence(obj->base.resv, &clflush->base.dma);
dma_fence_work_commit(&clflush->base);
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 3/4] drm/i915: move cpu_write_needs_clflush
2021-10-27 16:18 [PATCH v2 1/4] drm/i915/clflush: fixup handling of cache_dirty Matthew Auld
2021-10-27 16:18 ` [PATCH v2 2/4] drm/i915/clflush: disallow on discrete Matthew Auld
@ 2021-10-27 16:18 ` Matthew Auld
2021-10-27 16:18 ` [PATCH v2 4/4] drm/i915: stop setting cache_dirty on discrete Matthew Auld
2 siblings, 0 replies; 4+ messages in thread
From: Matthew Auld @ 2021-10-27 16:18 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, Thomas Hellström
Move it next to its partner in crime; gpu_write_needs_clflush. For
better readability lets keep gpu vs cpu at least in the same file.
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 12 ++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 ++-------------
drivers/gpu/drm/i915/i915_gem.c | 2 +-
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index b684a62bf3b0..d30d5a699788 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -22,6 +22,18 @@ static bool gpu_write_needs_clflush(struct drm_i915_gem_object *obj)
obj->cache_level == I915_CACHE_WT);
}
+bool i915_gem_cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
+{
+ if (obj->cache_dirty)
+ return false;
+
+ if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_WRITE))
+ return true;
+
+ /* Currently in use by HW (display engine)? Keep flushed. */
+ return i915_gem_object_is_framebuffer(obj);
+}
+
static void
flush_write_domain(struct drm_i915_gem_object *obj, unsigned int flush_domains)
{
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index a5479ac7a4ad..de0c8145cc4c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -522,6 +522,7 @@ void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj);
void i915_gem_object_flush_if_display(struct drm_i915_gem_object *obj);
void i915_gem_object_flush_if_display_locked(struct drm_i915_gem_object *obj);
+bool i915_gem_cpu_write_needs_clflush(struct drm_i915_gem_object *obj);
int __must_check
i915_gem_object_set_to_wc_domain(struct drm_i915_gem_object *obj, bool write);
@@ -542,23 +543,11 @@ void __i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj);
void __i915_gem_object_make_purgeable(struct drm_i915_gem_object *obj);
void i915_gem_object_make_purgeable(struct drm_i915_gem_object *obj);
-static inline bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
-{
- if (obj->cache_dirty)
- return false;
-
- if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_WRITE))
- return true;
-
- /* Currently in use by HW (display engine)? Keep flushed. */
- return i915_gem_object_is_framebuffer(obj);
-}
-
static inline void __start_cpu_write(struct drm_i915_gem_object *obj)
{
obj->read_domains = I915_GEM_DOMAIN_CPU;
obj->write_domain = I915_GEM_DOMAIN_CPU;
- if (cpu_write_needs_clflush(obj))
+ if (i915_gem_cpu_write_needs_clflush(obj))
obj->cache_dirty = true;
}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 981e383d1a5d..d0e642c82064 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -764,7 +764,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
* perspective, requiring manual detiling by the client.
*/
if (!i915_gem_object_has_struct_page(obj) ||
- cpu_write_needs_clflush(obj))
+ i915_gem_cpu_write_needs_clflush(obj))
/* Note that the gtt paths might fail with non-page-backed user
* pointers (e.g. gtt mappings when moving data between
* textures). Fallback to the shmem path in that case.
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 4/4] drm/i915: stop setting cache_dirty on discrete
2021-10-27 16:18 [PATCH v2 1/4] drm/i915/clflush: fixup handling of cache_dirty Matthew Auld
2021-10-27 16:18 ` [PATCH v2 2/4] drm/i915/clflush: disallow on discrete Matthew Auld
2021-10-27 16:18 ` [PATCH v2 3/4] drm/i915: move cpu_write_needs_clflush Matthew Auld
@ 2021-10-27 16:18 ` Matthew Auld
2 siblings, 0 replies; 4+ messages in thread
From: Matthew Auld @ 2021-10-27 16:18 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, Thomas Hellström
Should not be needed. Even with non-coherent display, we should be using
device local-memory there, and not system memory.
v2: also add a warning in i915_gem_clflush_object
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> #v1
---
drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 7 ++++++-
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 10 ++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.c | 7 +++++--
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 1 +
4 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
index aa91252c9324..539b2116b2d5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
@@ -75,6 +75,11 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj,
assert_object_held(obj);
+ if (IS_DGFX(i915)) {
+ WARN_ON_ONCE(obj->cache_dirty);
+ return false;
+ }
+
/*
* Stolen memory is always coherent with the GPU as it is explicitly
* marked as wc by the system, or the system is cache-coherent.
@@ -82,7 +87,7 @@ bool i915_gem_clflush_object(struct drm_i915_gem_object *obj,
* anything not backed by physical memory we consider to be always
* coherent and not need clflushing.
*/
- if (!i915_gem_object_has_struct_page(obj) || IS_DGFX(i915)) {
+ if (!i915_gem_object_has_struct_page(obj)) {
obj->cache_dirty = false;
return false;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index d30d5a699788..26532c07d467 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -18,18 +18,28 @@
static bool gpu_write_needs_clflush(struct drm_i915_gem_object *obj)
{
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+
+ if (IS_DGFX(i915))
+ return false;
+
return !(obj->cache_level == I915_CACHE_NONE ||
obj->cache_level == I915_CACHE_WT);
}
bool i915_gem_cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
{
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+
if (obj->cache_dirty)
return false;
if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_WRITE))
return true;
+ if (IS_DGFX(i915))
+ return false;
+
/* Currently in use by HW (display engine)? Keep flushed. */
return i915_gem_object_is_framebuffer(obj);
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 55b0f1df3192..591ee3cb7275 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -114,18 +114,21 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj)
void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
unsigned int cache_level)
{
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+
obj->cache_level = cache_level;
if (cache_level != I915_CACHE_NONE)
obj->cache_coherent = (I915_BO_CACHE_COHERENT_FOR_READ |
I915_BO_CACHE_COHERENT_FOR_WRITE);
- else if (HAS_LLC(to_i915(obj->base.dev)))
+ else if (HAS_LLC(i915))
obj->cache_coherent = I915_BO_CACHE_COHERENT_FOR_READ;
else
obj->cache_coherent = 0;
obj->cache_dirty =
- !(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_WRITE);
+ !(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_WRITE) &&
+ !IS_DGFX(i915);
}
bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 308e22a80af4..c4f684b7cc51 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -26,6 +26,7 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
/* Make the pages coherent with the GPU (flushing any swapin). */
if (obj->cache_dirty) {
+ WARN_ON_ONCE(IS_DGFX(i915));
obj->write_domain = 0;
if (i915_gem_object_has_struct_page(obj))
drm_clflush_sg(pages);
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-27 16:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-27 16:18 [PATCH v2 1/4] drm/i915/clflush: fixup handling of cache_dirty Matthew Auld
2021-10-27 16:18 ` [PATCH v2 2/4] drm/i915/clflush: disallow on discrete Matthew Auld
2021-10-27 16:18 ` [PATCH v2 3/4] drm/i915: move cpu_write_needs_clflush Matthew Auld
2021-10-27 16:18 ` [PATCH v2 4/4] drm/i915: stop setting cache_dirty on discrete Matthew Auld
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).