On Thu 26 Jan 2017, Chris Wilson wrote: > Since the workaround bo is used strictly as a write-only buffer, we need > only allocate one per screen and use the same one from all contexts. > > (The caveat here is during extension initialisation, where we write into > and read back register values from the buffer, but that is performed only > once for the first context - and baring synchronisation issues should not > be a problem. Safer would be to move that also to the screen.) > > v2: Give the workaround bo its own init function and don't piggy back > intel_bufmgr_init() since it is not that related. > > v3: Drop the reference count of the workaround bo for the context since > the context itself is owned by the screen (and so we can rely on the bo > existing for the lifetime of the context).
I like this idea, but I have questions and comments about the details. More questions than comments, really. Today, with only Mesa changes, could we effectively do the same as drm_intel_gem_bo_disable_implicit_sync(screen->workaround_bo); by hacking Mesa to set no read/write domain when emitting relocs for the workaround_bo? (I admit I don't fully understand the kernel's domain tracking). If that does work, then it just would require a small hack to brw_emit_pipe_control_write(). > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> > Cc: Kenneth Graunke <kenn...@whitecape.org> > Cc: Martin Peres <martin.pe...@linux.intel.com> > Cc: Chad Versace <chadvers...@chromium.org> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch> > --- > src/mesa/drivers/dri/i965/Makefile.am | 2 +- > src/mesa/drivers/dri/i965/brw_pipe_control.c | 12 +++++----- > src/mesa/drivers/dri/i965/intel_screen.c | 24 ++++++++++++++++++++ > src/mesa/drivers/dri/i965/intel_screen.h | 1 + > src/mesa/drivers/dri/i965/libdrm_compat.h | 33 > ++++++++++++++++++++++++++++ > 5 files changed, 64 insertions(+), 8 deletions(-) > create mode 100644 src/mesa/drivers/dri/i965/libdrm_compat.h > > diff --git a/src/mesa/drivers/dri/i965/Makefile.am > b/src/mesa/drivers/dri/i965/Makefile.am > index 6602a17995..b208563f7d 100644 > --- a/src/mesa/drivers/dri/i965/Makefile.am > +++ b/src/mesa/drivers/dri/i965/Makefile.am > @@ -77,7 +77,7 @@ noinst_LTLIBRARIES = \ > libi965_compiler.la \ > $(I965_PERGEN_LIBS) > > -libi965_dri_la_SOURCES = $(i965_FILES) > +libi965_dri_la_SOURCES = $(i965_FILES) libdrm_compat.h > libi965_dri_la_LIBADD = \ > $(top_builddir)/src/intel/common/libintel_common.la \ > $(top_builddir)/src/intel/isl/libisl.la \ > diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c > b/src/mesa/drivers/dri/i965/brw_pipe_control.c > index b8f740640f..22c946f744 100644 > --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c > +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c > @@ -371,20 +371,18 @@ brw_init_pipe_control(struct brw_context *brw, > /* We can't just use brw_state_batch to get a chunk of space for > * the gen6 workaround because it involves actually writing to > * the buffer, and the kernel doesn't let us write to the batch. > + * > + * As the screen has a long lifetime than the contexts derived from > + * it, we do not need to add our own reference count and can simply > + * rely on the bo always existing for the duration of the context. > */ > - brw->workaround_bo = drm_intel_bo_alloc(brw->bufmgr, > - "pipe_control workaround", > - 4096, 4096); > - if (brw->workaround_bo == NULL) > - return -ENOMEM; > + brw->workaround_bo = brw->screen->workaround_bo; > > brw->pipe_controls_since_last_cs_stall = 0; > - > return 0; > } > > void > brw_fini_pipe_control(struct brw_context *brw) > { > - drm_intel_bo_unreference(brw->workaround_bo); > } > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c > b/src/mesa/drivers/dri/i965/intel_screen.c > index 5f800008c1..6e788c41cc 100644 > --- a/src/mesa/drivers/dri/i965/intel_screen.c > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > @@ -107,6 +107,7 @@ DRI_CONF_END > #include "brw_context.h" > > #include "i915_drm.h" > +#include "libdrm_compat.h" > > /** > * For debugging purposes, this returns a time in seconds. > @@ -1030,6 +1031,7 @@ intelDestroyScreen(__DRIscreen * sPriv) > { > struct intel_screen *screen = sPriv->driverPrivate; > > + drm_intel_bo_unreference(screen->workaround_bo); > dri_bufmgr_destroy(screen->bufmgr); > driDestroyOptionInfo(&screen->optionCache); > > @@ -1210,6 +1212,25 @@ intel_init_bufmgr(struct intel_screen *screen) > } > > static bool > +intel_init_workaround_bo(struct intel_screen *screen) > +{ > + /* A small scratch bo shared by all contexts, primarily used > + * for doing PIPECONTROL serialisation writes that are discarded. > + */ > + screen->workaround_bo = > + drm_intel_bo_alloc(screen->bufmgr, "pipe_control w/a", 4096, 4096); > + > + /* We want to use this bo from any and all contexts, without undue > + * writing ordering between them. To prevent the kernel enforcing > + * the order due to writes from different contexts, we disable > + * the use of (the kernel's) implicit sync on this bo. > + */ > + drm_intel_gem_bo_disable_implicit_sync(screen->workaround_bo); > + > + return screen->workaround_bo != NULL; > +} > + > +static bool > intel_detect_swizzling(struct intel_screen *screen) > { > drm_intel_bo *buffer; > @@ -1675,6 +1696,9 @@ __DRIconfig **intelInitScreen2(__DRIscreen *dri_screen) > if (!intel_init_bufmgr(screen)) > return false; > > + if (!intel_init_workaround_bo(screen)) > + return false; > + > screen->deviceID = drm_intel_bufmgr_gem_get_devid(screen->bufmgr); > if (!gen_get_device_info(screen->deviceID, &screen->devinfo)) > return false; > diff --git a/src/mesa/drivers/dri/i965/intel_screen.h > b/src/mesa/drivers/dri/i965/intel_screen.h > index 890dd9044b..0fb83e724f 100644 > --- a/src/mesa/drivers/dri/i965/intel_screen.h > +++ b/src/mesa/drivers/dri/i965/intel_screen.h > @@ -74,6 +74,7 @@ struct intel_screen > #define KERNEL_ALLOWS_COMPUTE_DISPATCH (1<<4) > > dri_bufmgr *bufmgr; > + drm_intel_bo *workaround_bo; > > /** > * A unique ID for shader programs. > diff --git a/src/mesa/drivers/dri/i965/libdrm_compat.h > b/src/mesa/drivers/dri/i965/libdrm_compat.h > new file mode 100644 > index 0000000000..bef9a1286b > --- /dev/null > +++ b/src/mesa/drivers/dri/i965/libdrm_compat.h > @@ -0,0 +1,33 @@ > +/* > + * Copyright © 2016 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + */ > + > +#ifndef __LIBDRM_COMPAT_H > +#define __LIBDRM_COMPAT_H > + > +#include <intel_bufmgr.h> > + > +#ifndef HAVE_DRM_INTEL_GEM_BO_DISABLE_IMPLICIT_SYNC > +#define drm_intel_gem_bo_disable_implicit_sync(BO) do { } while (0) > +#endif > + > +#endif /* !__LIBDRM_COMPAT_H */ > -- > 2.11.0 > > _______________________________________________ > mesa-dev mailing list > mesa-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx