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

[Emil Velikov]

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.

Thanks
Emil