drm/virtio: Support partial maps of GEM objects

[PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Sasha Finkelstein via B4 Relay 11 months, 2 weeks ago

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Those are useful to implement coherent cross-vm mmap.

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 drivers/gpu/drm/virtio/virtgpu_vram.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
index 25df81c027837c248a746e41856b5aa7e216b8d5..64e2c6dbdd678ac4c0da89fdd4c9dbf937c2c335 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vram.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
@@ -56,12 +56,11 @@ static int virtio_gpu_vram_mmap(struct drm_gem_object *obj,
 	else if (vram->map_info == VIRTIO_GPU_MAP_CACHE_UNCACHED)
 		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
-	/* Partial mappings of GEM buffers don't happen much in practice. */
-	if (vm_size != vram->vram_node.size)
+	if (vm_size > vram->vram_node.size)
 		return -EINVAL;
 
 	ret = io_remap_pfn_range(vma, vma->vm_start,
-				 vram->vram_node.start >> PAGE_SHIFT,
+				 (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
 				 vm_size, vma->vm_page_prot);
 	return ret;
 }

---
base-commit: 643e2e259c2b25a2af0ae4c23c6e16586d9fd19c
change-id: 20250109-virtgpu-gem-partial-map-335ec40656d1

Re: [PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Dmitry Osipenko 11 months ago

Hi,

On 1/10/25 00:45, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> 
> Those are useful to implement coherent cross-vm mmap.
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_vram.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
> index 25df81c027837c248a746e41856b5aa7e216b8d5..64e2c6dbdd678ac4c0da89fdd4c9dbf937c2c335 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vram.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
> @@ -56,12 +56,11 @@ static int virtio_gpu_vram_mmap(struct drm_gem_object *obj,
>  	else if (vram->map_info == VIRTIO_GPU_MAP_CACHE_UNCACHED)
>  		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  
> -	/* Partial mappings of GEM buffers don't happen much in practice. */
> -	if (vm_size != vram->vram_node.size)
> +	if (vm_size > vram->vram_node.size)
>  		return -EINVAL;
>  
>  	ret = io_remap_pfn_range(vma, vma->vm_start,
> -				 vram->vram_node.start >> PAGE_SHIFT,
> +				 (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
>  				 vm_size, vma->vm_page_prot);
>  	return ret;
>  }

The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM
object based on the vma->vm_pgoff value when mmap is invoked. The
vma->vm_pgoff should be treated as zero here. Hence we can map a part of
GEM, but only from its start. See drm_gem_mmap().

BTW, partial mapping is wanted by Intel native context that I work on.
The intent of this patch is correct. There are practical uses for
partial mappings.

Please correct vma->vm_pgoff in v2.

-- 
Best regards,
Dmitry

Re: [PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Sasha Finkelstein 11 months ago

On Sun, 19 Jan 2025 at 12:50, Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
> >       ret = io_remap_pfn_range(vma, vma->vm_start,
> > -                              vram->vram_node.start >> PAGE_SHIFT,
> > +                              (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
> >                                vm_size, vma->vm_page_prot);
> >       return ret;
> >  }
>
> The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM
> object based on the vma->vm_pgoff value when mmap is invoked.

If my understanding is correct, vm_pgoff gets "unfaked" by
https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/virtio/virtgpu_vram.c#L48

> vma->vm_pgoff should be treated as zero here. Hence we can map a part of
> GEM, but only from its start. See drm_gem_mmap().

I've had a "v0" (not on ml) of this patch that always treated vma->vm_pgoff as
zero. This broke when anything tried to mmap with a non-zero offset. Adding
vm_pgoff made it work correctly.

> Please correct vma->vm_pgoff in v2.

I need apps to be able to mmap with a non-zero offset for my usecase.
While the correct value may be something else other than what is in
the current patch, 0 is definitely incorrect for at least some workloads.

Re: [PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Dmitry Osipenko 11 months ago

On 1/19/25 19:18, Sasha Finkelstein wrote:
> On Sun, 19 Jan 2025 at 12:50, Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>>       ret = io_remap_pfn_range(vma, vma->vm_start,
>>> -                              vram->vram_node.start >> PAGE_SHIFT,
>>> +                              (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
>>>                                vm_size, vma->vm_page_prot);
>>>       return ret;
>>>  }
>>
>> The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM
>> object based on the vma->vm_pgoff value when mmap is invoked.
> 
> If my understanding is correct, vm_pgoff gets "unfaked" by
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/virtio/virtgpu_vram.c#L48
> 
>> vma->vm_pgoff should be treated as zero here. Hence we can map a part of
>> GEM, but only from its start. See drm_gem_mmap().
> 
> I've had a "v0" (not on ml) of this patch that always treated vma->vm_pgoff as
> zero. This broke when anything tried to mmap with a non-zero offset. Adding
> vm_pgoff made it work correctly.

I've tested this patch. Partial mapping with a non-zero offset doesn't
work because drm_gem_mmap() rejects it. I'd want to see your sample code
that performs mmaping, maybe I'm missing something.

>> Please correct vma->vm_pgoff in v2.
> 
> I need apps to be able to mmap with a non-zero offset for my usecase.
> While the correct value may be something else other than what is in
> the current patch, 0 is definitely incorrect for at least some workloads.

drm_gem_mmap() uses drm_vma_offset_exact_lookup_locked() that doesn't
allow vma->vm_pgoff != node.start. AFAICT, no one driver supports
mapping with a non-zero offset, perhaps for a reason that I don't know
about.

-- 
Best regards,
Dmitry

Re: [PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Sasha Finkelstein 11 months ago

On Sun, 19 Jan 2025 at 21:02, Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
> I've tested this patch. Partial mapping with a non-zero offset doesn't
> work because drm_gem_mmap() rejects it. I'd want to see your sample code
> that performs mmaping, maybe I'm missing something.

The entire setup is a huge pile of moving parts, but here is the relevant
function that creates a fd from the cross domain info (this is a part
of the vm agent)
https://github.com/WhatAmISupposedToPutHere/krun/blob/pipewire/crates/muvm/src/guest/bridge/common.rs#L745
After the fd is created, it is passed to a pipewire client running
inside the vm,
which in turn does a simple mmap() (with a non-zero offset) on the relevant fd.
Not sure where exactly pipewire's code does that, the syscall was observed
via stracing the relevant process.

Re: [PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Dmitry Osipenko 11 months ago

On 1/19/25 23:02, Dmitry Osipenko wrote:
> On 1/19/25 19:18, Sasha Finkelstein wrote:
>> On Sun, 19 Jan 2025 at 12:50, Dmitry Osipenko
>> <dmitry.osipenko@collabora.com> wrote:
>>>>       ret = io_remap_pfn_range(vma, vma->vm_start,
>>>> -                              vram->vram_node.start >> PAGE_SHIFT,
>>>> +                              (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
>>>>                                vm_size, vma->vm_page_prot);
>>>>       return ret;
>>>>  }
>>>
>>> The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM
>>> object based on the vma->vm_pgoff value when mmap is invoked.
>>
>> If my understanding is correct, vm_pgoff gets "unfaked" by
>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/virtio/virtgpu_vram.c#L48
>>
>>> vma->vm_pgoff should be treated as zero here. Hence we can map a part of
>>> GEM, but only from its start. See drm_gem_mmap().
>>
>> I've had a "v0" (not on ml) of this patch that always treated vma->vm_pgoff as
>> zero. This broke when anything tried to mmap with a non-zero offset. Adding
>> vm_pgoff made it work correctly.
> 
> I've tested this patch. Partial mapping with a non-zero offset doesn't
> work because drm_gem_mmap() rejects it. I'd want to see your sample code
> that performs mmaping, maybe I'm missing something.
> 
>>> Please correct vma->vm_pgoff in v2.
>>
>> I need apps to be able to mmap with a non-zero offset for my usecase.
>> While the correct value may be something else other than what is in
>> the current patch, 0 is definitely incorrect for at least some workloads.
> 
> drm_gem_mmap() uses drm_vma_offset_exact_lookup_locked() that doesn't
> allow vma->vm_pgoff != node.start. AFAICT, no one driver supports
> mapping with a non-zero offset, perhaps for a reason that I don't know
> about.

See now that a non-zero mapping of a dmabuf might work. Will test it.

-- 
Best regards,
Dmitry

Re: [PATCH] drm/virtio: Support partial maps of GEM objects

Posted by Dmitry Osipenko 10 months, 4 weeks ago

On 1/19/25 23:23, Dmitry Osipenko wrote:
> On 1/19/25 23:02, Dmitry Osipenko wrote:
>> On 1/19/25 19:18, Sasha Finkelstein wrote:
>>> On Sun, 19 Jan 2025 at 12:50, Dmitry Osipenko
>>> <dmitry.osipenko@collabora.com> wrote:
>>>>>       ret = io_remap_pfn_range(vma, vma->vm_start,
>>>>> -                              vram->vram_node.start >> PAGE_SHIFT,
>>>>> +                              (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
>>>>>                                vm_size, vma->vm_page_prot);
>>>>>       return ret;
>>>>>  }
>>>>
>>>> The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM
>>>> object based on the vma->vm_pgoff value when mmap is invoked.
>>>
>>> If my understanding is correct, vm_pgoff gets "unfaked" by
>>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/virtio/virtgpu_vram.c#L48
>>>
>>>> vma->vm_pgoff should be treated as zero here. Hence we can map a part of
>>>> GEM, but only from its start. See drm_gem_mmap().
>>>
>>> I've had a "v0" (not on ml) of this patch that always treated vma->vm_pgoff as
>>> zero. This broke when anything tried to mmap with a non-zero offset. Adding
>>> vm_pgoff made it work correctly.
>>
>> I've tested this patch. Partial mapping with a non-zero offset doesn't
>> work because drm_gem_mmap() rejects it. I'd want to see your sample code
>> that performs mmaping, maybe I'm missing something.
>>
>>>> Please correct vma->vm_pgoff in v2.
>>>
>>> I need apps to be able to mmap with a non-zero offset for my usecase.
>>> While the correct value may be something else other than what is in
>>> the current patch, 0 is definitely incorrect for at least some workloads.
>>
>> drm_gem_mmap() uses drm_vma_offset_exact_lookup_locked() that doesn't
>> allow vma->vm_pgoff != node.start. AFAICT, no one driver supports
>> mapping with a non-zero offset, perhaps for a reason that I don't know
>> about.
> 
> See now that a non-zero mapping of a dmabuf might work. Will test it.

Works for dmabuf

> 
> -	/* Partial mappings of GEM buffers don't happen much in practice. */
> -	if (vm_size != vram->vram_node.size)
> +	if (vm_size > vram->vram_node.size)
>  		return -EINVAL;

This check should include the vm_pgoff, like that:

if (check_add_overflow(vma->vm_pgoff << PAGE_SHIFT, vm_size, &vm_end))
	return -EINVAL;

if (vm_end > vram->vram_node.size)
	return -EINVAL;

The size and offset are actually validated before this code is reached,
but doesn't hurt to keep the check around.

I corrected the check and applied patch to misc-next, thanks!

-- 
Best regards,
Dmitry