drm/panic: Remove build time dependency with FRAMEBUFFER_CONSOLE

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

Posted by Jocelyn Falempe 1 year, 5 months ago

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

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

Posted by Imre Deak 1 year, 5 months ago

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()

Posted by Daniel Vetter 1 year, 5 months ago

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 1/3] drm/panic: Add drm_panic_is_enabled()

Posted by Jocelyn Falempe 1 year, 5 months ago


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 1/3] drm/panic: Add drm_panic_is_enabled()

Posted by Jocelyn Falempe 1 year, 5 months ago


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 1/3] drm/panic: Add drm_panic_is_enabled()

Posted by Daniel Vetter 1 year, 4 months ago

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

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

Posted by Javier Martinez Canillas 1 year, 5 months ago

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