[1/9] drm: Add drm_mode_debug_printmodeline_raw

Commit Message

Tvrtko Ursulin April 27, 2016, 10:06 a.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Purpose is to enable drivers to print out just the mode
string with their own formatting.

Existing drm_mode_debug_printmodeline calls the new
helper and the format is unchanged in this patch.

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: [e-mail]
---
 drivers/gpu/drm/drm_modes.c | 29 ++++++++++++++++++++++-------
 include/drm/drm_modes.h     |  1 +
 2 files changed, 23 insertions(+), 7 deletions(-)

Comments

Jani Nikula April 27, 2016, 12:35 p.m. UTC | #1

On Wed, 27 Apr 2016, Tvrtko Ursulin <[e-mail]> wrote:
> From: Tvrtko Ursulin <[e-mail]>
>
> Purpose is to enable drivers to print out just the mode
> string with their own formatting.

Some alternatives that preserve the use of a single printk(), avoiding
garbled console output due to races (as discussed on intel-gfx in reply
to the cover letter [1]):

1) Simply add a prefix string parameter to use in
   drm_mode_debug_printmodeline(). Really easy and covers most
   needs. Maybe wrap this in a macro to use the caller's function name.

2) Model drm_mode_debug_printmodeline() after drm_ut_debug_printk(),
   adding a mode parameter. Maybe wrap this in a macro to use the
   caller's function name.

3) Add char *drm_mode_line(mode) that kmallocs a mode line string, or a
   drm_mode_line(mode, buf, bufsize) that prints the mode string to a
   statically allocated buffer.

BR,
Jani.


[1] http://mid.gmane.org/[e-mail]

>
> Existing drm_mode_debug_printmodeline calls the new
> helper and the format is unchanged in this patch.
>
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: [e-mail]
> ---
>  drivers/gpu/drm/drm_modes.c | 29 ++++++++++++++++++++++-------
>  include/drm/drm_modes.h     |  1 +
>  2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 7def3d58da18..fd4795e2c8db 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -42,6 +42,24 @@
>  #include "drm_crtc_internal.h"
>  
>  /**
> + * drm_mode_debug_printmodeline_raw - print a mode raw string to dmesg
> + * @mode: mode to print
> + *
> + * Describe @mode using DRM_DEBUG without the loglevel, drm prefix and newline.
> + */
> +void drm_mode_debug_printmodeline_raw(const struct drm_display_mode *mode)
> +{
> +	if (drm_debug & DRM_UT_KMS)
> +		printk("%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x",
> +		       mode->base.id, mode->name, mode->vrefresh, mode->clock,
> +		       mode->hdisplay, mode->hsync_start,
> +		       mode->hsync_end, mode->htotal,
> +		       mode->vdisplay, mode->vsync_start,
> +		       mode->vsync_end, mode->vtotal, mode->type, mode->flags);
> +}
> +EXPORT_SYMBOL(drm_mode_debug_printmodeline_raw);
> +
> +/**
>   * drm_mode_debug_printmodeline - print a mode to dmesg
>   * @mode: mode to print
>   *
> @@ -49,13 +67,10 @@
>   */
>  void drm_mode_debug_printmodeline(const struct drm_display_mode *mode)
>  {
> -	DRM_DEBUG_KMS("Modeline %d:\"%s\" %d %d %d %d %d %d %d %d %d %d "
> -			"0x%x 0x%x\n",
> -		mode->base.id, mode->name, mode->vrefresh, mode->clock,
> -		mode->hdisplay, mode->hsync_start,
> -		mode->hsync_end, mode->htotal,
> -		mode->vdisplay, mode->vsync_start,
> -		mode->vsync_end, mode->vtotal, mode->type, mode->flags);
> +	DRM_DEBUG_KMS("Modeline ");
> +	drm_mode_debug_printmodeline_raw(mode);
> +	if (drm_debug & DRM_UT_KMS)
> +		printk("\n");
>  }
>  EXPORT_SYMBOL(drm_mode_debug_printmodeline);
>  
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 625966a906f2..9bb0cd06b80e 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -437,6 +437,7 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>  			   const struct drm_mode_modeinfo *in);
>  void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
>  void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
> +void drm_mode_debug_printmodeline_raw(const struct drm_display_mode *mode);
>  
>  struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
>  				      int hdisplay, int vdisplay, int vrefresh,

Tvrtko Ursulin April 27, 2016, 12:58 p.m. UTC | #2

On 27/04/16 13:35, Jani Nikula wrote:
> On Wed, 27 Apr 2016, Tvrtko Ursulin <[e-mail]> wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> Purpose is to enable drivers to print out just the mode
>> string with their own formatting.
>
> Some alternatives that preserve the use of a single printk(), avoiding
> garbled console output due to races (as discussed on intel-gfx in reply
> to the cover letter [1]):
>
> 1) Simply add a prefix string parameter to use in
>     drm_mode_debug_printmodeline(). Really easy and covers most
>     needs. Maybe wrap this in a macro to use the caller's function name.
>
> 2) Model drm_mode_debug_printmodeline() after drm_ut_debug_printk(),
>     adding a mode parameter. Maybe wrap this in a macro to use the
>     caller's function name.

This sounds good to me...

> 3) Add char *drm_mode_line(mode) that kmallocs a mode line string, or a
>     drm_mode_line(mode, buf, bufsize) that prints the mode string to a
>     statically allocated buffer.

...but it only solved the modeline part of the story. Unless something 
like 3), which I specifically wanted to avoid. String handling etc.. 
best to be avoided in general if possible and more so for debug code 
only. Any potential bug in those is best avoided if they do not exist. 
And some of them log external input so even more so.

Something like debug_start/debug_print/debug_end would solve all that 
but that would be bigger and core.

I'll try to do 2) and see what to do with the rest...

Regards,

Tvrtko

Patch

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 7def3d58da18..fd4795e2c8db 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -42,6 +42,24 @@ 
 #include "drm_crtc_internal.h"
 
 /**
+ * drm_mode_debug_printmodeline_raw - print a mode raw string to dmesg
+ * @mode: mode to print
+ *
+ * Describe @mode using DRM_DEBUG without the loglevel, drm prefix and newline.
+ */
+void drm_mode_debug_printmodeline_raw(const struct drm_display_mode *mode)
+{
+	if (drm_debug & DRM_UT_KMS)
+		printk("%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x",
+		       mode->base.id, mode->name, mode->vrefresh, mode->clock,
+		       mode->hdisplay, mode->hsync_start,
+		       mode->hsync_end, mode->htotal,
+		       mode->vdisplay, mode->vsync_start,
+		       mode->vsync_end, mode->vtotal, mode->type, mode->flags);
+}
+EXPORT_SYMBOL(drm_mode_debug_printmodeline_raw);
+
+/**
  * drm_mode_debug_printmodeline - print a mode to dmesg
  * @mode: mode to print
  *
@@ -49,13 +67,10 @@ 
  */
 void drm_mode_debug_printmodeline(const struct drm_display_mode *mode)
 {
-	DRM_DEBUG_KMS("Modeline %d:\"%s\" %d %d %d %d %d %d %d %d %d %d "
-			"0x%x 0x%x\n",
-		mode->base.id, mode->name, mode->vrefresh, mode->clock,
-		mode->hdisplay, mode->hsync_start,
-		mode->hsync_end, mode->htotal,
-		mode->vdisplay, mode->vsync_start,
-		mode->vsync_end, mode->vtotal, mode->type, mode->flags);
+	DRM_DEBUG_KMS("Modeline ");
+	drm_mode_debug_printmodeline_raw(mode);
+	if (drm_debug & DRM_UT_KMS)
+		printk("\n");
 }
 EXPORT_SYMBOL(drm_mode_debug_printmodeline);
 
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 625966a906f2..9bb0cd06b80e 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -437,6 +437,7 @@  int drm_mode_convert_umode(struct drm_display_mode *out,
 			   const struct drm_mode_modeinfo *in);
 void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
+void drm_mode_debug_printmodeline_raw(const struct drm_display_mode *mode);
 
 struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
 				      int hdisplay, int vdisplay, int vrefresh,