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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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/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.

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux