[PATCH] drm/vkms: Add a DRM render node to vkms

[PATCH] drm/vkms: Add a DRM render node to vkms

[Yi Xie]

Some libraries including Mesa and virglrenderer require a render node to
fully function. By adding a render node to vkms those libraries will
work properly, supporting use cases like running crosvm with virgl GPU
support via llvmpipe on a headless virtual machine.

Signed-off-by: Yi Xie <yixie@google.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 293dbca50c31..8eea5d4dece8 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
 }
 
 static const struct drm_driver vkms_driver = {
-	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
+	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
 	.release		= vkms_release,
 	.fops			= &vkms_driver_fops,
 	DRM_GEM_SHMEM_DRIVER_OPS,
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Simon Ser]

On Thursday, January 5th, 2023 at 06:23, Yi Xie <yixie@google.com> wrote:

> Some libraries including Mesa and virglrenderer require a render node to
> fully function. By adding a render node to vkms those libraries will
> work properly, supporting use cases like running crosvm with virgl GPU
> support via llvmpipe on a headless virtual machine.

This doesn't sound like a good idea to me. Devices without render
capabilities should not fake it.

User-space (e.g. wlroots) relies on "no render node" to enable
software rendering (Pixman instead of GL).

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Daniel Vetter]

On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> Some libraries including Mesa and virglrenderer require a render node to
> fully function. By adding a render node to vkms those libraries will
> work properly, supporting use cases like running crosvm with virgl GPU
> support via llvmpipe on a headless virtual machine.

This is what vgem exists for. More or less at least ... I'm honestly not
really understanding what you're trying to fix here, it sounds a bit like
userspace being stupid.
-Daniel

> 
> Signed-off-by: Yi Xie <yixie@google.com>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 293dbca50c31..8eea5d4dece8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
>  }
>  
>  static const struct drm_driver vkms_driver = {
> -	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> +	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
>  	.release		= vkms_release,
>  	.fops			= &vkms_driver_fops,
>  	DRM_GEM_SHMEM_DRIVER_OPS,
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Yi Xie]

> This doesn't sound like a good idea to me. Devices without render
> capabilities should not fake it.
>
> User-space (e.g. wlroots) relies on "no render node" to enable
> software rendering (Pixman instead of GL).

We have virtgpu driver that exports a render node even when virgl is
not supported.
Mesa has special code path to enable software rendering on it:
https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
We can do the same for vkms to force software rendering.

On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > Some libraries including Mesa and virglrenderer require a render node to
> > fully function. By adding a render node to vkms those libraries will
> > work properly, supporting use cases like running crosvm with virgl GPU
> > support via llvmpipe on a headless virtual machine.
>
> This is what vgem exists for. More or less at least ... I'm honestly not
> really understanding what you're trying to fix here, it sounds a bit like
> userspace being stupid.
> -Daniel
The problem with vgem is that it crashes llvmpipe while working with vkms.
Looks like it's due to the same reason as described in this thread in Mesa:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830

Importing buffers allocated by vgem to vkms seems to be unexpected and
causes the crash. If we create a render node on vkms then llvmpipe will use
vkms to allocate buffers and it no longer crashes.

> >
> > Signed-off-by: Yi Xie <yixie@google.com>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index 293dbca50c31..8eea5d4dece8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> >  }
> >
> >  static const struct drm_driver vkms_driver = {
> > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> >       .release                = vkms_release,
> >       .fops                   = &vkms_driver_fops,
> >       DRM_GEM_SHMEM_DRIVER_OPS,
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Daniel Vetter]

On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > This doesn't sound like a good idea to me. Devices without render
> > capabilities should not fake it.
> >
> > User-space (e.g. wlroots) relies on "no render node" to enable
> > software rendering (Pixman instead of GL).
> 
> We have virtgpu driver that exports a render node even when virgl is
> not supported.
> Mesa has special code path to enable software rendering on it:
> https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> We can do the same for vkms to force software rendering.

Yeah that is the old kmsro mesa issue, for every combination of kms and
gem device you need one to make this work.

> On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > Some libraries including Mesa and virglrenderer require a render node to
> > > fully function. By adding a render node to vkms those libraries will
> > > work properly, supporting use cases like running crosvm with virgl GPU
> > > support via llvmpipe on a headless virtual machine.
> >
> > This is what vgem exists for. More or less at least ... I'm honestly not
> > really understanding what you're trying to fix here, it sounds a bit like
> > userspace being stupid.
> > -Daniel
> The problem with vgem is that it crashes llvmpipe while working with vkms.
> Looks like it's due to the same reason as described in this thread in Mesa:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830

I'm not finding any bug description in there and how/why something
crashes?

> Importing buffers allocated by vgem to vkms seems to be unexpected and
> causes the crash. If we create a render node on vkms then llvmpipe will use
> vkms to allocate buffers and it no longer crashes.

Uh importing vgem into virtio might not work because those sometimes need
special buffers iirc. But importing vgem into vkms really should work,
there's no technical reason it cannot. If it doesn't, then the right fix
would be to fix that, not paper around it.
-Daniel

> 
> > >
> > > Signed-off-by: Yi Xie <yixie@google.com>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > index 293dbca50c31..8eea5d4dece8 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > >  }
> > >
> > >  static const struct drm_driver vkms_driver = {
> > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > >       .release                = vkms_release,
> > >       .fops                   = &vkms_driver_fops,
> > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > --
> > > 2.39.0.314.g84b9a713c41-goog
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Yi Xie]

On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > This doesn't sound like a good idea to me. Devices without render
> > > capabilities should not fake it.
> > >
> > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > software rendering (Pixman instead of GL).
> >
> > We have virtgpu driver that exports a render node even when virgl is
> > not supported.
> > Mesa has special code path to enable software rendering on it:
> > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > We can do the same for vkms to force software rendering.
>
> Yeah that is the old kmsro mesa issue, for every combination of kms and
> gem device you need one to make this work.
>
> > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > fully function. By adding a render node to vkms those libraries will
> > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > support via llvmpipe on a headless virtual machine.
> > >
> > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > really understanding what you're trying to fix here, it sounds a bit like
> > > userspace being stupid.
> > > -Daniel
> > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > Looks like it's due to the same reason as described in this thread in Mesa:
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
>
> I'm not finding any bug description in there and how/why something
> crashes?

The discussion is in the comment section under the first comment by
Emil Velikov.
It's folded by default (inside "6 replies" at the bottom).

>
> > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > causes the crash. If we create a render node on vkms then llvmpipe will use
> > vkms to allocate buffers and it no longer crashes.
>
> Uh importing vgem into virtio might not work because those sometimes need
> special buffers iirc. But importing vgem into vkms really should work,
> there's no technical reason it cannot. If it doesn't, then the right fix
> would be to fix that, not paper around it.

The crash stack trace looks like this:
https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2

Even if we fix the crash issue with vgem, we still need to workaround
quite a few
places that has explicitly blocked vgem. A notable example is virglrenderer:
https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121

Actually I have tried to force running virglrenderer on vgem and it
didn't work. I
didn't look into why it wasn't working but I guess that's the reason
for blocking
vgem in the first place. Virglrenderer works well on vkms with render node
enabled though.

> -Daniel
>
> >
> > > >
> > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > >  }
> > > >
> > > >  static const struct drm_driver vkms_driver = {
> > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > >       .release                = vkms_release,
> > > >       .fops                   = &vkms_driver_fops,
> > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > --
> > > > 2.39.0.314.g84b9a713c41-goog
> > > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Daniel Vetter]

On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > This doesn't sound like a good idea to me. Devices without render
> > > > capabilities should not fake it.
> > > >
> > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > software rendering (Pixman instead of GL).
> > >
> > > We have virtgpu driver that exports a render node even when virgl is
> > > not supported.
> > > Mesa has special code path to enable software rendering on it:
> > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > We can do the same for vkms to force software rendering.
> >
> > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > gem device you need one to make this work.
> >
> > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > fully function. By adding a render node to vkms those libraries will
> > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > support via llvmpipe on a headless virtual machine.
> > > >
> > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > userspace being stupid.
> > > > -Daniel
> > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> >
> > I'm not finding any bug description in there and how/why something
> > crashes?
> 
> The discussion is in the comment section under the first comment by
> Emil Velikov.
> It's folded by default (inside "6 replies" at the bottom).
> 
> >
> > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > vkms to allocate buffers and it no longer crashes.
> >
> > Uh importing vgem into virtio might not work because those sometimes need
> > special buffers iirc. But importing vgem into vkms really should work,
> > there's no technical reason it cannot. If it doesn't, then the right fix
> > would be to fix that, not paper around it.
> 
> The crash stack trace looks like this:
> https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> 
> Even if we fix the crash issue with vgem, we still need to workaround
> quite a few
> places that has explicitly blocked vgem. A notable example is virglrenderer:
> https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> 
> Actually I have tried to force running virglrenderer on vgem and it
> didn't work. I
> didn't look into why it wasn't working but I guess that's the reason
> for blocking
> vgem in the first place. Virglrenderer works well on vkms with render node
> enabled though.

Ah ok. For next time around, copy a link to the comment you want, e.g.

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477

The 3 dots menu on each comments has an option to copy that link tag. That
also highlights the right comment.

On this issue, I'm concurring with Emil:

"- the import is broken
"IMHO that should be fixed, regardless of the rest"

The same should be done here. Unless it's a very special device, we should
be able to import vgem buffers.
-Daniel

> 
> > -Daniel
> >
> > >
> > > > >
> > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > ---
> > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > >  }
> > > > >
> > > > >  static const struct drm_driver vkms_driver = {
> > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > >       .release                = vkms_release,
> > > > >       .fops                   = &vkms_driver_fops,
> > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > --
> > > > > 2.39.0.314.g84b9a713c41-goog
> > > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Yi Xie]

On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > capabilities should not fake it.
> > > > >
> > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > software rendering (Pixman instead of GL).
> > > >
> > > > We have virtgpu driver that exports a render node even when virgl is
> > > > not supported.
> > > > Mesa has special code path to enable software rendering on it:
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > We can do the same for vkms to force software rendering.
> > >
> > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > gem device you need one to make this work.
> > >
> > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > support via llvmpipe on a headless virtual machine.
> > > > >
> > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > userspace being stupid.
> > > > > -Daniel
> > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > >
> > > I'm not finding any bug description in there and how/why something
> > > crashes?
> >
> > The discussion is in the comment section under the first comment by
> > Emil Velikov.
> > It's folded by default (inside "6 replies" at the bottom).
> >
> > >
> > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > vkms to allocate buffers and it no longer crashes.
> > >
> > > Uh importing vgem into virtio might not work because those sometimes need
> > > special buffers iirc. But importing vgem into vkms really should work,
> > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > would be to fix that, not paper around it.
> >
> > The crash stack trace looks like this:
> > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> >
> > Even if we fix the crash issue with vgem, we still need to workaround
> > quite a few
> > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> >
> > Actually I have tried to force running virglrenderer on vgem and it
> > didn't work. I
> > didn't look into why it wasn't working but I guess that's the reason
> > for blocking
> > vgem in the first place. Virglrenderer works well on vkms with render node
> > enabled though.
>
> Ah ok. For next time around, copy a link to the comment you want, e.g.
>
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
>
> The 3 dots menu on each comments has an option to copy that link tag. That
> also highlights the right comment.

Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.

>
> On this issue, I'm concurring with Emil:
>
> "- the import is broken
> "IMHO that should be fixed, regardless of the rest"
>
> The same should be done here. Unless it's a very special device, we should
> be able to import vgem buffers.

How about the fact that vgem is blocked explicitly in virglrenderer?
We will have
to remove it from block list and that may break something that
resulted in blocking
in this commit:
https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
I can't find the reason why it's blocking vgem though. It shouldn't be
related to
incompatibility with vkms/virtgpu.

Are there any concerns that enabling render node on vkms may cause problems?
What if we add a driver option to add render node on demand?

> -Daniel
>
> >
> > > -Daniel
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > >  }
> > > > > >
> > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > >       .release                = vkms_release,
> > > > > >       .fops                   = &vkms_driver_fops,
> > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > --
> > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Daniel Vetter]

On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote:
> On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > > capabilities should not fake it.
> > > > > >
> > > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > > software rendering (Pixman instead of GL).
> > > > >
> > > > > We have virtgpu driver that exports a render node even when virgl is
> > > > > not supported.
> > > > > Mesa has special code path to enable software rendering on it:
> > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > > We can do the same for vkms to force software rendering.
> > > >
> > > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > > gem device you need one to make this work.
> > > >
> > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > > support via llvmpipe on a headless virtual machine.
> > > > > >
> > > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > > userspace being stupid.
> > > > > > -Daniel
> > > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > > >
> > > > I'm not finding any bug description in there and how/why something
> > > > crashes?
> > >
> > > The discussion is in the comment section under the first comment by
> > > Emil Velikov.
> > > It's folded by default (inside "6 replies" at the bottom).
> > >
> > > >
> > > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > > vkms to allocate buffers and it no longer crashes.
> > > >
> > > > Uh importing vgem into virtio might not work because those sometimes need
> > > > special buffers iirc. But importing vgem into vkms really should work,
> > > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > > would be to fix that, not paper around it.
> > >
> > > The crash stack trace looks like this:
> > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> > >
> > > Even if we fix the crash issue with vgem, we still need to workaround
> > > quite a few
> > > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> > >
> > > Actually I have tried to force running virglrenderer on vgem and it
> > > didn't work. I
> > > didn't look into why it wasn't working but I guess that's the reason
> > > for blocking
> > > vgem in the first place. Virglrenderer works well on vkms with render node
> > > enabled though.
> >
> > Ah ok. For next time around, copy a link to the comment you want, e.g.
> >
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
> >
> > The 3 dots menu on each comments has an option to copy that link tag. That
> > also highlights the right comment.
> 
> Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.
> 
> >
> > On this issue, I'm concurring with Emil:
> >
> > "- the import is broken
> > "IMHO that should be fixed, regardless of the rest"
> >
> > The same should be done here. Unless it's a very special device, we should
> > be able to import vgem buffers.
> 
> How about the fact that vgem is blocked explicitly in virglrenderer?
> We will have
> to remove it from block list and that may break something that
> resulted in blocking
> in this commit:
> https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
> I can't find the reason why it's blocking vgem though. It shouldn't be
> related to
> incompatibility with vkms/virtgpu.
> 
> Are there any concerns that enabling render node on vkms may cause problems?
> What if we add a driver option to add render node on demand?

The thing is, that none of the other kms-only driver enable render nodes.
If we start adding them in one case just because userspace can't cope,
then we'll have an endless stream of these patches.

Instead of fixing userspace.

Note that the issue is very old for at least mesa3d, and the only fix is
kmsro, where you have to build a driver for each combo. Maybe this should
be done better, dunno. But adding render node in just vkms for this use
case really doesn't make much sense to me, and it smells very much like
opening a can of worms :-/
-Daniel

> > -Daniel
> >
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > > >
> > > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > > >  }
> > > > > > >
> > > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > > >       .release                = vkms_release,
> > > > > > >       .fops                   = &vkms_driver_fops,
> > > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > > --
> > > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Tao Wu(吴涛@Eng)]

Hi Daniel,

May I know what's the requirement for adding render node support to a
"gpu"?  Why we just export render node for every drm devices?
I read document here
https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes
and it seems render node allow multiple unprivileged clients
to work with the same gpu, I am not sure why we just enable it for all
kms-only device.
What's wrong if we enable it for all kms-only devices and also let
mesa to use llvmpipe with those devices by default.

Thanks!

On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote:
> > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > > > capabilities should not fake it.
> > > > > > >
> > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > > > software rendering (Pixman instead of GL).
> > > > > >
> > > > > > We have virtgpu driver that exports a render node even when virgl is
> > > > > > not supported.
> > > > > > Mesa has special code path to enable software rendering on it:
> > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > > > We can do the same for vkms to force software rendering.
> > > > >
> > > > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > > > gem device you need one to make this work.
> > > > >
> > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > >
> > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > > > support via llvmpipe on a headless virtual machine.
> > > > > > >
> > > > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > > > userspace being stupid.
> > > > > > > -Daniel
> > > > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > > > >
> > > > > I'm not finding any bug description in there and how/why something
> > > > > crashes?
> > > >
> > > > The discussion is in the comment section under the first comment by
> > > > Emil Velikov.
> > > > It's folded by default (inside "6 replies" at the bottom).
> > > >
> > > > >
> > > > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > > > vkms to allocate buffers and it no longer crashes.
> > > > >
> > > > > Uh importing vgem into virtio might not work because those sometimes need
> > > > > special buffers iirc. But importing vgem into vkms really should work,
> > > > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > > > would be to fix that, not paper around it.
> > > >
> > > > The crash stack trace looks like this:
> > > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> > > >
> > > > Even if we fix the crash issue with vgem, we still need to workaround
> > > > quite a few
> > > > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> > > >
> > > > Actually I have tried to force running virglrenderer on vgem and it
> > > > didn't work. I
> > > > didn't look into why it wasn't working but I guess that's the reason
> > > > for blocking
> > > > vgem in the first place. Virglrenderer works well on vkms with render node
> > > > enabled though.
> > >
> > > Ah ok. For next time around, copy a link to the comment you want, e.g.
> > >
> > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
> > >
> > > The 3 dots menu on each comments has an option to copy that link tag. That
> > > also highlights the right comment.
> >
> > Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.
> >
> > >
> > > On this issue, I'm concurring with Emil:
> > >
> > > "- the import is broken
> > > "IMHO that should be fixed, regardless of the rest"
> > >
> > > The same should be done here. Unless it's a very special device, we should
> > > be able to import vgem buffers.
> >
> > How about the fact that vgem is blocked explicitly in virglrenderer?
> > We will have
> > to remove it from block list and that may break something that
> > resulted in blocking
> > in this commit:
> > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
> > I can't find the reason why it's blocking vgem though. It shouldn't be
> > related to
> > incompatibility with vkms/virtgpu.
> >
> > Are there any concerns that enabling render node on vkms may cause problems?
> > What if we add a driver option to add render node on demand?
>
> The thing is, that none of the other kms-only driver enable render nodes.
> If we start adding them in one case just because userspace can't cope,
> then we'll have an endless stream of these patches.
>
> Instead of fixing userspace.
>
> Note that the issue is very old for at least mesa3d, and the only fix is
> kmsro, where you have to build a driver for each combo. Maybe this should
> be done better, dunno. But adding render node in just vkms for this use
> case really doesn't make much sense to me, and it smells very much like
> opening a can of worms :-/
> -Daniel
>
> > > -Daniel
> > >
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > > > >       .release                = vkms_release,
> > > > > > > >       .fops                   = &vkms_driver_fops,
> > > > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > > > --
> > > > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Daniel Vetter]

On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote:
> Hi Daniel,
> 
> May I know what's the requirement for adding render node support to a
> "gpu"?  Why we just export render node for every drm devices?
> I read document here
> https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes

Thus far we've only done it when there's actual rendering capability,
which generally means at least some private ioctls.

Which vkms just doens't have. And it's by far not the only such case.

Also note that display drivers side is _not_ shareable.
-Daniel

> and it seems render node allow multiple unprivileged clients
> to work with the same gpu, I am not sure why we just enable it for all
> kms-only device.
> What's wrong if we enable it for all kms-only devices and also let
> mesa to use llvmpipe with those devices by default.
> 
> Thanks!
> 
> On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote:
> > > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > > > > capabilities should not fake it.
> > > > > > > >
> > > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > > > > software rendering (Pixman instead of GL).
> > > > > > >
> > > > > > > We have virtgpu driver that exports a render node even when virgl is
> > > > > > > not supported.
> > > > > > > Mesa has special code path to enable software rendering on it:
> > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > > > > We can do the same for vkms to force software rendering.
> > > > > >
> > > > > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > > > > gem device you need one to make this work.
> > > > > >
> > > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > > > > support via llvmpipe on a headless virtual machine.
> > > > > > > >
> > > > > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > > > > userspace being stupid.
> > > > > > > > -Daniel
> > > > > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > > > > >
> > > > > > I'm not finding any bug description in there and how/why something
> > > > > > crashes?
> > > > >
> > > > > The discussion is in the comment section under the first comment by
> > > > > Emil Velikov.
> > > > > It's folded by default (inside "6 replies" at the bottom).
> > > > >
> > > > > >
> > > > > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > > > > vkms to allocate buffers and it no longer crashes.
> > > > > >
> > > > > > Uh importing vgem into virtio might not work because those sometimes need
> > > > > > special buffers iirc. But importing vgem into vkms really should work,
> > > > > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > > > > would be to fix that, not paper around it.
> > > > >
> > > > > The crash stack trace looks like this:
> > > > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> > > > >
> > > > > Even if we fix the crash issue with vgem, we still need to workaround
> > > > > quite a few
> > > > > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> > > > >
> > > > > Actually I have tried to force running virglrenderer on vgem and it
> > > > > didn't work. I
> > > > > didn't look into why it wasn't working but I guess that's the reason
> > > > > for blocking
> > > > > vgem in the first place. Virglrenderer works well on vkms with render node
> > > > > enabled though.
> > > >
> > > > Ah ok. For next time around, copy a link to the comment you want, e.g.
> > > >
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
> > > >
> > > > The 3 dots menu on each comments has an option to copy that link tag. That
> > > > also highlights the right comment.
> > >
> > > Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.
> > >
> > > >
> > > > On this issue, I'm concurring with Emil:
> > > >
> > > > "- the import is broken
> > > > "IMHO that should be fixed, regardless of the rest"
> > > >
> > > > The same should be done here. Unless it's a very special device, we should
> > > > be able to import vgem buffers.
> > >
> > > How about the fact that vgem is blocked explicitly in virglrenderer?
> > > We will have
> > > to remove it from block list and that may break something that
> > > resulted in blocking
> > > in this commit:
> > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
> > > I can't find the reason why it's blocking vgem though. It shouldn't be
> > > related to
> > > incompatibility with vkms/virtgpu.
> > >
> > > Are there any concerns that enabling render node on vkms may cause problems?
> > > What if we add a driver option to add render node on demand?
> >
> > The thing is, that none of the other kms-only driver enable render nodes.
> > If we start adding them in one case just because userspace can't cope,
> > then we'll have an endless stream of these patches.
> >
> > Instead of fixing userspace.
> >
> > Note that the issue is very old for at least mesa3d, and the only fix is
> > kmsro, where you have to build a driver for each combo. Maybe this should
> > be done better, dunno. But adding render node in just vkms for this use
> > case really doesn't make much sense to me, and it smells very much like
> > opening a can of worms :-/
> > -Daniel
> >
> > > > -Daniel
> > > >
> > > > >
> > > > > > -Daniel
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > > > > >       .release                = vkms_release,
> > > > > > > > >       .fops                   = &vkms_driver_fops,
> > > > > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > > > > --
> > > > > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Daniel Vetter
> > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > http://blog.ffwll.ch
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Yi Xie]

I have figured out the problem with importing buffers across vgem/vkms.

It's intentionally blocked by kernel here:
https://github.com/torvalds/linux/blob/a140a6a2d5ec0329ad05cd3532a91ad0ce58dceb/drivers/gpu/drm/drm_gem.c#L307

From the original patch https://patchwork.freedesktop.org/patch/172242/:
Reject mapping an imported dma-buf since it's an invalid use-case.

Looks like importing dumb buffers across different devices is
disallowed. Removing this check and then everything is working well on
vkms with vgem.

According to the patch thread we should use native map instead of dumb
map on imported buffers.

Since there is no native map ioctl in both vgem and vkms, I'm thinking
about adding a dumb_map_offset implementation in both of them with
that check removed.
From my testing vkms and vgem are now working happily together without
any (obvious) issues.

