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

[Chris Wilson]

Quoting Ben Widawsky (2017-07-07 19:42:25)
> 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.

I figured you were going towards per-gen versioning, which is kind of
why I liked the idea of table size -- but that only makes sense if
somehow the index has the same meaning across gen (which it won't).

> >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;
> }

Indeed 6 of one, half a dozen of the other. Whichever you pick, 3 years
down the line you wish you picked the other. The big advantage of using
an absolute version is that you can just stuff these into tables. Ok, I
like that more, a version parameter (that may be per-gen) worksforme.
-Chris