[PATCH drm-next 0/2] Enhance drm_panic Support for Virtio-GPU

[PATCH drm-next 0/2] Enhance drm_panic Support for Virtio-GPU

[Ryosuke Yasuoka]

Hi

This patch series proposes enhancement for drm_panic. While virtio-gpu
currently supports drm_panic [1], it is limited to vmapped shmem BOs.
IOW, it does not work in non-VT FB environments, such as GUI desktops.
This limitation arises because shmem BOs require vmap, which cannot be
used in a panic handler since vmap is sleepable and takes locks. To
address this, drm_panic needs an atomic variant of vmap.

The first patch (1/2) introduces atomic_vmap, and the second patch (2/2)
updates the existing virtio drm_panic implementation to use the
atomic_vmap. I've tested these changes in both Gnome and VT
environments, and they work correctly.

Best regards,
Ryosuke

[1] https://patchwork.freedesktop.org/patch/635658/ 

Ryosuke Yasuoka (2):
  vmalloc: Add atomic_vmap
  drm/virtio: Use atomic_vmap to work drm_panic in GUI

 drivers/gpu/drm/drm_gem.c              |  51 ++++++++++++
 drivers/gpu/drm/drm_gem_shmem_helper.c |  51 ++++++++++++
 drivers/gpu/drm/virtio/virtgpu_plane.c |  14 +++-
 include/drm/drm_gem.h                  |   1 +
 include/drm/drm_gem_shmem_helper.h     |   2 +
 include/linux/vmalloc.h                |   2 +
 mm/internal.h                          |   5 ++
 mm/vmalloc.c                           | 105 +++++++++++++++++++++++++
 8 files changed, 228 insertions(+), 3 deletions(-)


base-commit: e21cba704714c301d04c5fd37a693734b623872a
-- 
2.48.1

[PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Ryosuke Yasuoka]

Some drivers can use vmap in drm_panic, however, vmap is sleepable and
takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
requests pages with GFP_ATOMIC and maps KVA without locks and sleep.

Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
---
 include/linux/vmalloc.h |   2 +
 mm/internal.h           |   5 ++
 mm/vmalloc.c            | 105 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 31e9ffd936e3..c7a2a9a1976d 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -190,6 +190,8 @@ void * __must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags)
 extern void vfree(const void *addr);
 extern void vfree_atomic(const void *addr);
 
+extern void *atomic_vmap(struct page **pages, unsigned int count,
+			 unsigned long flags, pgprot_t prot);
 extern void *vmap(struct page **pages, unsigned int count,
 			unsigned long flags, pgprot_t prot);
 void *vmap_pfn(unsigned long *pfns, unsigned int count, pgprot_t prot);
diff --git a/mm/internal.h b/mm/internal.h
index 109ef30fee11..134b332bf5b9 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1278,6 +1278,11 @@ int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
 void free_zone_device_folio(struct folio *folio);
 int migrate_device_coherent_folio(struct folio *folio);
 
+struct vm_struct *atomic_get_vm_area_node(unsigned long size, unsigned long align,
+					  unsigned long shift, unsigned long flags,
+					  unsigned long start, unsigned long end, int node,
+					  gfp_t gfp_mask, const void *caller);
+
 struct vm_struct *__get_vm_area_node(unsigned long size,
 				     unsigned long align, unsigned long shift,
 				     unsigned long flags, unsigned long start,
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index a6e7acebe9ad..f5c93779c60a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1945,6 +1945,57 @@ static inline void setup_vmalloc_vm(struct vm_struct *vm,
 	va->vm = vm;
 }
 
+static struct vmap_area *atomic_alloc_vmap_area(unsigned long size,
+						unsigned long align,
+						unsigned long vstart, unsigned long vend,
+						int node, gfp_t gfp_mask,
+						unsigned long va_flags, struct vm_struct *vm)
+{
+	struct vmap_node *vn;
+	struct vmap_area *va;
+	unsigned long addr;
+
+	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
+		return ERR_PTR(-EINVAL);
+
+	if (unlikely(!vmap_initialized))
+		return ERR_PTR(-EBUSY);
+
+	va = kmem_cache_alloc_node(vmap_area_cachep, gfp_mask, node);
+	if (unlikely(!va))
+		return ERR_PTR(-ENOMEM);
+
+	/*
+	 * Only scan the relevant parts containing pointers to other objects
+	 * to avoid false negatives.
+	 */
+	kmemleak_scan_area(&va->rb_node, SIZE_MAX, gfp_mask);
+
+	addr = __alloc_vmap_area(&free_vmap_area_root, &free_vmap_area_list,
+				 size, align, vstart, vend);
+
+	trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend);
+
+	va->va_start = addr;
+	va->va_end = addr + size;
+	va->vm = NULL;
+	va->flags = va_flags;
+
+	vm->addr = (void *)va->va_start;
+	vm->size = va_size(va);
+	va->vm = vm;
+
+	vn = addr_to_node(va->va_start);
+
+	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
+
+	BUG_ON(!IS_ALIGNED(va->va_start, align));
+	BUG_ON(va->va_start < vstart);
+	BUG_ON(va->va_end > vend);
+
+	return va;
+}
+
 /*
  * Allocate a region of KVA of the specified size and alignment, within the
  * vstart and vend. If vm is passed in, the two will also be bound.
@@ -3106,6 +3157,33 @@ static void clear_vm_uninitialized_flag(struct vm_struct *vm)
 	vm->flags &= ~VM_UNINITIALIZED;
 }
 
+struct vm_struct *atomic_get_vm_area_node(unsigned long size, unsigned long align,
+					  unsigned long shift, unsigned long flags,
+					  unsigned long start, unsigned long end, int node,
+					  gfp_t gfp_mask, const void *caller)
+{
+	struct vmap_area *va;
+	struct vm_struct *area;
+
+	size = ALIGN(size, 1ul << shift);
+	if (unlikely(!size))
+		return NULL;
+
+	area = kzalloc_node(sizeof(*area), gfp_mask, node);
+	if (unlikely(!area))
+		return NULL;
+
+	size += PAGE_SIZE;
+	area->flags = flags;
+	area->caller = caller;
+
+	va = atomic_alloc_vmap_area(size, align, start, end, node, gfp_mask, 0, area);
+	if (IS_ERR(va))
+		return NULL;
+
+	return area;
+}
+
 struct vm_struct *__get_vm_area_node(unsigned long size,
 		unsigned long align, unsigned long shift, unsigned long flags,
 		unsigned long start, unsigned long end, int node,
@@ -3418,6 +3496,33 @@ void vunmap(const void *addr)
 }
 EXPORT_SYMBOL(vunmap);
 
+void *atomic_vmap(struct page **pages, unsigned int count,
+		  unsigned long flags, pgprot_t prot)
+{
+	struct vm_struct *area;
+	unsigned long addr;
+	unsigned long size;		/* In bytes */
+
+	if (count > totalram_pages())
+		return NULL;
+
+	size = (unsigned long)count << PAGE_SHIFT;
+	area = atomic_get_vm_area_node(size, 1, PAGE_SHIFT, flags,
+				       VMALLOC_START, VMALLOC_END,
+				       NUMA_NO_NODE, GFP_ATOMIC,
+				       __builtin_return_address(0));
+	if (!area)
+		return NULL;
+
+	addr = (unsigned long)area->addr;
+	if (vmap_pages_range(addr, addr + size, pgprot_nx(prot),
+			     pages, PAGE_SHIFT) < 0) {
+		return NULL;
+	}
+
+	return area->addr;
+}
+
 /**
  * vmap - map an array of pages into virtually contiguous space
  * @pages: array of page pointers
-- 
2.48.1

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Markus Elfring]

> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.

See also:
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc5#n94

Regards,
Markus

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Uladzislau Rezki]

On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
> 
> Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
> ---
>  include/linux/vmalloc.h |   2 +
>  mm/internal.h           |   5 ++
>  mm/vmalloc.c            | 105 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 112 insertions(+)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 31e9ffd936e3..c7a2a9a1976d 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -190,6 +190,8 @@ void * __must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags)
>  extern void vfree(const void *addr);
>  extern void vfree_atomic(const void *addr);
>  
> +extern void *atomic_vmap(struct page **pages, unsigned int count,
> +			 unsigned long flags, pgprot_t prot);
>  extern void *vmap(struct page **pages, unsigned int count,
>  			unsigned long flags, pgprot_t prot);
>  void *vmap_pfn(unsigned long *pfns, unsigned int count, pgprot_t prot);
> diff --git a/mm/internal.h b/mm/internal.h
> index 109ef30fee11..134b332bf5b9 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1278,6 +1278,11 @@ int numa_migrate_check(struct folio *folio, struct vm_fault *vmf,
>  void free_zone_device_folio(struct folio *folio);
>  int migrate_device_coherent_folio(struct folio *folio);
>  
> +struct vm_struct *atomic_get_vm_area_node(unsigned long size, unsigned long align,
> +					  unsigned long shift, unsigned long flags,
> +					  unsigned long start, unsigned long end, int node,
> +					  gfp_t gfp_mask, const void *caller);
> +
>  struct vm_struct *__get_vm_area_node(unsigned long size,
>  				     unsigned long align, unsigned long shift,
>  				     unsigned long flags, unsigned long start,
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index a6e7acebe9ad..f5c93779c60a 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1945,6 +1945,57 @@ static inline void setup_vmalloc_vm(struct vm_struct *vm,
>  	va->vm = vm;
>  }
>  
> +static struct vmap_area *atomic_alloc_vmap_area(unsigned long size,
> +						unsigned long align,
> +						unsigned long vstart, unsigned long vend,
> +						int node, gfp_t gfp_mask,
> +						unsigned long va_flags, struct vm_struct *vm)
> +{
> +	struct vmap_node *vn;
> +	struct vmap_area *va;
> +	unsigned long addr;
> +
> +	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (unlikely(!vmap_initialized))
> +		return ERR_PTR(-EBUSY);
> +
> +	va = kmem_cache_alloc_node(vmap_area_cachep, gfp_mask, node);
> +	if (unlikely(!va))
> +		return ERR_PTR(-ENOMEM);
> +
> +	/*
> +	 * Only scan the relevant parts containing pointers to other objects
> +	 * to avoid false negatives.
> +	 */
> +	kmemleak_scan_area(&va->rb_node, SIZE_MAX, gfp_mask);
> +
> +	addr = __alloc_vmap_area(&free_vmap_area_root, &free_vmap_area_list,
> +				 size, align, vstart, vend);
> +
> +	trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend);
> +
> +	va->va_start = addr;
> +	va->va_end = addr + size;
> +	va->vm = NULL;
> +	va->flags = va_flags;
> +
> +	vm->addr = (void *)va->va_start;
> +	vm->size = va_size(va);
> +	va->vm = vm;
> +
> +	vn = addr_to_node(va->va_start);
> +
> +	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
> +
> +	BUG_ON(!IS_ALIGNED(va->va_start, align));
> +	BUG_ON(va->va_start < vstart);
> +	BUG_ON(va->va_end > vend);
> +
> +	return va;
> +}
> +
>  /*
>   * Allocate a region of KVA of the specified size and alignment, within the
>   * vstart and vend. If vm is passed in, the two will also be bound.
> @@ -3106,6 +3157,33 @@ static void clear_vm_uninitialized_flag(struct vm_struct *vm)
>  	vm->flags &= ~VM_UNINITIALIZED;
>  }
>  
> +struct vm_struct *atomic_get_vm_area_node(unsigned long size, unsigned long align,
> +					  unsigned long shift, unsigned long flags,
> +					  unsigned long start, unsigned long end, int node,
> +					  gfp_t gfp_mask, const void *caller)
> +{
> +	struct vmap_area *va;
> +	struct vm_struct *area;
> +
> +	size = ALIGN(size, 1ul << shift);
> +	if (unlikely(!size))
> +		return NULL;
> +
> +	area = kzalloc_node(sizeof(*area), gfp_mask, node);
> +	if (unlikely(!area))
> +		return NULL;
> +
> +	size += PAGE_SIZE;
> +	area->flags = flags;
> +	area->caller = caller;
> +
> +	va = atomic_alloc_vmap_area(size, align, start, end, node, gfp_mask, 0, area);
> +	if (IS_ERR(va))
> +		return NULL;
> +
> +	return area;
> +}
> +
>  struct vm_struct *__get_vm_area_node(unsigned long size,
>  		unsigned long align, unsigned long shift, unsigned long flags,
>  		unsigned long start, unsigned long end, int node,
> @@ -3418,6 +3496,33 @@ void vunmap(const void *addr)
>  }
>  EXPORT_SYMBOL(vunmap);
>  
> +void *atomic_vmap(struct page **pages, unsigned int count,
> +		  unsigned long flags, pgprot_t prot)
> +{
> +	struct vm_struct *area;
> +	unsigned long addr;
> +	unsigned long size;		/* In bytes */
> +
> +	if (count > totalram_pages())
> +		return NULL;
> +
> +	size = (unsigned long)count << PAGE_SHIFT;
> +	area = atomic_get_vm_area_node(size, 1, PAGE_SHIFT, flags,
> +				       VMALLOC_START, VMALLOC_END,
> +				       NUMA_NO_NODE, GFP_ATOMIC,
> +				       __builtin_return_address(0));
> +	if (!area)
> +		return NULL;
> +
> +	addr = (unsigned long)area->addr;
> +	if (vmap_pages_range(addr, addr + size, pgprot_nx(prot),
> +			     pages, PAGE_SHIFT) < 0) {
> +		return NULL;
> +	}
> +
> +	return area->addr;
> +}
> +
>  /**
>   * vmap - map an array of pages into virtually contiguous space
>   * @pages: array of page pointers
> -- 
> 2.48.1
> 
It is copy-paste code, so it is odd. The proposal is not a way forward
to me. Unfortunately vmalloc is not compatible with GFP_ATOMIC, there
is at least one place it is a page-table allocation entries where it is
hard-coded to the GFP_KERNEL.

Doing this without locks and synchronizations is not possible.

--
Uladzislau Rezki

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Matthew Wilcox]

On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.

In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
is this supposed to work?

> +	vn = addr_to_node(va->va_start);
> +
> +	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);

If someone else is holding the vn->busy.lock because they're modifying the
busy tree, you'll corrupt the tree.  You can't just say "I can't take a
lock here, so I won't bother".  You need to figure out how to do something
safe without taking the lock.  For example, you could preallocate the
page tables and reserve a vmap area when the driver loads that would
then be usable for the panic situation.  I don't know that we have APIs
to let you do that today, but it's something that could be added.

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Jocelyn Falempe]

On 06/03/2025 05:52, Matthew Wilcox wrote:
> On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
>> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
>> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
>> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
> 
> In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
> is this supposed to work?
> 
>> +	vn = addr_to_node(va->va_start);
>> +
>> +	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
> 
> If someone else is holding the vn->busy.lock because they're modifying the
> busy tree, you'll corrupt the tree.  You can't just say "I can't take a
> lock here, so I won't bother".  You need to figure out how to do something
> safe without taking the lock.  For example, you could preallocate the
> page tables and reserve a vmap area when the driver loads that would
> then be usable for the panic situation.  I don't know that we have APIs
> to let you do that today, but it's something that could be added.
> 
Regarding the lock, it should be possible to use the trylock() variant, 
and fail if the lock is already taken. (In the panic handler, only 1 CPU 
remain active, so it's unlikely the lock would be released anyway).

If we need to pre-allocate the page table and reserve the vmap area, 
maybe it would be easier to just always vmap() the primary framebuffer, 
so it can be used in the panic handler?

Best regards,

-- 

Jocelyn

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Uladzislau Rezki]

On Thu, Mar 06, 2025 at 02:24:51PM +0100, Jocelyn Falempe wrote:
> On 06/03/2025 05:52, Matthew Wilcox wrote:
> > On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
> > > Some drivers can use vmap in drm_panic, however, vmap is sleepable and
> > > takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
> > > requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
> > 
> > In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
> > is this supposed to work?
> > 
> > > +	vn = addr_to_node(va->va_start);
> > > +
> > > +	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
> > 
> > If someone else is holding the vn->busy.lock because they're modifying the
> > busy tree, you'll corrupt the tree.  You can't just say "I can't take a
> > lock here, so I won't bother".  You need to figure out how to do something
> > safe without taking the lock.  For example, you could preallocate the
> > page tables and reserve a vmap area when the driver loads that would
> > then be usable for the panic situation.  I don't know that we have APIs
> > to let you do that today, but it's something that could be added.
> > 
> Regarding the lock, it should be possible to use the trylock() variant, and
> fail if the lock is already taken. (In the panic handler, only 1 CPU remain
> active, so it's unlikely the lock would be released anyway).
> 
> If we need to pre-allocate the page table and reserve the vmap area, maybe
> it would be easier to just always vmap() the primary framebuffer, so it can
> be used in the panic handler?
> 
We can reserve a vmap space for ATOMIC or NOWAIT allocations. As for PTE
part, we can also populate reserved space, because after that operation
those are never get released.

The question is how many users need this. As for this particular case i
am in line with Jocelyn Falempe. Allocate for DRM and write on panic.

--
Uladzislau Rezki

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Simona Vetter]

On Thu, Mar 06, 2025 at 02:24:51PM +0100, Jocelyn Falempe wrote:
> On 06/03/2025 05:52, Matthew Wilcox wrote:
> > On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
> > > Some drivers can use vmap in drm_panic, however, vmap is sleepable and
> > > takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
> > > requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
> > 
> > In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
> > is this supposed to work?
> > 
> > > +	vn = addr_to_node(va->va_start);
> > > +
> > > +	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
> > 
> > If someone else is holding the vn->busy.lock because they're modifying the
> > busy tree, you'll corrupt the tree.  You can't just say "I can't take a
> > lock here, so I won't bother".  You need to figure out how to do something
> > safe without taking the lock.  For example, you could preallocate the
> > page tables and reserve a vmap area when the driver loads that would
> > then be usable for the panic situation.  I don't know that we have APIs
> > to let you do that today, but it's something that could be added.
> > 
> Regarding the lock, it should be possible to use the trylock() variant, and
> fail if the lock is already taken. (In the panic handler, only 1 CPU remain
> active, so it's unlikely the lock would be released anyway).
> 
> If we need to pre-allocate the page table and reserve the vmap area, maybe
> it would be easier to just always vmap() the primary framebuffer, so it can
> be used in the panic handler?

Yeah I really don't like the idea of creating some really brittle one-off
core mm code just so we don't have to vmap a buffer unconditionally. I
think even better would be if drm_panic can cope with non-linear buffers,
it's entirely fine if the drawing function absolutely crawls and sets each
individual byte ...

The only thing you're allowed to do in panic is try_lock on a raw spinlock
(plus some really scare lockless tricks), imposing that on core mm sounds
like a non-starter to me.

Cheers, Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Jocelyn Falempe]

On 06/03/2025 16:52, Simona Vetter wrote:
> On Thu, Mar 06, 2025 at 02:24:51PM +0100, Jocelyn Falempe wrote:
>> On 06/03/2025 05:52, Matthew Wilcox wrote:
>>> On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
>>>> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
>>>> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
>>>> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
>>>
>>> In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
>>> is this supposed to work?
>>>
>>>> +	vn = addr_to_node(va->va_start);
>>>> +
>>>> +	insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
>>>
>>> If someone else is holding the vn->busy.lock because they're modifying the
>>> busy tree, you'll corrupt the tree.  You can't just say "I can't take a
>>> lock here, so I won't bother".  You need to figure out how to do something
>>> safe without taking the lock.  For example, you could preallocate the
>>> page tables and reserve a vmap area when the driver loads that would
>>> then be usable for the panic situation.  I don't know that we have APIs
>>> to let you do that today, but it's something that could be added.
>>>
>> Regarding the lock, it should be possible to use the trylock() variant, and
>> fail if the lock is already taken. (In the panic handler, only 1 CPU remain
>> active, so it's unlikely the lock would be released anyway).
>>
>> If we need to pre-allocate the page table and reserve the vmap area, maybe
>> it would be easier to just always vmap() the primary framebuffer, so it can
>> be used in the panic handler?
> 
> Yeah I really don't like the idea of creating some really brittle one-off
> core mm code just so we don't have to vmap a buffer unconditionally. I
> think even better would be if drm_panic can cope with non-linear buffers,
> it's entirely fine if the drawing function absolutely crawls and sets each
> individual byte ...

It already supports some non-linear buffer, like Nvidia block-linear:
https://elixir.bootlin.com/linux/v6.13.5/source/drivers/gpu/drm/nouveau/dispnv50/wndw.c#L606

And I've also sent some patches to support Intel's 4-tile and Y-tile format:
https://patchwork.freedesktop.org/patch/637200/?series=141936&rev=5
https://patchwork.freedesktop.org/patch/637202/?series=141936&rev=5

Hopefully Color Compression can be disabled on intel's GPU, otherwise 
that would be a bit harder to implement than tiling.

> 
> The only thing you're allowed to do in panic is try_lock on a raw spinlock
> (plus some really scare lockless tricks), imposing that on core mm sounds
> like a non-starter to me.
> 
> Cheers, Sima

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Ryosuke Yasuoka]

On Fri, Mar 7, 2025 at 4:55 PM Jocelyn Falempe <jfalempe@redhat.com> wrote:
>
> On 06/03/2025 16:52, Simona Vetter wrote:
> > On Thu, Mar 06, 2025 at 02:24:51PM +0100, Jocelyn Falempe wrote:
> >> On 06/03/2025 05:52, Matthew Wilcox wrote:
> >>> On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
> >>>> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
> >>>> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
> >>>> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
> >>>
> >>> In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
> >>> is this supposed to work?
> >>>
> >>>> +  vn = addr_to_node(va->va_start);
> >>>> +
> >>>> +  insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
> >>>
> >>> If someone else is holding the vn->busy.lock because they're modifying the
> >>> busy tree, you'll corrupt the tree.  You can't just say "I can't take a
> >>> lock here, so I won't bother".  You need to figure out how to do something
> >>> safe without taking the lock.  For example, you could preallocate the
> >>> page tables and reserve a vmap area when the driver loads that would
> >>> then be usable for the panic situation.  I don't know that we have APIs
> >>> to let you do that today, but it's something that could be added.
> >>>
> >> Regarding the lock, it should be possible to use the trylock() variant, and
> >> fail if the lock is already taken. (In the panic handler, only 1 CPU remain
> >> active, so it's unlikely the lock would be released anyway).
> >>
> >> If we need to pre-allocate the page table and reserve the vmap area, maybe
> >> it would be easier to just always vmap() the primary framebuffer, so it can
> >> be used in the panic handler?
> >
> > Yeah I really don't like the idea of creating some really brittle one-off
> > core mm code just so we don't have to vmap a buffer unconditionally. I
> > think even better would be if drm_panic can cope with non-linear buffers,
> > it's entirely fine if the drawing function absolutely crawls and sets each
> > individual byte ...
>
> It already supports some non-linear buffer, like Nvidia block-linear:
> https://elixir.bootlin.com/linux/v6.13.5/source/drivers/gpu/drm/nouveau/dispnv50/wndw.c#L606
>
> And I've also sent some patches to support Intel's 4-tile and Y-tile format:
> https://patchwork.freedesktop.org/patch/637200/?series=141936&rev=5
> https://patchwork.freedesktop.org/patch/637202/?series=141936&rev=5
>
> Hopefully Color Compression can be disabled on intel's GPU, otherwise
> that would be a bit harder to implement than tiling.
>
> >
> > The only thing you're allowed to do in panic is try_lock on a raw spinlock
> > (plus some really scare lockless tricks), imposing that on core mm sounds
> > like a non-starter to me.
> >
> > Cheers, Sima
>

Thank you all for your comments.
I understand adding atomic_vmap is not possible as vmalloc is not compatible
with GFP_ATOMIC. I'll re-implement this by pre-allocating the page table and
reserve the vmap area while the kernel is alive. It'll might be
allocated in driver
codes so maybe I don't need to add any features in core mm code.

Best regards,
Ryosuke

Re: [PATCH drm-next 1/2] vmalloc: Add atomic_vmap

[Jocelyn Falempe]

On 09/03/2025 09:07, Ryosuke Yasuoka wrote:
> On Fri, Mar 7, 2025 at 4:55 PM Jocelyn Falempe <jfalempe@redhat.com> wrote:
>>
>> On 06/03/2025 16:52, Simona Vetter wrote:
>>> On Thu, Mar 06, 2025 at 02:24:51PM +0100, Jocelyn Falempe wrote:
>>>> On 06/03/2025 05:52, Matthew Wilcox wrote:
>>>>> On Thu, Mar 06, 2025 at 12:25:53AM +0900, Ryosuke Yasuoka wrote:
>>>>>> Some drivers can use vmap in drm_panic, however, vmap is sleepable and
>>>>>> takes locks. Since drm_panic will vmap in panic handler, atomic_vmap
>>>>>> requests pages with GFP_ATOMIC and maps KVA without locks and sleep.
>>>>>
>>>>> In addition to the implicit GFP_KERNEL allocations Vlad mentioned, how
>>>>> is this supposed to work?
>>>>>
>>>>>> +  vn = addr_to_node(va->va_start);
>>>>>> +
>>>>>> +  insert_vmap_area(va, &vn->busy.root, &vn->busy.head);
>>>>>
>>>>> If someone else is holding the vn->busy.lock because they're modifying the
>>>>> busy tree, you'll corrupt the tree.  You can't just say "I can't take a
>>>>> lock here, so I won't bother".  You need to figure out how to do something
>>>>> safe without taking the lock.  For example, you could preallocate the
>>>>> page tables and reserve a vmap area when the driver loads that would
>>>>> then be usable for the panic situation.  I don't know that we have APIs
>>>>> to let you do that today, but it's something that could be added.
>>>>>
>>>> Regarding the lock, it should be possible to use the trylock() variant, and
>>>> fail if the lock is already taken. (In the panic handler, only 1 CPU remain
>>>> active, so it's unlikely the lock would be released anyway).
>>>>
>>>> If we need to pre-allocate the page table and reserve the vmap area, maybe
>>>> it would be easier to just always vmap() the primary framebuffer, so it can
>>>> be used in the panic handler?
>>>
>>> Yeah I really don't like the idea of creating some really brittle one-off
>>> core mm code just so we don't have to vmap a buffer unconditionally. I
>>> think even better would be if drm_panic can cope with non-linear buffers,
>>> it's entirely fine if the drawing function absolutely crawls and sets each
>>> individual byte ...
>>
>> It already supports some non-linear buffer, like Nvidia block-linear:
>> https://elixir.bootlin.com/linux/v6.13.5/source/drivers/gpu/drm/nouveau/dispnv50/wndw.c#L606
>>
>> And I've also sent some patches to support Intel's 4-tile and Y-tile format:
>> https://patchwork.freedesktop.org/patch/637200/?series=141936&rev=5
>> https://patchwork.freedesktop.org/patch/637202/?series=141936&rev=5
>>
>> Hopefully Color Compression can be disabled on intel's GPU, otherwise
>> that would be a bit harder to implement than tiling.
>>
>>>
>>> The only thing you're allowed to do in panic is try_lock on a raw spinlock
>>> (plus some really scare lockless tricks), imposing that on core mm sounds
>>> like a non-starter to me.
>>>
>>> Cheers, Sima
>>
> 
> Thank you all for your comments.
> I understand adding atomic_vmap is not possible as vmalloc is not compatible
> with GFP_ATOMIC. I'll re-implement this by pre-allocating the page table and
> reserve the vmap area while the kernel is alive. It'll might be
> allocated in driver
> codes so maybe I don't need to add any features in core mm code.

Maybe another way to do that, would be to atomically kmap only one page 
at a time. And when drawing the panic screen, make sure that for each 
pixel the right page is mapped.
Would kmap_local_page() fit for this purpose?

Best regards,

-- 

Jocelyn


> 
> Best regards,
> Ryosuke
> 

[PATCH drm-next 2/2] drm/virtio: Use atomic_vmap to work drm_panic in GUI

[Ryosuke Yasuoka]

virtio drm_panic supports only vmapped shmem BO because there is no
atomic vmap feature. Now atomic_vmap is supported, so drm_panic tries to
vmap addr if it is not mapped.

Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
---
 drivers/gpu/drm/drm_gem.c              | 51 ++++++++++++++++++++++++++
 drivers/gpu/drm/drm_gem_shmem_helper.c | 51 ++++++++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 14 +++++--
 include/drm/drm_gem.h                  |  1 +
 include/drm/drm_gem_shmem_helper.h     |  2 +
 5 files changed, 116 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ee811764c3df..eebfaef3a52e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -535,6 +535,57 @@ static void drm_gem_check_release_batch(struct folio_batch *fbatch)
 	cond_resched();
 }
 
+struct page **drm_gem_atomic_get_pages(struct drm_gem_object *obj)
+{
+	struct address_space *mapping;
+	struct page **pages;
+	struct folio *folio;
+	long i, j, npages;
+
+	if (WARN_ON(!obj->filp))
+		return ERR_PTR(-EINVAL);
+
+	/* This is the shared memory object that backs the GEM resource */
+	mapping = obj->filp->f_mapping;
+
+	/* We already BUG_ON() for non-page-aligned sizes in
+	 * drm_gem_object_init(), so we should never hit this unless
+	 * driver author is doing something really wrong:
+	 */
+	WARN_ON((obj->size & (PAGE_SIZE - 1)) != 0);
+
+	npages = obj->size >> PAGE_SHIFT;
+
+	pages = kmalloc_array(npages, sizeof(struct page *), GFP_ATOMIC);
+	if (pages == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	mapping_set_unevictable(mapping);
+
+	i = 0;
+	while (i < npages) {
+		long nr;
+
+		folio = shmem_read_folio_gfp(mapping, i,
+				GFP_ATOMIC);
+		if (IS_ERR(folio))
+			return ERR_PTR(-ENOMEM);
+		nr = min(npages - i, folio_nr_pages(folio));
+		for (j = 0; j < nr; j++, i++)
+			pages[i] = folio_file_page(folio, i);
+
+		/* Make sure shmem keeps __GFP_DMA32 allocated pages in the
+		 * correct region during swapin. Note that this requires
+		 * __GFP_DMA32 to be set in mapping_gfp_mask(inode->i_mapping)
+		 * so shmem can relocate pages during swapin if required.
+		 */
+		BUG_ON(mapping_gfp_constraint(mapping, __GFP_DMA32) &&
+				(folio_pfn(folio) >= 0x00100000UL));
+	}
+
+	return pages;
+}
+
 /**
  * drm_gem_get_pages - helper to allocate backing pages for a GEM object
  * from shmem
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 5ab351409312..789dfd726a36 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -186,6 +186,34 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
 
+static int drm_gem_shmem_atomic_get_pages(struct drm_gem_shmem_object *shmem)
+{
+	struct drm_gem_object *obj = &shmem->base;
+	struct page **pages;
+
+	pages = drm_gem_atomic_get_pages(obj);
+	if (IS_ERR(pages)) {
+		drm_dbg_kms(obj->dev, "Failed to get pages (%ld)\n",
+			    PTR_ERR(pages));
+		shmem->pages_use_count = 0;
+		return PTR_ERR(pages);
+	}
+
+	/*
+	 * TODO: Allocating WC pages which are correctly flushed is only
+	 * supported on x86. Ideal solution would be a GFP_WC flag, which also
+	 * ttm_pool.c could use.
+	 */
+#ifdef CONFIG_X86
+	if (shmem->map_wc)
+		set_pages_array_wc(pages, obj->size >> PAGE_SHIFT);
+#endif
+
+	shmem->pages = pages;
+
+	return 0;
+}
+
 static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
@@ -317,6 +345,29 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem)
 }
 EXPORT_SYMBOL(drm_gem_shmem_unpin);
 
+int drm_gem_shmem_atomic_vmap(struct drm_gem_shmem_object *shmem,
+			      struct iosys_map *map)
+{
+	struct drm_gem_object *obj = &shmem->base;
+	int ret = 0;
+
+	pgprot_t prot = PAGE_KERNEL;
+
+	ret = drm_gem_shmem_atomic_get_pages(shmem);
+	if (ret)
+		return -ENOMEM;
+
+	if (shmem->map_wc)
+		prot = pgprot_writecombine(prot);
+	shmem->vaddr = atomic_vmap(shmem->pages, obj->size >> PAGE_SHIFT,
+				   VM_MAP, prot);
+	if (!shmem->vaddr)
+		return -ENOMEM;
+	iosys_map_set_vaddr(map, shmem->vaddr);
+
+	return 0;
+}
+
 /*
  * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object
  * @shmem: shmem GEM object
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a6f5a78f436a..2a977c5cf42a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -500,11 +500,19 @@ static int virtio_drm_get_scanout_buffer(struct drm_plane *plane,
 
 	bo = gem_to_virtio_gpu_obj(plane->state->fb->obj[0]);
 
-	/* Only support mapped shmem bo */
-	if (virtio_gpu_is_vram(bo) || bo->base.base.import_attach || !bo->base.vaddr)
+	if (virtio_gpu_is_vram(bo) || bo->base.base.import_attach)
 		return -ENODEV;
 
-	iosys_map_set_vaddr(&sb->map[0], bo->base.vaddr);
+	/* try to vmap it if possible */
+	if (!bo->base.vaddr) {
+		int ret;
+
+		ret = drm_gem_shmem_atomic_vmap(&bo->base, &sb->map[0]);
+		if (ret)
+			return ret;
+	} else {
+		iosys_map_set_vaddr(&sb->map[0], bo->base.vaddr);
+	}
 
 	sb->format = plane->state->fb->format;
 	sb->height = plane->state->fb->height;
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index fdae947682cd..cfed66bc12ef 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -529,6 +529,7 @@ void drm_gem_free_mmap_offset(struct drm_gem_object *obj);
 int drm_gem_create_mmap_offset(struct drm_gem_object *obj);
 int drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size);
 
+struct page **drm_gem_atomic_get_pages(struct drm_gem_object *obj);
 struct page **drm_gem_get_pages(struct drm_gem_object *obj);
 void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
 		bool dirty, bool accessed);
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index d22e3fb53631..86a357945f42 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -105,6 +105,8 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem);
 void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
 int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);
 void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem);
+int drm_gem_shmem_atomic_vmap(struct drm_gem_shmem_object *shmem,
+			      struct iosys_map *map);
 int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 		       struct iosys_map *map);
 void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
-- 
2.48.1

Re: [PATCH drm-next 2/2] drm/virtio: Use atomic_vmap to work drm_panic in GUI

[kernel test robot]

Hi Ryosuke,

kernel test robot noticed the following build errors:

[auto build test ERROR on e21cba704714c301d04c5fd37a693734b623872a]

url:    https://github.com/intel-lab-lkp/linux/commits/Ryosuke-Yasuoka/vmalloc-Add-atomic_vmap/20250305-232918
base:   e21cba704714c301d04c5fd37a693734b623872a
patch link:    https://lore.kernel.org/r/20250305152555.318159-3-ryasuoka%40redhat.com
patch subject: [PATCH drm-next 2/2] drm/virtio: Use atomic_vmap to work drm_panic in GUI
config: i386-buildonly-randconfig-001-20250306 (https://download.01.org/0day-ci/archive/20250307/202503070700.ePiUr1E6-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250307/202503070700.ePiUr1E6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503070700.ePiUr1E6-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-mgr-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-bridge-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-region-test.o
>> ERROR: modpost: "drm_gem_atomic_get_pages" [drivers/gpu/drm/drm_shmem_helper.ko] undefined!
>> ERROR: modpost: "atomic_vmap" [drivers/gpu/drm/drm_shmem_helper.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH drm-next 2/2] drm/virtio: Use atomic_vmap to work drm_panic in GUI

[kernel test robot]

Hi Ryosuke,

kernel test robot noticed the following build errors:

[auto build test ERROR on e21cba704714c301d04c5fd37a693734b623872a]

url:    https://github.com/intel-lab-lkp/linux/commits/Ryosuke-Yasuoka/vmalloc-Add-atomic_vmap/20250305-232918
base:   e21cba704714c301d04c5fd37a693734b623872a
patch link:    https://lore.kernel.org/r/20250305152555.318159-3-ryasuoka%40redhat.com
patch subject: [PATCH drm-next 2/2] drm/virtio: Use atomic_vmap to work drm_panic in GUI
config: i386-buildonly-randconfig-003-20250306 (https://download.01.org/0day-ci/archive/20250307/202503071022.q1pg7suf-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250307/202503071022.q1pg7suf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503071022.q1pg7suf-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-mgr-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-bridge-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-region-test.o
ERROR: modpost: "drm_gem_atomic_get_pages" [drivers/gpu/drm/drm_shmem_helper.ko] undefined!
ERROR: modpost: "atomic_vmap" [drivers/gpu/drm/drm_shmem_helper.ko] undefined!
>> ERROR: modpost: "drm_gem_shmem_atomic_vmap" [drivers/gpu/drm/virtio/virtio-gpu.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki