Some DRM helpers assume that all potential color planes of a framebuffer are available; even if the color format didn't specify them. Non-existing planes are silently ignored. This behavior is inconsistent with other helpers and apparently leads to subtle bugs with uninitialized GEM buffer mappings. [1]
Change all affected code to look at the framebuffer format's number of color planes and only process these planes. The update has been discussed in [2] before.
Tested with GEM SHMEM helpers on simpledrm.
[1] https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de... [2] https://lore.kernel.org/dri-devel/877dc0d9-c6c6-022c-20d8-14b33e863934@suse....
Thomas Zimmermann (4): drm/gem: Share code between drm_gem_fb_{begin,end}_cpu_access() drm/gem: Ignore color planes that are unused by framebuffer format drm/gem-vram: Ignore planes that are unused by framebuffer format drm/gem: Warn on trying to use a non-existing framebuffer plane
drivers/gpu/drm/drm_gem_atomic_helper.c | 6 +- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 103 +++++++++---------- drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++++--- include/drm/drm_gem_framebuffer_helper.h | 10 +- 4 files changed, 81 insertions(+), 74 deletions(-)
base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3
The error-recovery code in drm_gem_fb_begin() is the same pattern as drm_gem_fb_end(). Implement both this an internal helper. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------ 1 file changed, 26 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index f4619803acd0..2fcacab9f812 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, } EXPORT_SYMBOL(drm_gem_fb_vunmap);
+static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir, + unsigned int num_planes) +{ + struct dma_buf_attachment *import_attach; + struct drm_gem_object *obj; + int ret; + + while (num_planes) { + --num_planes; + obj = drm_gem_fb_get_obj(fb, num_planes); + if (!obj) + continue; + import_attach = obj->import_attach; + if (!import_attach) + continue; + ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir); + if (ret) + drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret); + } +} + /** * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access * @fb: the framebuffer @@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct struct dma_buf_attachment *import_attach; struct drm_gem_object *obj; size_t i; - int ret, ret2; + int ret;
for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) { obj = drm_gem_fb_get_obj(fb, i); @@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct continue; ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir); if (ret) - goto err_dma_buf_end_cpu_access; + goto err___drm_gem_fb_end_cpu_access; }
return 0;
-err_dma_buf_end_cpu_access: - while (i) { - --i; - obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; - import_attach = obj->import_attach; - if (!import_attach) - continue; - ret2 = dma_buf_end_cpu_access(import_attach->dmabuf, dir); - if (ret2) { - drm_err(fb->dev, - "dma_buf_end_cpu_access() failed during error handling: %d\n", - ret2); - } - } - +err___drm_gem_fb_end_cpu_access: + __drm_gem_fb_end_cpu_access(fb, dir, i); return ret; } EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access); @@ -472,23 +478,7 @@ EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access); */ void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir) { - size_t i = ARRAY_SIZE(fb->obj); - struct dma_buf_attachment *import_attach; - struct drm_gem_object *obj; - int ret; - - while (i) { - --i; - obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; - import_attach = obj->import_attach; - if (!import_attach) - continue; - ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir); - if (ret) - drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret); - } + __drm_gem_fb_end_cpu_access(fb, dir, ARRAY_SIZE(fb->obj)); } EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
Hello Thomas,
On 5/9/22 10:15, Thomas Zimmermann wrote:
The error-recovery code in drm_gem_fb_begin() is the same pattern as drm_gem_fb_end(). Implement both this an internal helper. No
Maybe "both of these using using an" or something like that ?
functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------ 1 file changed, 26 insertions(+), 36 deletions(-)
Nice cleanup. For the patch:
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
I just have a few minor comments below.
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index f4619803acd0..2fcacab9f812 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, } EXPORT_SYMBOL(drm_gem_fb_vunmap);
+static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,
unsigned int num_planes)+{
- struct dma_buf_attachment *import_attach;
- struct drm_gem_object *obj;
- int ret;
- while (num_planes) {
--num_planes;obj = drm_gem_fb_get_obj(fb, num_planes);if (!obj)continue;import_attach = obj->import_attach;if (!import_attach)continue;ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);if (ret)drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);
I wonder if would be useful to also print the plane index and access mode here.
- }
+}
/**
- drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
- @fb: the framebuffer
@@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct struct dma_buf_attachment *import_attach; struct drm_gem_object *obj; size_t i;
- int ret, ret2;
int ret;
for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) { obj = drm_gem_fb_get_obj(fb, i);
@@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct continue; ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir); if (ret)
goto err_dma_buf_end_cpu_access;
goto err___drm_gem_fb_end_cpu_access;
I wouldn't rename this. As I read it, the goto label doesn't denote what function is called but rather what action is being taken in an error path.
Since finally what's being done is a dma_buf_end_cpu_access in the helper, I think that just keeping err_dma_buf_end_cpu_access is enough. Also, the additional underscores added make it harder to read IMO.
Hi Javier
Am 16.05.22 um 15:00 schrieb Javier Martinez Canillas:
Hello Thomas,
On 5/9/22 10:15, Thomas Zimmermann wrote:
The error-recovery code in drm_gem_fb_begin() is the same pattern as drm_gem_fb_end(). Implement both this an internal helper. No
Maybe "both of these using using an" or something like that ?
Of course.
functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------ 1 file changed, 26 insertions(+), 36 deletions(-)
Nice cleanup. For the patch:
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
I just have a few minor comments below.
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index f4619803acd0..2fcacab9f812 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, } EXPORT_SYMBOL(drm_gem_fb_vunmap);
+static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,
unsigned int num_planes)+{
- struct dma_buf_attachment *import_attach;
- struct drm_gem_object *obj;
- int ret;
- while (num_planes) {
--num_planes;obj = drm_gem_fb_get_obj(fb, num_planes);if (!obj)continue;import_attach = obj->import_attach;if (!import_attach)continue;ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);if (ret)drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);I wonder if would be useful to also print the plane index and access mode here.
Ok.
- }
+}
- /**
- drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
- @fb: the framebuffer
@@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct struct dma_buf_attachment *import_attach; struct drm_gem_object *obj; size_t i;
- int ret, ret2;
int ret;
for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) { obj = drm_gem_fb_get_obj(fb, i);
@@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct continue; ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir); if (ret)
goto err_dma_buf_end_cpu_access;
goto err___drm_gem_fb_end_cpu_access;I wouldn't rename this. As I read it, the goto label doesn't denote what function is called but rather what action is being taken in an error path.
Since finally what's being done is a dma_buf_end_cpu_access in the helper, I think that just keeping err_dma_buf_end_cpu_access is enough. Also, the additional underscores added make it harder to read IMO.
I usually name the goto labels after the function that comes next. It's not really pretty here, but being inconsistent would probably annoy me more than the underscores.
Best regards Thomas
Only handle color planes that exist in a framebuffer's color format. Ignore non-existing planes.
So far, several helpers assumed that all 4 planes are available and silently ignored non-existing planes. This lead to subtil bugs with uninitialized data in instances of struct iosys_map. [1]
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Link: https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de... # 1 --- drivers/gpu/drm/drm_gem_atomic_helper.c | 6 ++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 37 +++++++++++--------- include/drm/drm_gem_framebuffer_helper.h | 10 ++---- 3 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index a5026f617739..f16d60217c6c 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -169,8 +169,10 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj = drm_gem_fb_get_obj(state->fb, i); struct dma_fence *new;
- if (WARN_ON_ONCE(!obj)) - continue; + if (!obj) { + ret = -EINVAL; + goto error; + }
ret = dma_resv_get_singleton(obj->resv, usage, &new); if (ret) diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2fcacab9f812..09e90e19cd93 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -92,9 +92,9 @@ drm_gem_fb_init(struct drm_device *dev, */ void drm_gem_fb_destroy(struct drm_framebuffer *fb) { - size_t i; + unsigned int i;
- for (i = 0; i < ARRAY_SIZE(fb->obj); i++) + for (i = 0; i < fb->format->num_planes; i++) drm_gem_object_put(fb->obj[i]);
drm_framebuffer_cleanup(fb); @@ -329,24 +329,26 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty); * The argument returns the addresses of the data stored in each BO. This * is different from @map if the framebuffer's offsets field is non-zero. * + * Both, @map and @data, must each refer to arrays with at least + * fb->format->num_planes elements. + * * 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 iosys_map map[static DRM_FORMAT_MAX_PLANES], - struct iosys_map data[DRM_FORMAT_MAX_PLANES]) +int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map, + struct iosys_map *data) { struct drm_gem_object *obj; unsigned int i; int ret;
- for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { + for (i = 0; i < fb->format->num_planes; ++i) { obj = drm_gem_fb_get_obj(fb, i); if (!obj) { - iosys_map_clear(&map[i]); - continue; + ret = -EINVAL; + goto err_drm_gem_vunmap; } ret = drm_gem_vmap(obj, &map[i]); if (ret) @@ -354,7 +356,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb, }
if (data) { - for (i = 0; i < DRM_FORMAT_MAX_PLANES; ++i) { + for (i = 0; i < fb->format->num_planes; ++i) { memcpy(&data[i], &map[i], sizeof(data[i])); if (iosys_map_is_null(&data[i])) continue; @@ -385,10 +387,9 @@ EXPORT_SYMBOL(drm_gem_fb_vmap); * * See drm_gem_fb_vmap() for more information. */ -void drm_gem_fb_vunmap(struct drm_framebuffer *fb, - struct iosys_map map[static DRM_FORMAT_MAX_PLANES]) +void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map) { - unsigned int i = DRM_FORMAT_MAX_PLANES; + unsigned int i = fb->format->num_planes; struct drm_gem_object *obj;
while (i) { @@ -442,13 +443,15 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct { struct dma_buf_attachment *import_attach; struct drm_gem_object *obj; - size_t i; + unsigned int i; int ret;
- for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) { + for (i = 0; i < fb->format->num_planes; ++i) { obj = drm_gem_fb_get_obj(fb, i); - if (!obj) - continue; + if (!obj) { + ret = -EINVAL; + goto err___drm_gem_fb_end_cpu_access; + } import_attach = obj->import_attach; if (!import_attach) continue; @@ -478,7 +481,7 @@ EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access); */ void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir) { - __drm_gem_fb_end_cpu_access(fb, dir, ARRAY_SIZE(fb->obj)); + __drm_gem_fb_end_cpu_access(fb, dir, fb->format->num_planes); } EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h index 1091e4fa08cb..d302521f3dd4 100644 --- a/include/drm/drm_gem_framebuffer_helper.h +++ b/include/drm/drm_gem_framebuffer_helper.h @@ -4,8 +4,6 @@ #include <linux/dma-buf.h> #include <linux/iosys-map.h>
-#include <drm/drm_fourcc.h> - struct drm_afbc_framebuffer; struct drm_device; struct drm_fb_helper_surface_size; @@ -39,11 +37,9 @@ 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 iosys_map map[static DRM_FORMAT_MAX_PLANES], - struct iosys_map data[DRM_FORMAT_MAX_PLANES]); -void drm_gem_fb_vunmap(struct drm_framebuffer *fb, - struct iosys_map map[static DRM_FORMAT_MAX_PLANES]); +int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map, + struct iosys_map *data); +void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map); 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);
On 5/9/22 10:16, Thomas Zimmermann wrote:
Only handle color planes that exist in a framebuffer's color format. Ignore non-existing planes.
So far, several helpers assumed that all 4 planes are available and silently ignored non-existing planes. This lead to subtil bugs with uninitialized data in instances of struct iosys_map. [1]
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Link: https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de... # 1
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
Only handle color planes that exist in a framebuffer's color format. Ignore non-existing planes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++++++++++++++++++--------- 1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 123045b58fec..3305541a10ab 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -9,6 +9,7 @@ #include <drm/drm_file.h> #include <drm/drm_framebuffer.h> #include <drm/drm_gem_atomic_helper.h> +#include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_ttm_helper.h> #include <drm/drm_gem_vram_helper.h> #include <drm/drm_managed.h> @@ -648,17 +649,22 @@ int drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *new_state) { - size_t i; + struct drm_framebuffer *fb = new_state->fb; struct drm_gem_vram_object *gbo; + struct drm_gem_object *obj; + unsigned int i; int ret;
- if (!new_state->fb) + if (!fb) return 0;
- for (i = 0; i < ARRAY_SIZE(new_state->fb->obj); ++i) { - if (!new_state->fb->obj[i]) - continue; - gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]); + for (i = 0; i < fb->format->num_planes; ++i) { + obj = drm_gem_fb_get_obj(fb, i); + if (!obj) { + ret = -EINVAL; + goto err_drm_gem_vram_unpin; + } + gbo = drm_gem_vram_of_gem(obj); ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); if (ret) goto err_drm_gem_vram_unpin; @@ -673,7 +679,10 @@ drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane, err_drm_gem_vram_unpin: while (i) { --i; - gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]); + obj = drm_gem_fb_get_obj(fb, i); + if (!obj) + continue; + gbo = drm_gem_vram_of_gem(obj); drm_gem_vram_unpin(gbo); } return ret; @@ -694,16 +703,19 @@ void drm_gem_vram_plane_helper_cleanup_fb(struct drm_plane *plane, struct drm_plane_state *old_state) { - size_t i; + struct drm_framebuffer *fb = old_state->fb; struct drm_gem_vram_object *gbo; + struct drm_gem_object *obj; + unsigned int i;
- if (!old_state->fb) + if (!fb) return;
- for (i = 0; i < ARRAY_SIZE(old_state->fb->obj); ++i) { - if (!old_state->fb->obj[i]) + for (i = 0; i < fb->format->num_planes; ++i) { + obj = drm_gem_fb_get_obj(fb, i); + if (!obj) continue; - gbo = drm_gem_vram_of_gem(old_state->fb->obj[i]); + gbo = drm_gem_vram_of_gem(obj); drm_gem_vram_unpin(gbo); } }
On 5/9/22 10:16, Thomas Zimmermann wrote:
Only handle color planes that exist in a framebuffer's color format. Ignore non-existing planes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
@@ -673,7 +679,10 @@ drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane, err_drm_gem_vram_unpin: while (i) { --i;
gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]);
obj = drm_gem_fb_get_obj(fb, i);if (!obj)continue;gbo = drm_gem_vram_of_gem(obj);
The code in this error path to unpin the GEM vram objects...
} return ret; @@ -694,16 +703,19 @@ void drm_gem_vram_plane_helper_cleanup_fb(struct drm_plane *plane, struct drm_plane_state *old_state) {
- size_t i;
- struct drm_framebuffer *fb = old_state->fb; struct drm_gem_vram_object *gbo;
- struct drm_gem_object *obj;
- unsigned int i;
- if (!old_state->fb)
- if (!fb) return;
- for (i = 0; i < ARRAY_SIZE(old_state->fb->obj); ++i) {
if (!old_state->fb->obj[i])
- for (i = 0; i < fb->format->num_planes; ++i) {
obj = drm_gem_fb_get_obj(fb, i);if (!obj) continue;
gbo = drm_gem_vram_of_gem(old_state->fb->obj[i]);
gbo = drm_gem_vram_of_gem(obj);
... and this, seems to be basically the same.
So maybe as a follow-up you can do a similar cleanup like you did in patch 1/4 and use a helper function to handle both.
Hi
Am 16.05.22 um 15:34 schrieb Javier Martinez Canillas:
On 5/9/22 10:16, Thomas Zimmermann wrote:
Only handle color planes that exist in a framebuffer's color format. Ignore non-existing planes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
@@ -673,7 +679,10 @@ drm_gem_vram_plane_helper_prepare_fb(struct drm_plane *plane, err_drm_gem_vram_unpin: while (i) { --i;
gbo = drm_gem_vram_of_gem(new_state->fb->obj[i]);
obj = drm_gem_fb_get_obj(fb, i);if (!obj)continue;gbo = drm_gem_vram_of_gem(obj);The code in this error path to unpin the GEM vram objects...
} return ret; @@ -694,16 +703,19 @@ void drm_gem_vram_plane_helper_cleanup_fb(struct drm_plane *plane, struct drm_plane_state *old_state) {
- size_t i;
- struct drm_framebuffer *fb = old_state->fb; struct drm_gem_vram_object *gbo;
- struct drm_gem_object *obj;
- unsigned int i;
- if (!old_state->fb)
- if (!fb) return;
- for (i = 0; i < ARRAY_SIZE(old_state->fb->obj); ++i) {
if (!old_state->fb->obj[i])
- for (i = 0; i < fb->format->num_planes; ++i) {
obj = drm_gem_fb_get_obj(fb, i);if (!obj) continue;
gbo = drm_gem_vram_of_gem(old_state->fb->obj[i]);
gbo = drm_gem_vram_of_gem(obj);... and this, seems to be basically the same.
So maybe as a follow-up you can do a similar cleanup like you did in patch 1/4 and use a helper function to handle both.
I was thinking the same when I wrote this patch and somehow decided against it. But since you also mention it, I'm going to add this change to the patchset.
Best regards Thomas
Warn if callers of drm_gem_fb_get_obj() try to use a GEM buffer for a non-existing or unset plane.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- 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 09e90e19cd93..291f9ae4359d 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -53,7 +53,11 @@ MODULE_IMPORT_NS(DMA_BUF); struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, unsigned int plane) { - if (plane >= ARRAY_SIZE(fb->obj)) + struct drm_device *dev = fb->dev; + + if (drm_WARN_ON_ONCE(dev, plane >= ARRAY_SIZE(fb->obj))) + return NULL; + else if (drm_WARN_ON_ONCE(dev, !fb->obj[plane])) return NULL;
return fb->obj[plane];
On 5/9/22 10:16, Thomas Zimmermann wrote:
Warn if callers of drm_gem_fb_get_obj() try to use a GEM buffer for a non-existing or unset plane.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
pinging for review
Am 09.05.22 um 10:15 schrieb Thomas Zimmermann:
Some DRM helpers assume that all potential color planes of a framebuffer are available; even if the color format didn't specify them. Non-existing planes are silently ignored. This behavior is inconsistent with other helpers and apparently leads to subtle bugs with uninitialized GEM buffer mappings. [1]
Change all affected code to look at the framebuffer format's number of color planes and only process these planes. The update has been discussed in [2] before.
Tested with GEM SHMEM helpers on simpledrm.
[1] https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de... [2] https://lore.kernel.org/dri-devel/877dc0d9-c6c6-022c-20d8-14b33e863934@suse....
Thomas Zimmermann (4): drm/gem: Share code between drm_gem_fb_{begin,end}_cpu_access() drm/gem: Ignore color planes that are unused by framebuffer format drm/gem-vram: Ignore planes that are unused by framebuffer format drm/gem: Warn on trying to use a non-existing framebuffer plane
drivers/gpu/drm/drm_gem_atomic_helper.c | 6 +- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 103 +++++++++---------- drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++++--- include/drm/drm_gem_framebuffer_helper.h | 10 +- 4 files changed, 81 insertions(+), 74 deletions(-)
base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3
Den 09.05.2022 10.15, skrev Thomas Zimmermann:
Some DRM helpers assume that all potential color planes of a framebuffer are available; even if the color format didn't specify them. Non-existing planes are silently ignored. This behavior is inconsistent with other helpers and apparently leads to subtle bugs with uninitialized GEM buffer mappings. [1]
Change all affected code to look at the framebuffer format's number of color planes and only process these planes. The update has been discussed in [2] before.
Tested with GEM SHMEM helpers on simpledrm.
Tested with drm/gud, now there's only one UBSAN warning:
Tested-by: Noralf Trønnes noralf@tronnes.org
I was hoping to get some time to give a review, but it doesn't look like I'm able to do that.
Noralf.
[1] https://lore.kernel.org/dri-devel/20210730183511.20080-1-tzimmermann@suse.de... [2] https://lore.kernel.org/dri-devel/877dc0d9-c6c6-022c-20d8-14b33e863934@suse....
Thomas Zimmermann (4): drm/gem: Share code between drm_gem_fb_{begin,end}_cpu_access() drm/gem: Ignore color planes that are unused by framebuffer format drm/gem-vram: Ignore planes that are unused by framebuffer format drm/gem: Warn on trying to use a non-existing framebuffer plane
drivers/gpu/drm/drm_gem_atomic_helper.c | 6 +- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 103 +++++++++---------- drivers/gpu/drm/drm_gem_vram_helper.c | 36 ++++--- include/drm/drm_gem_framebuffer_helper.h | 10 +- 4 files changed, 81 insertions(+), 74 deletions(-)
base-commit: b0b93865a24c910fcbfa6e6fa0955fae930a56d3
dri-devel@lists.freedesktop.org
-
Javier Martinez Canillas -
Noralf Trønnes -
Thomas Zimmermann