| Message ID | 20170126105858.25769-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <chris@chris-wilson.co.uk> > Cc: Kenneth Graunke <kenneth@whitecape.org> > Cc: Martin Peres <martin.peres@linux.intel.com> > Cc: Chad Versace <chadversary@chromium.org> > Cc: Daniel Vetter <daniel.vetter@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-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu, Jan 26, 2017 at 09:39:51AM -0800, Chad Versace wrote: > 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(). Yes, for anything that is totally scratch just not setting the write hazard is the same. For something like the seqno page where we have multiple engines that we do want to be preserved, not settting the write hazzard had the consequence that page could be lost under memory pressure or across resume. (As usual there are some details that this part of the ABI had to be relaxed because userspace didn't have this flag.) But that doesn't sell many bananas. -Chris
On Thu, Jan 26, 2017 at 09:39:51AM -0800, Chad Versace wrote: > 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(). However... There is a hack that requires the write hazard for gen6 pipecontrols unless you use the noreloc patches (hw limitation causing pipecontrols to always use ggtt offsets not the ppgtt you have normally). -Chris
On Thu 26 Jan 2017, Chris Wilson wrote: > On Thu, Jan 26, 2017 at 09:39:51AM -0800, Chad Versace wrote: > > 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(). > > Yes, for anything that is totally scratch just not setting the write > hazard is the same. For something like the seqno page where we have > multiple engines that we do want to be preserved, not settting the write > hazzard had the consequence that page could be lost under memory pressure > or across resume. (As usual there are some details that this part of the > ABI had to be relaxed because userspace didn't have this flag.) > But that doesn't sell many bananas. Good. That's how I thought it worked.
On Thu 26 Jan 2017, Chad Versace wrote: > 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 <chris@chris-wilson.co.uk> > > Cc: Kenneth Graunke <kenneth@whitecape.org> > > Cc: Martin Peres <martin.peres@linux.intel.com> > > Cc: Chad Versace <chadversary@chromium.org> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c > > + /* 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); > > +#ifndef HAVE_DRM_INTEL_GEM_BO_DISABLE_IMPLICIT_SYNC > > +#define drm_intel_gem_bo_disable_implicit_sync(BO) do { } while (0) > > +#endif Until Mesa can actually disable the implicit sync, I think this patch should be postponed. If it landed now, it may cause additional unneccessary stalls between contexts. Chrome OS uses many contexts in the same process, so if problems exist, they'll exhibit on CrOS. Perhaps the extra stalls will be imperceptible, but I don't want to take the risk.
On 27 January 2017 at 00:01, Chad Versace <chadversary@chromium.org> wrote: > On Thu 26 Jan 2017, Chad Versace wrote: >> 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 <chris@chris-wilson.co.uk> >> > Cc: Kenneth Graunke <kenneth@whitecape.org> >> > Cc: Martin Peres <martin.peres@linux.intel.com> >> > Cc: Chad Versace <chadversary@chromium.org> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c > >> > + /* 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); > >> > +#ifndef HAVE_DRM_INTEL_GEM_BO_DISABLE_IMPLICIT_SYNC >> > +#define drm_intel_gem_bo_disable_implicit_sync(BO) do { } while (0) >> > +#endif > > Until Mesa can actually disable the implicit sync, I think this patch > should be postponed. If it landed now, it may cause additional > unneccessary stalls between contexts. Chrome OS uses many contexts in > the same process, so if problems exist, they'll exhibit on CrOS. Perhaps > the extra stalls will be imperceptible, but I don't want to take the > risk. Afaict the libdrm API is fine although we're missing a drm_intel_bufmgr_gem_can_disable_implicit_sync() call. We'd want to check that and fallback when applicable ? Please don't use wrappers like this in mesa. Just roll a new libdrm and bump the requirement. Thanks Emil
On Fri, Jan 27, 2017 at 06:20:46PM +0000, Emil Velikov wrote: > On 27 January 2017 at 00:01, Chad Versace <chadversary@chromium.org> wrote: > > On Thu 26 Jan 2017, Chad Versace wrote: > >> 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 <chris@chris-wilson.co.uk> > >> > Cc: Kenneth Graunke <kenneth@whitecape.org> > >> > Cc: Martin Peres <martin.peres@linux.intel.com> > >> > Cc: Chad Versace <chadversary@chromium.org> > >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > >> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c > > > >> > + /* 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); > > > >> > +#ifndef HAVE_DRM_INTEL_GEM_BO_DISABLE_IMPLICIT_SYNC > >> > +#define drm_intel_gem_bo_disable_implicit_sync(BO) do { } while (0) > >> > +#endif > > > > Until Mesa can actually disable the implicit sync, I think this patch > > should be postponed. If it landed now, it may cause additional > > unneccessary stalls between contexts. Chrome OS uses many contexts in > > the same process, so if problems exist, they'll exhibit on CrOS. Perhaps > > the extra stalls will be imperceptible, but I don't want to take the > > risk. > Afaict the libdrm API is fine although we're missing a > drm_intel_bufmgr_gem_can_disable_implicit_sync() call. > We'd want to check that and fallback when applicable ? > > Please don't use wrappers like this in mesa. Just roll a new libdrm > and bump the requirement. I was told that there was a preference now for a shortlived compat layer because distro's were unhappy. -Chris
On 27 January 2017 at 18:30, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Jan 27, 2017 at 06:20:46PM +0000, Emil Velikov wrote: >> On 27 January 2017 at 00:01, Chad Versace <chadversary@chromium.org> wrote: >> > On Thu 26 Jan 2017, Chad Versace wrote: >> >> 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 <chris@chris-wilson.co.uk> >> >> > Cc: Kenneth Graunke <kenneth@whitecape.org> >> >> > Cc: Martin Peres <martin.peres@linux.intel.com> >> >> > Cc: Chad Versace <chadversary@chromium.org> >> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> > >> >> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c >> > >> >> > + /* 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); >> > >> >> > +#ifndef HAVE_DRM_INTEL_GEM_BO_DISABLE_IMPLICIT_SYNC >> >> > +#define drm_intel_gem_bo_disable_implicit_sync(BO) do { } while (0) >> >> > +#endif >> > >> > Until Mesa can actually disable the implicit sync, I think this patch >> > should be postponed. If it landed now, it may cause additional >> > unneccessary stalls between contexts. Chrome OS uses many contexts in >> > the same process, so if problems exist, they'll exhibit on CrOS. Perhaps >> > the extra stalls will be imperceptible, but I don't want to take the >> > risk. >> Afaict the libdrm API is fine although we're missing a >> drm_intel_bufmgr_gem_can_disable_implicit_sync() call. >> We'd want to check that and fallback when applicable ? >> >> Please don't use wrappers like this in mesa. Just roll a new libdrm >> and bump the requirement. > > I was told that there was a preference now for a shortlived compat layer > because distro's were unhappy. As long as there's a libdrm release distros should be fine. Obviously there's always the case of someone being unhappy - in one example a distro decided to freeze their libdrm package on the exact version which badly broke nouveau. Ilia can tell you how many times he had to repeat the same suggestion - downgrade or update to a local package :-\ -Emil
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 */
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). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Kenneth Graunke <kenneth@whitecape.org> Cc: Martin Peres <martin.peres@linux.intel.com> Cc: Chad Versace <chadversary@chromium.org> Cc: Daniel Vetter <daniel.vetter@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