Nouveau - Sep 2020 - [PATCH 0/6] drm/nouveau: Support sync FDs and sync objects

On Wed, Sep 23, 2020 at 11:18:53AM +0200, Thierry Reding
wrote:
> On Fri, Aug 28, 2020 at 12:40:10PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding at nvidia.com>
> > 
> > Hi,
> > 
> > This series implements a new IOCTL to submit push buffers that can
> > optionally return a sync FD or sync object to userspace. This is
useful
> > in cases where userspace wants to synchronize operations between the
GPU
> > and another driver (such as KMS for display). Among other things this
> > allows extensions such as eglDupNativeFenceFDANDROID to be
implemented.
> > 
> > Note that patch 4 modifies the ABI introduced in patch 3 by allowing
DRM
> > sync objects to be passed rather than only sync FDs. It also allows
any
> > number of sync FDs/objects to be passed in or emitted. I think those
are
> > useful features, but I left them in a separate patch in case everybody
> > else thinks that this won't be needed. If we decide to merge the
new ABI
> > then patch 4 should be squashed into patch 3.
> > 
> > The corresponding userspace changes can be found here:
> > 
> >   libdrm:
https://gitlab.freedesktop.org/tagr/drm/-/commits/nouveau-sync-fd-v2/
> >   mesa:
https://gitlab.freedesktop.org/tagr/mesa/-/commits/nouveau-sync-fd/
> > 
> > I've verified that this works with kmscube's --atomic mode and
Weston.
> 
> Hi Ben,
> 
> any thoughts on this series? I realize that this is somewhat suboptimal
> because we're effectively adding a duplicate of the existing IOCTL with
> only the "minor" extension of adding sync FDs/objects, but at the
same
> time I don't have any good ideas on what else to add to make this more
> appealing or if you have any plans of your own to address this in the
> future.
drm core automatically zero-extends ioctl structs both ways, so if all you
do is add more stuff to the top level ioctl struct at the bottom, there's
no need to duplicate any code. At least as long as you guarantee that 0 =old
behaviour for both in and out parameters.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Wed, Sep 23, 2020 at 05:21:24PM +0200, Daniel Vetter
wrote:
> On Wed, Sep 23, 2020 at 11:18:53AM +0200, Thierry Reding wrote:
> > On Fri, Aug 28, 2020 at 12:40:10PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding at nvidia.com>
> > > 
> > > Hi,
> > > 
> > > This series implements a new IOCTL to submit push buffers that
can
> > > optionally return a sync FD or sync object to userspace. This is
useful
> > > in cases where userspace wants to synchronize operations between
the GPU
> > > and another driver (such as KMS for display). Among other things
this
> > > allows extensions such as eglDupNativeFenceFDANDROID to be
implemented.
> > > 
> > > Note that patch 4 modifies the ABI introduced in patch 3 by
allowing DRM
> > > sync objects to be passed rather than only sync FDs. It also
allows any
> > > number of sync FDs/objects to be passed in or emitted. I think
those are
> > > useful features, but I left them in a separate patch in case
everybody
> > > else thinks that this won't be needed. If we decide to merge
the new ABI
> > > then patch 4 should be squashed into patch 3.
> > > 
> > > The corresponding userspace changes can be found here:
> > > 
> > >   libdrm:
https://gitlab.freedesktop.org/tagr/drm/-/commits/nouveau-sync-fd-v2/
> > >   mesa:
https://gitlab.freedesktop.org/tagr/mesa/-/commits/nouveau-sync-fd/
> > > 
> > > I've verified that this works with kmscube's --atomic
mode and Weston.
> > 
> > Hi Ben,
> > 
> > any thoughts on this series? I realize that this is somewhat
suboptimal
> > because we're effectively adding a duplicate of the existing IOCTL
with
> > only the "minor" extension of adding sync FDs/objects, but
at the same
> > time I don't have any good ideas on what else to add to make this
more
> > appealing or if you have any plans of your own to address this in the
> > future.
> 
> drm core automatically zero-extends ioctl structs both ways, so if all you
> do is add more stuff to the top level ioctl struct at the bottom,
there's
> no need to duplicate any code. At least as long as you guarantee that 0
=> old behaviour for both in and out parameters.
But that only works if the structure size remains fixed, right? In this
case, however, we have to extend the structure with additional fields,
so the size is going to change and therefore the IOCTL number will also
change.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20200924/13668912/attachment.sig>

On Thu, Sep 24, 2020 at 12:05 PM Thierry Reding
<thierry.reding at gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 05:21:24PM +0200, Daniel Vetter wrote:
> > On Wed, Sep 23, 2020 at 11:18:53AM +0200, Thierry Reding wrote:
> > > On Fri, Aug 28, 2020 at 12:40:10PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding at nvidia.com>
> > > >
> > > > Hi,
> > > >
> > > > This series implements a new IOCTL to submit push buffers
that can
> > > > optionally return a sync FD or sync object to userspace.
This is useful
> > > > in cases where userspace wants to synchronize operations
between the GPU
> > > > and another driver (such as KMS for display). Among other
things this
> > > > allows extensions such as eglDupNativeFenceFDANDROID to be
implemented.
> > > >
> > > > Note that patch 4 modifies the ABI introduced in patch 3 by
allowing DRM
> > > > sync objects to be passed rather than only sync FDs. It also
allows any
> > > > number of sync FDs/objects to be passed in or emitted. I
think those are
> > > > useful features, but I left them in a separate patch in case
everybody
> > > > else thinks that this won't be needed. If we decide to
merge the new ABI
> > > > then patch 4 should be squashed into patch 3.
> > > >
> > > > The corresponding userspace changes can be found here:
> > > >
> > > >   libdrm:
https://gitlab.freedesktop.org/tagr/drm/-/commits/nouveau-sync-fd-v2/
> > > >   mesa:
https://gitlab.freedesktop.org/tagr/mesa/-/commits/nouveau-sync-fd/
> > > >
> > > > I've verified that this works with kmscube's
--atomic mode and Weston.
> > >
> > > Hi Ben,
> > >
> > > any thoughts on this series? I realize that this is somewhat
suboptimal
> > > because we're effectively adding a duplicate of the existing
IOCTL with
> > > only the "minor" extension of adding sync FDs/objects,
but at the same
> > > time I don't have any good ideas on what else to add to make
this more
> > > appealing or if you have any plans of your own to address this in
the
> > > future.
> >
> > drm core automatically zero-extends ioctl structs both ways, so if all
you
> > do is add more stuff to the top level ioctl struct at the bottom,
there's
> > no need to duplicate any code. At least as long as you guarantee that
0 => > old behaviour for both in and out parameters.
>
> But that only works if the structure size remains fixed, right? In this
> case, however, we have to extend the structure with additional fields,
> so the size is going to change and therefore the IOCTL number will also
> change.
Nope, drm_ioctl() is pretty much magic, and will zero-extend size
mismatches in both ways. Which means you can run userspace compile
against old kernels (so user_sz > kernel_sz) and you can run old
userspace on new kernels (so user_sz < kernel_sz) and it will all work
correctly. No need to allocate new ioctl numbers for this case, just
extend at the bottom. We're doing this pretty much all the time.

You might still want a getparam (or explicit flag, if all versions of
that ioctl validated the flags correctly) since doing since a dummy
pushbuf on an old kernel won't result in anything getting rejected.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch