<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof ContentPasted0 ContentPasted1 ContentPasted2">
> On 27/06/2023 14:28, Jani Nikula wrote:
<div class="ContentPasted0">>> On Tue, 09 May 2023, fei.yang@intel.com wrote:</div>
<div class="ContentPasted0">>>> From: Fei Yang <fei.yang@intel.com></div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> This patch is a preparation for replacing enum i915_cache_level with</div>
<div class="ContentPasted0">>>> PAT index. Caching policy for buffer objects is set through the PAT</div>
<div class="ContentPasted0">>>> index in PTE, the old i915_cache_level is not sufficient to represent</div>
<div class="ContentPasted0">>>> all caching modes supported by the hardware.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> Preparing the transition by adding some platform dependent data</div>
<div class="ContentPasted0">>>> structures and helper functions to translate the cache_level to pat_index.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> cachelevel_to_pat: a platform dependent array mapping cache_level to</div>
<div class="ContentPasted0">>>> pat_index.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> max_pat_index: the maximum PAT index recommended in hardware specification</div>
<div class="ContentPasted0">>>> Needed for validating the PAT index passed in from user</div>
<div class="ContentPasted0">>>> space.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> i915_gem_get_pat_index: function to convert cache_level to PAT index.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> obj_to_i915(obj): macro moved to header file for wider usage.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> I915_MAX_CACHE_LEVEL: upper bound of i915_cache_level for the</div>
<div class="ContentPasted0">>>> convenience of coding.</div>
<div class="ContentPasted0">>>></div>
<div class="ContentPasted0">>>> Cc: Chris Wilson <chris.p.wilson@linux.intel.com></div>
<div class="ContentPasted0">>>> Cc: Matt Roper <matthew.d.roper@intel.com></div>
<div class="ContentPasted0">>>> Signed-off-by: Fei Yang <fei.yang@intel.com></div>
<div class="ContentPasted0">>>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com></div>
<div class="ContentPasted0">>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com></div>
<div class="ContentPasted0">>></div>
<div class="ContentPasted0">>> [snip]</div>
<div class="ContentPasted0">>></div>
<div class="ContentPasted0">>>> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c</div>
<div class="ContentPasted0">>>> b/drivers/gpu/drm/i915/selftests/mock_gem_device.c</div>
<div class="ContentPasted0">>>> index f6a7c0bd2955..0eda8b4ee17f 100644</div>
<div class="ContentPasted0">>>> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c</div>
<div class="ContentPasted0">>>> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c</div>
<div class="ContentPasted0">>>> @@ -123,7 +123,9 @@ struct drm_i915_private *mock_gem_device(void)</div>
<div class="ContentPasted0">>>> static struct dev_iommu fake_iommu = { .priv = (void *)-1 };</div>
>>> #endif
<div class="ContentPasted1">>>> struct drm_i915_private *i915;</div>
<div class="ContentPasted1">>>> + struct intel_device_info *i915_info;</div>
<div class="ContentPasted1">>>> struct pci_dev *pdev;</div>
<div class="ContentPasted1">>>> + unsigned int i;</div>
<div class="ContentPasted1">>>> int ret;</div>
<div class="ContentPasted1">>>></div>
<div class="ContentPasted1">>>> pdev = kzalloc(sizeof(*pdev), GFP_KERNEL); @@ -180,6 +182,13 @@</div>
<div class="ContentPasted1">>>> struct drm_i915_private *mock_gem_device(void)</div>
<div class="ContentPasted1">>>> I915_GTT_PAGE_SIZE_2M;</div>
<div class="ContentPasted1">>>></div>
<div class="ContentPasted1">>>> RUNTIME_INFO(i915)->memory_regions = REGION_SMEM;</div>
<div class="ContentPasted1">>>> +</div>
<div class="ContentPasted1">>>> + /* simply use legacy cache level for mock device */</div>
<div class="ContentPasted1">>>> + i915_info = (struct intel_device_info *)INTEL_INFO(i915);</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> This is not okay. It's not okay to modify device info at runtime. This</div>
<div class="ContentPasted1">>> is why we've separated runtime info from device info. This is why</div>
<div class="ContentPasted1">>> we've made device info const, and localized the modifications to a</div>
<div class="ContentPasted1">>> couple of places.</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> If you need to modify it, it belongs in runtime info. Even if it's</div>
<div class="ContentPasted1">>> only ever modified for mock devices.</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> We were at the brink of being able to finally convert INTEL_INFO()</div>
<div class="ContentPasted1">>> into a pointer to const rodata [1], where you are unable to modify it,</div>
<div class="ContentPasted1">>> but this having been merged as commit 5e352e32aec2 ("drm/i915:</div>
<div class="ContentPasted1">>> preparation for using PAT index") sets us back. (With [1] this oopses</div>
<div class="ContentPasted1">>> trying to modify read-only data.)</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> This has been posted to the public list 20+ times, and nobody noticed</div>
<div class="ContentPasted1">>> or pointed this out?!</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> Throwing away const should be a huge red flag to any developer or</div>
<div class="ContentPasted1">>> reviewer. Hell, *any* cast should be.</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>> I've got a patch ready moving cachelevel_to_pat and max_pat_index to</div>
<div class="ContentPasted1">>> runtime info. It's not great, since we'd be doing it only for the mock</div>
<div class="ContentPasted1">>> device. Better ideas? I'm not waiting long.</div>
<div class="ContentPasted1">>></div>
<div class="ContentPasted1">>></div>
>> BR,
<div class="ContentPasted2">>> Jani.</div>
<div class="ContentPasted2">>></div>
<div class="ContentPasted2">>></div>
<div class="ContentPasted2">>> [1]</div>
<div class="ContentPasted2">>> https://patchwork.freedesktop.org/patch/msgid/0badc36ce6dd6b030507bdfd</div>
<div class="ContentPasted2">>> 8a42ab984fb38d12.1686236840.git.jani.nikula@intel.com</div>
<div class="ContentPasted2">>></div>
<div class="ContentPasted2">>></div>
<div class="ContentPasted2">>>> + i915_info->max_pat_index = 3;</div>
<div class="ContentPasted2">>>> + for (i = 0; i < I915_MAX_CACHE_LEVEL; i++)</div>
<div class="ContentPasted2">>>> + i915_info->cachelevel_to_pat[i] = i;</div>
<div class="ContentPasted2">>>> +</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">> I'd simply suggest having a local static const table for the mock device.</div>
<div class="ContentPasted2">> It should be trivial once i915->__info becomes a pointer so in that series</div>
<div class="ContentPasted2">> I guess.</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">> While I am here - Fei - any plans to work on the promised cleanup?</div>
<div class="ContentPasted2">> Abstracting the caching modes with a hw agnostic sw/driver representation,</div>
<div class="ContentPasted2">> if you remember what we discussed.</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">Yes, I'm still working on that as a side task. Hopefully I would be able to</div>
<div class="ContentPasted2">post something to the mailing list after the July 4th holiday.</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">> Regards,</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">> Tvrtko</div>
<br>
</div>
</body>
</html>