On Mon, Oct 23, 2017 at 10:32:43PM +0000, Jordan Justen wrote: > On 2017-10-19 16:30:44, Kristian Høgsberg wrote: > > On Thu, Oct 19, 2017 at 4:18 PM, Kenneth Graunke <kenneth@xxxxxxxxxxxxx> wrote: > > > The kernel doesn't initialize the value of the INSTPM or CS_DEBUG_MODE2 > > > registers at context initialization time. Instead, they're inherited > > > from whatever happened to be running on the GPU prior to first run of a > > > new context. So, when we started setting these, other contexts in the > > > system started inheriting our values. Since this controls whether > > > 3DSTATE_CONSTANT_* takes a pointer or an offset, getting the wrong > > > setting is fatal for almost any process which isn't expecting this. > > > > > > Unfortunately, VA-API and Beignet don't initialize this (nor does older > > > Mesa), so they will die horribly if we start doing this. UXA and SNA > > > don't use any push constants, so they are unaffected. > > > > > > The kernel developers are apparently uninterested in making the proto- > > > context initialize these registers to guarantee deterministic behavior. > > > > Could somebody from the kernel team elaborate here? This is obviously > > broken and undermines the security and containerization that hw > > contexts are supposed to provide. I'm really curious what the thinking > > is here... > > > > Kristian > > Cc intel-gfx, maintainers Is this the null-state/golden-context related discussions? I assume we are ok for older platforms, but the problem would be only for CNL+ where we are not adding the null context initialization yet. Am I getting it right? > > > > > > Apparently, the "Default Value" of registers in the documentation is a > > > total lie, and cannot be relied upon by userspace. So, we need to find > > > a new solution. One would be to patch VA-API and Beignet to initialize > > > these, then get every distributor to ship those in tandem with the newer > > > Mesa version. I'm unclear when va-intel-driver releases might happen. > > > Another option would be to hack Mesa to restore the register back to the > > > historical default (relative mode) at the end of each batch. This is > > > also gross, as it has non-zero cost, and is also relying on userspace to > > > be "polite" to other broken userspace. A large part of the motivation > > > for contexts was to provide proper process isolation, so we should not > > > have to do this. But, we're already doing it in some cases, because > > > our kernel fixes to enforce isolation were reverted. > > > > > > In the meantime, I guess we'll just revert this patch and abandon using > > > the feature. It will lead to fewer pushed UBOs on Broadwell+, which may > > > lead to lower performance, but I don't have any data on the impact. > > > > > > Cc: "17.2" <mesa-stable@xxxxxxxxxxxxxxxxxxxxx> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102774 > > > --- > > > src/mesa/drivers/dri/i965/brw_state_upload.c | 24 ------------------------ > > > src/mesa/drivers/dri/i965/intel_screen.c | 2 +- > > > 2 files changed, 1 insertion(+), 25 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c b/src/mesa/drivers/dri/i965/brw_state_upload.c > > > index 16f44d03bbe..23e4ebda259 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_state_upload.c > > > +++ b/src/mesa/drivers/dri/i965/brw_state_upload.c > > > @@ -101,30 +101,6 @@ brw_upload_initial_gpu_state(struct brw_context *brw) > > > OUT_BATCH(0); > > > ADVANCE_BATCH(); > > > } > > > - > > > - /* Set the "CONSTANT_BUFFER Address Offset Disable" bit, so > > > - * 3DSTATE_CONSTANT_XS buffer 0 is an absolute address. > > > - * > > > - * On Gen6-7.5, we use an execbuf parameter to do this for us. > > > - * However, the kernel ignores that when execlists are in use. > > > - * Fortunately, we can just write the registers from userspace > > > - * on Gen8+, and they're context saved/restored. > > > - */ > > > - if (devinfo->gen >= 9) { > > > - BEGIN_BATCH(3); > > > - OUT_BATCH(MI_LOAD_REGISTER_IMM | (3 - 2)); > > > - OUT_BATCH(CS_DEBUG_MODE2); > > > - OUT_BATCH(REG_MASK(CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE) | > > > - CSDBG2_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE); > > > - ADVANCE_BATCH(); > > > - } else if (devinfo->gen == 8) { > > > - BEGIN_BATCH(3); > > > - OUT_BATCH(MI_LOAD_REGISTER_IMM | (3 - 2)); > > > - OUT_BATCH(INSTPM); > > > - OUT_BATCH(REG_MASK(INSTPM_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE) | > > > - INSTPM_CONSTANT_BUFFER_ADDRESS_OFFSET_DISABLE); > > > - ADVANCE_BATCH(); > > > - } > > > } > > > > > > static inline const struct brw_tracked_state * > > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c > > > index ea04a72e860..776c2898d5b 100644 > > > --- a/src/mesa/drivers/dri/i965/intel_screen.c > > > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > > > @@ -2510,7 +2510,7 @@ __DRIconfig **intelInitScreen2(__DRIscreen *dri_screen) > > > screen->compiler = brw_compiler_create(screen, devinfo); > > > screen->compiler->shader_debug_log = shader_debug_log_mesa; > > > screen->compiler->shader_perf_log = shader_perf_log_mesa; > > > - screen->compiler->constant_buffer_0_is_relative = devinfo->gen < 8; > > > + screen->compiler->constant_buffer_0_is_relative = true; > > > screen->compiler->supports_pull_constants = true; > > > > > > screen->has_exec_fence = > > > -- > > > 2.14.2 > > > > > > _______________________________________________ > > > mesa-dev mailing list > > > mesa-dev@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
