Re: [PATCH v3 09/14] drm/panic: Split draw_panic_plane()

[Jocelyn Falempe <jfalempe@redhat.com>]

On 9/2/26 14:13, Thomas Zimmermann wrote:
> Move locking and parameters from draw_panic_plane() into the new
> helper drm_panic_display_panic_screen(). Call draw_panic_plane() from
> there.
> 
> The new helper drm_panic_display_panic_screen() is now the DRM core's
> interface for displaying a panic screen. The code remaining in
> draw_panic_plane() still does all the heavy lifting. It will become a
> plane helper for DRM drivers.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> v2:
> - use panic_type argument in draw_panic_plane() (Sashiko)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/drm_panic.c | 62 ++++++++++++++++++++-----------------
>   1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 690e96e8f1f9..624042c42a7f 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -936,42 +936,25 @@ static void drm_panic_clear_description(void)
>   	desc_line->txt = NULL;
>   }
>   
> -static void draw_panic_plane(struct drm_plane *plane, const char *description)
> +static void draw_panic_plane(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;
> -	unsigned long flags;
> -#if defined(CONFIG_DRM_PANIC_FOREGROUND_COLOR)
> -	u32 fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR;
> -#else
> -	u32 fg_color = 0x00ffffff;
> -#endif
> -#if defined(CONFIG_DRM_PANIC_BACKGROUND_COLOR)
> -	u32 bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR;
> -#else
> -	u32 bg_color = 0x00000000;
> -#endif
> -#if IS_ENABLED(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> -	unsigned int qr_version = panic_qr_version;
> -#else
> -	unsigned int qr_version = 0;
> -#endif
> -
> -	if (!drm_panic_trylock(plane->dev, flags))
> -		return;
>   
>   	ret = plane->helper_private->get_scanout_buffer(plane, &sb);
>   
>   	if (ret || !drm_panic_is_format_supported(sb.format))
> -		goto unlock;
> +		return;
>   
>   	/* One of these should be set, or it can't draw pixels */
>   	if (!sb.set_pixel && !sb.pages && iosys_map_is_null(&sb.map[0]))
> -		goto unlock;
> +		return;
>   
>   	drm_panic_set_description(description);
>   
> -	ret = draw_panic_dispatch(&sb, drm_panic_type, fg_color, bg_color, qr_version);
> +	ret = draw_panic_dispatch(&sb, panic_type, fg_color, bg_color, qr_version);
>   	if (!ret) {
>   		/*
>   		 * Only flush if we have a panic screen to display. Otherwise
> @@ -982,9 +965,33 @@ static void draw_panic_plane(struct drm_plane *plane, const char *description)
>   	}
>   
>   	drm_panic_clear_description();
> +}
>   
> -unlock:
> -	drm_panic_unlock(plane->dev, flags);
> +static void drm_panic_display_panic_screen(struct drm_plane *plane, const char *description)
> +{
> +#if defined(CONFIG_DRM_PANIC_FOREGROUND_COLOR)
> +	u32 fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR;
> +#else
> +	u32 fg_color = 0x00ffffff;
> +#endif
> +#if defined(CONFIG_DRM_PANIC_BACKGROUND_COLOR)
> +	u32 bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR;
> +#else
> +	u32 bg_color = 0x00000000;
> +#endif
> +#if IS_ENABLED(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> +	unsigned int qr_version = panic_qr_version;
> +#else
> +	unsigned int qr_version = 0;
> +#endif
> +	struct drm_device *dev = plane->dev;
> +	unsigned long flags;
> +
> +	if (drm_panic_trylock(dev, flags)) {
> +		draw_panic_plane(plane, description, drm_panic_type,
> +				 fg_color, bg_color, qr_version);
> +		drm_panic_unlock(dev, flags);
> +	}
>   }
>   
>   static struct drm_plane *to_drm_plane(struct kmsg_dumper *kd)
> @@ -997,10 +1004,9 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai
>   	struct drm_plane *plane = to_drm_plane(dumper);
>   
>   	if (detail->reason == KMSG_DUMP_PANIC)
> -		draw_panic_plane(plane, detail->description);
> +		drm_panic_display_panic_screen(plane, detail->description);
>   }
>   
> -
>   /*
>    * DEBUG FS, This is currently unsafe.
>    * Create one file per plane, so it's possible to debug one plane at a time.
> @@ -1017,7 +1023,7 @@ static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_
>   	if (kstrtobool_from_user(user_buf, count, &run) == 0 && run) {
>   		struct drm_plane *plane = file->private_data;
>   
> -		draw_panic_plane(plane, "Test from debugfs");
> +		drm_panic_display_panic_screen(plane, "Test from debugfs");
>   	}
>   	return count;
>   }