[Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings

[Ben Widawsky]

On 17-07-07 11:34:48, Chris Wilson wrote:
>Quoting Ben Widawsky (2017-07-07 00:27:01)
>>  drivers/gpu/drm/i915/i915_drv.c |  3 +++
>>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>>  drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
>>  include/uapi/drm/i915_drm.h     |  8 ++++++++
>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 9167a73f3c69..26c27b6ae814 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>>                 if (!value)
>>                         return -ENODEV;
>>                 break;
>> +       case I915_PARAM_MOCS_TABLE_VERSION:
>> +               value = INTEL_INFO(dev_priv)->mocs_version;
>
>If we use intel_mocs_get_table_version() we can put this magic number
>in intel_mocs.c next to the tables, where we can keep its history and
>hopefully be able to remember to update it.
>

Yeah, that seems like an improvement to me as well.

>> +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
>> + * non-optimal settings for the MOCS table. As a result, we were required to use a
>> + * small subset, and later add new settings. This param allows userspace to
>> + * determine which settings are there.
>> + */
>> +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */
>
>How are you planing to share this? When we update we bump this number,
>and then mesa copies it across and uses it after verifying it as 0,1 on
>an old kernel.
>
>I don't think you want to expose the updated constant here, but symbolic
>names for each version? (What would be the point?)
>

At least one thing wrong here is we would need per GEN constants, which is maybe
what you meant and I misunderstood. Assuming you had per GEN constants, which I
don't like, I believe everything works out fine. So, I'll remove this compile
time MOCS versioning.

>Next question, why a version number and not just the number of entries
>defined? Each index is defined by ABI once assigned, so the number of
>entries still operates as a version number and allows easy checking.
>
>	if (advanced_cacheing_idx < kernel_max_mocs)
>		return advanced_cacheing_idx;
>	if (default_cacheing_idx < kernel_max_mocs)
>		return default_cacheing_idx;
>
>	return follow_pte_idx;
>
>give or take the smarts to choose the preferred indices for any
>particular scenario.
>
>In the future, if we finally get user defined mocs, the table_size will
>then give the start of the user modifiable indices (presming they want
>to keep the predefined entries for compatibility?))
>-Chris

Yes, I considered this as well. I see no difference really as to one versus the
other. In fact, if you're to support multiple table versions, I think it's
actually easier with a pure version:

switch (kernel_mocs_version) {
case 3:
	return new_best_cacheing_index;
case 2:
	return old_best_cacheing_index;
case 1:
	return naive_best_index;
}