* [PATCH v3 0/5] drm: Provide framebuffer vmap helpers
@ 2021-07-30 18:35 Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 1/5] drm: Define DRM_FORMAT_MAX_PLANES Thomas Zimmermann
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2021-07-30 18:35 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel, noralf,
rodrigosiqueiramelo, melissa.srw, hamohammed.sa
Cc: dri-devel, Thomas Zimmermann
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/5] drm: Define DRM_FORMAT_MAX_PLANES
2021-07-30 18:35 [PATCH v3 0/5] drm: Provide framebuffer vmap helpers Thomas Zimmermann
@ 2021-07-30 18:35 ` Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 2/5] drm/gem: Provide drm_gem_fb_{vmap,vunmap}() Thomas Zimmermann
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2021-07-30 18:35 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel, noralf,
rodrigosiqueiramelo, melissa.srw, hamohammed.sa
Cc: dri-devel, Thomas Zimmermann, Sam Ravnborg
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];
};
/**
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/5] drm/gem: Provide drm_gem_fb_{vmap,vunmap}()
2021-07-30 18:35 [PATCH v3 0/5] drm: Provide framebuffer vmap helpers Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 1/5] drm: Define DRM_FORMAT_MAX_PLANES Thomas Zimmermann
@ 2021-07-30 18:35 ` Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 3/5] drm/gem: Clear mapping addresses for unused framebuffer planes Thomas Zimmermann
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2021-07-30 18:35 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel, noralf,
rodrigosiqueiramelo, melissa.srw, hamohammed.sa
Cc: dri-devel, Thomas Zimmermann, Sam Ravnborg
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);
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/5] drm/gem: Clear mapping addresses for unused framebuffer planes
2021-07-30 18:35 [PATCH v3 0/5] drm: Provide framebuffer vmap helpers Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 1/5] drm: Define DRM_FORMAT_MAX_PLANES Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 2/5] drm/gem: Provide drm_gem_fb_{vmap,vunmap}() Thomas Zimmermann
@ 2021-07-30 18:35 ` Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap() Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 5/5] drm/vkms: Map output " Thomas Zimmermann
4 siblings, 0 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2021-07-30 18:35 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel, noralf,
rodrigosiqueiramelo, melissa.srw, hamohammed.sa
Cc: dri-devel, Thomas Zimmermann, Sam Ravnborg
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]);
}
}
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()
2021-07-30 18:35 [PATCH v3 0/5] drm: Provide framebuffer vmap helpers Thomas Zimmermann
` (2 preceding siblings ...)
2021-07-30 18:35 ` [PATCH v3 3/5] drm/gem: Clear mapping addresses for unused framebuffer planes Thomas Zimmermann
@ 2021-07-30 18:35 ` Thomas Zimmermann
2022-05-06 14:01 ` Noralf Trønnes
2021-07-30 18:35 ` [PATCH v3 5/5] drm/vkms: Map output " Thomas Zimmermann
4 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2021-07-30 18:35 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel, noralf,
rodrigosiqueiramelo, melissa.srw, hamohammed.sa
Cc: dri-devel, Thomas Zimmermann, Sam Ravnborg
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;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 5/5] drm/vkms: Map output framebuffer BOs with drm_gem_fb_vmap()
2021-07-30 18:35 [PATCH v3 0/5] drm: Provide framebuffer vmap helpers Thomas Zimmermann
` (3 preceding siblings ...)
2021-07-30 18:35 ` [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap() Thomas Zimmermann
@ 2021-07-30 18:35 ` Thomas Zimmermann
4 siblings, 0 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2021-07-30 18:35 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel, noralf,
rodrigosiqueiramelo, melissa.srw, hamohammed.sa
Cc: dri-devel, Thomas Zimmermann, Rodrigo Siqueira, Sam Ravnborg
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,
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()
2021-07-30 18:35 ` [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap() Thomas Zimmermann
@ 2022-05-06 14:01 ` Noralf Trønnes
2022-05-06 14:11 ` Thomas Zimmermann
0 siblings, 1 reply; 10+ messages in thread
From: Noralf Trønnes @ 2022-05-06 14:01 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: hamohammed.sa, rodrigosiqueiramelo, airlied, melissa.srw,
dri-devel, Sam Ravnborg
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;
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()
2022-05-06 14:01 ` Noralf Trønnes
@ 2022-05-06 14:11 ` Thomas Zimmermann
2022-05-09 8:32 ` Thomas Zimmermann
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2022-05-06 14:11 UTC (permalink / raw)
To: Noralf Trønnes
Cc: hamohammed.sa, rodrigosiqueiramelo, airlied, dri-devel,
melissa.srw, Sam Ravnborg
[-- Attachment #1.1: Type: text/plain, Size: 8043 bytes --]
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;
>> }
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()
2022-05-06 14:11 ` Thomas Zimmermann
@ 2022-05-09 8:32 ` Thomas Zimmermann
2022-05-09 20:10 ` Noralf Trønnes
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2022-05-09 8:32 UTC (permalink / raw)
To: Noralf Trønnes
Cc: hamohammed.sa, rodrigosiqueiramelo, airlied, dri-devel,
melissa.srw, Sam Ravnborg
[-- Attachment #1.1: Type: text/plain, Size: 9361 bytes --]
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_framebuffer_helper.c#L348
[2]
https://lore.kernel.org/dri-devel/20220509081602.474-1-tzimmermann@suse.de/T/#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;
>>> }
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap()
2022-05-09 8:32 ` Thomas Zimmermann
@ 2022-05-09 20:10 ` Noralf Trønnes
0 siblings, 0 replies; 10+ messages in thread
From: Noralf Trønnes @ 2022-05-09 20:10 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: hamohammed.sa, rodrigosiqueiramelo, airlied, dri-devel,
melissa.srw, Noralf Trønnes, Sam Ravnborg
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_framebuffer_helper.c#L348
>
> [2]
> https://lore.kernel.org/dri-devel/20220509081602.474-1-tzimmermann@suse.de/T/#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;
>>>> }
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-05-09 20:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-30 18:35 [PATCH v3 0/5] drm: Provide framebuffer vmap helpers Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 1/5] drm: Define DRM_FORMAT_MAX_PLANES Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 2/5] drm/gem: Provide drm_gem_fb_{vmap,vunmap}() Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 3/5] drm/gem: Clear mapping addresses for unused framebuffer planes Thomas Zimmermann
2021-07-30 18:35 ` [PATCH v3 4/5] drm/gud: Map framebuffer BOs with drm_gem_fb_vmap() Thomas Zimmermann
2022-05-06 14:01 ` Noralf Trønnes
2022-05-06 14:11 ` Thomas Zimmermann
2022-05-09 8:32 ` Thomas Zimmermann
2022-05-09 20:10 ` Noralf Trønnes
2021-07-30 18:35 ` [PATCH v3 5/5] drm/vkms: Map output " Thomas Zimmermann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.