There are other drivers doing the same thing, for example virtgpu:
https://github.com/torvalds/linux/blob/a140a6a2d5ec0329ad05cd3532a91ad0ce58dceb/drivers/gpu/drm/virtio/virtgpu_gem.c#L102

Does this sound like a better idea than adding a render node?

On Fri, Jan 6, 2023 at 6:54 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote:
> > Hi Daniel,
> >
> > May I know what's the requirement for adding render node support to a
> > "gpu"?  Why we just export render node for every drm devices?
> > I read document here
> > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes
>
> Thus far we've only done it when there's actual rendering capability,
> which generally means at least some private ioctls.
>
> Which vkms just doens't have. And it's by far not the only such case.
>
> Also note that display drivers side is _not_ shareable.
> -Daniel
>
> > and it seems render node allow multiple unprivileged clients
> > to work with the same gpu, I am not sure why we just enable it for all
> > kms-only device.
> > What's wrong if we enable it for all kms-only devices and also let
> > mesa to use llvmpipe with those devices by default.
> >
> > Thanks!
> >
> > On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote:
> > > > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > > > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > >
> > > > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > > > > > capabilities should not fake it.
> > > > > > > > >
> > > > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > > > > > software rendering (Pixman instead of GL).
> > > > > > > >
> > > > > > > > We have virtgpu driver that exports a render node even when virgl is
> > > > > > > > not supported.
> > > > > > > > Mesa has special code path to enable software rendering on it:
> > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > > > > > We can do the same for vkms to force software rendering.
> > > > > > >
> > > > > > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > > > > > gem device you need one to make this work.
> > > > > > >
> > > > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > > > > > support via llvmpipe on a headless virtual machine.
> > > > > > > > >
> > > > > > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > > > > > userspace being stupid.
> > > > > > > > > -Daniel
> > > > > > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > > > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > > > > > >
> > > > > > > I'm not finding any bug description in there and how/why something
> > > > > > > crashes?
> > > > > >
> > > > > > The discussion is in the comment section under the first comment by
> > > > > > Emil Velikov.
> > > > > > It's folded by default (inside "6 replies" at the bottom).
> > > > > >
> > > > > > >
> > > > > > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > > > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > > > > > vkms to allocate buffers and it no longer crashes.
> > > > > > >
> > > > > > > Uh importing vgem into virtio might not work because those sometimes need
> > > > > > > special buffers iirc. But importing vgem into vkms really should work,
> > > > > > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > > > > > would be to fix that, not paper around it.
> > > > > >
> > > > > > The crash stack trace looks like this:
> > > > > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> > > > > >
> > > > > > Even if we fix the crash issue with vgem, we still need to workaround
> > > > > > quite a few
> > > > > > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > > > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> > > > > >
> > > > > > Actually I have tried to force running virglrenderer on vgem and it
> > > > > > didn't work. I
> > > > > > didn't look into why it wasn't working but I guess that's the reason
> > > > > > for blocking
> > > > > > vgem in the first place. Virglrenderer works well on vkms with render node
> > > > > > enabled though.
> > > > >
> > > > > Ah ok. For next time around, copy a link to the comment you want, e.g.
> > > > >
> > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
> > > > >
> > > > > The 3 dots menu on each comments has an option to copy that link tag. That
> > > > > also highlights the right comment.
> > > >
> > > > Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.
> > > >
> > > > >
> > > > > On this issue, I'm concurring with Emil:
> > > > >
> > > > > "- the import is broken
> > > > > "IMHO that should be fixed, regardless of the rest"
> > > > >
> > > > > The same should be done here. Unless it's a very special device, we should
> > > > > be able to import vgem buffers.
> > > >
> > > > How about the fact that vgem is blocked explicitly in virglrenderer?
> > > > We will have
> > > > to remove it from block list and that may break something that
> > > > resulted in blocking
> > > > in this commit:
> > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
> > > > I can't find the reason why it's blocking vgem though. It shouldn't be
> > > > related to
> > > > incompatibility with vkms/virtgpu.
> > > >
> > > > Are there any concerns that enabling render node on vkms may cause problems?
> > > > What if we add a driver option to add render node on demand?
> > >
> > > The thing is, that none of the other kms-only driver enable render nodes.
> > > If we start adding them in one case just because userspace can't cope,
> > > then we'll have an endless stream of these patches.
> > >
> > > Instead of fixing userspace.
> > >
> > > Note that the issue is very old for at least mesa3d, and the only fix is
> > > kmsro, where you have to build a driver for each combo. Maybe this should
> > > be done better, dunno. But adding render node in just vkms for this use
> > > case really doesn't make much sense to me, and it smells very much like
> > > opening a can of worms :-/
> > > -Daniel
> > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > > -Daniel
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > > > > > >       .release                = vkms_release,
> > > > > > > > > >       .fops                   = &vkms_driver_fops,
> > > > > > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > > > > > --
> > > > > > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Daniel Vetter
> > > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > > http://blog.ffwll.ch
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Daniel Vetter]

On Fri, Jan 06, 2023 at 07:02:22PM +0900, Yi Xie wrote:
> I have figured out the problem with importing buffers across vgem/vkms.
> 
> It's intentionally blocked by kernel here:
> https://github.com/torvalds/linux/blob/a140a6a2d5ec0329ad05cd3532a91ad0ce58dceb/drivers/gpu/drm/drm_gem.c#L307

Uh yes. That is not your problem, because your userspace is using the
wrong interface. For a dma-buf either use mmap on the dma-buf fd, or on
the buffer handle from the original exporters. Importers should not
forward dma-buf mmap.

> From the original patch https://patchwork.freedesktop.org/patch/172242/:
> Reject mapping an imported dma-buf since it's an invalid use-case.
> 
> Looks like importing dumb buffers across different devices is
> disallowed. Removing this check and then everything is working well on
> vkms with vgem.

No. The entire point of dma-buf sharing is sharing them across devices and
process.

> According to the patch thread we should use native map instead of dumb
> map on imported buffers.
> 
> Since there is no native map ioctl in both vgem and vkms, I'm thinking
> about adding a dumb_map_offset implementation in both of them with
> that check removed.

No please not. You might need a dumb mmap offset on vgem, so that your sw
render code can upload the buffer. But I think that already exists on
vgem.

> From my testing vkms and vgem are now working happily together without
> any (obvious) issues.
> 
> There are other drivers doing the same thing, for example virtgpu:
> https://github.com/torvalds/linux/blob/a140a6a2d5ec0329ad05cd3532a91ad0ce58dceb/drivers/gpu/drm/virtio/virtgpu_gem.c#L102

Uh why is virtio allowing this, this is bad :-(

That kinda highlights that virtgpu should probably use a lot more of the
helpers we've gained over the last 9 years since Dave merged this code.
Would be great if you or someone can look into this?

Allowing dumb mmap on imported buffers is no good at all.
-Daniel

> 
> Does this sound like a better idea than adding a render node?
> 
> On Fri, Jan 6, 2023 at 6:54 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote:
> > > Hi Daniel,
> > >
> > > May I know what's the requirement for adding render node support to a
> > > "gpu"?  Why we just export render node for every drm devices?
> > > I read document here
> > > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes
> >
> > Thus far we've only done it when there's actual rendering capability,
> > which generally means at least some private ioctls.
> >
> > Which vkms just doens't have. And it's by far not the only such case.
> >
> > Also note that display drivers side is _not_ shareable.
> > -Daniel
> >
> > > and it seems render node allow multiple unprivileged clients
> > > to work with the same gpu, I am not sure why we just enable it for all
> > > kms-only device.
> > > What's wrong if we enable it for all kms-only devices and also let
> > > mesa to use llvmpipe with those devices by default.
> > >
> > > Thanks!
> > >
> > > On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote:
> > > > > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > > > > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > > > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > > > > > > capabilities should not fake it.
> > > > > > > > > >
> > > > > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > > > > > > software rendering (Pixman instead of GL).
> > > > > > > > >
> > > > > > > > > We have virtgpu driver that exports a render node even when virgl is
> > > > > > > > > not supported.
> > > > > > > > > Mesa has special code path to enable software rendering on it:
> > > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > > > > > > We can do the same for vkms to force software rendering.
> > > > > > > >
> > > > > > > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > > > > > > gem device you need one to make this work.
> > > > > > > >
> > > > > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > > > > > > support via llvmpipe on a headless virtual machine.
> > > > > > > > > >
> > > > > > > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > > > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > > > > > > userspace being stupid.
> > > > > > > > > > -Daniel
> > > > > > > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > > > > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > > > > > > >
> > > > > > > > I'm not finding any bug description in there and how/why something
> > > > > > > > crashes?
> > > > > > >
> > > > > > > The discussion is in the comment section under the first comment by
> > > > > > > Emil Velikov.
> > > > > > > It's folded by default (inside "6 replies" at the bottom).
> > > > > > >
> > > > > > > >
> > > > > > > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > > > > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > > > > > > vkms to allocate buffers and it no longer crashes.
> > > > > > > >
> > > > > > > > Uh importing vgem into virtio might not work because those sometimes need
> > > > > > > > special buffers iirc. But importing vgem into vkms really should work,
> > > > > > > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > > > > > > would be to fix that, not paper around it.
> > > > > > >
> > > > > > > The crash stack trace looks like this:
> > > > > > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> > > > > > >
> > > > > > > Even if we fix the crash issue with vgem, we still need to workaround
> > > > > > > quite a few
> > > > > > > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > > > > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> > > > > > >
> > > > > > > Actually I have tried to force running virglrenderer on vgem and it
> > > > > > > didn't work. I
> > > > > > > didn't look into why it wasn't working but I guess that's the reason
> > > > > > > for blocking
> > > > > > > vgem in the first place. Virglrenderer works well on vkms with render node
> > > > > > > enabled though.
> > > > > >
> > > > > > Ah ok. For next time around, copy a link to the comment you want, e.g.
> > > > > >
> > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
> > > > > >
> > > > > > The 3 dots menu on each comments has an option to copy that link tag. That
> > > > > > also highlights the right comment.
> > > > >
> > > > > Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.
> > > > >
> > > > > >
> > > > > > On this issue, I'm concurring with Emil:
> > > > > >
> > > > > > "- the import is broken
> > > > > > "IMHO that should be fixed, regardless of the rest"
> > > > > >
> > > > > > The same should be done here. Unless it's a very special device, we should
> > > > > > be able to import vgem buffers.
> > > > >
> > > > > How about the fact that vgem is blocked explicitly in virglrenderer?
> > > > > We will have
> > > > > to remove it from block list and that may break something that
> > > > > resulted in blocking
> > > > > in this commit:
> > > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
> > > > > I can't find the reason why it's blocking vgem though. It shouldn't be
> > > > > related to
> > > > > incompatibility with vkms/virtgpu.
> > > > >
> > > > > Are there any concerns that enabling render node on vkms may cause problems?
> > > > > What if we add a driver option to add render node on demand?
> > > >
> > > > The thing is, that none of the other kms-only driver enable render nodes.
> > > > If we start adding them in one case just because userspace can't cope,
> > > > then we'll have an endless stream of these patches.
> > > >
> > > > Instead of fixing userspace.
> > > >
> > > > Note that the issue is very old for at least mesa3d, and the only fix is
> > > > kmsro, where you have to build a driver for each combo. Maybe this should
> > > > be done better, dunno. But adding render node in just vkms for this use
> > > > case really doesn't make much sense to me, and it smells very much like
> > > > opening a can of worms :-/
> > > > -Daniel
> > > >
> > > > > > -Daniel
> > > > > >
> > > > > > >
> > > > > > > > -Daniel
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > > > > > > >       .release                = vkms_release,
> > > > > > > > > > >       .fops                   = &vkms_driver_fops,
> > > > > > > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > > > > > > --
> > > > > > > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Daniel Vetter
> > > > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > > > http://blog.ffwll.ch
> > > > > > > >
> > > > > > > > --
> > > > > > > > Daniel Vetter
> > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > http://blog.ffwll.ch
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Tao Wu(吴涛@Eng)]

On Fri, Jan 6, 2023 at 1:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote:
> > Hi Daniel,
> >
> > May I know what's the requirement for adding render node support to a
> > "gpu"?  Why we just export render node for every drm devices?
> > I read document here
> > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes
>
> Thus far we've only done it when there's actual rendering capability,
> which generally means at least some private ioctls.
Hi Daniel, it looks like vgem is exporting render node by default.
Per my understanding, vgem provides some  DRM API so users can play
with graphic buffers. I am feeling it's natural have a v*** device
which provide
the surperset which vgem and vkms provides, so it sounds like it's
natural add rendernode to vkms, or do the opposite, add kms related
stuff to vgem. I still don't get the point: what kind of issue it
could bring if we just
add render node to vkms? If your point is, we don't do that for other
kms only devices, then my question is, how about we just enable render
node for every DRM driver? what could go wrong with this approach?

thanks!
>
> Which vkms just doens't have. And it's by far not the only such case.
>
> Also note that display drivers side is _not_ shareable.
> -Daniel
>
> > and it seems render node allow multiple unprivileged clients
> > to work with the same gpu, I am not sure why we just enable it for all
> > kms-only device.
> > What's wrong if we enable it for all kms-only devices and also let
> > mesa to use llvmpipe with those devices by default.
> >
> > Thanks!
> >
> > On Thu, Jan 5, 2023 at 7:35 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Fri, Jan 06, 2023 at 12:16:07AM +0900, Yi Xie wrote:
> > > > On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Thu, Jan 05, 2023 at 11:10:23PM +0900, Yi Xie wrote:
> > > > > > On Thu, Jan 5, 2023 at 10:48 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > >
> > > > > > > On Thu, Jan 05, 2023 at 09:52:26PM +0900, Yi Xie wrote:
> > > > > > > > > This doesn't sound like a good idea to me. Devices without render
> > > > > > > > > capabilities should not fake it.
> > > > > > > > >
> > > > > > > > > User-space (e.g. wlroots) relies on "no render node" to enable
> > > > > > > > > software rendering (Pixman instead of GL).
> > > > > > > >
> > > > > > > > We have virtgpu driver that exports a render node even when virgl is
> > > > > > > > not supported.
> > > > > > > > Mesa has special code path to enable software rendering on it:
> > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/egl/drivers/dri2/platform_device.c#L296
> > > > > > > > We can do the same for vkms to force software rendering.
> > > > > > >
> > > > > > > Yeah that is the old kmsro mesa issue, for every combination of kms and
> > > > > > > gem device you need one to make this work.
> > > > > > >
> > > > > > > > On Thu, Jan 5, 2023 at 8:36 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jan 05, 2023 at 02:23:25PM +0900, Yi Xie wrote:
> > > > > > > > > > Some libraries including Mesa and virglrenderer require a render node to
> > > > > > > > > > fully function. By adding a render node to vkms those libraries will
> > > > > > > > > > work properly, supporting use cases like running crosvm with virgl GPU
> > > > > > > > > > support via llvmpipe on a headless virtual machine.
> > > > > > > > >
> > > > > > > > > This is what vgem exists for. More or less at least ... I'm honestly not
> > > > > > > > > really understanding what you're trying to fix here, it sounds a bit like
> > > > > > > > > userspace being stupid.
> > > > > > > > > -Daniel
> > > > > > > > The problem with vgem is that it crashes llvmpipe while working with vkms.
> > > > > > > > Looks like it's due to the same reason as described in this thread in Mesa:
> > > > > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830
> > > > > > >
> > > > > > > I'm not finding any bug description in there and how/why something
> > > > > > > crashes?
> > > > > >
> > > > > > The discussion is in the comment section under the first comment by
> > > > > > Emil Velikov.
> > > > > > It's folded by default (inside "6 replies" at the bottom).
> > > > > >
> > > > > > >
> > > > > > > > Importing buffers allocated by vgem to vkms seems to be unexpected and
> > > > > > > > causes the crash. If we create a render node on vkms then llvmpipe will use
> > > > > > > > vkms to allocate buffers and it no longer crashes.
> > > > > > >
> > > > > > > Uh importing vgem into virtio might not work because those sometimes need
> > > > > > > special buffers iirc. But importing vgem into vkms really should work,
> > > > > > > there's no technical reason it cannot. If it doesn't, then the right fix
> > > > > > > would be to fix that, not paper around it.
> > > > > >
> > > > > > The crash stack trace looks like this:
> > > > > > https://gist.github.com/imxieyi/03053ae79cee2e614850fd41829e1da2
> > > > > >
> > > > > > Even if we fix the crash issue with vgem, we still need to workaround
> > > > > > quite a few
> > > > > > places that has explicitly blocked vgem. A notable example is virglrenderer:
> > > > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/vrend_winsys_gbm.c#L121
> > > > > >
> > > > > > Actually I have tried to force running virglrenderer on vgem and it
> > > > > > didn't work. I
> > > > > > didn't look into why it wasn't working but I guess that's the reason
> > > > > > for blocking
> > > > > > vgem in the first place. Virglrenderer works well on vkms with render node
> > > > > > enabled though.
> > > > >
> > > > > Ah ok. For next time around, copy a link to the comment you want, e.g.
> > > > >
> > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
> > > > >
> > > > > The 3 dots menu on each comments has an option to copy that link tag. That
> > > > > also highlights the right comment.
> > > >
> > > > Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.
> > > >
> > > > >
> > > > > On this issue, I'm concurring with Emil:
> > > > >
> > > > > "- the import is broken
> > > > > "IMHO that should be fixed, regardless of the rest"
> > > > >
> > > > > The same should be done here. Unless it's a very special device, we should
> > > > > be able to import vgem buffers.
> > > >
> > > > How about the fact that vgem is blocked explicitly in virglrenderer?
> > > > We will have
> > > > to remove it from block list and that may break something that
> > > > resulted in blocking
> > > > in this commit:
> > > > https://gitlab.freedesktop.org/virgl/virglrenderer/-/commit/2cb686dd46df27e9600f9df734303ec57bb38772
> > > > I can't find the reason why it's blocking vgem though. It shouldn't be
> > > > related to
> > > > incompatibility with vkms/virtgpu.
> > > >
> > > > Are there any concerns that enabling render node on vkms may cause problems?
> > > > What if we add a driver option to add render node on demand?
> > >
> > > The thing is, that none of the other kms-only driver enable render nodes.
> > > If we start adding them in one case just because userspace can't cope,
> > > then we'll have an endless stream of these patches.
> > >
> > > Instead of fixing userspace.
> > >
> > > Note that the issue is very old for at least mesa3d, and the only fix is
> > > kmsro, where you have to build a driver for each combo. Maybe this should
> > > be done better, dunno. But adding render node in just vkms for this use
> > > case really doesn't make much sense to me, and it smells very much like
> > > opening a can of worms :-/
> > > -Daniel
> > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > > -Daniel
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yi Xie <yixie@google.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
> > > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > index 293dbca50c31..8eea5d4dece8 100644
> > > > > > > > > > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > > > > > > > @@ -113,7 +113,7 @@ static void vkms_config_debugfs_init(struct drm_minor *minor)
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > >  static const struct drm_driver vkms_driver = {
> > > > > > > > > > -     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > > > > > > > > > +     .driver_features        = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM | DRIVER_RENDER,
> > > > > > > > > >       .release                = vkms_release,
> > > > > > > > > >       .fops                   = &vkms_driver_fops,
> > > > > > > > > >       DRM_GEM_SHMEM_DRIVER_OPS,
> > > > > > > > > > --
> > > > > > > > > > 2.39.0.314.g84b9a713c41-goog
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Daniel Vetter
> > > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > > http://blog.ffwll.ch
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Simon Ser]

On Friday, January 6th, 2023 at 23:28, Tao Wu <lepton@google.com> wrote:

> On Fri, Jan 6, 2023 at 1:54 AM Daniel Vetter daniel@ffwll.ch wrote:
> 
> > On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote:
> > 
> > > Hi Daniel,
> > > 
> > > May I know what's the requirement for adding render node support to a
> > > "gpu"? Why we just export render node for every drm devices?
> > > I read document here
> > > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes
> > 
> > Thus far we've only done it when there's actual rendering capability,
> > which generally means at least some private ioctls.
> 
> Hi Daniel, it looks like vgem is exporting render node by default.
> Per my understanding, vgem provides some DRM API so users can play
> with graphic buffers. I am feeling it's natural have a v*** device
> which provide
> the surperset which vgem and vkms provides, so it sounds like it's
> natural add rendernode to vkms, or do the opposite, add kms related
> stuff to vgem. I still don't get the point: what kind of issue it
> could bring if we just
> add render node to vkms? If your point is, we don't do that for other
> kms only devices, then my question is, how about we just enable render
> node for every DRM driver? what could go wrong with this approach?

This is wrong for at least two reasons:

- A render node has a semantic value: it indicates whether a device has
  rendering capabilities. If we attach a render node to vkms, we lie
  because vkms has no such capability.
- This would regress user-space. wlroots would no longer accept to
  start with Pixman on vkms, because it detects a render node on the
  device.

I'd advise moving away from abusing DRM dumb buffers in Mesa.

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Tao Wu(吴涛@Eng)]

On Sat, Jan 7, 2023 at 2:45 AM Simon Ser <contact@emersion.fr> wrote:
>
> On Friday, January 6th, 2023 at 23:28, Tao Wu <lepton@google.com> wrote:
>
> > On Fri, Jan 6, 2023 at 1:54 AM Daniel Vetter daniel@ffwll.ch wrote:
> >
> > > On Thu, Jan 05, 2023 at 01:40:28PM -0800, Tao Wu(吴涛@Eng) wrote:
> > >
> > > > Hi Daniel,
> > > >
> > > > May I know what's the requirement for adding render node support to a
> > > > "gpu"? Why we just export render node for every drm devices?
> > > > I read document here
> > > > https://www.kernel.org/doc/html/v4.8/gpu/drm-uapi.html#render-nodes
> > >
> > > Thus far we've only done it when there's actual rendering capability,
> > > which generally means at least some private ioctls.
> >
> > Hi Daniel, it looks like vgem is exporting render node by default.
> > Per my understanding, vgem provides some DRM API so users can play
> > with graphic buffers. I am feeling it's natural have a v*** device
> > which provide
> > the surperset which vgem and vkms provides, so it sounds like it's
> > natural add rendernode to vkms, or do the opposite, add kms related
> > stuff to vgem. I still don't get the point: what kind of issue it
> > could bring if we just
> > add render node to vkms? If your point is, we don't do that for other
> > kms only devices, then my question is, how about we just enable render
> > node for every DRM driver? what could go wrong with this approach?
>
> This is wrong for at least two reasons:
>
> - A render node has a semantic value: it indicates whether a device has
>   rendering capabilities. If we attach a render node to vkms, we lie
>   because vkms has no such capability.
Thanks, then my question is, why vgem provides render node? What do you
mean by "rendering capabilities" here? Are you meaning things like GPU
acceleration?
> - This would regress user-space. wlroots would no longer accept to
>   start with Pixman on vkms, because it detects a render node on the
>   device.
>
> I'd advise moving away from abusing DRM dumb buffers in Mesa.

Re: [PATCH] drm/vkms: Add a DRM render node to vkms

[Michel Dänzer]

On 1/5/23 16:16, Yi Xie wrote:
> On Thu, Jan 5, 2023 at 11:16 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> Ah ok. For next time around, copy a link to the comment you want, e.g.
>>
>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5830#note_582477
>>
>> The 3 dots menu on each comments has an option to copy that link tag. That
>> also highlights the right comment.
> 
> Thanks for the tips! Actually you need to sign in to reveal that 3 dots menu.

The date after the username at the top of each GitLab comment is a clickable link for the comment as well.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer