Re: [PATCH v8 5/9] drm/i915: Add intel_bo_panic_setup and intel_bo_panic_finish

[Jocelyn Falempe <jfalempe@redhat.com>]

On 06/06/2025 15:24, Jani Nikula wrote:
> On Fri, 06 Jun 2025, Jocelyn Falempe <jfalempe@redhat.com> wrote:
>> Implement both functions for i915 and xe, they prepare the work for
>> drm_panic support.
>> They both use kmap_try_from_panic(), and map one page at a time, to
>> write the panic screen on the framebuffer.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>
>>
>> v5:
>>   * Use iosys_map for intel_bo_panic_map().
>>
>> v7:
>>   * Return int for i915_gem_object_panic_map() (Ville Syrjälä)
>>
>> v8:
>>   * Complete rewrite, to use kmap_try_from_panic() which is safe
>>     to call from a panic handler
>>
>>   drivers/gpu/drm/i915/display/intel_bo.c    | 11 +++
>>   drivers/gpu/drm/i915/display/intel_bo.h    |  3 +
>>   drivers/gpu/drm/i915/gem/i915_gem_object.h |  4 +
>>   drivers/gpu/drm/i915/gem/i915_gem_pages.c  | 92 ++++++++++++++++++++++
>>   drivers/gpu/drm/xe/display/intel_bo.c      | 55 +++++++++++++
>>   5 files changed, 165 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_bo.c b/drivers/gpu/drm/i915/display/intel_bo.c
>> index fbd16d7b58d9..83dbd8ae16fe 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bo.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bo.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: MIT
>>   /* Copyright © 2024 Intel Corporation */
>>   
>> +#include <drm/drm_panic.h>
>>   #include "gem/i915_gem_mman.h"
>>   #include "gem/i915_gem_object.h"
>>   #include "gem/i915_gem_object_frontbuffer.h"
>> @@ -57,3 +58,13 @@ void intel_bo_describe(struct seq_file *m, struct drm_gem_object *obj)
>>   {
>>   	i915_debugfs_describe_obj(m, to_intel_bo(obj));
>>   }
>> +
>> +int intel_bo_panic_setup(struct drm_gem_object *obj, struct drm_scanout_buffer *sb)
>> +{
>> +	return i915_gem_object_panic_setup(to_intel_bo(obj), sb);
>> +}
>> +
>> +void intel_bo_panic_finish(struct drm_gem_object *obj)
>> +{
>> +	return i915_gem_object_panic_finish(to_intel_bo(obj));
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_bo.h b/drivers/gpu/drm/i915/display/intel_bo.h
>> index ea7a2253aaa5..9ac087ea275d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bo.h
>> +++ b/drivers/gpu/drm/i915/display/intel_bo.h
>> @@ -4,6 +4,7 @@
>>   #ifndef __INTEL_BO__
>>   #define __INTEL_BO__
>>   
>> +#include <drm/drm_panic.h>
>>   #include <linux/types.h>
>>   
>>   struct drm_gem_object;
>> @@ -23,5 +24,7 @@ struct intel_frontbuffer *intel_bo_set_frontbuffer(struct drm_gem_object *obj,
>>   						   struct intel_frontbuffer *front);
>>   
>>   void intel_bo_describe(struct seq_file *m, struct drm_gem_object *obj);
>> +int intel_bo_panic_setup(struct drm_gem_object *obj, struct drm_scanout_buffer *sb);
>> +void intel_bo_panic_finish(struct drm_gem_object *obj);
>>   
>>   #endif /* __INTEL_BO__ */
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> index c34f41605b46..9a0c1019dcad 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> @@ -9,6 +9,7 @@
>>   #include <drm/drm_gem.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_device.h>
>> +#include <drm/drm_panic.h>
>>   
>>   #include "intel_memory_region.h"
>>   #include "i915_gem_object_types.h"
>> @@ -691,6 +692,9 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>>   int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>>   int i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>>   
>> +int i915_gem_object_panic_setup(struct drm_i915_gem_object *obj, struct drm_scanout_buffer *sb);
>> +void i915_gem_object_panic_finish(struct drm_i915_gem_object *obj);
>> +
>>   /**
>>    * i915_gem_object_pin_map - return a contiguous mapping of the entire object
>>    * @obj: the object to map into kernel address space
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> index 7f83f8bdc8fb..9bdbac3d9433 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> @@ -3,6 +3,7 @@
>>    * Copyright © 2014-2016 Intel Corporation
>>    */
>>   
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_cache.h>
>>   #include <linux/vmalloc.h>
>>   
>> @@ -354,6 +355,97 @@ static void *i915_gem_object_map_pfn(struct drm_i915_gem_object *obj,
>>   	return vaddr ?: ERR_PTR(-ENOMEM);
>>   }
>>   
>> +static struct page **i915_panic_pages;
>> +static int i915_panic_page = -1;
>> +static void *i915_panic_vaddr;
> 
> How do the per module variables work when you have multiple devices?

The panic handler is mono-threaded, and call each plane of each driver 
in turn. So even if those variables are shared by multiple devices, it 
won't be a problem.

i915_gem_object_panic_setup() is called first to init the variables.
then i915_gem_object_panic_page_set_pixel() is called for each pixel
and finally i915_gem_object_panic_finish() to free the resources.

Then i915_gem_object_panic_setup() can be called for another plane or 
another device.

hum, while writing this, I'm probably missing a kfree(i915_panic_pages) 
in i915_gem_object_panic_finish(), I will add that in next version.

Best regards,

-- 

Jocelyn
> 
> BR,
> Jani.
> 
>> +
>> +static void i915_panic_kunmap(void)
>> +{
>> +	if (i915_panic_vaddr) {
>> +		drm_clflush_virt_range(i915_panic_vaddr, PAGE_SIZE);
>> +		kunmap_local(i915_panic_vaddr);
>> +		i915_panic_vaddr = NULL;
>> +	}
>> +}
>> +
>> +static struct page **i915_gem_object_panic_pages(struct drm_i915_gem_object *obj)
>> +{
>> +	unsigned long n_pages = obj->base.size >> PAGE_SHIFT, i;
>> +	struct page *page;
>> +	struct page **pages;
>> +	struct sgt_iter iter;
>> +
>> +	pages = kvmalloc_array(n_pages, sizeof(*pages), GFP_ATOMIC);
>> +	if (!pages)
>> +		return NULL;
>> +
>> +	i = 0;
>> +	for_each_sgt_page(page, iter, obj->mm.pages)
>> +		pages[i++] = page;
>> +	return pages;
>> +}
>> +
>> +/*
>> + * The scanout buffer pages are not mapped, so for each pixel,
>> + * use kmap_local_page_try_from_panic() to map the page, and write the pixel.
>> + * Try to keep the map from the previous pixel, to avoid too much map/unmap.
>> + */
>> +static void i915_gem_object_panic_page_set_pixel(struct drm_scanout_buffer *sb, unsigned int x,
>> +						 unsigned int y, u32 color)
>> +{
>> +	unsigned int new_page;
>> +	unsigned int offset;
>> +
>> +	offset = y * sb->pitch[0] + x * sb->format->cpp[0];
>> +
>> +	new_page = offset >> PAGE_SHIFT;
>> +	offset = offset % PAGE_SIZE;
>> +	if (new_page != i915_panic_page) {
>> +		i915_panic_kunmap();
>> +		i915_panic_page = new_page;
>> +		i915_panic_vaddr = kmap_local_page_try_from_panic(
>> +				   i915_panic_pages[i915_panic_page]);
>> +	}
>> +	if (i915_panic_vaddr) {
>> +		u32 *pix = i915_panic_vaddr + offset;
>> +		*pix = color;
>> +	}
>> +}
>> +
>> +/*
>> + * Setup the gem framebuffer for drm_panic access.
>> + * Use current vaddr if it exists, or setup a list of pages.
>> + * pfn is not supported yet.
>> + */
>> +int i915_gem_object_panic_setup(struct drm_i915_gem_object *obj, struct drm_scanout_buffer *sb)
>> +{
>> +	enum i915_map_type has_type;
>> +	void *ptr;
>> +
>> +	ptr = page_unpack_bits(obj->mm.mapping, &has_type);
>> +	if (ptr) {
>> +		if (i915_gem_object_has_iomem(obj))
>> +			iosys_map_set_vaddr_iomem(&sb->map[0], (void __iomem *)ptr);
>> +		else
>> +			iosys_map_set_vaddr(&sb->map[0], ptr);
>> +
>> +		return 0;
>> +	}
>> +	if (i915_gem_object_has_struct_page(obj)) {
>> +		i915_panic_pages = i915_gem_object_panic_pages(obj);
>> +		sb->set_pixel = i915_gem_object_panic_page_set_pixel;
>> +		i915_panic_page = -1;
>> +		return 0;
>> +	}
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +void i915_gem_object_panic_finish(struct drm_i915_gem_object *obj)
>> +{
>> +	i915_panic_kunmap();
>> +	i915_panic_page = -1;
>> +}
>> +
>>   /* get, pin, and map the pages of the object into kernel space */
>>   void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>>   			      enum i915_map_type type)
>> diff --git a/drivers/gpu/drm/xe/display/intel_bo.c b/drivers/gpu/drm/xe/display/intel_bo.c
>> index 27437c22bd70..eb9a3400c110 100644
>> --- a/drivers/gpu/drm/xe/display/intel_bo.c
>> +++ b/drivers/gpu/drm/xe/display/intel_bo.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: MIT
>>   /* Copyright © 2024 Intel Corporation */
>>   
>> +#include <drm/drm_cache.h>
>>   #include <drm/drm_gem.h>
>>   
>>   #include "xe_bo.h"
>> @@ -59,3 +60,57 @@ void intel_bo_describe(struct seq_file *m, struct drm_gem_object *obj)
>>   {
>>   	/* FIXME */
>>   }
>> +
>> +static int xe_panic_page = -1;
>> +static void *xe_panic_vaddr;
>> +static struct xe_bo *xe_panic_bo;
>> +
>> +static void xe_panic_kunmap(void)
>> +{
>> +	if (xe_panic_vaddr) {
>> +		drm_clflush_virt_range(xe_panic_vaddr, PAGE_SIZE);
>> +		kunmap_local(xe_panic_vaddr);
>> +		xe_panic_vaddr = NULL;
>> +	}
>> +}
>> +/*
>> + * The scanout buffer pages are not mapped, so for each pixel,
>> + * use kmap_local_page_try_from_panic() to map the page, and write the pixel.
>> + * Try to keep the map from the previous pixel, to avoid too much map/unmap.
>> + */
>> +static void xe_panic_page_set_pixel(struct drm_scanout_buffer *sb, unsigned int x,
>> +				    unsigned int y, u32 color)
>> +{
>> +	unsigned int new_page;
>> +	unsigned int offset;
>> +
>> +	offset = y * sb->pitch[0] + x * sb->format->cpp[0];
>> +
>> +	new_page = offset >> PAGE_SHIFT;
>> +	offset = offset % PAGE_SIZE;
>> +	if (new_page != xe_panic_page) {
>> +		xe_panic_kunmap();
>> +		xe_panic_page = new_page;
>> +		xe_panic_vaddr = ttm_bo_kmap_try_from_panic(&xe_panic_bo->ttm,
>> +							    xe_panic_page);
>> +	}
>> +	if (xe_panic_vaddr) {
>> +		u32 *pix = xe_panic_vaddr + offset;
>> +		*pix = color;
>> +	}
>> +}
>> +
>> +int intel_bo_panic_setup(struct drm_gem_object *obj, struct drm_scanout_buffer *sb)
>> +{
>> +	struct xe_bo *bo = gem_to_xe_bo(obj);
>> +
>> +	xe_panic_bo = bo;
>> +	sb->set_pixel = xe_panic_page_set_pixel;
>> +	return 0;
>> +}
>> +
>> +void intel_bo_panic_finish(struct drm_gem_object *obj)
>> +{
>> +	xe_panic_kunmap();
>> +	xe_panic_page = -1;
>> +}
>