On 2025/10/15 14:07, Kasireddy, Vivek wrote:
Hi Akihiko,
Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob to identify blob
resources
Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob to identify
blob
resources
On 2025/10/04 8:35, Vivek Kasireddy wrote:
The res->blob pointer may not be valid (non-NULL) for some blobs
where the backing storage is not memfd based. Therefore, we cannot
use it to determine if a resource is a blob or not. Instead, we
could use res->blob_size to make this determination as it is
non-zero for blob resources regardless of where their backing
storage is located.
I think this patch is no longer necessary since now you add code to
mmap() VFIO storage with "[PATCH v1 7/7] virtio-gpu-udmabuf: Create
dmabuf for blobs associated with VFIO devices".
Right, but given that mmap() can still fail for various reasons and this
use-case can work as long as dmabuf creation succeeds, I think it makes
sense to not rely on res->blob to determine if a resource is blob or not.
I think the code will be simpler by making resource creation fail when
mmap() fails, and I am concerned that the guest may mulfunction with
such an incomplete resource.
AFAICT, mmap() is a slow, optional path except for the cursor (which needs
further improvement). So, failing resource creation when mmap() fails
does not seem like a good idea to me given the fact that supporting mmap()
is considered optional for dmabuf providers. And, even with vfio, mmap()
can be blocked for various reasons by the kernel driver IIUC.
Being slow doesn't matter, and it is not just for the cursor, but there
are several other functions modified; I guess the resulting resource
will be unusable except for accelerations like virgl.
To motivate the proposed patch, there should be a use-case that requires
to have a resource without mmap(), not one that "can work" a resource
without mmap(). It is extraneous complexity otherwise.
Such a use case should be explained in the patch message and perhaps
also with a comment in the code. The current patch message needs an
update as it sounds like it is unnecessary when theere is code to mmap()
VFIO-based backing storage, which this series has already gained.
Although VFIO supports mmap(), it is not guaranteed to work in all cases
and with a different dmabuf provider (in the future), it may not be possible
at all.
The fact that mmap() is being optional for DMA-BUF and VFIO is
insufficient, but what matters here is whether a DMA-BUF that lacks
mmap() is usable for graphics.
Reading the cover letter, I suppose you are importing a mmap-incapable
DMA-BUF exported by a dGPU, and the imported DMA-BUF is used with virgl
or something. Explaining the use case will show that there is a
mmap-incapable DMA-BUF usable for graphics.
Regards,
Akihiko Odaki
