Re: [PATCH v6 4/8] drm/i915/gem: Add i915_gem_object_panic_map()

[Jocelyn Falempe]

On 01/04/2025 19:47, Ville Syrjälä wrote:
On Tue, Apr 01, 2025 at 02:51:10PM +0200, Jocelyn Falempe wrote:
Prepare the work for drm_panic support. This is used to map the
current framebuffer, so the CPU can overwrite it with the panic
message.
Signed-off-by: Jocelyn Falempe <jfale...@redhat.com>
---

v5:
  * Use iosys_map for intel_bo_panic_map().

  drivers/gpu/drm/i915/display/intel_bo.c    |  5 ++++
  drivers/gpu/drm/i915/display/intel_bo.h    |  1 +
  drivers/gpu/drm/i915/gem/i915_gem_object.h |  2 ++
  drivers/gpu/drm/i915/gem/i915_gem_pages.c  | 29 ++++++++++++++++++++++
  drivers/gpu/drm/xe/display/intel_bo.c      | 10 ++++++++
  5 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_bo.c 
b/drivers/gpu/drm/i915/display/intel_bo.c
index fbd16d7b58d9..ac904e9ec7d5 100644
--- a/drivers/gpu/drm/i915/display/intel_bo.c
+++ b/drivers/gpu/drm/i915/display/intel_bo.c
@@ -57,3 +57,8 @@ void intel_bo_describe(struct seq_file *m, struct 
drm_gem_object *obj)
  {
        i915_debugfs_describe_obj(m, to_intel_bo(obj));
  }
+
+void intel_bo_panic_map(struct drm_gem_object *obj, struct iosys_map *map)
+{
+       i915_gem_object_panic_map(to_intel_bo(obj), map);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_bo.h 
b/drivers/gpu/drm/i915/display/intel_bo.h
index ea7a2253aaa5..5b6c63d99786 100644
--- a/drivers/gpu/drm/i915/display/intel_bo.h
+++ b/drivers/gpu/drm/i915/display/intel_bo.h
@@ -23,5 +23,6 @@ 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);
+void intel_bo_panic_map(struct drm_gem_object *obj, struct iosys_map *map);
  
  #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 a5f34542135c..b16092707ea5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -692,6 +692,8 @@ 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);
  
+void i915_gem_object_panic_map(struct drm_i915_gem_object *obj, struct iosys_map *map);
+
  /**
   * 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 8780aa243105..718bea6474d7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -355,6 +355,35 @@ static void *i915_gem_object_map_pfn(struct 
drm_i915_gem_object *obj,
        return vaddr ?: ERR_PTR(-ENOMEM);
  }
  
+/* Map the current framebuffer for CPU access. Called from panic handler, so no
+ * need to pin or cleanup.
+ */
+void i915_gem_object_panic_map(struct drm_i915_gem_object *obj, struct 
iosys_map *map)
+{
+       enum i915_map_type has_type;
+       void *ptr;
+
+       ptr = page_unpack_bits(obj->mm.mapping, &has_type);
+
+
+       if (!ptr) {
+               if (i915_gem_object_has_struct_page(obj))
+                       ptr = i915_gem_object_map_page(obj, I915_MAP_WB);
+               else
+                       ptr = i915_gem_object_map_pfn(obj, I915_MAP_WB);

WB mapping would require clflushing to make it to the display.
Is that being done somewhere?

Yes, it's done in intel_panic_flush() in patch 5, otherwise the panic 
screen is not displayed.


+
+               if (IS_ERR(ptr))
+                       return;

What happens when the mapping fails?

In intel_get_scanout_buffer(), the iosys_map is cleared before calling 
this function. Then it checks iosys_map_is_null(), and returns an error 
if it is.
I can add a comment, or I can change the function type to return an int, 
 that would probably be cleaner.


+
+               obj->mm.mapping = page_pack_bits(ptr, I915_MAP_WB);
+       }
+
+       if (i915_gem_object_has_iomem(obj))
+               iosys_map_set_vaddr_iomem(map, (void __iomem *) ptr);
+       else
+               iosys_map_set_vaddr(map, ptr);
+}
+
  /* 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..c68166a64336 100644
--- a/drivers/gpu/drm/xe/display/intel_bo.c
+++ b/drivers/gpu/drm/xe/display/intel_bo.c
@@ -59,3 +59,13 @@ void intel_bo_describe(struct seq_file *m, struct 
drm_gem_object *obj)
  {
        /* FIXME */
  }
+
+void intel_bo_panic_map(struct drm_gem_object *obj, struct iosys_map *map)
+{
+       struct xe_bo *bo = gem_to_xe_bo(obj);
+       int ret;
+
+       ret = ttm_bo_vmap(&bo->ttm, map);
+       if (ret)
+               iosys_map_clear(map);
+}
--
2.49.0