Re: [PATCH v1 9/9] drivers: hv: dxgkrnl: Implement DXGSYNCFILE

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 17, 2022 at 9:34 AM Iouri Tarassov
<iourit@linux.microsoft.com> wrote:
>
>
> On 1/14/2022 10:03 AM, Daniel Vetter wrote:
> > Hi all,
> >
> > On Wed, Jan 12, 2022 at 11:55:14AM -0800, Iouri Tarassov wrote:
> > > Implement the LX_DXCREATESYNCFILE IOCTL (D3DKMTCreateSyncFile).
> > >
> > > dxgsyncfile is built on top of the Linux sync_file object and
> > > provides a way for the user mode to synchronize with the execution
> > > of the device DMA packets.
> > >
> > > The IOCTL creates a dxgsyncfile object for the given GPU synchronization
> > > object and a fence value. A sync_object file descriptor is returned to
> > > the caller. The caller could wait for the object by using poll().
> > > When the GPU synchronization object is signaled on the host, the host
> > > sends a message to the virtual machine and the sync_file object is
> > > signaled.
> > >
> > > Signed-off-by: Iouri Tarassov <iourit@linux.microsoft.com>
> >
> > Adding dri-devel, which get_maintainers.pl should have done automatically
> > with the dma_fence wildcard match. Not sure why that didn't happen.
> >
> > > +struct dxgsyncpoint {
> > > +   struct dxghostevent     hdr;
> > > +   struct dma_fence        base;
> >
> > This doesn't work unfortuntately. For better or worse memory fences like
> > monitored fences from wddm have completely different semantics from
> > dma_fence. You could probably hack this to be self-consistent for hyper-v,
> > but the problem is that then hv would have incompatible locking/nesting
> > rules compared to everything else, and dma_fence matter for memory
> > management so this includes whether you're allowed to kmalloc(GFP_KERNEL)
> > or not, and that's just a bit too much.
> >
> > I discussed this quickly with Jesse on irc and it sounds like the reason
> > you want the dma_fence is just to emulate the sync_file interface for
> > android. I think the correct solution here is to create a hv_dxg_sync_file
> > fd, which emulates the exact ioctls that Android needs, but with a wddm
> > monitored fence underneath instead of a dma_fence underneath.
> >
> > This way we guarantee that no one ever accidentally mixes these
> > incompatible concepts up in the kernel, and Android should still be able
> > to happily run under hyperv.
> >
> > Thoughts?
> >
> > Also pls cc me on this sync work since even if you drop dma_fence use
> > completely I'd like to follow this a bit.
>
> Hi Daniel,
>
> Thank you for the review and feedback.
> I will get this addressed.

btw another idea I had over the w/e: Another option might be to allow
different backends for sync_file, and then making sure that you cannot
ever mix dma_fence and hv_dxg_fence type sync_file up (in e.g. the
merge ioctl).

The issue is that fundamentally dma_fence and memory fences (or umf
for userspace memory fences as we tend to call them) aren't
compatible, but some of the interop plans we have is to allow stuffing
either of them into fence container objects like sync_file. So going
that route for wddm monitored fence support too could be a really
future-proof approach, plus it'd allow you to still share the
sync_file interface code. Not that it's going to be much code sharing,
since all the implementation code needs to be distinct.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch