Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230720001443.2380-1-dakr@redhat.com> <20230720001443.2380-12-dakr@redhat.com>
 <CAOFGe945tp344=g-++=EAT89t0qJHZ=3yeW-k9OTbGNJodvwAg@mail.gmail.com>
In-Reply-To: <CAOFGe945tp344=g-++=EAT89t0qJHZ=3yeW-k9OTbGNJodvwAg@mail.gmail.com>
From:   Dave Airlie <airlied@gmail.com>
Date:   Mon, 24 Jul 2023 09:54:34 +1000
Message-ID: <CAPM=9tyQQTn=+Af1kWySde75T=MLFeboTErLv3NxqtQ8pvqtzA@mail.gmail.com>
Subject: Re: [PATCH drm-misc-next v8 11/12] drm/nouveau: implement new VM_BIND uAPI
To:     Faith Ekstrand <faith@gfxstrand.net>
Cc:     Danilo Krummrich <dakr@redhat.com>, daniel@ffwll.ch,
        tzimmermann@suse.de, mripard@kernel.org, corbet@lwn.net,
        christian.koenig@amd.com, bskeggs@redhat.com,
        Liam.Howlett@oracle.com, matthew.brost@intel.com,
        boris.brezillon@collabora.com, alexdeucher@gmail.com,
        ogabbay@kernel.org, bagasdotme@gmail.com, willy@infradead.org,
        jason@jlekstrand.net, donald.robson@imgtec.com,
        dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, 23 Jul 2023 at 01:12, Faith Ekstrand <faith@gfxstrand.net> wrote:
>
> On Wed, Jul 19, 2023 at 7:15=E2=80=AFPM Danilo Krummrich <dakr@redhat.com=
> wrote:
>>
>> This commit provides the implementation for the new uapi motivated by th=
e
>> Vulkan API. It allows user mode drivers (UMDs) to:
>>
>> 1) Initialize a GPU virtual address (VA) space via the new
>>    DRM_IOCTL_NOUVEAU_VM_INIT ioctl for UMDs to specify the portion of VA
>>    space managed by the kernel and userspace, respectively.
>>
>> 2) Allocate and free a VA space region as well as bind and unbind memory
>>    to the GPUs VA space via the new DRM_IOCTL_NOUVEAU_VM_BIND ioctl.
>>    UMDs can request the named operations to be processed either
>>    synchronously or asynchronously. It supports DRM syncobjs
>>    (incl. timelines) as synchronization mechanism. The management of the
>>    GPU VA mappings is implemented with the DRM GPU VA manager.
>>
>> 3) Execute push buffers with the new DRM_IOCTL_NOUVEAU_EXEC ioctl. The
>>    execution happens asynchronously. It supports DRM syncobj (incl.
>>    timelines) as synchronization mechanism. DRM GEM object locking is
>>    handled with drm_exec.
>>
>> Both, DRM_IOCTL_NOUVEAU_VM_BIND and DRM_IOCTL_NOUVEAU_EXEC, use the DRM
>> GPU scheduler for the asynchronous paths.
>
>
> IDK where the best place to talk about this is but this seems as good as =
any.
>
> I've been looking into why the Vulkan CTS runs about 2x slower for me on =
the new UAPI and I created a little benchmark to facilitate testing:
>
> https://gitlab.freedesktop.org/mesa/crucible/-/merge_requests/141
>
> The test, roughly, does the following:
>  1. Allocates and binds 1000 BOs
>  2. Constructs a pushbuf that executes a no-op compute shader.
>  3. Does a single EXEC/wait combo to warm up the kernel
>  4. Loops 10,000 times, doing SYNCOBJ_RESET (fast), EXEC, and then SYNCOB=
J_WAIT and times the loop
>
> Of course, there's a bit of userspace driver overhead but that's negledga=
ble.
>
> If you drop the top patch which allocates 1k buffers, the submit time on =
the old uAPI is 54 us/exec vs. 66 us/exec on the new UAPI. This includes th=
e time to do a SYNCOBJ_RESET (fast), EXEC, and SYNCOBJ_WAIT. The Intel driv=
er, by comparison, is 33us/exec so it's not syncobj overhead. This is a bit=
 concerning (you'd think the new thing would be faster) but what really has=
 me concerned is the 1k buffer case.
>
> If you include the top patch in the crucible MR, it allocates 1000 BOs an=
d VM_BINDs them. All the binding is done before the warmup EXEC. Suddenly, =
the submit time jumps to 257 us/exec with the new UAPI. The old UAPI is muc=
h worse (1134 us/exec) but that's not the point. Once we've done the first =
EXEC and created our VM bindings, the cost per EXEC shouldn't change at all=
 based on the number of BOs bound.  Part of the point of VM_BIND is to get =
all that binding logic and BO walking off the EXEC path.
>
> Normally, I wouldn't be too worried about a little performance problem li=
ke this. This is the first implementation and we can improve it later. I ge=
t that. However, I suspect the solution to this problem involves more UAPI =
and I want to make sure we have it all before we call this all done and dus=
ted and land it.
>
> The way AMD solves this problem as well as the new Xe driver for Intel is=
 to have a concept of internal vs. external BOs. Basically, there's an INTE=
RNAL bit specified somewhere in BO creation that has a few userspace implic=
ations:
>  1. In the Xe world where VMs are objects, INTERNAL BOs are assigned a VM=
 on creation and can never be bound to any other VM.
>  2. Any attempt to export an INTERNAL BO via prime or a similar mechanism=
 will fail with -EINVAL (I think?).

Okay I've done a first pass at implementing this,

https://gitlab.freedesktop.org/nouvelles/kernel/-/commit/005a0005ff80ec85f3=
e9a314cae0576b7441076c

Danilo we should probably pick that up for the next iteration. with
corresponding mesa work it gets the latency for me from 180us down to
70us.

Dave.