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;
Message-ID: <a453d562-7e93-aef3-a533-171f572b6ee3@collabora.com>
Date:   Fri, 12 May 2023 05:33:22 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v6 0/3] Add sync object UAPI support to VirtIO-GPU driver
To:     Gurchetan Singh <gurchetansingh@chromium.org>,
        Rob Clark <robdclark@gmail.com>
Cc:     Gerd Hoffmann <kraxel@redhat.com>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, kernel@collabora.com,
        virtualization@lists.linux-foundation.org,
        David Airlie <airlied@redhat.com>,
        Chia-I Wu <olvaffe@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        =?UTF-8?B?TWFyZWsgT2zFocOhaw==?= <maraeo@gmail.com>,
        Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>,
        Emil Velikov <emil.velikov@collabora.com>
References: <20230416115237.798604-1-dmitry.osipenko@collabora.com>
 <141b928d-6165-f282-b8e6-f140cb09333d@collabora.com>
 <CAAfnVBnrUotph4TYJVu9Bohqv3m80t90V34TNhh-Tspxwsj-ZQ@mail.gmail.com>
 <CAF6AEGs4fuq4i8UJdO5hvgHTNhzFMKGZ87+w1oyvL0LAqWio6A@mail.gmail.com>
 <CAAfnVBkLhYVaSG3U_QUZwXLFv-XT=9F2v2pgrCDQQBgNZ3MSWA@mail.gmail.com>
Content-Language: en-US
From:   Dmitry Osipenko <dmitry.osipenko@collabora.com>
In-Reply-To: <CAAfnVBkLhYVaSG3U_QUZwXLFv-XT=9F2v2pgrCDQQBgNZ3MSWA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/12/23 03:17, Gurchetan Singh wrote:
...
> Can we get one of the Mesa MRs reviewed first?  There's currently no
> virtio-intel MR AFAICT, and the amdgpu one is marked as "Draft:".
> 
> Even for the amdgpu, Pierre suggests the feature "will be marked as
> experimental both in Mesa and virglrenderer" and we can revise as needed.
> The DRM requirements seem to warn against adding an UAPI too hastily...
> 
> You can get the deqp-vk 1.2 tests to pass with the current UAPI, if you
> just change your mesa <--> virglrenderer protocol a little.  Perhaps that
> way is even better, since you plumb the in sync-obj into host-side command
> submission.
> 
> Without inter-context sharing of the fence, this MR really only adds guest
> kernel syntactic sugar.
> 
> Note I'm not against syntactic sugar, but I just want to point out that you
> can likely merge the native context work without any UAPI changes, in case
> it's not clear.
> 
> If this was for the core drm syncobj implementation, and not just
>> driver ioctl parsing and wiring up the core helpers, I would agree
>> with you.
>>
> 
> There are several possible and viable paths to get the features in question
> (VK1.2 syncobjs, and inter-context fence sharing).  There are paths
> entirely without the syncobj, paths that only use the syncobj for the
> inter-context fence sharing case and create host syncobjs for VK1.2, paths
> that also use guest syncobjs in every proxied command submission.
> 
> It's really hard to tell which one is better.  Here's my suggestion:
> 
> 1) Get the native contexts reviewed/merged in Mesa/virglrenderer using the
> current UAPI.  Options for VK1.2 include: pushing down the syncobjs to the
> host, and simulating the syncobj (as already done).  It's fine to mark
> these contexts as "experimental" like msm-experimental.  That will allow
> you to experiment with the protocols, come up with tests, and hopefully
> determine an answer to the host versus guest syncobj question.
> 
> 2) Once you've completed (1), try to add UAPI changes for features that are
> missing or things that are suboptimal with the knowledge gained from doing
> (2).
> 
> WDYT?

Having syncobj support available by DRM driver is a mandatory
requirement for native contexts because userspace (Mesa) relies on sync
objects support presence. In particular, Intel Mesa driver checks
whether DRM driver supports sync objects to decide which features are
available, ANV depends on the syncobj support.

I'm not familiar with a history of Venus and its limitations. Perhaps
the reason it's using host-side syncobjs is to have 1:1 Vulkan API
mapping between guest and host. Not sure if Venus could use guest
syncobjs instead or there are problems with that.

When syncobj was initially added to kernel, it was done from the needs
of supporting Vulkan wait API. For Venus the actual Vulkan driver is on
host side, while for native contexts it's on guest side. Native contexts
don't need syncobj on host side, it will be unnecessary overhead for
every nctx to have it on host. Hence, if there is no good reason for
host-side syncobjs, then why do that?

Native contexts pass deqp synchronization tests, they use sync objects
universally for both GL and VK. Games work, piglit/deqp passing. What
else you're wanting to test? Turnip?

The AMDGPU code has been looked and it looks good. It's a draft for now
because of the missing sync objects UAPI and other virglrender/Qemu
changes required to get KMS working. Maybe it will be acceptable to
merge the Mesa part once kernel will get sync objects supported, will
need to revisit it.

I'm not opening MR for virtio-intel because it has open questions that
need to be resolved first.

-- 
Best regards,
Dmitry