Re: [PATCH v3 00/14] drm: Add a driver for CSF-based Mali GPUs

[Boris Brezillon <boris.brezillon@collabora.com>]

Hi,

On Sun, 10 Dec 2023 13:58:51 +0900
Tatsuyuki Ishi <ishitatsuyuki@gmail.com> wrote:

> > On Dec 5, 2023, at 2:32, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > 
> > Hello,
> > 
> > This is the 3rd version of the kernel driver for Mali CSF-based GPUs.
> > 
> > With all the DRM dependencies being merged (drm-sched single entity and
> > drm_gpuvm), I thought now was a good time to post a new version. Note
> > that the iommu series we depend on [1] has been merged recently. The
> > only remaining dependency that hasn't been merged yet is this rather
> > trival drm_gpuvm [2] patch.
> > 
> > As for v2, I pushed a branch based on drm-misc-next and containing
> > all the dependencies that are not yet available in drm-misc-next
> > here[3], and another [4] containing extra patches to have things
> > working on rk3588. The CSF firmware binary can be found here[5], and
> > should be placed under /lib/firmware/arm/mali/arch10.8/mali_csffw.bin.
> > 
> > The mesa MR adding v10 support on top of panthor is available here [6].
> > 
> > Regarding the GPL2+MIT relicensing, Liviu did an audit and found two
> > more people that I didn't spot initially: Clément Péron for the devfreq
> > code, and Alexey Sheplyakov for some bits in panthor_gpu.c. Both are
> > Cc-ed on the relevant patches. The rest of the code is either new, or
> > covered by the Linaro, Arm and Collabora acks.
> > 
> > And here is a non-exhaustive changelog, check each commit for a detailed
> > changelog.
> > 
> > v3;
> > - Quite a few changes at the MMU/sched level to make the fix some
> >  race conditions and deadlocks
> > - Addition of the a sync-only VM_BIND operation (to support
> >  vkQueueSparseBind with zero commands).  
> 
> Hi Boris,
> 
> Just wanted to point out that vkQueueBindSparse's semantics is rather different
> from vkQueueSubmit when it comes to synchronization.  In short,
> vkQueueBindSparse does not operate on a particular timeline (aka scheduling
> queue / drm_sched_entity).  The property of following a timeline order is known
> as “submission order” [1] in Vulkan, and applies to vkQueueSubmit only and not
> vkQueueBindSparse.

Hm, okay. I really though the same ordering guarantees applied to
sparse binding queues too, as the spec [1] says

"
Batches begin execution in the order they appear in pBindInfo, but
may complete out of order.
"

which means things are submitted in order inside a vkQueueSparseBind
context, so I was expecting the submission ordering guarantee to apply
across vkQueueSparseBind() calls on the same queue too. Just want to
mention that all kernel implementations I have seen so far assume
VM_BIND submissions on a given queue are serialized (that's how
drm_sched works, and Xe, Nouveau and Panthor are basing their VM_BIND
implemenation on drm_sched).

Maybe Faith, or anyone deeply involved in the Vulkan specification, can
confirm that submission ordering guarantees are relaxed on sparse
binding queues.

> 
> Hence, an implementation that takes full advantage of Vulkan semantics would
> essentially have an infinite amount of VM_BIND contexts.

Uh, that's definitely not how I understood it initially...

> It would also not need
> sync-only VM_BIND submissions, assuming that drmSyncobjTransfer works.

Sure, if each vkQueueSparseBind() has its own timeline, an internal
timeline-syncobj with a bunch of drmSyncobjTransfer() calls would do the
trick (might require several ioctls() to merge input syncobjs, but
that's probably not the end of the world).

> 
> I’m not saying that the driver should always be implemented that way; in
> particular, app developers are also confused by the semantics and native Vulkan
> games can be terribly wrong [2].

Okay, thanks for the pointer. If I may, I find this semantics utterly
confusing, to say the least. At the very least, the vkQueueBindSparse()
doc could be update so it clearly reflects the facts submission order
is not guaranteed across vkQueueBindSparse() calls, because right now
it's only suggested in the [3], by the lack of vkQueueBindSparse()
mention in the first bullet, which can be interpreted as a genuine
omission rather than an expected behavior.

Regards,

Boris

[1]https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkQueueBindSparse.html
[2]https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkQueueBindSparse.html#VUID-vkQueueBindSparse-pBindInfo-parameter
[3]https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#synchronization-submission-order