Add the new helpers drm_gem_fb_vmap() and drm_gem_fb_vunmap(), which provide vmap/vunmap for all BOs of a framebuffer. Convert shadow- plane helpers, gud and vkms.
Callers of GEM vmap and vunmap functions used to do the minimum work or get some detail wrong. Therefore shadow-plane helpers were intro- duced to implement the details for all callers. The vmapping code in the shadow-plane helpers is also useful for gud and vkms. So it makes sense to provide rsp helpers. Simply call drm_gem_fb_vmap() to retrieve mappings of all of a framebuffer's BOs.
Future work: besides the mapping's addresses, drm_gem_fb_vmap() should also return the mappings with the framebuffer data offset added. These are the addresses were the actual image data is located. A follow-up set of patches will implement this feature.
v3: * free instances of struct vkms_writeback_job on cleanup or errors v2: * update commit message for first patch (Maxime) * fix error handling after DRM_FORMAT_MAX_PLANES changes (kernel test robot) * fix includes (kernel test robot) * use [static N] notations for array parameters
Thomas Zimmermann (5): drm: Define DRM_FORMAT_MAX_PLANES drm/gem: Provide drm_gem_fb_{vmap,vunmap}() drm/gem: Clear mapping addresses for unused framebuffer planes drm/gud: Map framebuffer BOs with drm_gem_fb_vmap() drm/vkms: Map output framebuffer BOs with drm_gem_fb_vmap()
drivers/gpu/drm/drm_gem_atomic_helper.c | 37 +------- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 96 ++++++++++++++++++-- drivers/gpu/drm/gud/gud_pipe.c | 10 +- drivers/gpu/drm/vkms/vkms_composer.c | 2 +- drivers/gpu/drm/vkms/vkms_drv.h | 6 +- drivers/gpu/drm/vkms/vkms_writeback.c | 28 +++--- include/drm/drm_fourcc.h | 13 ++- include/drm/drm_framebuffer.h | 8 +- include/drm/drm_gem_atomic_helper.h | 3 +- include/drm/drm_gem_framebuffer_helper.h | 6 ++ 10 files changed, 139 insertions(+), 70 deletions(-)
base-commit: 2bda1ca4d4acb4892556fec3a7ea1f02afcd40bb prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24 -- 2.32.0
DRM uses a magic number of 4 for the maximum number of planes per color format. Declare this constant via DRM_FORMAT_MAX_PLANES and update the related code. Some code depends on the length of arrays that are now declared with DRM_FORMAT_MAX_PLANES. Convert it from '4' to ARRAY_SIZE.
v2: * mention usage of ARRAY_SIZE() in the commit message (Maxime) * also fix error handling in drm_gem_fb_init_with_funcs() (kernel test robot) * include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 19 +++++++++++-------- include/drm/drm_fourcc.h | 13 +++++++++---- include/drm/drm_framebuffer.h | 8 ++++---- include/drm/drm_gem_atomic_helper.h | 3 ++- 4 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 67bc9edc1d98..421e029a6b3e 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -48,7 +48,7 @@ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, unsigned int plane) { - if (plane >= 4) + if (plane >= ARRAY_SIZE(fb->obj)) return NULL;
return fb->obj[plane]; @@ -62,7 +62,8 @@ drm_gem_fb_init(struct drm_device *dev, struct drm_gem_object **obj, unsigned int num_planes, const struct drm_framebuffer_funcs *funcs) { - int ret, i; + unsigned int i; + int ret;
drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
@@ -86,9 +87,9 @@ drm_gem_fb_init(struct drm_device *dev, */ void drm_gem_fb_destroy(struct drm_framebuffer *fb) { - int i; + size_t i;
- for (i = 0; i < 4; i++) + for (i = 0; i < ARRAY_SIZE(fb->obj); i++) drm_gem_object_put(fb->obj[i]);
drm_framebuffer_cleanup(fb); @@ -145,8 +146,9 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, const struct drm_framebuffer_funcs *funcs) { const struct drm_format_info *info; - struct drm_gem_object *objs[4]; - int ret, i; + struct drm_gem_object *objs[DRM_FORMAT_MAX_PLANES]; + unsigned int i; + int ret;
info = drm_get_format_info(dev, mode_cmd); if (!info) { @@ -187,9 +189,10 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, return 0;
err_gem_object_put: - for (i--; i >= 0; i--) + while (i > 0) { + --i; drm_gem_object_put(objs[i]); - + } return ret; } EXPORT_SYMBOL_GPL(drm_gem_fb_init_with_funcs); diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 3b138d4ae67e..22aa64d07c79 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -25,6 +25,11 @@ #include <linux/types.h> #include <uapi/drm/drm_fourcc.h>
+/** + * DRM_FORMAT_MAX_PLANES - maximum number of planes a DRM format can have + */ +#define DRM_FORMAT_MAX_PLANES 4u + /* * DRM formats are little endian. Define host endian variants for the * most common formats here, to reduce the #ifdefs needed in drivers. @@ -78,7 +83,7 @@ struct drm_format_info { * triplet @char_per_block, @block_w, @block_h for better * describing the pixel format. */ - u8 cpp[4]; + u8 cpp[DRM_FORMAT_MAX_PLANES];
/** * @char_per_block: @@ -104,7 +109,7 @@ struct drm_format_info { * information from their drm_mode_config.get_format_info hook * if they want the core to be validating the pitch. */ - u8 char_per_block[4]; + u8 char_per_block[DRM_FORMAT_MAX_PLANES]; };
/** @@ -113,7 +118,7 @@ struct drm_format_info { * Block width in pixels, this is intended to be accessed through * drm_format_info_block_width() */ - u8 block_w[4]; + u8 block_w[DRM_FORMAT_MAX_PLANES];
/** * @block_h: @@ -121,7 +126,7 @@ struct drm_format_info { * Block height in pixels, this is intended to be accessed through * drm_format_info_block_height() */ - u8 block_h[4]; + u8 block_h[DRM_FORMAT_MAX_PLANES];
/** @hsub: Horizontal chroma subsampling factor */ u8 hsub; diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index be658ebbec72..f67c5b7bcb68 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -27,12 +27,12 @@ #include <linux/list.h> #include <linux/sched.h>
+#include <drm/drm_fourcc.h> #include <drm/drm_mode_object.h>
struct drm_clip_rect; struct drm_device; struct drm_file; -struct drm_format_info; struct drm_framebuffer; struct drm_gem_object;
@@ -147,7 +147,7 @@ struct drm_framebuffer { * @pitches: Line stride per buffer. For userspace created object this * is copied from drm_mode_fb_cmd2. */ - unsigned int pitches[4]; + unsigned int pitches[DRM_FORMAT_MAX_PLANES]; /** * @offsets: Offset from buffer start to the actual pixel data in bytes, * per buffer. For userspace created object this is copied from @@ -165,7 +165,7 @@ struct drm_framebuffer { * data (even for linear buffers). Specifying an x/y pixel offset is * instead done through the source rectangle in &struct drm_plane_state. */ - unsigned int offsets[4]; + unsigned int offsets[DRM_FORMAT_MAX_PLANES]; /** * @modifier: Data layout modifier. This is used to describe * tiling, or also special layouts (like compression) of auxiliary @@ -210,7 +210,7 @@ struct drm_framebuffer { * This is used by the GEM framebuffer helpers, see e.g. * drm_gem_fb_create(). */ - struct drm_gem_object *obj[4]; + struct drm_gem_object *obj[DRM_FORMAT_MAX_PLANES]; };
#define obj_to_fb(x) container_of(x, struct drm_framebuffer, base) diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h index d82c23622156..f9f8b6f0494a 100644 --- a/include/drm/drm_gem_atomic_helper.h +++ b/include/drm/drm_gem_atomic_helper.h @@ -5,6 +5,7 @@
#include <linux/dma-buf-map.h>
+#include <drm/drm_fourcc.h> #include <drm/drm_plane.h>
struct drm_simple_display_pipe; @@ -40,7 +41,7 @@ struct drm_shadow_plane_state { * The memory mappings stored in map should be established in the plane's * prepare_fb callback and removed in the cleanup_fb callback. */ - struct dma_buf_map map[4]; + struct dma_buf_map map[DRM_FORMAT_MAX_PLANES]; };
/**
Move framebuffer vmap code from shadow-buffered plane state into the new interfaces drm_gem_fb_vmap() and drm_gem_fb_vunmap(). These functions provide mappings of a framebuffer's BOs into kernel address space. No functional changes.
v2: * using [static N] for array parameters enables compile-time checks * include <drm/drm_fourcc.h> for DRM_FORMAT_MAX_PLANES (kernel test robot)
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/drm_gem_atomic_helper.c | 37 +--------- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 73 ++++++++++++++++++++ include/drm/drm_gem_framebuffer_helper.h | 6 ++ 3 files changed, 82 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index 26af09b959d4..b1cc19e47165 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -330,10 +330,7 @@ int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *p { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; - struct drm_gem_object *obj; - struct dma_buf_map map; int ret; - size_t i;
if (!fb) return 0; @@ -342,27 +339,7 @@ int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *p if (ret) return ret;
- for (i = 0; i < ARRAY_SIZE(shadow_plane_state->map); ++i) { - obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; - ret = drm_gem_vmap(obj, &map); - if (ret) - goto err_drm_gem_vunmap; - shadow_plane_state->map[i] = map; - } - - return 0; - -err_drm_gem_vunmap: - while (i) { - --i; - obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; - drm_gem_vunmap(obj, &shadow_plane_state->map[i]); - } - return ret; + return drm_gem_fb_vmap(fb, shadow_plane_state->map); } EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
@@ -374,25 +351,17 @@ EXPORT_SYMBOL(drm_gem_prepare_shadow_fb); * This function implements struct &drm_plane_helper_funcs.cleanup_fb. * This function unmaps all buffer objects of the plane's framebuffer. * - * See drm_gem_prepare_shadow_fb() for more inforamtion. + * See drm_gem_prepare_shadow_fb() for more information. */ void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state) { struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state); struct drm_framebuffer *fb = plane_state->fb; - size_t i = ARRAY_SIZE(shadow_plane_state->map); - struct drm_gem_object *obj;
if (!fb) return;
- while (i) { - --i; - obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; - drm_gem_vunmap(obj, &shadow_plane_state->map[i]); - } + drm_gem_fb_vunmap(fb, shadow_plane_state->map); } EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 421e029a6b3e..243affbad437 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -15,6 +15,8 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_modeset_helper.h>
+#include "drm_internal.h" + #define AFBC_HEADER_SIZE 16 #define AFBC_TH_LAYOUT_ALIGNMENT 8 #define AFBC_HDR_ALIGN 64 @@ -309,6 +311,77 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, } EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
+ +/** + * drm_gem_fb_vmap - maps all framebuffer BOs into kernel address space + * @fb: the framebuffer + * @map: returns the mapping's address for each BO + * + * This function maps all buffer objects of the given framebuffer into + * kernel address space and stores them in struct dma_buf_map. If the + * mapping operation fails for one of the BOs, the function unmaps the + * already established mappings automatically. + * + * See drm_gem_fb_vunmap() for unmapping. + * + * Returns: + * 0 on success, or a negative errno code otherwise. + */ +int drm_gem_fb_vmap(struct drm_framebuffer *fb, + struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES]) +{ + struct drm_gem_object *obj; + unsigned int i; + int ret; + + for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { + obj = drm_gem_fb_get_obj(fb, i); + if (!obj) + continue; + ret = drm_gem_vmap(obj, &map[i]); + if (ret) + goto err_drm_gem_vunmap; + } + + return 0; + +err_drm_gem_vunmap: + while (i) { + --i; + obj = drm_gem_fb_get_obj(fb, i); + if (!obj) + continue; + drm_gem_vunmap(obj, &map[i]); + } + return ret; +} +EXPORT_SYMBOL(drm_gem_fb_vmap); + +/** + * drm_gem_fb_vunmap - unmaps framebuffer BOs from kernel address space + * @fb: the framebuffer + * @map: mapping addresses as returned by drm_gem_fb_vmap() + * + * This function unmaps all buffer objects of the given framebuffer. + * + * See drm_gem_fb_vmap() for more information. + */ +void drm_gem_fb_vunmap(struct drm_framebuffer *fb, + struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES]) +{ + unsigned int i = DRM_FORMAT_MAX_PLANES; + struct drm_gem_object *obj; + + while (i) { + --i; + obj = drm_gem_fb_get_obj(fb, i); + if (!obj) + continue; + drm_gem_vunmap(obj, &map[i]); + } +} +EXPORT_SYMBOL(drm_gem_fb_vunmap); + /** * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access * @fb: the framebuffer diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h index 5705722f0855..ff2024dd7b77 100644 --- a/include/drm/drm_gem_framebuffer_helper.h +++ b/include/drm/drm_gem_framebuffer_helper.h @@ -4,6 +4,8 @@ #include <linux/dma-buf.h> #include <linux/dma-buf-map.h>
+#include <drm/drm_fourcc.h> + struct drm_afbc_framebuffer; struct drm_device; struct drm_fb_helper_surface_size; @@ -37,6 +39,10 @@ struct drm_framebuffer * drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd);
+int drm_gem_fb_vmap(struct drm_framebuffer *fb, + struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES]); +void drm_gem_fb_vunmap(struct drm_framebuffer *fb, + struct dma_buf_map map[static DRM_FORMAT_MAX_PLANES]); int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir); void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
Set the returned mapping address to NULL if a framebuffer plane does not have a BO associated with it. Likewise, ignore mappings of NULL during framebuffer unmap operations. Allows users of the functions to perform unmap operations of certain BOs by themselfes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 243affbad437..02928607a716 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -336,8 +336,10 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb,
for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { obj = drm_gem_fb_get_obj(fb, i); - if (!obj) + if (!obj) { + dma_buf_map_clear(&map[i]); continue; + } ret = drm_gem_vmap(obj, &map[i]); if (ret) goto err_drm_gem_vunmap; @@ -377,6 +379,8 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, obj = drm_gem_fb_get_obj(fb, i); if (!obj) continue; + if (dma_buf_map_is_null(&map[i])) + continue; drm_gem_vunmap(obj, &map[i]); } }
Abstract the framebuffer details by mapping its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().
The call to drm_gem_fb_vmap() ensures that all BOs are mapped correctly. Gud still only supports single-plane formats.
No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Noralf Trønnes noralf@tronnes.org Acked-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/gud/gud_pipe.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 4d7a26b68a2e..7e009f562b30 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -14,8 +14,8 @@ #include <drm/drm_format_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> #include <drm/drm_simple_kms_helper.h> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, { struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; u8 compression = gdrm->compression; - struct dma_buf_map map; + struct dma_buf_map map[DRM_FORMAT_MAX_PLANES]; void *vaddr, *buf; size_t pitch, len; int ret = 0; @@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, if (len > gdrm->bulk_len) return -E2BIG;
- ret = drm_gem_shmem_vmap(fb->obj[0], &map); + ret = drm_gem_fb_vmap(fb, map); if (ret) return ret;
- vaddr = map.vaddr + fb->offsets[0]; + vaddr = map[0].vaddr + fb->offsets[0];
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret) @@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, end_cpu_access: drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); vunmap: - drm_gem_shmem_vunmap(fb->obj[0], &map); + drm_gem_fb_vunmap(fb, map);
return ret; }
Hi Thomas,
I'm getting this on Ubuntu 22.04:
[ 0.000000] Linux version 5.15.0-27-generic (buildd@ubuntu) (gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #28-Ubuntu SMP Thu Apr 14 04:55:28 UTC 2022 (Ubuntu 5.15.0-27.28-generic 5.15.30)
[ 4.830866] usb 2-3.1: new high-speed USB device number 4 using xhci_hcd [ 4.935546] usb 2-3.1: New USB device found, idVendor=1d50, idProduct=614d, bcdDevice= 1.00 [ 4.935553] usb 2-3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 4.935556] usb 2-3.1: Product: Raspberry Pi 4 Display Gadget [ 4.935558] usb 2-3.1: Manufacturer: Raspberry Pi [ 4.935560] usb 2-3.1: SerialNumber: 100000003b40d6c6
[ 7.497361] [drm] Initialized gud 1.0.0 20200422 for 2-3.1:1.0 on minor 0
[ 7.573048] gud 2-3.1:1.0: [drm] fb1: guddrmfb frame buffer device
[ 9.199402] ================================================================================ [ 9.199411] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:224:9 [ 9.199416] load of value 226 is not a valid value for type '_Bool' [ 9.199420] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199424] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199427] Workqueue: events_long gud_flush_work [gud] [ 9.199440] Call Trace: [ 9.199443] <TASK> [ 9.199447] show_stack+0x52/0x58 [ 9.199456] dump_stack_lvl+0x4a/0x5f [ 9.199464] dump_stack+0x10/0x12 [ 9.199468] ubsan_epilogue+0x9/0x45 [ 9.199473] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199478] drm_gem_fb_vmap.cold+0x10/0x3d [drm_kms_helper] [ 9.199519] gud_prep_flush+0xaa/0x410 [gud] [ 9.199525] ? check_preempt_curr+0x5d/0x70 [ 9.199533] ? update_load_avg+0x82/0x620 [ 9.199540] ? set_next_entity+0xb7/0x200 [ 9.199545] gud_flush_work+0x1e0/0x430 [gud] [ 9.199551] ? psi_task_switch+0x1e7/0x220 [ 9.199557] process_one_work+0x22b/0x3d0 [ 9.199564] worker_thread+0x53/0x410 [ 9.199570] ? process_one_work+0x3d0/0x3d0 [ 9.199575] kthread+0x12a/0x150 [ 9.199579] ? set_kthread_struct+0x50/0x50 [ 9.199584] ret_from_fork+0x22/0x30 [ 9.199593] </TASK> [ 9.199595] ================================================================================
[ 9.199598] ================================================================================ [ 9.199600] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:194:9 [ 9.199604] load of value 226 is not a valid value for type '_Bool' [ 9.199606] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199610] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199612] Workqueue: events_long gud_flush_work [gud] [ 9.199618] Call Trace: [ 9.199619] <TASK> [ 9.199621] show_stack+0x52/0x58 [ 9.199627] dump_stack_lvl+0x4a/0x5f [ 9.199633] dump_stack+0x10/0x12 [ 9.199637] ubsan_epilogue+0x9/0x45 [ 9.199641] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199646] drm_gem_fb_vmap.cold+0x24/0x3d [drm_kms_helper] [ 9.199675] gud_prep_flush+0xaa/0x410 [gud] [ 9.199682] ? check_preempt_curr+0x5d/0x70 [ 9.199688] ? update_load_avg+0x82/0x620 [ 9.199693] ? update_load_avg+0x82/0x620 [ 9.199697] gud_flush_work+0x1e0/0x430 [gud] [ 9.199702] ? psi_task_switch+0x1e7/0x220 [ 9.199706] process_one_work+0x22b/0x3d0 [ 9.199713] worker_thread+0x53/0x410 [ 9.199718] ? process_one_work+0x3d0/0x3d0 [ 9.199723] kthread+0x12a/0x150 [ 9.199728] ? set_kthread_struct+0x50/0x50 [ 9.199732] ret_from_fork+0x22/0x30 [ 9.199741] </TASK> [ 9.199743] ================================================================================
It's the "if (map->is_iomem)" statement in dma_buf_map_clear() and dma_buf_map_is_null() that triggers this.
I tried 5.18.0-rc5 and the problem is still present.
UBSAN entries in the config:
CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y CONFIG_UBSAN=y # CONFIG_UBSAN_TRAP is not set CONFIG_CC_HAS_UBSAN_BOUNDS=y CONFIG_UBSAN_BOUNDS=y CONFIG_UBSAN_ONLY_BOUNDS=y CONFIG_UBSAN_SHIFT=y # CONFIG_UBSAN_DIV_ZERO is not set CONFIG_UBSAN_BOOL=y CONFIG_UBSAN_ENUM=y # CONFIG_UBSAN_ALIGNMENT is not set CONFIG_UBSAN_SANITIZE_ALL=y # CONFIG_TEST_UBSAN is not set
Continuing further down.
Den 30.07.2021 20.35, skrev Thomas Zimmermann:
Abstract the framebuffer details by mapping its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().
The call to drm_gem_fb_vmap() ensures that all BOs are mapped correctly. Gud still only supports single-plane formats.
No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Noralf Trønnes noralf@tronnes.org Acked-by: Sam Ravnborg sam@ravnborg.org
drivers/gpu/drm/gud/gud_pipe.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 4d7a26b68a2e..7e009f562b30 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -14,8 +14,8 @@ #include <drm/drm_format_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> #include <drm/drm_simple_kms_helper.h> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, { struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; u8 compression = gdrm->compression;
- struct dma_buf_map map;
- struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
Zeroing map solves the problem:
struct iosys_map map[DRM_FORMAT_MAX_PLANES] = {};
I don't understand the conditional clearing in dma_buf_map_clear/iosys_map_clear(), the doc says: Clears all fields to zero. If I zero the whole structure unconditionally this also keeps UBSAN happy.
Noralf.
void *vaddr, *buf; size_t pitch, len; int ret = 0; @@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, if (len > gdrm->bulk_len) return -E2BIG;
- ret = drm_gem_shmem_vmap(fb->obj[0], &map);
- ret = drm_gem_fb_vmap(fb, map); if (ret) return ret;
- vaddr = map.vaddr + fb->offsets[0];
vaddr = map[0].vaddr + fb->offsets[0];
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret)
@@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, end_cpu_access: drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); vunmap:
- drm_gem_shmem_vunmap(fb->obj[0], &map);
drm_gem_fb_vunmap(fb, map);
return ret;
}
Hi
Am 06.05.22 um 16:01 schrieb Noralf Trønnes:
Hi Thomas,
I'm getting this on Ubuntu 22.04:
[ 0.000000] Linux version 5.15.0-27-generic (buildd@ubuntu) (gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #28-Ubuntu SMP Thu Apr 14 04:55:28 UTC 2022 (Ubuntu 5.15.0-27.28-generic 5.15.30)
[ 4.830866] usb 2-3.1: new high-speed USB device number 4 using xhci_hcd [ 4.935546] usb 2-3.1: New USB device found, idVendor=1d50, idProduct=614d, bcdDevice= 1.00 [ 4.935553] usb 2-3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 4.935556] usb 2-3.1: Product: Raspberry Pi 4 Display Gadget [ 4.935558] usb 2-3.1: Manufacturer: Raspberry Pi [ 4.935560] usb 2-3.1: SerialNumber: 100000003b40d6c6
[ 7.497361] [drm] Initialized gud 1.0.0 20200422 for 2-3.1:1.0 on minor 0
[ 7.573048] gud 2-3.1:1.0: [drm] fb1: guddrmfb frame buffer device
[ 9.199402]
[ 9.199411] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:224:9 [ 9.199416] load of value 226 is not a valid value for type '_Bool' [ 9.199420] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199424] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199427] Workqueue: events_long gud_flush_work [gud] [ 9.199440] Call Trace: [ 9.199443] <TASK> [ 9.199447] show_stack+0x52/0x58 [ 9.199456] dump_stack_lvl+0x4a/0x5f [ 9.199464] dump_stack+0x10/0x12 [ 9.199468] ubsan_epilogue+0x9/0x45 [ 9.199473] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199478] drm_gem_fb_vmap.cold+0x10/0x3d [drm_kms_helper] [ 9.199519] gud_prep_flush+0xaa/0x410 [gud] [ 9.199525] ? check_preempt_curr+0x5d/0x70 [ 9.199533] ? update_load_avg+0x82/0x620 [ 9.199540] ? set_next_entity+0xb7/0x200 [ 9.199545] gud_flush_work+0x1e0/0x430 [gud] [ 9.199551] ? psi_task_switch+0x1e7/0x220 [ 9.199557] process_one_work+0x22b/0x3d0 [ 9.199564] worker_thread+0x53/0x410 [ 9.199570] ? process_one_work+0x3d0/0x3d0 [ 9.199575] kthread+0x12a/0x150 [ 9.199579] ? set_kthread_struct+0x50/0x50 [ 9.199584] ret_from_fork+0x22/0x30 [ 9.199593] </TASK> [ 9.199595] ================================================================================
[ 9.199598]
[ 9.199600] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:194:9 [ 9.199604] load of value 226 is not a valid value for type '_Bool' [ 9.199606] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199610] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199612] Workqueue: events_long gud_flush_work [gud] [ 9.199618] Call Trace: [ 9.199619] <TASK> [ 9.199621] show_stack+0x52/0x58 [ 9.199627] dump_stack_lvl+0x4a/0x5f [ 9.199633] dump_stack+0x10/0x12 [ 9.199637] ubsan_epilogue+0x9/0x45 [ 9.199641] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199646] drm_gem_fb_vmap.cold+0x24/0x3d [drm_kms_helper] [ 9.199675] gud_prep_flush+0xaa/0x410 [gud] [ 9.199682] ? check_preempt_curr+0x5d/0x70 [ 9.199688] ? update_load_avg+0x82/0x620 [ 9.199693] ? update_load_avg+0x82/0x620 [ 9.199697] gud_flush_work+0x1e0/0x430 [gud] [ 9.199702] ? psi_task_switch+0x1e7/0x220 [ 9.199706] process_one_work+0x22b/0x3d0 [ 9.199713] worker_thread+0x53/0x410 [ 9.199718] ? process_one_work+0x3d0/0x3d0 [ 9.199723] kthread+0x12a/0x150 [ 9.199728] ? set_kthread_struct+0x50/0x50 [ 9.199732] ret_from_fork+0x22/0x30 [ 9.199741] </TASK> [ 9.199743] ================================================================================
It's the "if (map->is_iomem)" statement in dma_buf_map_clear() and dma_buf_map_is_null() that triggers this.
I tried 5.18.0-rc5 and the problem is still present.
UBSAN entries in the config:
CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y CONFIG_UBSAN=y # CONFIG_UBSAN_TRAP is not set CONFIG_CC_HAS_UBSAN_BOUNDS=y CONFIG_UBSAN_BOUNDS=y CONFIG_UBSAN_ONLY_BOUNDS=y CONFIG_UBSAN_SHIFT=y # CONFIG_UBSAN_DIV_ZERO is not set CONFIG_UBSAN_BOOL=y CONFIG_UBSAN_ENUM=y # CONFIG_UBSAN_ALIGNMENT is not set CONFIG_UBSAN_SANITIZE_ALL=y # CONFIG_TEST_UBSAN is not set
Continuing further down.
Den 30.07.2021 20.35, skrev Thomas Zimmermann:
Abstract the framebuffer details by mapping its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().
The call to drm_gem_fb_vmap() ensures that all BOs are mapped correctly. Gud still only supports single-plane formats.
No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Noralf Trønnes noralf@tronnes.org Acked-by: Sam Ravnborg sam@ravnborg.org
drivers/gpu/drm/gud/gud_pipe.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 4d7a26b68a2e..7e009f562b30 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -14,8 +14,8 @@ #include <drm/drm_format_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> #include <drm/drm_simple_kms_helper.h> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, { struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; u8 compression = gdrm->compression;
- struct dma_buf_map map;
- struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
Zeroing map solves the problem:
struct iosys_map map[DRM_FORMAT_MAX_PLANES] = {};I don't understand the conditional clearing in dma_buf_map_clear/iosys_map_clear(), the doc says: Clears all fields to zero. If I zero the whole structure unconditionally this also keeps UBSAN happy.
Thanks for debugging this problem. It's uninitialized and some of the internal helpers look at all planes, even if they are empty. I have a patchset to fix that throughout the DRM modules. I'll post on Monday.
If we need a quick fix, we could do the zeroing everywhere.
Best regards Thomas
Noralf.
void *vaddr, *buf; size_t pitch, len; int ret = 0; @@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, if (len > gdrm->bulk_len) return -E2BIG;
- ret = drm_gem_shmem_vmap(fb->obj[0], &map);
- ret = drm_gem_fb_vmap(fb, map); if (ret) return ret;
- vaddr = map.vaddr + fb->offsets[0];
vaddr = map[0].vaddr + fb->offsets[0];
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret)
@@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, end_cpu_access: drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); vunmap:
- drm_gem_shmem_vunmap(fb->obj[0], &map);
drm_gem_fb_vunmap(fb, map);
return ret; }
Hi Noralf
Am 06.05.22 um 16:11 schrieb Thomas Zimmermann:
Hi
Am 06.05.22 um 16:01 schrieb Noralf Trønnes:
Hi Thomas,
I'm getting this on Ubuntu 22.04:
[ 0.000000] Linux version 5.15.0-27-generic (buildd@ubuntu) (gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #28-Ubuntu SMP Thu Apr 14 04:55:28 UTC 2022 (Ubuntu 5.15.0-27.28-generic 5.15.30)
[ 4.830866] usb 2-3.1: new high-speed USB device number 4 using xhci_hcd [ 4.935546] usb 2-3.1: New USB device found, idVendor=1d50, idProduct=614d, bcdDevice= 1.00 [ 4.935553] usb 2-3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 4.935556] usb 2-3.1: Product: Raspberry Pi 4 Display Gadget [ 4.935558] usb 2-3.1: Manufacturer: Raspberry Pi [ 4.935560] usb 2-3.1: SerialNumber: 100000003b40d6c6
[ 7.497361] [drm] Initialized gud 1.0.0 20200422 for 2-3.1:1.0 on minor 0
[ 7.573048] gud 2-3.1:1.0: [drm] fb1: guddrmfb frame buffer device
[ 9.199402]
[ 9.199411] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:224:9 [ 9.199416] load of value 226 is not a valid value for type '_Bool' [ 9.199420] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199424] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199427] Workqueue: events_long gud_flush_work [gud] [ 9.199440] Call Trace: [ 9.199443] <TASK> [ 9.199447] show_stack+0x52/0x58 [ 9.199456] dump_stack_lvl+0x4a/0x5f [ 9.199464] dump_stack+0x10/0x12 [ 9.199468] ubsan_epilogue+0x9/0x45 [ 9.199473] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199478] drm_gem_fb_vmap.cold+0x10/0x3d [drm_kms_helper] [ 9.199519] gud_prep_flush+0xaa/0x410 [gud] [ 9.199525] ? check_preempt_curr+0x5d/0x70 [ 9.199533] ? update_load_avg+0x82/0x620 [ 9.199540] ? set_next_entity+0xb7/0x200 [ 9.199545] gud_flush_work+0x1e0/0x430 [gud] [ 9.199551] ? psi_task_switch+0x1e7/0x220 [ 9.199557] process_one_work+0x22b/0x3d0 [ 9.199564] worker_thread+0x53/0x410 [ 9.199570] ? process_one_work+0x3d0/0x3d0 [ 9.199575] kthread+0x12a/0x150 [ 9.199579] ? set_kthread_struct+0x50/0x50 [ 9.199584] ret_from_fork+0x22/0x30 [ 9.199593] </TASK> [ 9.199595] ================================================================================
[ 9.199598]
[ 9.199600] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:194:9 [ 9.199604] load of value 226 is not a valid value for type '_Bool' [ 9.199606] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199610] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199612] Workqueue: events_long gud_flush_work [gud] [ 9.199618] Call Trace: [ 9.199619] <TASK> [ 9.199621] show_stack+0x52/0x58 [ 9.199627] dump_stack_lvl+0x4a/0x5f [ 9.199633] dump_stack+0x10/0x12 [ 9.199637] ubsan_epilogue+0x9/0x45 [ 9.199641] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199646] drm_gem_fb_vmap.cold+0x24/0x3d [drm_kms_helper] [ 9.199675] gud_prep_flush+0xaa/0x410 [gud] [ 9.199682] ? check_preempt_curr+0x5d/0x70 [ 9.199688] ? update_load_avg+0x82/0x620 [ 9.199693] ? update_load_avg+0x82/0x620 [ 9.199697] gud_flush_work+0x1e0/0x430 [gud] [ 9.199702] ? psi_task_switch+0x1e7/0x220 [ 9.199706] process_one_work+0x22b/0x3d0 [ 9.199713] worker_thread+0x53/0x410 [ 9.199718] ? process_one_work+0x3d0/0x3d0 [ 9.199723] kthread+0x12a/0x150 [ 9.199728] ? set_kthread_struct+0x50/0x50 [ 9.199732] ret_from_fork+0x22/0x30 [ 9.199741] </TASK> [ 9.199743] ================================================================================
It's the "if (map->is_iomem)" statement in dma_buf_map_clear() and dma_buf_map_is_null() that triggers this.
I tried 5.18.0-rc5 and the problem is still present.
UBSAN entries in the config:
CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y CONFIG_UBSAN=y # CONFIG_UBSAN_TRAP is not set CONFIG_CC_HAS_UBSAN_BOUNDS=y CONFIG_UBSAN_BOUNDS=y CONFIG_UBSAN_ONLY_BOUNDS=y CONFIG_UBSAN_SHIFT=y # CONFIG_UBSAN_DIV_ZERO is not set CONFIG_UBSAN_BOOL=y CONFIG_UBSAN_ENUM=y # CONFIG_UBSAN_ALIGNMENT is not set CONFIG_UBSAN_SANITIZE_ALL=y # CONFIG_TEST_UBSAN is not set
Continuing further down.
Den 30.07.2021 20.35, skrev Thomas Zimmermann:
Abstract the framebuffer details by mapping its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().
The call to drm_gem_fb_vmap() ensures that all BOs are mapped correctly. Gud still only supports single-plane formats.
No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Noralf Trønnes noralf@tronnes.org Acked-by: Sam Ravnborg sam@ravnborg.org
drivers/gpu/drm/gud/gud_pipe.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 4d7a26b68a2e..7e009f562b30 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -14,8 +14,8 @@ #include <drm/drm_format_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> #include <drm/drm_simple_kms_helper.h> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, { struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; u8 compression = gdrm->compression; - struct dma_buf_map map; + struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
Zeroing map solves the problem:
struct iosys_map map[DRM_FORMAT_MAX_PLANES] = {};
I don't understand the conditional clearing in dma_buf_map_clear/iosys_map_clear(), the doc says: Clears all fields to zero. If I zero the whole structure unconditionally this also keeps UBSAN happy.
iomap_sys_clear() assumes that the instance is already initialized. Hence, calling it at [1] with un-zeroed, stack-allocated memory operates on undefined state. It doesn't matter for the result, though. I guess the semantics of iosys_sys_clear() are not stellar.
Thanks for debugging this problem. It's uninitialized and some of the internal helpers look at all planes, even if they are empty. I have a patchset to fix that throughout the DRM modules. I'll post on Monday.
I have posted that patchset at [2]. If you have the time, I'd appreciate if you could give it a test run.
Best regards Thomas
[1] https://elixir.bootlin.com/linux/v5.17.5/source/drivers/gpu/drm/drm_gem_fram... [2] https://lore.kernel.org/dri-devel/20220509081602.474-1-tzimmermann@suse.de/T...
If we need a quick fix, we could do the zeroing everywhere.
Best regards Thomas
Noralf.
void *vaddr, *buf; size_t pitch, len; int ret = 0; @@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, if (len > gdrm->bulk_len) return -E2BIG; - ret = drm_gem_shmem_vmap(fb->obj[0], &map); + ret = drm_gem_fb_vmap(fb, map); if (ret) return ret; - vaddr = map.vaddr + fb->offsets[0]; + vaddr = map[0].vaddr + fb->offsets[0]; ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret) @@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, end_cpu_access: drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); vunmap: - drm_gem_shmem_vunmap(fb->obj[0], &map); + drm_gem_fb_vunmap(fb, map); return ret; }
Den 09.05.2022 10.32, skrev Thomas Zimmermann:
Hi Noralf
Am 06.05.22 um 16:11 schrieb Thomas Zimmermann:
Hi
Am 06.05.22 um 16:01 schrieb Noralf Trønnes:
Hi Thomas,
I'm getting this on Ubuntu 22.04:
[ 0.000000] Linux version 5.15.0-27-generic (buildd@ubuntu) (gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0, GNU ld (GNU Binutils for Ubuntu) 2.38) #28-Ubuntu SMP Thu Apr 14 04:55:28 UTC 2022 (Ubuntu 5.15.0-27.28-generic 5.15.30)
[ 4.830866] usb 2-3.1: new high-speed USB device number 4 using xhci_hcd [ 4.935546] usb 2-3.1: New USB device found, idVendor=1d50, idProduct=614d, bcdDevice= 1.00 [ 4.935553] usb 2-3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 4.935556] usb 2-3.1: Product: Raspberry Pi 4 Display Gadget [ 4.935558] usb 2-3.1: Manufacturer: Raspberry Pi [ 4.935560] usb 2-3.1: SerialNumber: 100000003b40d6c6
[ 7.497361] [drm] Initialized gud 1.0.0 20200422 for 2-3.1:1.0 on minor 0
[ 7.573048] gud 2-3.1:1.0: [drm] fb1: guddrmfb frame buffer device
[ 9.199402]
[ 9.199411] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:224:9 [ 9.199416] load of value 226 is not a valid value for type '_Bool' [ 9.199420] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199424] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199427] Workqueue: events_long gud_flush_work [gud] [ 9.199440] Call Trace: [ 9.199443] <TASK> [ 9.199447] show_stack+0x52/0x58 [ 9.199456] dump_stack_lvl+0x4a/0x5f [ 9.199464] dump_stack+0x10/0x12 [ 9.199468] ubsan_epilogue+0x9/0x45 [ 9.199473] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199478] drm_gem_fb_vmap.cold+0x10/0x3d [drm_kms_helper] [ 9.199519] gud_prep_flush+0xaa/0x410 [gud] [ 9.199525] ? check_preempt_curr+0x5d/0x70 [ 9.199533] ? update_load_avg+0x82/0x620 [ 9.199540] ? set_next_entity+0xb7/0x200 [ 9.199545] gud_flush_work+0x1e0/0x430 [gud] [ 9.199551] ? psi_task_switch+0x1e7/0x220 [ 9.199557] process_one_work+0x22b/0x3d0 [ 9.199564] worker_thread+0x53/0x410 [ 9.199570] ? process_one_work+0x3d0/0x3d0 [ 9.199575] kthread+0x12a/0x150 [ 9.199579] ? set_kthread_struct+0x50/0x50 [ 9.199584] ret_from_fork+0x22/0x30 [ 9.199593] </TASK> [ 9.199595] ================================================================================
[ 9.199598]
[ 9.199600] UBSAN: invalid-load in /build/linux-HMZHpV/linux-5.15.0/include/linux/dma-buf-map.h:194:9 [ 9.199604] load of value 226 is not a valid value for type '_Bool' [ 9.199606] CPU: 0 PID: 113 Comm: kworker/0:2 Not tainted 5.15.0-27-generic #28-Ubuntu [ 9.199610] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS L71 Ver. 01.44 04/12/2018 [ 9.199612] Workqueue: events_long gud_flush_work [gud] [ 9.199618] Call Trace: [ 9.199619] <TASK> [ 9.199621] show_stack+0x52/0x58 [ 9.199627] dump_stack_lvl+0x4a/0x5f [ 9.199633] dump_stack+0x10/0x12 [ 9.199637] ubsan_epilogue+0x9/0x45 [ 9.199641] __ubsan_handle_load_invalid_value.cold+0x44/0x49 [ 9.199646] drm_gem_fb_vmap.cold+0x24/0x3d [drm_kms_helper] [ 9.199675] gud_prep_flush+0xaa/0x410 [gud] [ 9.199682] ? check_preempt_curr+0x5d/0x70 [ 9.199688] ? update_load_avg+0x82/0x620 [ 9.199693] ? update_load_avg+0x82/0x620 [ 9.199697] gud_flush_work+0x1e0/0x430 [gud] [ 9.199702] ? psi_task_switch+0x1e7/0x220 [ 9.199706] process_one_work+0x22b/0x3d0 [ 9.199713] worker_thread+0x53/0x410 [ 9.199718] ? process_one_work+0x3d0/0x3d0 [ 9.199723] kthread+0x12a/0x150 [ 9.199728] ? set_kthread_struct+0x50/0x50 [ 9.199732] ret_from_fork+0x22/0x30 [ 9.199741] </TASK> [ 9.199743] ================================================================================
It's the "if (map->is_iomem)" statement in dma_buf_map_clear() and dma_buf_map_is_null() that triggers this.
I tried 5.18.0-rc5 and the problem is still present.
UBSAN entries in the config:
CONFIG_ARCH_HAS_UBSAN_SANITIZE_ALL=y CONFIG_UBSAN=y # CONFIG_UBSAN_TRAP is not set CONFIG_CC_HAS_UBSAN_BOUNDS=y CONFIG_UBSAN_BOUNDS=y CONFIG_UBSAN_ONLY_BOUNDS=y CONFIG_UBSAN_SHIFT=y # CONFIG_UBSAN_DIV_ZERO is not set CONFIG_UBSAN_BOOL=y CONFIG_UBSAN_ENUM=y # CONFIG_UBSAN_ALIGNMENT is not set CONFIG_UBSAN_SANITIZE_ALL=y # CONFIG_TEST_UBSAN is not set
Continuing further down.
Den 30.07.2021 20.35, skrev Thomas Zimmermann:
Abstract the framebuffer details by mapping its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunmap().
The call to drm_gem_fb_vmap() ensures that all BOs are mapped correctly. Gud still only supports single-plane formats.
No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Noralf Trønnes noralf@tronnes.org Acked-by: Sam Ravnborg sam@ravnborg.org
drivers/gpu/drm/gud/gud_pipe.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 4d7a26b68a2e..7e009f562b30 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -14,8 +14,8 @@ #include <drm/drm_format_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_framebuffer.h> +#include <drm/drm_gem.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_print.h> #include <drm/drm_rect.h> #include <drm/drm_simple_kms_helper.h> @@ -152,7 +152,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, { struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach; u8 compression = gdrm->compression; - struct dma_buf_map map; + struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
Zeroing map solves the problem:
struct iosys_map map[DRM_FORMAT_MAX_PLANES] = {};
I don't understand the conditional clearing in dma_buf_map_clear/iosys_map_clear(), the doc says: Clears all fields to zero. If I zero the whole structure unconditionally this also keeps UBSAN happy.
iomap_sys_clear() assumes that the instance is already initialized. Hence, calling it at [1] with un-zeroed, stack-allocated memory operates on undefined state. It doesn't matter for the result, though. I guess the semantics of iosys_sys_clear() are not stellar.
I did a quick look through the struct iosys_map users and found these using a stack allocated variable that has not been initialized:
These call dma_buf_vmap() directly: drm_gem_cma_prime_import_sg_table_vmap igt_dmabuf_export_vmap igt_dmabuf_import_ownership igt_dmabuf_import etnaviv_gem_prime_vmap_impl
Ends up calling dma_buf_vmap() if the bo is imported: panfrost_perfcnt_enable_locked lima_sched_build_error_task_list tegra_bo_mmap mipi_dbi_fb_dirty mipi_dbi_buf_copy gud_prep_flush
Ends up calling iosys_map_is_null() at least: ast_cursor_plane_init
Ends up calling iosys_map_is_equal(): ast_cursor_plane_destroy
Thanks for debugging this problem. It's uninitialized and some of the internal helpers look at all planes, even if they are empty. I have a patchset to fix that throughout the DRM modules. I'll post on Monday.
I have posted that patchset at [2]. If you have the time, I'd appreciate if you could give it a test run.
I'll see if I can do that.
Noralf.
Best regards Thomas
[1] https://elixir.bootlin.com/linux/v5.17.5/source/drivers/gpu/drm/drm_gem_fram...
[2] https://lore.kernel.org/dri-devel/20220509081602.474-1-tzimmermann@suse.de/T...
If we need a quick fix, we could do the zeroing everywhere.
Best regards Thomas
Noralf.
void *vaddr, *buf; size_t pitch, len; int ret = 0; @@ -162,11 +162,11 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, if (len > gdrm->bulk_len) return -E2BIG; - ret = drm_gem_shmem_vmap(fb->obj[0], &map); + ret = drm_gem_fb_vmap(fb, map); if (ret) return ret; - vaddr = map.vaddr + fb->offsets[0]; + vaddr = map[0].vaddr + fb->offsets[0]; ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret) @@ -225,7 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb, end_cpu_access: drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); vunmap: - drm_gem_shmem_vunmap(fb->obj[0], &map); + drm_gem_fb_vunmap(fb, map); return ret; }
Abstract the framebuffer details by mappings its BOs with a call to drm_gem_fb_vmap(). Unmap with drm_gem_fb_vunamp().
Before, the output address with stored as raw pointer in the priv field of struct drm_writeback_job. Introduce the new type struct vkms_writeback_job, which holds the output mappings addresses while the writeback job is active.
The patchset also cleans up some internal casting an setup of the output addresses. No functional changes.
v3: * free instances of struct vkms_writeback_job on cleanup or errors
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Rodrigo Siqueira Rodrigo.Siqueira@amd.com Acked-by: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/vkms/vkms_composer.c | 2 +- drivers/gpu/drm/vkms/vkms_drv.h | 6 +++++- drivers/gpu/drm/vkms/vkms_writeback.c | 28 +++++++++++++++------------ 3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index ead8fff81f30..49f109c3a2b3 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -257,7 +257,7 @@ void vkms_composer_worker(struct work_struct *work) return;
if (wb_pending) - vaddr_out = crtc_state->active_writeback; + vaddr_out = crtc_state->active_writeback->map[0].vaddr;
ret = compose_active_planes(&vaddr_out, primary_composer, crtc_state); diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 8c731b6dcba7..8bc9e3f52e1f 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -20,6 +20,10 @@ #define XRES_MAX 8192 #define YRES_MAX 8192
+struct vkms_writeback_job { + struct dma_buf_map map[DRM_FORMAT_MAX_PLANES]; +}; + struct vkms_composer { struct drm_framebuffer fb; struct drm_rect src, dst; @@ -57,7 +61,7 @@ struct vkms_crtc_state { int num_active_planes; /* stack of active planes for crc computation, should be in z order */ struct vkms_plane_state **active_planes; - void *active_writeback; + struct vkms_writeback_job *active_writeback;
/* below four are protected by vkms_output.composer_lock */ bool crc_pending; diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 0935686475a0..425b6c6b8cad 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -65,41 +65,45 @@ static int vkms_wb_connector_get_modes(struct drm_connector *connector) static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector, struct drm_writeback_job *job) { - struct drm_gem_object *gem_obj; - struct dma_buf_map map; + struct vkms_writeback_job *vkmsjob; int ret;
if (!job->fb) return 0;
- gem_obj = drm_gem_fb_get_obj(job->fb, 0); - ret = drm_gem_shmem_vmap(gem_obj, &map); + vkmsjob = kzalloc(sizeof(*vkmsjob), GFP_KERNEL); + if (!vkmsjob) + return -ENOMEM; + + ret = drm_gem_fb_vmap(job->fb, vkmsjob->map); if (ret) { DRM_ERROR("vmap failed: %d\n", ret); - return ret; + goto err_kfree; }
- job->priv = map.vaddr; + job->priv = vkmsjob;
return 0; + +err_kfree: + kfree(vkmsjob); + return ret; }
static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector, struct drm_writeback_job *job) { - struct drm_gem_object *gem_obj; + struct vkms_writeback_job *vkmsjob = job->priv; struct vkms_device *vkmsdev; - struct dma_buf_map map;
if (!job->fb) return;
- gem_obj = drm_gem_fb_get_obj(job->fb, 0); - dma_buf_map_set_vaddr(&map, job->priv); - drm_gem_shmem_vunmap(gem_obj, &map); + drm_gem_fb_vunmap(job->fb, vkmsjob->map);
- vkmsdev = drm_device_to_vkms_device(gem_obj->dev); + vkmsdev = drm_device_to_vkms_device(job->fb->dev); vkms_set_composer(&vkmsdev->output, false); + kfree(vkmsjob); }
static void vkms_wb_atomic_commit(struct drm_connector *conn,
dri-devel@lists.freedesktop.org
-
Noralf Trønnes -
Thomas Zimmermann