On Fri, May 12, 2023 at 2:23 PM Gurchetan Singh <gurchetansi...@chromium.org> wrote: > > > > On Thu, May 11, 2023 at 7:33 PM Dmitry Osipenko > <dmitry.osipe...@collabora.com> wrote: >> >> 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. > > > Why not submit a Venus MR? It's already in-tree, and you can see how your > API works in scenarios with a host side timeline semaphore (aka syncobj). I > think they are also interested in fencing/sync improvements. > >> >> >> 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? > > > Depends on your threading model. You can have the following scenarios: > > 1) N guest contexts : 1 host thread > 2) N guest contexts : N host threads for each context > 3) 1:1 thread > > I think the native context is single-threaded (1), IIRC? If the goal is to > push command submission to the host (for inter-context sharing), I think > you'll at-least want (2). For a 1:1 model (a la gfxstream), one host thread > can put another thread's out_sync_objs as it's in_sync_objs (in the same > virtgpu context). I think that's kind of the goal of timeline semaphores, > with the example given by Khronos as with a compute thread + a graphics > thread. > > I'm not saying one threading model is better than any other, perhaps the > native context using the host driver in the guest is so good, it doesn't > matter. I'm just saying these are the types of discussions we can have if we > tried to get one the Mesa MRs merged first ;-) > >> >> 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? > > > Turnip would also fulfill the requirements, since most of the native context > stuff is already wired for freedreno. > >> >> >> 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. > > > Get it out of draft mode then :-). How long would that take? > > Also, there's crosvm which builds on standard Linux, so I wouldn't consider > QEMU patches as a requirement. Just Mesa/virglrenderer part. > >> >> Maybe it will be acceptable to >> merge the Mesa part once kernel will get sync objects supported, will >> need to revisit it. > > > You can think of my commentary as the following suggestions: > > - You can probably get native contexts and deqp-vk 1.2 working with the > current UAPI > - It would be terrific to see inter-context fence sharing working (with the > wait pushed down to the host), that's something the current UAPI can't do > - Work iteratively (i.e, it's fine to merge Mesa/virglrenderer MRs as > "experimental") and in steps, no need to figure everything out at once > > Now these are just suggestions, and while I think they are good, you can > safely ignore them. > > But there's also the DRM requirements, which state "userspace side must be > fully reviewed and tested to the standards of that user-space project.". So > I think to meet the minimum requirements, I think we should at-least have one > of the following (not all, just one) reviewed: > > 1) venus using the new uapi > 2) gfxstream vk using the new uapi > 3) amdgpu nctx out of "draft" mode and using the new uapi. > 4) virtio-intel using new uapi > 5) turnip using your new uapi
forgot to mention this earlier, but https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23533 Dmitry, you can also add, if you haven't already: Tested-by: Rob Clark <robdcl...@gmail.com> > Depending on which one you chose, maybe we can get it done within 1-2 weeks? > >> I'm not opening MR for virtio-intel because it has open questions that >> need to be resolved first. >> >> -- >> Best regards, >> Dmitry >>
