[PATCH 0/3] drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

[PATCH 0/3] drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

[Jocelyn Falempe]

When proposing to enable DRM_PANIC on Fedora, some users raised concern about the need to disable VT_CONSOLE.
So this is my new attempt to avoid fbcon/vt_console to overwrite the panic screen.
This time it doesn't involve any locking, so it should be safe.
I added a skip_panic option in struct fb_info, and check if this option and the panic_cpu are set in fb_is_inactive(), to prevent any framebuffer operation.
Also skip_panic is only true if the drm driver supports drm_panic, so you will still get the VT panic info on drivers that don't have drm_panic support yet.

Jocelyn Falempe (3):
  drm/panic: Add drm_panic_is_enabled()
  fbcon: Add an option to disable fbcon in panic.
  drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

 drivers/gpu/drm/Kconfig          |  2 +-
 drivers/gpu/drm/drm_fb_helper.c  |  2 ++
 drivers/gpu/drm/drm_panic.c      | 20 ++++++++++++++++++++
 drivers/video/fbdev/core/fbcon.c |  7 ++++++-
 include/drm/drm_panic.h          |  2 ++
 include/linux/fb.h               |  1 +
 6 files changed, 32 insertions(+), 2 deletions(-)


base-commit: a237f217bad50c381773da5b00442710d1449098
-- 
2.45.2

[PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Jocelyn Falempe]

It allows to check if the drm device supports drm_panic.
Prepare the work to have better integration with fbcon and vtconsole.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
 include/drm/drm_panic.h     |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 948aed00595e..d9a25c2d0a65 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -703,6 +703,26 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
 static void debugfs_register_plane(struct drm_plane *plane, int index) {}
 #endif /* CONFIG_DRM_PANIC_DEBUG */
 
+/**
+ * drm_panic_is_enabled
+ * @dev: the drm device that may supports drm_panic
+ *
+ * returns true if the drm device supports drm_panic
+ */
+bool drm_panic_is_enabled(struct drm_device *dev)
+{
+	struct drm_plane *plane;
+
+	if (!dev->mode_config.num_total_plane)
+		return false;
+
+	drm_for_each_plane(plane, dev)
+		if (plane->helper_private && plane->helper_private->get_scanout_buffer)
+			return true;
+	return false;
+}
+EXPORT_SYMBOL(drm_panic_is_enabled);
+
 /**
  * drm_panic_register() - Initialize DRM panic for a device
  * @dev: the drm device on which the panic screen will be displayed.
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
index 73bb3f3d9ed9..c3a358dc3e27 100644
--- a/include/drm/drm_panic.h
+++ b/include/drm/drm_panic.h
@@ -148,11 +148,13 @@ struct drm_scanout_buffer {
 
 #ifdef CONFIG_DRM_PANIC
 
+bool drm_panic_is_enabled(struct drm_device *dev);
 void drm_panic_register(struct drm_device *dev);
 void drm_panic_unregister(struct drm_device *dev);
 
 #else
 
+bool drm_panic_is_enabled(struct drm_device *dev) {return false; }
 static inline void drm_panic_register(struct drm_device *dev) {}
 static inline void drm_panic_unregister(struct drm_device *dev) {}
 
-- 
2.45.2

[PATCH 2/3] fbcon: Add an option to disable fbcon in panic.

[Jocelyn Falempe]

This is required to avoid conflict between DRM_PANIC, and fbcon. If
a drm device already handle panic with drm_panic, it should set
the skip_panic field in fb_info, so that fbcon will stay quiet, and
not overwrite the panic_screen.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c  | 2 ++
 drivers/video/fbdev/core/fbcon.c | 7 ++++++-
 include/linux/fb.h               | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index e2e19f49342e..3662d664d8f9 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -40,6 +40,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
 
@@ -524,6 +525,7 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
 	fb_helper->info = info;
 	info->skip_vt_switch = true;
 
+	info->skip_panic = drm_panic_is_enabled(fb_helper->dev);
 	return info;
 
 err_release:
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 3f7333dca508..498d9c07df80 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -270,12 +270,17 @@ static int fbcon_get_rotate(struct fb_info *info)
 	return (ops) ? ops->rotate : 0;
 }
 
+static bool fbcon_skip_panic(struct fb_info *info)
+{
+	return (info->skip_panic && unlikely(atomic_read(&panic_cpu) != PANIC_CPU_INVALID));
+}
+
 static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 
 	return (info->state != FBINFO_STATE_RUNNING ||
-		vc->vc_mode != KD_TEXT || ops->graphics);
+		vc->vc_mode != KD_TEXT || ops->graphics || fbcon_skip_panic(info));
 }
 
 static int get_color(struct vc_data *vc, struct fb_info *info,
diff --git a/include/linux/fb.h b/include/linux/fb.h
index db7d97b10964..865dad03e73e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -510,6 +510,7 @@ struct fb_info {
 	void *par;
 
 	bool skip_vt_switch; /* no VT switch on suspend/resume required */
+	bool skip_panic; /* Do not write to the fb after a panic */
 };
 
 /* This will go away
-- 
2.45.2

[PATCH 3/3] drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

[Jocelyn Falempe]

Now that fbcon has the skip_panic option, there is no more conflicts
between drm_panic and fbcon.
Remove the build time dependency, so they can be both enabled.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 6dd0016fc9cd..a22cab218004 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -107,7 +107,7 @@ config DRM_KMS_HELPER
 
 config DRM_PANIC
 	bool "Display a user-friendly message when a kernel panic occurs"
-	depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE)
+	depends on DRM
 	select FONT_SUPPORT
 	help
 	  Enable a drm panic handler, which will display a user-friendly message
-- 
2.45.2

Re: [PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe@redhat.com> writes:

Hello Jocelyn,

> It allows to check if the drm device supports drm_panic.
> Prepare the work to have better integration with fbcon and vtconsole.
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
>  include/drm/drm_panic.h     |  2 ++
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 948aed00595e..d9a25c2d0a65 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -703,6 +703,26 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
>  static void debugfs_register_plane(struct drm_plane *plane, int index) {}
>  #endif /* CONFIG_DRM_PANIC_DEBUG */
>  
> +/**
> + * drm_panic_is_enabled
> + * @dev: the drm device that may supports drm_panic
> + *
> + * returns true if the drm device supports drm_panic
> + */
> +bool drm_panic_is_enabled(struct drm_device *dev)
> +{
> +	struct drm_plane *plane;
> +
> +	if (!dev->mode_config.num_total_plane)
> +		return false;
> +
> +	drm_for_each_plane(plane, dev)
> +		if (plane->helper_private && plane->helper_private->get_scanout_buffer)
> +			return true;
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_panic_is_enabled);
> +

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/3] fbcon: Add an option to disable fbcon in panic.

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe@redhat.com> writes:

> This is required to avoid conflict between DRM_PANIC, and fbcon. If
> a drm device already handle panic with drm_panic, it should set
> the skip_panic field in fb_info, so that fbcon will stay quiet, and
> not overwrite the panic_screen.
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---

This makes sense to me as well.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/3] drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe@redhat.com> writes:

> Now that fbcon has the skip_panic option, there is no more conflicts
> between drm_panic and fbcon.
> Remove the build time dependency, so they can be both enabled.
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 6dd0016fc9cd..a22cab218004 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -107,7 +107,7 @@ config DRM_KMS_HELPER
>  
>  config DRM_PANIC
>  	bool "Display a user-friendly message when a kernel panic occurs"
> -	depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE)
> +	depends on DRM

This is great. Thanks for finding an alternative approach! I don't see any
issues this time, because there is no locking involved. But let's see what
others think about it.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/3] fbcon: Add an option to disable fbcon in panic.

[Daniel Vetter]

On Wed, Jul 17, 2024 at 10:48:40AM +0200, Jocelyn Falempe wrote:
> This is required to avoid conflict between DRM_PANIC, and fbcon. If
> a drm device already handle panic with drm_panic, it should set
> the skip_panic field in fb_info, so that fbcon will stay quiet, and
> not overwrite the panic_screen.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c  | 2 ++
>  drivers/video/fbdev/core/fbcon.c | 7 ++++++-
>  include/linux/fb.h               | 1 +
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index e2e19f49342e..3662d664d8f9 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -40,6 +40,7 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_panic.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -524,6 +525,7 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
>  	fb_helper->info = info;
>  	info->skip_vt_switch = true;
>  
> +	info->skip_panic = drm_panic_is_enabled(fb_helper->dev);
>  	return info;
>  
>  err_release:

Bit a bikeshed, but I'd split this patch out since it's for drm's fbdev
emulation, not the fbcon core code. With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 3f7333dca508..498d9c07df80 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -270,12 +270,17 @@ static int fbcon_get_rotate(struct fb_info *info)
>  	return (ops) ? ops->rotate : 0;
>  }
>  
> +static bool fbcon_skip_panic(struct fb_info *info)
> +{
> +	return (info->skip_panic && unlikely(atomic_read(&panic_cpu) != PANIC_CPU_INVALID));
> +}
> +
>  static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info)
>  {
>  	struct fbcon_ops *ops = info->fbcon_par;
>  
>  	return (info->state != FBINFO_STATE_RUNNING ||
> -		vc->vc_mode != KD_TEXT || ops->graphics);
> +		vc->vc_mode != KD_TEXT || ops->graphics || fbcon_skip_panic(info));
>  }
>  
>  static int get_color(struct vc_data *vc, struct fb_info *info,
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index db7d97b10964..865dad03e73e 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -510,6 +510,7 @@ struct fb_info {
>  	void *par;
>  
>  	bool skip_vt_switch; /* no VT switch on suspend/resume required */
> +	bool skip_panic; /* Do not write to the fb after a panic */
>  };
>  
>  /* This will go away
> -- 
> 2.45.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Daniel Vetter]

On Wed, Jul 17, 2024 at 10:48:39AM +0200, Jocelyn Falempe wrote:
> It allows to check if the drm device supports drm_panic.
> Prepare the work to have better integration with fbcon and vtconsole.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
>  include/drm/drm_panic.h     |  2 ++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 948aed00595e..d9a25c2d0a65 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -703,6 +703,26 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
>  static void debugfs_register_plane(struct drm_plane *plane, int index) {}
>  #endif /* CONFIG_DRM_PANIC_DEBUG */
>  
> +/**
> + * drm_panic_is_enabled
> + * @dev: the drm device that may supports drm_panic
> + *
> + * returns true if the drm device supports drm_panic
> + */
> +bool drm_panic_is_enabled(struct drm_device *dev)
> +{
> +	struct drm_plane *plane;
> +
> +	if (!dev->mode_config.num_total_plane)
> +		return false;
> +
> +	drm_for_each_plane(plane, dev)
> +		if (plane->helper_private && plane->helper_private->get_scanout_buffer)
> +			return true;
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_panic_is_enabled);

This feels like overkill since you currently only have one user in the
fbdev emulation code, but maybe useful in some other places ...

> +
>  /**
>   * drm_panic_register() - Initialize DRM panic for a device
>   * @dev: the drm device on which the panic screen will be displayed.
> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> index 73bb3f3d9ed9..c3a358dc3e27 100644
> --- a/include/drm/drm_panic.h
> +++ b/include/drm/drm_panic.h
> @@ -148,11 +148,13 @@ struct drm_scanout_buffer {
>  
>  #ifdef CONFIG_DRM_PANIC
>  
> +bool drm_panic_is_enabled(struct drm_device *dev);

Since it's internal only, this should be in
drivers/gpu/drm/drm_crtc_internal.h and not int he include for drivers.
With that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  void drm_panic_register(struct drm_device *dev);
>  void drm_panic_unregister(struct drm_device *dev);

These two are only used in drm.ko. Can you please move them to
drm_crtc_internal.h too and drop the EXPORT_SYMBOL in a follow-up patch?
We're trying to limit the exported interface and official headers to
really only the pieces drivers actually need.

Thanks, Sima

>  
>  #else
>  
> +bool drm_panic_is_enabled(struct drm_device *dev) {return false; }
>  static inline void drm_panic_register(struct drm_device *dev) {}
>  static inline void drm_panic_unregister(struct drm_device *dev) {}
>  
> -- 
> 2.45.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/3] drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

[Daniel Vetter]

On Wed, Jul 17, 2024 at 12:09:46PM +0200, Javier Martinez Canillas wrote:
> Jocelyn Falempe <jfalempe@redhat.com> writes:
> 
> > Now that fbcon has the skip_panic option, there is no more conflicts
> > between drm_panic and fbcon.
> > Remove the build time dependency, so they can be both enabled.
> >
> > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> > ---
> >  drivers/gpu/drm/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 6dd0016fc9cd..a22cab218004 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -107,7 +107,7 @@ config DRM_KMS_HELPER
> >  
> >  config DRM_PANIC
> >  	bool "Display a user-friendly message when a kernel panic occurs"
> > -	depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE)
> > +	depends on DRM
> 
> This is great. Thanks for finding an alternative approach! I don't see any
> issues this time, because there is no locking involved. But let's see what
> others think about it.

Looks like it should work, I did check the history of fbcon_is_active and
we've used that to force/disable panic output for fbcon in the past. So I
think it's the right tool.

> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Sima
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Jocelyn Falempe]

On 17/07/2024 17:08, Daniel Vetter wrote:
> On Wed, Jul 17, 2024 at 10:48:39AM +0200, Jocelyn Falempe wrote:
>> It allows to check if the drm device supports drm_panic.
>> Prepare the work to have better integration with fbcon and vtconsole.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
>>   include/drm/drm_panic.h     |  2 ++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>> index 948aed00595e..d9a25c2d0a65 100644
>> --- a/drivers/gpu/drm/drm_panic.c
>> +++ b/drivers/gpu/drm/drm_panic.c
>> @@ -703,6 +703,26 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
>>   static void debugfs_register_plane(struct drm_plane *plane, int index) {}
>>   #endif /* CONFIG_DRM_PANIC_DEBUG */
>>   
>> +/**
>> + * drm_panic_is_enabled
>> + * @dev: the drm device that may supports drm_panic
>> + *
>> + * returns true if the drm device supports drm_panic
>> + */
>> +bool drm_panic_is_enabled(struct drm_device *dev)
>> +{
>> +	struct drm_plane *plane;
>> +
>> +	if (!dev->mode_config.num_total_plane)
>> +		return false;
>> +
>> +	drm_for_each_plane(plane, dev)
>> +		if (plane->helper_private && plane->helper_private->get_scanout_buffer)
>> +			return true;
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(drm_panic_is_enabled);
> 
> This feels like overkill since you currently only have one user in the
> fbdev emulation code, but maybe useful in some other places ...
> 
>> +
>>   /**
>>    * drm_panic_register() - Initialize DRM panic for a device
>>    * @dev: the drm device on which the panic screen will be displayed.
>> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
>> index 73bb3f3d9ed9..c3a358dc3e27 100644
>> --- a/include/drm/drm_panic.h
>> +++ b/include/drm/drm_panic.h
>> @@ -148,11 +148,13 @@ struct drm_scanout_buffer {
>>   
>>   #ifdef CONFIG_DRM_PANIC
>>   
>> +bool drm_panic_is_enabled(struct drm_device *dev);
> 
> Since it's internal only, this should be in
> drivers/gpu/drm/drm_crtc_internal.h and not int he include for drivers.

Yes, that makes sense, drivers won't need that API.

> With that:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>>   void drm_panic_register(struct drm_device *dev);
>>   void drm_panic_unregister(struct drm_device *dev);
> 
> These two are only used in drm.ko. Can you please move them to
> drm_crtc_internal.h too and drop the EXPORT_SYMBOL in a follow-up patch?
> We're trying to limit the exported interface and official headers to
> really only the pieces drivers actually need.

Sure, I'll add this to my next drm_panic series.

> 
> Thanks, Sima
> 
>>   
>>   #else
>>   
>> +bool drm_panic_is_enabled(struct drm_device *dev) {return false; }
>>   static inline void drm_panic_register(struct drm_device *dev) {}
>>   static inline void drm_panic_unregister(struct drm_device *dev) {}
>>   
>> -- 
>> 2.45.2
>>
> 

Best regards,

-- 

Jocelyn

Re: [PATCH 2/3] fbcon: Add an option to disable fbcon in panic.

[Jocelyn Falempe]

On 17/07/2024 17:04, Daniel Vetter wrote:
> On Wed, Jul 17, 2024 at 10:48:40AM +0200, Jocelyn Falempe wrote:
>> This is required to avoid conflict between DRM_PANIC, and fbcon. If
>> a drm device already handle panic with drm_panic, it should set
>> the skip_panic field in fb_info, so that fbcon will stay quiet, and
>> not overwrite the panic_screen.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c  | 2 ++
>>   drivers/video/fbdev/core/fbcon.c | 7 ++++++-
>>   include/linux/fb.h               | 1 +
>>   3 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index e2e19f49342e..3662d664d8f9 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -40,6 +40,7 @@
>>   #include <drm/drm_fourcc.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_modeset_helper_vtables.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_print.h>
>>   #include <drm/drm_vblank.h>
>>   
>> @@ -524,6 +525,7 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
>>   	fb_helper->info = info;
>>   	info->skip_vt_switch = true;
>>   
>> +	info->skip_panic = drm_panic_is_enabled(fb_helper->dev);
>>   	return info;
>>   
>>   err_release:
> 
> Bit a bikeshed, but I'd split this patch out since it's for drm's fbdev
> emulation, not the fbcon core code. With that:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Agreed, I considered doing that when writing the patch, but as it was 1 
line, I kept it with the fbcon change.

Thanks,

-- 

Jocelyn

> 

Re: [PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Jocelyn Falempe]

On 18/07/2024 09:04, Jocelyn Falempe wrote:
> 
> 
> On 17/07/2024 17:08, Daniel Vetter wrote:
>> On Wed, Jul 17, 2024 at 10:48:39AM +0200, Jocelyn Falempe wrote:
>>> It allows to check if the drm device supports drm_panic.
>>> Prepare the work to have better integration with fbcon and vtconsole.
>>>
>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>>> ---
>>>   drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
>>>   include/drm/drm_panic.h     |  2 ++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>>> index 948aed00595e..d9a25c2d0a65 100644
>>> --- a/drivers/gpu/drm/drm_panic.c
>>> +++ b/drivers/gpu/drm/drm_panic.c
>>> @@ -703,6 +703,26 @@ static void debugfs_register_plane(struct 
>>> drm_plane *plane, int index)
>>>   static void debugfs_register_plane(struct drm_plane *plane, int 
>>> index) {}
>>>   #endif /* CONFIG_DRM_PANIC_DEBUG */
>>> +/**
>>> + * drm_panic_is_enabled
>>> + * @dev: the drm device that may supports drm_panic
>>> + *
>>> + * returns true if the drm device supports drm_panic
>>> + */
>>> +bool drm_panic_is_enabled(struct drm_device *dev)
>>> +{
>>> +    struct drm_plane *plane;
>>> +
>>> +    if (!dev->mode_config.num_total_plane)
>>> +        return false;
>>> +
>>> +    drm_for_each_plane(plane, dev)
>>> +        if (plane->helper_private && 
>>> plane->helper_private->get_scanout_buffer)
>>> +            return true;
>>> +    return false;
>>> +}
>>> +EXPORT_SYMBOL(drm_panic_is_enabled);
>>
>> This feels like overkill since you currently only have one user in the
>> fbdev emulation code, but maybe useful in some other places ...
>>
>>> +
>>>   /**
>>>    * drm_panic_register() - Initialize DRM panic for a device
>>>    * @dev: the drm device on which the panic screen will be displayed.
>>> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
>>> index 73bb3f3d9ed9..c3a358dc3e27 100644
>>> --- a/include/drm/drm_panic.h
>>> +++ b/include/drm/drm_panic.h
>>> @@ -148,11 +148,13 @@ struct drm_scanout_buffer {
>>>   #ifdef CONFIG_DRM_PANIC
>>> +bool drm_panic_is_enabled(struct drm_device *dev);
>>
>> Since it's internal only, this should be in
>> drivers/gpu/drm/drm_crtc_internal.h and not int he include for drivers.
> 
> Yes, that makes sense, drivers won't need that API.
> 
>> With that:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>>   void drm_panic_register(struct drm_device *dev);
>>>   void drm_panic_unregister(struct drm_device *dev);
>>
>> These two are only used in drm.ko. Can you please move them to
>> drm_crtc_internal.h too and drop the EXPORT_SYMBOL in a follow-up patch?
>> We're trying to limit the exported interface and official headers to
>> really only the pieces drivers actually need.
> 
> Sure, I'll add this to my next drm_panic series.

I think this also applies to drm_panic_init() and drm_panic_exit(), that 
I introduce in my QR code series:
https://patchwork.freedesktop.org/patch/604890/?series=135944&rev=2
I will move them to drm_crtc_internal.h

> 
>>
>> Thanks, Sima
>>
>>>   #else
>>> +bool drm_panic_is_enabled(struct drm_device *dev) {return false; }
>>>   static inline void drm_panic_register(struct drm_device *dev) {}
>>>   static inline void drm_panic_unregister(struct drm_device *dev) {}
>>> -- 
>>> 2.45.2
>>>
>>
> 
> Best regards,
> 

Re: [PATCH 0/3] drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

[Jocelyn Falempe]

On 17/07/2024 10:48, Jocelyn Falempe wrote:
> When proposing to enable DRM_PANIC on Fedora, some users raised concern about the need to disable VT_CONSOLE.
> So this is my new attempt to avoid fbcon/vt_console to overwrite the panic screen.
> This time it doesn't involve any locking, so it should be safe.
> I added a skip_panic option in struct fb_info, and check if this option and the panic_cpu are set in fb_is_inactive(), to prevent any framebuffer operation.
> Also skip_panic is only true if the drm driver supports drm_panic, so you will still get the VT panic info on drivers that don't have drm_panic support yet.
> 

Thanks all,

I've just pushed them to drm-misc-next, with the required changes.
(splitting patch 2 in 2, and moving function prototype to 
drm_crtc_internal.h).

Best regards,

-- 

Jocelyn


> Jocelyn Falempe (3):
>    drm/panic: Add drm_panic_is_enabled()
>    fbcon: Add an option to disable fbcon in panic.
>    drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE
> 
>   drivers/gpu/drm/Kconfig          |  2 +-
>   drivers/gpu/drm/drm_fb_helper.c  |  2 ++
>   drivers/gpu/drm/drm_panic.c      | 20 ++++++++++++++++++++
>   drivers/video/fbdev/core/fbcon.c |  7 ++++++-
>   include/drm/drm_panic.h          |  2 ++
>   include/linux/fb.h               |  1 +
>   6 files changed, 32 insertions(+), 2 deletions(-)
> 
> 
> base-commit: a237f217bad50c381773da5b00442710d1449098

Re: [PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Imre Deak]

Hi,

On Wed, Jul 17, 2024 at 10:48:39AM +0200, Jocelyn Falempe wrote:
> It allows to check if the drm device supports drm_panic.
> Prepare the work to have better integration with fbcon and vtconsole.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
>  include/drm/drm_panic.h     |  2 ++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 948aed00595e..d9a25c2d0a65 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -703,6 +703,26 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
>  static void debugfs_register_plane(struct drm_plane *plane, int index) {}
>  #endif /* CONFIG_DRM_PANIC_DEBUG */
>  
> +/**
> + * drm_panic_is_enabled
> + * @dev: the drm device that may supports drm_panic
> + *
> + * returns true if the drm device supports drm_panic
> + */
> +bool drm_panic_is_enabled(struct drm_device *dev)
> +{
> +	struct drm_plane *plane;
> +
> +	if (!dev->mode_config.num_total_plane)
> +		return false;
> +
> +	drm_for_each_plane(plane, dev)
> +		if (plane->helper_private && plane->helper_private->get_scanout_buffer)
> +			return true;
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_panic_is_enabled);
> +
>  /**
>   * drm_panic_register() - Initialize DRM panic for a device
>   * @dev: the drm device on which the panic screen will be displayed.
> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> index 73bb3f3d9ed9..c3a358dc3e27 100644
> --- a/include/drm/drm_panic.h
> +++ b/include/drm/drm_panic.h
> @@ -148,11 +148,13 @@ struct drm_scanout_buffer {
>  
>  #ifdef CONFIG_DRM_PANIC
>  
> +bool drm_panic_is_enabled(struct drm_device *dev);
>  void drm_panic_register(struct drm_device *dev);
>  void drm_panic_unregister(struct drm_device *dev);
>  
>  #else
>  
> +bool drm_panic_is_enabled(struct drm_device *dev) {return false; }

This was moved to drivers/gpu/drm/drm_crtc_internal.h in the applied
version and I can't find that version on the mailing list; imo this kind
of change requires a resend.

Also, the above breaks CONFIG_DRM_PANIC=n builds:

In file included from drivers/gpu/drm/drm_atomic_uapi.c:43:
drivers/gpu/drm/drm_crtc_internal.h:322:6: error: no previous prototype for ‘drm_panic_is_enabled’ [-Werror=missing-prototypes]
  322 | bool drm_panic_is_enabled(struct drm_device *dev) {return false; }

Stubs like the above must be an inline function.

>  static inline void drm_panic_register(struct drm_device *dev) {}
>  static inline void drm_panic_unregister(struct drm_device *dev) {}
>  
> -- 
> 2.45.2
> 

Re: [PATCH 1/3] drm/panic: Add drm_panic_is_enabled()

[Daniel Vetter]

On Thu, Jul 18, 2024 at 11:30:05AM +0200, Jocelyn Falempe wrote:
> 
> 
> On 18/07/2024 09:04, Jocelyn Falempe wrote:
> > 
> > 
> > On 17/07/2024 17:08, Daniel Vetter wrote:
> > > On Wed, Jul 17, 2024 at 10:48:39AM +0200, Jocelyn Falempe wrote:
> > > > It allows to check if the drm device supports drm_panic.
> > > > Prepare the work to have better integration with fbcon and vtconsole.
> > > > 
> > > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> > > > ---
> > > >   drivers/gpu/drm/drm_panic.c | 20 ++++++++++++++++++++
> > > >   include/drm/drm_panic.h     |  2 ++
> > > >   2 files changed, 22 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> > > > index 948aed00595e..d9a25c2d0a65 100644
> > > > --- a/drivers/gpu/drm/drm_panic.c
> > > > +++ b/drivers/gpu/drm/drm_panic.c
> > > > @@ -703,6 +703,26 @@ static void debugfs_register_plane(struct
> > > > drm_plane *plane, int index)
> > > >   static void debugfs_register_plane(struct drm_plane *plane,
> > > > int index) {}
> > > >   #endif /* CONFIG_DRM_PANIC_DEBUG */
> > > > +/**
> > > > + * drm_panic_is_enabled
> > > > + * @dev: the drm device that may supports drm_panic
> > > > + *
> > > > + * returns true if the drm device supports drm_panic
> > > > + */
> > > > +bool drm_panic_is_enabled(struct drm_device *dev)
> > > > +{
> > > > +    struct drm_plane *plane;
> > > > +
> > > > +    if (!dev->mode_config.num_total_plane)
> > > > +        return false;
> > > > +
> > > > +    drm_for_each_plane(plane, dev)
> > > > +        if (plane->helper_private &&
> > > > plane->helper_private->get_scanout_buffer)
> > > > +            return true;
> > > > +    return false;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_panic_is_enabled);
> > > 
> > > This feels like overkill since you currently only have one user in the
> > > fbdev emulation code, but maybe useful in some other places ...
> > > 
> > > > +
> > > >   /**
> > > >    * drm_panic_register() - Initialize DRM panic for a device
> > > >    * @dev: the drm device on which the panic screen will be displayed.
> > > > diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> > > > index 73bb3f3d9ed9..c3a358dc3e27 100644
> > > > --- a/include/drm/drm_panic.h
> > > > +++ b/include/drm/drm_panic.h
> > > > @@ -148,11 +148,13 @@ struct drm_scanout_buffer {
> > > >   #ifdef CONFIG_DRM_PANIC
> > > > +bool drm_panic_is_enabled(struct drm_device *dev);
> > > 
> > > Since it's internal only, this should be in
> > > drivers/gpu/drm/drm_crtc_internal.h and not int he include for drivers.
> > 
> > Yes, that makes sense, drivers won't need that API.
> > 
> > > With that:
> > > 
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > >   void drm_panic_register(struct drm_device *dev);
> > > >   void drm_panic_unregister(struct drm_device *dev);
> > > 
> > > These two are only used in drm.ko. Can you please move them to
> > > drm_crtc_internal.h too and drop the EXPORT_SYMBOL in a follow-up patch?
> > > We're trying to limit the exported interface and official headers to
> > > really only the pieces drivers actually need.
> > 
> > Sure, I'll add this to my next drm_panic series.
> 
> I think this also applies to drm_panic_init() and drm_panic_exit(), that I
> introduce in my QR code series:
> https://patchwork.freedesktop.org/patch/604890/?series=135944&rev=2
> I will move them to drm_crtc_internal.h

Yup.
-Sima

> 
> > 
> > > 
> > > Thanks, Sima
> > > 
> > > >   #else
> > > > +bool drm_panic_is_enabled(struct drm_device *dev) {return false; }
> > > >   static inline void drm_panic_register(struct drm_device *dev) {}
> > > >   static inline void drm_panic_unregister(struct drm_device *dev) {}
> > > > -- 
> > > > 2.45.2
> > > > 
> > > 
> > 
> > Best regards,
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch