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

[Emil Velikov <emil.l.velikov@gmail.com>]

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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev