On 14/06/2022 18:55, Matthew Auld wrote:
On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe
<adrian.larumbe@xxxxxxxxxxxxx> wrote:
When i915_gem_object_set_cache_level sets the GEM object's cache_dirty to
true, in the case of TTM that will sometimes be overwritten when getting
the object's pages, more specifically for shmem-placed objects for which
its ttm structure has just been populated.
This wasn't an issue so far, even though intel_dpt_create was setting the
object's cache level to 'none', regardless of the platform and memory
placement of the framebuffer. However, commit 2f0ec95ed20c ("drm/i915/ttm:
dont trample cache_level overrides during ttm move") makes sure the cache
level set by older backends soon to be managed by TTM isn't altered after
their TTM bo ttm structure is populated.
However this led to the 'obj->cache_dirty = true' set in
i915_gem_object_set_cache_level to stick around rather than being reset
inside i915_ttm_adjust_gem_after_move after calling ttm_tt_populate in
__i915_ttm_get_pages, which eventually led to a warning in DGFX platforms.
There also seems to be no need for this statement to be kept in
i915_gem_object_set_cache_level, since i915_gem_object_set_cache_coherency
is already taking care of it, and also considering whether it's a discrete
platform.
Remove statement altogether.
Signed-off-by: Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_domain.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index 3e5d6057b3ef..b2c9e16bfa55 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -273,10 +273,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
return ret;
/* Always invalidate stale cachelines */
- if (obj->cache_level != cache_level) {
+ if (obj->cache_level != cache_level)
i915_gem_object_set_cache_coherency(obj, cache_level);
- obj->cache_dirty = true;
Maybe ban calling this on dgpu or have it fail silently? At the ioctl
level this should already be banned.
Ignoring dgpu, the cache_dirty handling is quite thorny on non-LLC
platforms. I'm not sure if there are other historical reasons for
having this here, but one big issue is that we are not allowed to
freely set cache_dirty = false, unless we are certain that the pages
underneath have been populated and the potential flush-on-acquire
completed. See the kernel-doc for @cache_dirty for more details.
given the commit "068b1bd09253 drm/i915: stop setting cache_dirty on
discrete"
with it's justification of cache_dirty should not be set on discreet as
it is not needed, I think this patch should change to set
obj->cache_dirty = !IS_DGFX(to_i915(obj->base.dev));
along with the assignment in flush_write_domain()
and should be considered a fix for that patch.
It should keep the asignment for integrated as it's original purpose
still holds there.
- }
/* The cache-level will be applied when each vma is rebound. */
return i915_gem_object_unbind(obj,
--
2.36.1
