[Mesa-dev] [PATCH] i965: Revert absolute mode for constant buffer pointers.

Mon Oct 23 22:53:15 UTC 2017

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 at whitecape.org> 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 at lists.freedesktop.org>
> > > 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev