[Intel-gfx] [Mesa-dev] [PATCH] i965: Share the workaround bo between all contexts

[Chris Wilson]

On Fri, Jan 27, 2017 at 06:20:46PM +0000, Emil Velikov wrote:
> On 27 January 2017 at 00:01, Chad Versace <chadversary at 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 at chris-wilson.co.uk>
> >> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> >> > Cc: Martin Peres <martin.peres at linux.intel.com>
> >> > Cc: Chad Versace <chadversary at chromium.org>
> >> > Cc: Daniel Vetter <daniel.vetter at 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

-- 
Chris Wilson, Intel Open Source Technology Centre