Am 18.08.26 um 15:19 schrieb [email protected]:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
to consider:
- [High] The `drm_plane_helper_display_panic_screen` helper unconditionally dereferences
`plane->helper_private` and `plane->helper_private->get_scanout_buffer`.
However, by adding `DRM_PANIC_PLANE_FUNCS` to shared plane function tables (like
`skl_plane_funcs` in i915 and `dm_plane_funcs` in amdgpu), the `display_panic_screen`
callback is now applied to all planes (including cursors and overlays). These
non-primary planes use different helper functions (e.g., `intel_plane_helper_funcs` or
`dm_plane_helper_funcs`) that do not implement `get_scanout_buffer`. This causes
`drm_panic_register` to mistakenly register panic dumpers for these planes, and upon a
panic, the handler executes a NULL pointer dereference, triggering a nested panic.
Will be fixed by testing for get_scanout_buffer from
drm_plane_helper_display_panic_screen(). I'll also add a check to only
register panic screens for primary planes.
- [Medium] The patch replaces `get_scanout_buffer` with `display_panic_screen`
as the signal for panic support. However, it fails to add
`DRM_PANIC_PLANE_FUNCS` to the legacy fallback `primary_plane_funcs` in
`drm_modeset_helper.c`. Older AMDGPU DCE drivers (e.g., DCE 6.0, 8.0, 10.0) use
`drm_crtc_init()` which implicitly relies on this legacy structure for their
primary planes. Although these drivers implement `get_scanout_buffer` in their
helper functions (`dce_v10_0_drm_primary_plane_helper_funcs`), their planes now
lack the `display_panic_screen` callback. Consequently,
`drm_panic_is_enabled()` returns false and their panic dumpers are never
registered, silently breaking panic screen support on these hardware
generations.
Will be added.
- [Low] `drm_plane_helper_display_panic_screen` accepts a `panic_type`
parameter (an `enum drm_panic_type`). However, when it calls
`draw_panic_dispatch()`, it passes the global module parameter `drm_panic_type`
instead of the passed-in `panic_type`. This logic bug prevents drivers from
tailoring the panic screen implementation via the helper as intended.
Fixed in an earlier patch.
--
--- Patch [9]: [PATCH 09/12] drm/panic: Display panic screen via per-plane
callback ---
commit 69fa8c39afc844867e193df3659c6f6f1fb762f3
Author: Thomas Zimmermann <[email protected]>
drm/panic: Display panic screen via per-plane callback
This commit adds display_panic_screen to struct drm_plane_funcs to replace
get_scanout_buffer as the signal for panic support. It provides the helper
drm_plane_helper_display_panic_screen() and the DRM_PANIC_PLANE_FUNCS macro
to update drivers that implement panic support.
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index 824ef3ce5de0a..eadc22a255bb5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
[ ... ]
@@ -2177,6 +2178,7 @@ static const struct drm_plane_funcs dm_plane_funcs = {
.atomic_set_property = dm_atomic_plane_set_property,
.atomic_get_property = dm_atomic_plane_get_property,
#endif
+ DRM_PANIC_PLANE_FUNCS,
};
[Severity: High]
Does adding this macro to shared plane function tables (like dm_plane_funcs
or skl_plane_funcs) apply the panic screen callback to all planes, including
cursors and overlays?
Non-primary planes use different helper functions that do not implement
get_scanout_buffer. Since drm_plane_helper_display_panic_screen()
unconditionally dereferences plane->helper_private->get_scanout_buffer,
could this cause a NULL pointer dereference in the panic handler for these
planes and trigger a nested panic?
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 2263a11efdf63..2ecb4d1f6d470 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
[ ... ]
+void drm_plane_helper_display_panic_screen(struct drm_plane *plane, const char
*description,
+ enum drm_panic_type panic_type,
+ u32 fg_color, u32 bg_color, unsigned
int qr_version)
{
struct drm_scanout_buffer sb = { };
int ret;
[Severity: Low]
Is the panic_type parameter correctly passed to the dispatch function?
The function accepts a panic_type argument, but the implementation appears
to pass the global module parameter drm_panic_type to draw_panic_dispatch()
instead of the passed-in argument. Does this discard the caller's requested
panic type?
diff --git a/include/drm/drm_panic_helper.h b/include/drm/drm_panic_helper.h
new file mode 100644
index 0000000000000..e296620bbe34b
--- /dev/null
+++ b/include/drm/drm_panic_helper.h
[ ... ]
+#if IS_ENABLED(CONFIG_DRM_PANIC)
+#define DRM_PANIC_PLANE_FUNCS \
+ .display_panic_screen = drm_plane_helper_display_panic_screen
+#else
+#define DRM_PANIC_PLANE_FUNCS \
+ .display_panic_screen = NULL
+#endif
[Severity: Medium]
Does the legacy fallback primary_plane_funcs in drm_modeset_helper.c also
need to be updated with DRM_PANIC_PLANE_FUNCS?
Older AMDGPU DCE drivers use drm_crtc_init(), which implicitly relies on
this legacy structure for their primary planes. Although these drivers
implement get_scanout_buffer() in their helper functions, lacking this
macro in the fallback could cause drm_panic_is_enabled() to return false
and silently drop panic screen support on these hardware generations.
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
