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

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)