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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
