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

[Jani Nikula]

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/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;
> +
>  	intel_memory_regions_hw_probe(i915);
>  
>  	spin_lock_init(&i915->gpu_error.lock);

-- 
Jani Nikula, Intel Open Source Graphics Center