[Intel-gfx] [RFC 1/2] drm/i915: Refactor PAT/object cache handling

[Tvrtko Ursulin]

On 14/07/2023 06:36, Yang, Fei wrote:
> [snip]
>> @@ -326,10 +330,10 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data,
>>                goto out;
>>        }
>>
>> -     if (i915_gem_object_has_cache_level(obj, I915_CACHE_LLC) ||
>> -         i915_gem_object_has_cache_level(obj, I915_CACHE_L3_LLC))
>> +     if (i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_WB) &&
>> +         i915_gem_object_has_cache_flag(obj, I915_CACHE_FLAG_COH1W) == 1)
> 
> Need to check L3 flag as well? The original code has L3_LLC.
> if (i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_WB) &&
>      (i915_gem_object_has_cache_flag(obj, I915_CACHE_FLAG_COH1W) == 1 ||
>       i915_gem_object_has_cache_flag(obj, I915_CACHE_FLAG_L3) == 1))

Don't think so, because I have:

   [2] = __I915_CACHE(WB, BIT(I915_CACHE_FLAG_COH1W) | BIT(I915_CACHE_FLAG_L3)), \

So L3 and COH1W are both set for the equivalent of old I915_CACHE_L3_LLC, meaning I915_CACHING_CACHED will be returned for either - same as old code.

> 
>>                args->caching = I915_CACHING_CACHED;
> 
> [snip]
>> +int i915_cache_init(struct drm_i915_private *i915)
>> +{
>> +     struct drm_printer p = drm_info_printer(i915->drm.dev);
>> +     char buf[I915_CACHE_NAME_LEN];
>> +     int ret;
>> +
>> +     i915->cache = HAS_LLC(i915) ? I915_CACHE_CACHED : I915_CACHE_NONE;
>> +     i915_cache_print(buf, sizeof(buf), "", i915->cache);
>> +     drm_printf(&p, "Coherent cache mode %s", buf);
>> +
>> +     ret = i915_cache_find_pat(i915, I915_CACHE_NONE);
>> +     if (ret < 0)
>> +             return -ENODEV;
>> +     drm_info(&i915->drm, "Using PAT index %u for uncached access\n", ret);
>> +     i915->pat_uc = ret;
>> +
>> +     ret = i915_cache_find_pat(i915, I915_CACHE_CACHED);
>> +     if (ret < 0)
>> +             return -ENODEV;
>> +     drm_info(&i915->drm, "Using PAT index %u for write-back access\n", ret);
>> +     i915->pat_wb = ret;
> 
> I think i915->pat_wt is needed as well, because display driver has code like this,
> HAS_WT(i915) ? I915_CACHE_WT : I915_CACHE_NONE
> 
> How did you handle functions like initial_plane_vma() and i915_gem_object_pin_to_display_plane()?

They have:

	i915_gem_object_set_cache_coherency(obj, HAS_WT(i915) ?
					    I915_CACHE_WT : I915_CACHE_NONE);

Where HAS_WT is equal to HAS_EDRAM. So it is not always WT. I don't have the history/background of that but that is how it is.

So I could add i915->pat_wt but a) I don't think it is right for display (display would need more like i915->display_pat), and b) there would be no call sites for it.

i915->display_pat (or display_cache actually) would be similar to how I added i915->cache, not be best name I know, but it is replacing many occurrences of:

   cache_level = HAS_LLC(mem->i915) ? I915_CACHE_LLC : I915_CACHE_NONE;
   i915_gem_object_set_cache_coherency(obj, cache_level)

I am unsure what would be the proper name for i915->cache, which would make it correctly self-documenting?

> 
>> +     return 0;
>> +}
> 
> [snip]
>> -#define TGL_CACHELEVEL \
>> -     .cachelevel_to_pat = { \
>> -             [I915_CACHE_NONE]   = 3, \
>> -             [I915_CACHE_LLC]    = 0, \
>> -             [I915_CACHE_L3_LLC] = 0, \
>> -             [I915_CACHE_WT]     = 2, \
>> +/*
>> + * TODO/QQQ
>> + *
>> + * PAT 0 - is it 1-way or 2-way?
> 
> 1-way
> 
>> + */
>> +#define GEN12_CACHE_MODES \
>> +     .cache_modes = { \
>> +             [0] = _I915_CACHE(WB, COH1W), \
>> +             [1] = I915_CACHE(WC), \
>> +             [2] = I915_CACHE(WT), \
>> +             [3] = I915_CACHE(UC), \
>>        }
> 
> [snip]
>> -#define PVC_CACHELEVEL \
>> -     .cachelevel_to_pat = { \
>> -             [I915_CACHE_NONE]   = 0, \
>> -             [I915_CACHE_LLC]    = 3, \
>> -             [I915_CACHE_L3_LLC] = 3, \
>> -             [I915_CACHE_WT]     = 2, \
>> +/*
>> + * TODO/QQQ
>> + *
>> + * PAT 3 - is it 1-way or 2-way?
> 
> 1-way
> 
> PVC access is always coherent, it should have 1-way for [5] and [7] as well.

Thanks x2!

Regards,

Tvrtko

> 
>> + */
>> +#define PVC_CACHE_MODES \
>> +     .cache_modes = { \
>> +             [0] = I915_CACHE(UC), \
>> +             [1] = I915_CACHE(WC), \
>> +             [2] = I915_CACHE(WT), \
>> +             [3] = _I915_CACHE(WB, COH1W), \
>> +             [4] = _I915_CACHE(WT, CLOS1), \
>> +             [5] = _I915_CACHE(WB, CLOS1), \
>> +             [6] = _I915_CACHE(WT, CLOS2), \
>> +             [7] = _I915_CACHE(WB, CLOS2), \
>>        }