|
> On 27/06/2023 14:28, Jani Nikula wrote:
>> On Tue, 09 May 2023, fei.yang@xxxxxxxxx wrote:
>>> From: Fei Yang <fei.yang@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxx>
>>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>> Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
>>> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
>>
>> [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@xxxxxxxxx
>>
>>
>>> + 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
|
