[Intel-gfx] [PATCH 2/3] drm/i915/ttm: don't overwrite cache_dirty after setting coherency

[Robert Beckett]

On 14/06/2022 18:55, Matthew Auld wrote:
> On Tue, 14 Jun 2022 at 02:14, Adrian Larumbe
> <adrian.larumbe at 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 at 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
>>