|
> Hi Jani and Tvrtko,
>
>>>> 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?!
>
> That's not cool, indeed.
>
>>> 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/0badc36ce6dd6b030507bdfd8a42ab984fb38d12.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.
>
> Fei... do you have bandwidth to look into this or do you want me
> to try Tvrtko's suggestion out?
Please go ahead Andi if you have time to help on this.
> Thank you Jani for reporting it and thank you Tvrtko for
> proposing the fix.
Sorry for the trouble here.
> Andi
|
