Re: [Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings
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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings
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; } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings
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. > +/* 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?) 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings
From: Ben WidawskyStarting with GEN9, Memory Object Control State (MOCS) becomes an index into a table as opposed to the direct programming within the command. The table has 62 usable entries (ie 6 bits can represent all settings), and each buffer type may use one of these 62 entries to describe cacheability type, and age (and some other less useful fields). Because we hadn't dealt with MOCS settings like this, we didn't think ahead too well and have ended up with a mess for GEN9 (and soon GEN10) platform. The plan for for future platforms is that the ideal MOCS settings will be determined, defined, and written in the public PRMs. After this point, the i915.ko will absorb these settings and sometime afterwards flip the alpha switch. All driver releases without the final MOCS table must be considered alpha. Here on, userspace can assume the MOCS table is definitively done. There will be some reserved entries for 'oh shit' scenarios. This avoids versioning the MOCS table which leaves somewhat of a mess in userspace trying to handle arbitrarily many MOCS versions. But we do have a mess on GEN9. In the beginning, the MOCS table entries were pre-populated by the hardware based on estimations made prior to tapeout and we could just use that. Subsequently much performance tuning was done to determine optimal settings that the i915 driver should load on top of the hardware defaults. That was posted last as v6 of the original per-engine MOCS settings: https://patchwork.freedesktop.org/patch/53237/. Since the MOCS table is not context saved/restored, it isn't feasible to let userspace upload its own MOCS table. After a good amount of debate, it was decided that we'd utilize only the minimal set of entires in mesa anyway, and so we took only those entries for our MOCS entries. Now we've come to the realization that indeed there are other MOCS entries which are more optimal for various buffer types and workloads. The problem is that the meaning of the indices is ABI (we assume index 0 is the uncached entry, and that there are only 3 entries total). What this patch [simply] aims to do is expose a parameter to inform userspace which "version" of the table was loaded by i915. Upon sufficient data, new entries can be added, and the version can be bumped. For example, from my original mesa mocs branch: commit c9b0481bce24af032386701de0266eb5bc24e988 Author: Ben Widawsky Date: Fri Apr 8 10:21:16 2016 -0700 i965: Use PTE mocs Signed-off-by: Ben Widawsky diff --git a/src/mesa/drivers/dri/i965/brw_mocs.c b/src/mesa/drivers/dri/i965/brw_mocs.c index 5df154eb86..b7bfdab671 100644 --- a/src/mesa/drivers/dri/i965/brw_mocs.c +++ b/src/mesa/drivers/dri/i965/brw_mocs.c @@ -14,6 +14,9 @@ /* Skylake: MOCS is now an index into an array of 62 different caching * configurations programmed by the kernel. */ + +/* TC=PTE, LeCC=PTE, LRUM=3, L3CC=WB */ +#define SKL_MOCS_PTE_PTE (3 << 1) /* TC=LLC/eLLC, LeCC=WB, LRUM=3, L3CC=WB */ #define SKL_MOCS_WB (2 << 1) /* TC=LLC/eLLC, LeCC=PTE, LRUM=3, L3CC=WB */ @@ -26,6 +29,9 @@ brw_mocs_get_control_state(const struct brw_context *brw, switch (brw->gen) { default: case 9: + if (brw->intelScreen->mocs_version > 1) + return SKL_MOCS_PTE_PTE; + return type == INTEL_MOCS_PTE ? SKL_MOCS_PTE : SKL_MOCS_WB; case 8: return type == INTEL_MOCS_PTE ? BDW_MOCS_PTE : BDW_MOCS_WB; tl;dr: A versioned MOCS table will allow userspace to be aware of new and potentially interesting cacheability settings. Next GEN platforms will not be considered production worthy until the MOCS table is finalized. v2: Update 1.5 year old patch. Add comments. Update commit message. Signed-off-by: Ben Widawsky --- 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; + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index effbe4f72a64..9b30f6e6ef9b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -859,6 +859,8 @@ struct intel_device_info { u16 degamma_lut_size; u16