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. - [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. - [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. --
--- 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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
