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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/06/2023 14:28, Jani Nikula wrote:
> On Tue, 09 May 2023, fei.yang@intel.com wrote:
>> From: Fei Yang <fei.yang@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@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Signed-off-by: Fei Yang <fei.yang@intel.com>
>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@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/0badc36ce6dd6b030507bdfd8a42ab984fb38d12.1686236840.git.jani.nikula@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.

Regards,

Tvrtko