Re: [PATCH 09/12] drm/panic: Display panic screen via per-plane callback

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