Re: [v2 1/3] virtio-gpu: Add support for VIRTIO_GPU_BLOB_FLAG_USE_USERPTR flag

[Honglei Huang <honghuan@amd.com>]

On 2025/11/21 15:36, Akihiko Odaki wrote:
> On 2025/11/21 15:50, Honglei Huang wrote:
>>
>>
>> On 2025/11/21 14:12, Akihiko Odaki wrote:
>>>
>>>
>>> On 2025/11/21 14:43, Honglei Huang wrote:
>>>>
>>>>
>>>> On 2025/11/21 13:28, Akihiko Odaki wrote:
>>>>> On 2025/11/21 14:21, Honglei Huang wrote:
>>>>>>
>>>>>>
>>>>>> On 2025/11/21 11:39, Akihiko Odaki wrote:
>>>>>>> On 2025/11/21 12:14, Honglei Huang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2025/11/21 10:56, Akihiko Odaki wrote:
>>>>>>>>> On 2025/11/21 11:47, Honglei Huang wrote:
>>>>>>>>>> Add support for the USE_USERPTR blob flag in virtio-gpu to enable
>>>>>>>>>> user pointer mapping for blob resources. This allows guest 
>>>>>>>>>> applications
>>>>>>>>>> to use user-allocated memory for GPU resources more efficiently.
>>>>>>>>>>
>>>>>>>>>> Changes include:
>>>>>>>>>> - Add VIRTIO_GPU_BLOB_FLAG_USE_USERPTR flag definition
>>>>>>>>>> - Enhance blob resource creation to handle userptr flag properly
>>>>>>>>>> - Remove arbitrary nr_entries limit (16384) in mapping creation
>>>>>>>>>> - Add conditional handling for userptr vs regular blob mapping
>>>>>>>>>
>>>>>>>>> I don't see the added conditional handling.
>>>>>>>>
>>>>>>>> Sorry, the additional handing is replaced by the fixing of value 
>>>>>>>> check.
>>>>>>>> I will correct this commit message in the next version.
>>>>>>>
>>>>>>> Not just the commit message, but it also questions the utility of 
>>>>>>> VIRTIO_GPU_BLOB_FLAG_USE_USERPTR and 
>>>>>>> VIRTIO_GPU_F_RESOURCE_USERPTR. Neither of them adds a new 
>>>>>>> functionality. They should be dropped if they are also replaced 
>>>>>>> with the fix.
>>>>>>>
>>>>>>
>>>>>> Yes totally agreed, it is my mistaken, I shouldn't mix the code 
>>>>>> for fixing and the code for adding new features in one submission.
>>>>>>
>>>>>> Actually this patch set are for another components upstream test, 
>>>>>> for the sake of convenience, I have added both the fix and feature 
>>>>>> here,
>>>>>> that is a bad idea.
>>>>>>
>>>>>> Will split the fix part into previous thread.
>>>>>>
>>>>>> And for the check value fix thread, will send v4 as the final 
>>>>>> version.
>>>>>
>>>>> Splitting fixes and features is a good idea, but that's not what I 
>>>>> meant.
>>>>>
>>>>> What I pointed out is that, it seems that one of the "features" you 
>>>>> are adding, namely VIRTIO_GPU_F_RESOURCE_USERPTR does nothing at at 
>>>>> all. So I'm wondering if you forgot to add a real implementation or 
>>>>> the feature is just no longer necessary.
>>>>
>>>> Understood, actually the resource of flag 
>>>> VIRTIO_GPU_F_RESOURCE_USERPTR just reuses the feature of 
>>>> VIRTIO_GPU_BLOB_MEM_GUEST: using the virtio_gpu_create_mapping_iov 
>>>> function to map the iov from guest.
>>>>
>>>> In qemu, the handing of VIRTIO_GPU_F_RESOURCE_USERPTR and 
>>>> VIRTIO_GPU_BLOB_MEM_GUEST basically same.
>>>>
>>>> The VIRTIO_GPU_F_RESOURCE_USERPTR is from guest userspace, but the
>>>> VIRTIO_GPU_BLOB_MEM_GUEST comes from guest kernel.
>>>> So in VIRTIO kernel and virglrenderer they are different, see VIRTIO 
>>>> kerenl: [1], and virglrenderer: [2].
>>>>
>>>> May I need to change the organizational form of this patch set?
>>>>
>>>> [1]: https://lore.kernel.org/lkml/20251112074548.3718563-1- 
>>>> honglei1.huang@amd.com/
>>>> [2]: https://gitlab.freedesktop.org/virgl/virglrenderer/-/ 
>>>> merge_requests/1568/ 
>>>> diffs#14086999aaf57fc68a3d7d639ab280c3a2672430:~:text=if%20(args%2D%3Eblob_flags%20%26%20VIRGL_RENDERER_BLOB_FLAG_USE_USERPTR)%20%7B
>>> Why does virglrenderer need to distinguish userspace and kernel?
>>
>> It seems like the blob resource in virglrenderer doesn't support the 
>> guest resource/iov resource, virglrenderer doesn't let guest kernel 
>> resource go into get_blob callback.
>>
>> But some feature I am working on really want to let guest userptr 
>> resource get in get_blob callback.
>>
>> So I use this flag to extend the feature of virglrenderer blob callback.
>>
>> The different is:
>> guest kernel resource -> won't go to blob callback.
>> guest userptr resource -> go to blob callback.
> 
> Now I understand what you want to achieve. But there should be no reason 
> to distinguish kernel and userspace; you just need to run some code to 
> set up a resource for hsakmt using some memory.
> 
> In that case, you can simply add another callback that achieves the goal 
> to virglrenderer. There is no need to change the guest-visible interface.

Yes, totally agreed, I come to realized that maybe there is no 
difference between guest blob and userptr resource in qemu/virglrenderer 
when I replied to you. The handle paths can be unified.

Let me think carefully and simplify it in the next version.