<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">
> Hi Jani and Tvrtko,
<div class="ContentPasted0">></div>
<div class="ContentPasted0">>>>> This patch is a preparation for replacing enum i915_cache_level with PAT</div>
<div class="ContentPasted0">>>>> index. Caching policy for buffer objects is set through the PAT index in</div>
<div class="ContentPasted0">>>>> PTE, the old i915_cache_level is not sufficient to represent all caching</div>
<div class="ContentPasted0">>>>> modes supported by the hardware.</div>
<div class="ContentPasted0">>>>></div>
<div class="ContentPasted0">>>>> Preparing the transition by adding some platform dependent data structures</div>
<div class="ContentPasted0">>>>> 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 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>
<div class="ContentPasted0">>>>> #endif</div>
<div class="ContentPasted0">>>>> struct drm_i915_private *i915;</div>
<div class="ContentPasted0">>>>> + struct intel_device_info *i915_info;</div>
>>>> struct pci_dev *pdev;
<div class="ContentPasted1">>>>> + unsigned int i;</div>
<div class="ContentPasted1">>>>> int ret;</div>
<div class="ContentPasted1">>>>> pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);</div>
<div class="ContentPasted1">>>>> @@ -180,6 +182,13 @@ struct drm_i915_private *mock_gem_device(void)</div>
<div class="ContentPasted1">>>>> I915_GTT_PAGE_SIZE_2M;</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 we've</div>
<div class="ContentPasted1">>>> made device info const, and localized the modifications to a couple of</div>
<div class="ContentPasted1">>>> places.</div>
<div class="ContentPasted1">>>></div>
<div class="ContentPasted1">>>> If you need to modify it, it belongs in runtime info. Even if it's only</div>
<div class="ContentPasted1">>>> 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() into</div>
<div class="ContentPasted1">>>> a pointer to const rodata [1], where you are unable to modify it, but</div>
<div class="ContentPasted1">>>> this having been merged as commit 5e352e32aec2 ("drm/i915: preparation</div>
<div class="ContentPasted1">>>> for using PAT index") sets us back. (With [1] this oopses trying to</div>
<div class="ContentPasted1">>>> 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 or</div>
<div class="ContentPasted1">>>> pointed this out?!</div>
<div class="ContentPasted1">></div>
<div class="ContentPasted1">> That's not cool, indeed.</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>
<div class="ContentPasted1">>>> BR,</div>
<div class="ContentPasted1">>>> Jani.</div>
<div class="ContentPasted1">>>></div>
>>>
<div class="ContentPasted2">>>> [1] https://patchwork.freedesktop.org/patch/msgid/0badc36ce6dd6b030507bdfd8a42ab984fb38d12.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. It</div>
<div class="ContentPasted2">>> should be trivial once i915->__info becomes a pointer so in that series I</div>
<div class="ContentPasted2">>> guess.</div>
<div class="ContentPasted2">></div>
<div class="ContentPasted2">> Fei... do you have bandwidth to look into this or do you want me</div>
<div class="ContentPasted2">> to try Tvrtko's suggestion out?</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">Please go ahead Andi if you have time to help on this.</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">> Thank you Jani for reporting it and thank you Tvrtko for</div>
<div class="ContentPasted2">> proposing the fix.</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">Sorry for the trouble here.</div>
<div><br class="ContentPasted2">
</div>
<div class="ContentPasted2">> Andi</div>
<br>
</div>
</body>
</html>