[Intel-gfx] [PATCH v8 1/2] drm/i915: preparation for using PAT index

[Yang, Fei]

> On 27/06/2023 14:28, Jani Nikula wrote:
>> On Tue, 09 May 2023, fei.yang at intel.com wrote:
>>> From: Fei Yang <fei.yang at intel.com>
>>>
>>> This patch is a preparation for replacing enum i915_cache_level with
>>> PAT index. Caching policy for buffer objects is set through the PAT
>>> index in PTE, the old i915_cache_level is not sufficient to represent
>>> all caching modes supported by the hardware.
>>>
>>> Preparing the transition by adding some platform dependent data
>>> structures and helper functions to translate the cache_level to pat_index.
>>>
>>> cachelevel_to_pat: a platform dependent array mapping cache_level to
>>>                     pat_index.
>>>
>>> max_pat_index: the maximum PAT index recommended in hardware specification
>>>                 Needed for validating the PAT index passed in from user
>>>                 space.
>>>
>>> i915_gem_get_pat_index: function to convert cache_level to PAT index.
>>>
>>> obj_to_i915(obj): macro moved to header file for wider usage.
>>>
>>> I915_MAX_CACHE_LEVEL: upper bound of i915_cache_level for the
>>>                        convenience of coding.
>>>
>>> Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>> Signed-off-by: Fei Yang <fei.yang at intel.com>
>>> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda at intel.com>
>>
>> [snip]
>>
>>> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>>> b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>>> index f6a7c0bd2955..0eda8b4ee17f 100644
>>> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>>> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
>>> @@ -123,7 +123,9 @@ struct drm_i915_private *mock_gem_device(void)
>>>     static struct dev_iommu fake_iommu = { .priv = (void *)-1 };
>>>   #endif
>>>     struct drm_i915_private *i915;
>>> +   struct intel_device_info *i915_info;
>>>     struct pci_dev *pdev;
>>> +   unsigned int i;
>>>     int ret;
>>>
>>>     pdev = kzalloc(sizeof(*pdev), GFP_KERNEL); @@ -180,6 +182,13 @@
>>> struct drm_i915_private *mock_gem_device(void)
>>>             I915_GTT_PAGE_SIZE_2M;
>>>
>>>     RUNTIME_INFO(i915)->memory_regions = REGION_SMEM;
>>> +
>>> +   /* simply use legacy cache level for mock device */
>>> +   i915_info = (struct intel_device_info *)INTEL_INFO(i915);
>>
>> This is not okay. It's not okay to modify device info at runtime. This
>> is why we've separated runtime info from device info. This is why
>> we've made device info const, and localized the modifications to a
>> couple of places.
>>
>> If you need to modify it, it belongs in runtime info. Even if it's
>> only ever modified for mock devices.
>>
>> We were at the brink of being able to finally convert INTEL_INFO()
>> into a pointer to const rodata [1], where you are unable to modify it,
>> but this having been merged as commit 5e352e32aec2 ("drm/i915:
>> preparation for using PAT index") sets us back. (With [1] this oopses
>> trying to modify read-only data.)
>>
>> This has been posted to the public list 20+ times, and nobody noticed
>> or pointed this out?!
>>
>> Throwing away const should be a huge red flag to any developer or
>> reviewer. Hell, *any* cast should be.
>>
>> I've got a patch ready moving cachelevel_to_pat and max_pat_index to
>> runtime info. It's not great, since we'd be doing it only for the mock
>> device. Better ideas? I'm not waiting long.
>>
>>
>> BR,
>> Jani.
>>
>>
>> [1]
>> https://patchwork.freedesktop.org/patch/msgid/0badc36ce6dd6b030507bdfd
>> 8a42ab984fb38d12.1686236840.git.jani.nikula at intel.com
>>
>>
>>> +   i915_info->max_pat_index = 3;
>>> +   for (i = 0; i < I915_MAX_CACHE_LEVEL; i++)
>>> +           i915_info->cachelevel_to_pat[i] = i;
>>> +
>
> I'd simply suggest having a local static const table for the mock device.
> It should be trivial once i915->__info becomes a pointer so in that series
> I guess.
>
> While I am here - Fei - any plans to work on the promised cleanup?
> Abstracting the caching modes with a hw agnostic sw/driver representation,
> if you remember what we discussed.

Yes, I'm still working on that as a side task. Hopefully I would be able to
post something to the mailing list after the July 4th holiday.

> Regards,
>
> Tvrtko

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20230627/b50c08ab/attachment.htm>