On Tue, 28 Jun 2022 at 22:11, Robert Beckett <bob.beckett@xxxxxxxxxxxxx> wrote:
>
>
>
> 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));
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
> >>
