Jordan Justen <jordan.l.jus...@intel.com> writes: > On 2015-12-08 08:43:53, Francisco Jerez wrote: >> This is going to require some rather intrusive kernel changes to fix >> properly, in the meantime (and forever on at least pre-v4.1 kernels) >> we'll have to restore the hardware defaults at the end of every batch >> in which the L3 configuration was changed to avoid interfering with >> the DDX and GL clients that use an older non-L3-aware version of Mesa. >> >> Reviewed-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com> >> Reviewed-by: Kristian Høgsberg <k...@bitplanet.net> >> >> v4: Optimize look-up of the default configuration by assuming it's the >> first entry of the L3 config array in order to avoid an FPS >> regression in GpuTest Triangle and SynMark OglBatch2-7 on most >> affected platforms. >> --- >> src/mesa/drivers/dri/i965/brw_state.h | 4 +++ >> src/mesa/drivers/dri/i965/gen7_l3_state.c | 51 >> +++++++++++++++++++++++++++ >> src/mesa/drivers/dri/i965/intel_batchbuffer.c | 7 ++++ >> src/mesa/drivers/dri/i965/intel_batchbuffer.h | 6 +++- >> 4 files changed, 67 insertions(+), 1 deletion(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_state.h >> b/src/mesa/drivers/dri/i965/brw_state.h >> index 49f301a..b7c0039 100644 >> --- a/src/mesa/drivers/dri/i965/brw_state.h >> +++ b/src/mesa/drivers/dri/i965/brw_state.h >> @@ -380,6 +380,10 @@ void gen7_update_binding_table_from_array(struct >> brw_context *brw, >> void gen7_disable_hw_binding_tables(struct brw_context *brw); >> void gen7_reset_hw_bt_pool_offsets(struct brw_context *brw); >> >> +/* gen7_l3_state.c */ >> +void >> +gen7_restore_default_l3_config(struct brw_context *brw); >> + >> #ifdef __cplusplus >> } >> #endif >> diff --git a/src/mesa/drivers/dri/i965/gen7_l3_state.c >> b/src/mesa/drivers/dri/i965/gen7_l3_state.c >> index 7956935..7fa7336 100644 >> --- a/src/mesa/drivers/dri/i965/gen7_l3_state.c >> +++ b/src/mesa/drivers/dri/i965/gen7_l3_state.c >> @@ -520,3 +520,54 @@ const struct brw_tracked_state gen7_l3_state = { >> }, >> .emit = emit_l3_state >> }; >> + >> +/** >> + * Hack to restore the default L3 configuration. >> + * >> + * This will be called at the end of every batch in order to reset the L3 >> + * configuration to the default values for the time being until the kernel >> is >> + * fixed. Until kernel commit 6702cf16e0ba8b0129f5aa1b6609d4e9c70bc13b >> + * (included in v4.1) we would set the MI_RESTORE_INHIBIT bit when >> submitting >> + * batch buffers for the default context used by the DDX, which meant that >> any >> + * context state changed by the GL would leak into the DDX, the assumption >> + * being that the DDX would initialize any state it cares about manually. >> The >> + * DDX is however not careful enough to program an L3 configuration >> + * explicitly, and it makes assumptions about it (URB size) which won't hold >> + * and cause it to misrender if we let our L3 set-up to leak into the DDX. >> + * >> + * Since v4.1 of the Linux kernel the default context is saved and restored >> + * normally, so it's far less likely for our L3 programming to interfere >> with >> + * other contexts -- In fact restoring the default L3 configuration at the >> end >> + * of the batch will be redundant most of the time. A kind of state leak is >> + * still possible though if the context making assumptions about L3 state is >> + * created immediately after our context was active (e.g. without the DDX >> + * default context being scheduled in between) because at present the DRM >> + * doesn't fully initialize the contents of newly created contexts and >> instead >> + * sets the MI_RESTORE_INHIBIT flag causing it to inherit the state from the >> + * last active context. >> + * >> + * It's possible to realize such a scenario if, say, an X server (or a GL >> + * application using an outdated non-L3-aware Mesa version) is started while >> + * another GL application is running and happens to have modified the L3 >> + * configuration, or if no X server is running at all and a GL application >> + * using a non-L3-aware Mesa version is started after another GL application >> + * ran and modified the L3 configuration -- The latter situation can >> actually >> + * be reproduced easily on IVB in our CI system. >> + */ >> +void >> +gen7_restore_default_l3_config(struct brw_context *brw) >> +{ >> + const struct brw_device_info *devinfo = brw->intelScreen->devinfo; >> + /* For efficiency assume that the first entry of the array matches the >> + * default configuration. >> + */ >> + const struct brw_l3_config *const cfg = get_l3_configs(devinfo); >> + assert(cfg == get_l3_config(devinfo, >> + get_default_l3_weights(devinfo, false, >> false))); >> + >> + if (cfg != brw->l3.config && brw->can_do_pipelined_register_writes) { >> + setup_l3_config(brw, cfg); >> + update_urb_size(brw, cfg); >> + brw->l3.config = cfg; >> + } >> +} >> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c >> b/src/mesa/drivers/dri/i965/intel_batchbuffer.c >> index 0363bd3..f778074 100644 >> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c >> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c >> @@ -208,6 +208,13 @@ brw_finish_batch(struct brw_context *brw) >> brw_emit_query_end(brw); >> >> if (brw->batch.ring == RENDER_RING) { >> + /* Work around L3 state leaks into contexts set MI_RESTORE_INHIBIT >> which >> + * assume that the L3 cache is configured according to the hardware >> + * defaults. > > Should we add a comment to the tables saying that the first entry > should be the default? > Sure. Added and pushed.
> Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com> > Thanks. >> + */ >> + if (brw->gen >= 7) >> + gen7_restore_default_l3_config(brw); >> + >> /* We may also need to snapshot and disable OA counters. */ >> brw_perf_monitor_finish_batch(brw); >> >> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h >> b/src/mesa/drivers/dri/i965/intel_batchbuffer.h >> index 2b177d3..f473690 100644 >> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h >> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h >> @@ -30,8 +30,12 @@ extern "C" { >> * - 5 dwords for initial mi_flush >> * - 2 dwords for CC state setup >> * - 5 dwords for the required pipe control at the end >> + * - Restoring L3 configuration: (24 dwords = 96 bytes) >> + * - 2*6 dwords for two PIPE_CONTROL flushes. >> + * - 7 dwords for L3 configuration set-up. >> + * - 5 dwords for L3 atomic set-up (on HSW). >> */ >> -#define BATCH_RESERVED 152 >> +#define BATCH_RESERVED 248 >> >> struct intel_batchbuffer; >> >> -- >> 2.5.1 >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev