diff mbox series

[v2,4/4] virtio-gpu: Don't require udmabuf when blob support is enabled

Message ID 20220913105022.81953-5-antonio.caggiano@collabora.com
State New
Headers show
Series virtio-gpu: Blob resources | expand

Commit Message

Antonio Caggiano Sept. 13, 2022, 10:50 a.m. UTC 
From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Host blobs don't need udmabuf, it's only needed by guest blobs. The host
blobs are utilized by the Mesa virgl driver when persistent memory mapping
is needed by a GL buffer, otherwise virgl driver doesn't use blobs.
Persistent mapping support bumps GL version from 4.3 to 4.5 in guest.
Relax the udmabuf requirement.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reviewed-by: Antonio Caggiano <antonio.caggiano@collabora.com>
---
 hw/display/virtio-gpu.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Comments

Gerd Hoffmann Sept. 23, 2022, 12:32 p.m. UTC | #1 
On Tue, Sep 13, 2022 at 12:50:22PM +0200, Antonio Caggiano wrote:
> From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> Host blobs don't need udmabuf, it's only needed by guest blobs. The host
> blobs are utilized by the Mesa virgl driver when persistent memory mapping
> is needed by a GL buffer, otherwise virgl driver doesn't use blobs.
> Persistent mapping support bumps GL version from 4.3 to 4.5 in guest.
> Relax the udmabuf requirement.

What about blob=on,virgl=off?

In that case qemu manages the resources and continued to require
udmabuf.

Patches 1-3 look good, queued them up.

thanks,
  Gerd
Dmitry Osipenko Sept. 26, 2022, 6:32 p.m. UTC | #2 
On 9/23/22 15:32, Gerd Hoffmann wrote:
> On Tue, Sep 13, 2022 at 12:50:22PM +0200, Antonio Caggiano wrote:
>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>
>> Host blobs don't need udmabuf, it's only needed by guest blobs. The host
>> blobs are utilized by the Mesa virgl driver when persistent memory mapping
>> is needed by a GL buffer, otherwise virgl driver doesn't use blobs.
>> Persistent mapping support bumps GL version from 4.3 to 4.5 in guest.
>> Relax the udmabuf requirement.
> 
> What about blob=on,virgl=off?
> 
> In that case qemu manages the resources and continued to require
> udmabuf.

The udmabuf is used only by the blob resource-creation command in Qemu.
I couldn't find when we could hit that udmabuf code path in Qemu because
BLOB_MEM_GUEST resource type is used only by crosvm+Venus when crosvm
uses a dedicated render-server for virglrenderer.

Summarizing:

  - only BLOB_MEM_GUEST resources require udmabuf
  - /dev/udmabuf isn't accessible by normal user
  - udmabuf driver isn't shipped by all of the popular Linux distros,
for example Debian doesn't ship it

Because of all of the above, I don't think it makes sense to
hard-require udmabuf at the start of Qemu. It's much better to fail
resource creation dynamically.
Gerd Hoffmann Sept. 27, 2022, 8:32 a.m. UTC | #3 
On Mon, Sep 26, 2022 at 09:32:40PM +0300, Dmitry Osipenko wrote:
> On 9/23/22 15:32, Gerd Hoffmann wrote:
> > On Tue, Sep 13, 2022 at 12:50:22PM +0200, Antonio Caggiano wrote:
> >> From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>
> >> Host blobs don't need udmabuf, it's only needed by guest blobs. The host
> >> blobs are utilized by the Mesa virgl driver when persistent memory mapping
> >> is needed by a GL buffer, otherwise virgl driver doesn't use blobs.
> >> Persistent mapping support bumps GL version from 4.3 to 4.5 in guest.
> >> Relax the udmabuf requirement.
> > 
> > What about blob=on,virgl=off?
> > 
> > In that case qemu manages the resources and continued to require
> > udmabuf.
> 
> The udmabuf is used only by the blob resource-creation command in Qemu.
> I couldn't find when we could hit that udmabuf code path in Qemu because
> BLOB_MEM_GUEST resource type is used only by crosvm+Venus when crosvm
> uses a dedicated render-server for virglrenderer.

Recent enough linux guest driver will use BLOB_MEM_GUEST resources
with blob=on + virgl=off

>   - /dev/udmabuf isn't accessible by normal user
>   - udmabuf driver isn't shipped by all of the popular Linux distros,
> for example Debian doesn't ship it

That's why blob resources are off by default.

> Because of all of the above, I don't think it makes sense to
> hard-require udmabuf at the start of Qemu. It's much better to fail
> resource creation dynamically.

Disagree.  When virgl/venus is enabled, then yes, qemu would let
virglrenderer manage resources and I'm ok with whatever requirements
virglrenderer has.  When qemu manages resources by itself udmabuf is
a hard requirement for blob support though.

take care,
  Gerd
Dmitry Osipenko Sept. 27, 2022, 10:44 a.m. UTC | #4 
On 9/27/22 11:32, Gerd Hoffmann wrote:
> On Mon, Sep 26, 2022 at 09:32:40PM +0300, Dmitry Osipenko wrote:
>> On 9/23/22 15:32, Gerd Hoffmann wrote:
>>> On Tue, Sep 13, 2022 at 12:50:22PM +0200, Antonio Caggiano wrote:
>>>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>>
>>>> Host blobs don't need udmabuf, it's only needed by guest blobs. The host
>>>> blobs are utilized by the Mesa virgl driver when persistent memory mapping
>>>> is needed by a GL buffer, otherwise virgl driver doesn't use blobs.
>>>> Persistent mapping support bumps GL version from 4.3 to 4.5 in guest.
>>>> Relax the udmabuf requirement.
>>>
>>> What about blob=on,virgl=off?
>>>
>>> In that case qemu manages the resources and continued to require
>>> udmabuf.
>>
>> The udmabuf is used only by the blob resource-creation command in Qemu.
>> I couldn't find when we could hit that udmabuf code path in Qemu because
>> BLOB_MEM_GUEST resource type is used only by crosvm+Venus when crosvm
>> uses a dedicated render-server for virglrenderer.
> 
> Recent enough linux guest driver will use BLOB_MEM_GUEST resources
> with blob=on + virgl=off

I reproduced this case today using "-device
virtio-gpu-device,blob=true". You're right, it doesn't work without udmabuf.

[    8.369306] virtio_gpu_dequeue_ctrl_func: 90 callbacks suppressed
[    8.369311] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
0x1200 (command 0x105)
[    8.371848] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
0x1205 (command 0x104)
[    8.373085] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
0x1200 (command 0x105)
[    8.376273] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
0x1205 (command 0x104)
[    8.416972] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
0x1200 (command 0x105)
[    8.418841] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
0x1205 (command 0x104)

I see now why you're wanting to keep the udmabuf requirement for
blob=on,virgl=off.


>>   - /dev/udmabuf isn't accessible by normal user
>>   - udmabuf driver isn't shipped by all of the popular Linux distros,
>> for example Debian doesn't ship it
> 
> That's why blob resources are off by default.
> 
>> Because of all of the above, I don't think it makes sense to
>> hard-require udmabuf at the start of Qemu. It's much better to fail
>> resource creation dynamically.
> 
> Disagree.  When virgl/venus is enabled, then yes, qemu would let
> virglrenderer manage resources and I'm ok with whatever requirements
> virglrenderer has.  When qemu manages resources by itself udmabuf is
> a hard requirement for blob support though.

Let's try to relax the udmabuf requirement only for blob=on,virgl=on.
I'll update this patch.

Thank you very much for the review!
Kasireddy, Vivek Sept. 27, 2022, 9:57 p.m. UTC | #5 
Hi Dmitry,

> 
> On 9/27/22 11:32, Gerd Hoffmann wrote:
> > On Mon, Sep 26, 2022 at 09:32:40PM +0300, Dmitry Osipenko wrote:
> >> On 9/23/22 15:32, Gerd Hoffmann wrote:
> >>> On Tue, Sep 13, 2022 at 12:50:22PM +0200, Antonio Caggiano wrote:
> >>>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>>
> >>>> Host blobs don't need udmabuf, it's only needed by guest blobs. The host
> >>>> blobs are utilized by the Mesa virgl driver when persistent memory mapping
> >>>> is needed by a GL buffer, otherwise virgl driver doesn't use blobs.
> >>>> Persistent mapping support bumps GL version from 4.3 to 4.5 in guest.
> >>>> Relax the udmabuf requirement.
> >>>
> >>> What about blob=on,virgl=off?
> >>>
> >>> In that case qemu manages the resources and continued to require
> >>> udmabuf.
> >>
> >> The udmabuf is used only by the blob resource-creation command in Qemu.
> >> I couldn't find when we could hit that udmabuf code path in Qemu because
> >> BLOB_MEM_GUEST resource type is used only by crosvm+Venus when crosvm
> >> uses a dedicated render-server for virglrenderer.
> >
> > Recent enough linux guest driver will use BLOB_MEM_GUEST resources
> > with blob=on + virgl=off
> 
> I reproduced this case today using "-device
> virtio-gpu-device,blob=true". You're right, it doesn't work without udmabuf.
> 
> [    8.369306] virtio_gpu_dequeue_ctrl_func: 90 callbacks suppressed
> [    8.369311] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
> 0x1200 (command 0x105)
> [    8.371848] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
> 0x1205 (command 0x104)
> [    8.373085] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
> 0x1200 (command 0x105)
> [    8.376273] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
> 0x1205 (command 0x104)
> [    8.416972] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
> 0x1200 (command 0x105)
> [    8.418841] [drm:virtio_gpu_dequeue_ctrl_func] *ERROR* response
> 0x1205 (command 0x104)
> 
> I see now why you're wanting to keep the udmabuf requirement for
> blob=on,virgl=off.
> 
> 
> >>   - /dev/udmabuf isn't accessible by normal user
> >>   - udmabuf driver isn't shipped by all of the popular Linux distros,
> >> for example Debian doesn't ship it
> >
> > That's why blob resources are off by default.
> >
> >> Because of all of the above, I don't think it makes sense to
> >> hard-require udmabuf at the start of Qemu. It's much better to fail
> >> resource creation dynamically.
> >
> > Disagree.  When virgl/venus is enabled, then yes, qemu would let
> > virglrenderer manage resources and I'm ok with whatever requirements
> > virglrenderer has.  When qemu manages resources by itself udmabuf is
> > a hard requirement for blob support though.
> 
> Let's try to relax the udmabuf requirement only for blob=on,virgl=on.
> I'll update this patch.
[Kasireddy, Vivek] In addition to what Gerd mentioned and what you have
discovered, I wanted to briefly add that we use Udmabuf/Guest Blobs for
Headless GPU passthrough use-cases (blob=on and virgl=off). Here are some
more details about our use-case:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592#note_851314

Thanks,
Vivek

> 
> Thank you very much for the review!
> 
> --
> Best regards,
> Dmitry
>
diff mbox series

Patch

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index f79693d44d..767142cf5d 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -367,7 +367,9 @@  static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
         return;
     }
 
-    virtio_gpu_init_udmabuf(res);
+    if (cblob.blob_mem == VIRTIO_GPU_BLOB_MEM_GUEST) {
+        virtio_gpu_init_udmabuf(res);
+    }
     QTAILQ_INSERT_HEAD(&g->reslist, res, next);
 }
 
@@ -1319,19 +1321,13 @@  void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
     VirtIOGPU *g = VIRTIO_GPU(qdev);
 
-    if (virtio_gpu_blob_enabled(g->parent_obj.conf)) {
-        if (!virtio_gpu_have_udmabuf()) {
-            error_setg(errp, "cannot enable blob resources without udmabuf");
-            return;
-        }
-
 #ifndef HAVE_VIRGL_RESOURCE_BLOB
-        if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
-            error_setg(errp, "Linked virglrenderer does not support blob resources");
-            return;
-        }
-#endif
+    if (virtio_gpu_blob_enabled(g->parent_obj.conf) &&
+        virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
+        error_setg(errp, "Linked virglrenderer does not support blob resources");
+        return;
     }
+#endif
 
     if (!virtio_gpu_base_device_realize(qdev,
                                         virtio_gpu_handle_ctrl_cb,