Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Jun 25, 2022 at 03:58:17PM +0200, Christian König wrote:
> Am 24.06.22 um 22:34 schrieb Daniel Vetter:
> > Digging out of a hole, apologies to everyone.
> 
> No problem, I'm totally overworked as well.
> 
> > On Fri, Jun 17, 2022 at 03:08:00PM +0200, Christian König wrote:
> > > Am 17.06.22 um 15:03 schrieb Bas Nieuwenhuizen:
> > > > [SNIP]
> > > BOOKKEEP is exactly for that, but as discussed with Daniel that's not what
> > > we want in the kernel.
> > Not sure which Daniel you talked to, but this wasn't me.
> 
> Hui what? Of course I'm talking about you.
> 
> > > When you mix implicit with explicit synchronization (OpenGL with RADV for
> > > example) it should be mandatory for the OpenGL to wait for any RADV
> > > submission before issuing an operation.
> > > 
> > > What you want to do is intentionally not supported.
> > vk is very intentional in it's rejecting of any implicit sync.
> 
> [SNIP]
> 
> > We should probably also document this in the kerneldoc for the BOOKKEEPING
> > usage that this is the fence type that vulkan cs should use in all
> > drivers, otherwise this will become an endless mess of driver specific
> > hacks (i.e. the world we currently live in).
> 
> Well, Daniel somehow we are somehow not talking about the same thing here :)
> 
> I've documented exactly what you describe above in the initial patch which
> added BOOKKEEPING (I've still called it OTHER in that iteration):
> 
> > >/+ /**/
> > >/+ * @DMA_RESV_USAGE_OTHER: No implicit sync./
> > >/+ */
> > >/+ * This should be used for operations which don't want to add an/
> > >/+ * implicit dependency at all, but still have a dependency on memory/
> > >/+ * management./
> > >/+ */
> > >/+ * This might include things like preemption fences as well as device/
> > >/+ * page table updates or even userspace command submissions./
> > >/+ */
> > >/+ * The kernel memory management *always* need to wait for those fences/
> > >/+ * before moving or freeing the resource protected by the dma_resv/
> > >/+ * object./
> > >/+ *//
> > >/+ DMA_RESV_USAGE_OTHER/
> 
> Later on I've even explicitly mentioned that this is for Vulkan submissions.
> 
> But it was *you* who made me remove that with the explanation that we have
> to use READ for that or we break existing userspace.

Hm the only discussion I've found I actually mentioend we should highlight
that vk should use OTHER even more than what you had. Quoting myself:

> +      * This might include things like preemption fences as well as device
> +      * page table updates or even userspace command submissions.

I think we should highlight a bit more that for explicitly synchronized
userspace like vk OTHER is the normal case. So really not an exception.
Ofc aside from amdkgf there's currently no driver doing this, but really
we should have lots of them ...

See https://lore.kernel.org/dri-devel/YZ+y+Uwo809qtvs5@phenom.ffwll.local/

I didn't find anything else. So not sure how we managed to create
confusion here :-(

> I mean that still makes a lot of sense to me because if I'm not completely
> mistaken we do have use cases which break, especially Vulkan+encoding.

Yeah I think we only have some communication fumble here, nothing else :-)

And yes libva doesn't have any support for vk's explicit sync model, so
that will just fall flat on its face. Might motivate a few folks to fix
libva :-)

Note that on i915 side it's exactly the same, we've also been setting the
READ fence thus far. Since the breakage will be introduced by upgrading
mesa we'll at least avoid the kernel regression complaints, or at least I
hope we can get away with that.

Since really I don't have any idea how it could be fixed otherwise, except
through some really, really terrible hacks. Maybe kernel module option or
so.

Anyway I think all we need is just a patch to the dma_resv docs to explain
that USAGE_BOOKKEEPING is what vulkan userspace wants, and why. Bas,
you're up to typing that?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch