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

[Chad Versace]

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.