| Message ID | 20220614011350.122168-3-adrian.larumbe@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remove shmem backend and region and unify them with TTM | expand |
On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe <adrian.larumbe@collabora.com> 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@collabora.com> > --- > 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. > - } > > /* The cache-level will be applied when each vma is rebound. */ > return i915_gem_object_unbind(obj, > -- > 2.36.1 >
On 14/06/2022 18:55, Matthew Auld wrote: > On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe > <adrian.larumbe@collabora.com> 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@collabora.com> >> --- >> 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 >>
On Tue, 28 Jun 2022 at 22:11, Robert Beckett <bob.beckett@collabora.com> wrote: > > > > On 14/06/2022 18:55, Matthew Auld wrote: > > On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe > > <adrian.larumbe@collabora.com> 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@collabora.com> > >> --- > >> 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)); Yeah, seems reasonable to me. > > along with the assignment in flush_write_domain() I think this one should already be covered by the check in gpu_write_needs_flush(). > > 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 > >>
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; - } /* The cache-level will be applied when each vma is rebound. */ return i915_gem_object_unbind(obj,
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@collabora.com> --- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)