[PATCH 0/2] drm: Message logging cleanups

[PATCH 0/2] drm: Message logging cleanups

[Joe Perches]

Reduce the text space required and verify debug arguments.

Joe Perches (2):
  drm: Create and use drm_err
  drm: Verify debug message arguments

 drivers/gpu/drm/drm_irq.c               |    9 +++++----
 drivers/gpu/drm/drm_stub.c              |   21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.c       |    6 +++---
 drivers/gpu/drm/i915/intel_display.c    |    8 ++++----
 drivers/gpu/drm/radeon/radeon_display.c |    5 +++--
 include/drm/drmP.h                      |   24 +++++++++---------------
 6 files changed, 45 insertions(+), 28 deletions(-)

-- 
1.7.4.2.g597a6.dirty

[PATCH 1/2] drm: Create and use drm_err

[Joe Perches]

Reduce drm text size ~1% by using drm_err and
printf extension %pV to emit error messages.

Remove unused macro DRM_MEM_ERROR.

$ size drivers/gpu/drm/built-in.o*
   text	   data	    bss	    dec	    hex	filename
 361159	   9663	    256	 371078	  5a986	drivers/gpu/drm/built-in.o.new
 365416	   9663	    256	 375335	  5ba27	drivers/gpu/drm/built-in.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/drm_stub.c |   21 +++++++++++++++++++++
 include/drm/drmP.h         |   21 +++++++--------------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 001273d..6d7b083 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -62,6 +62,26 @@ struct idr drm_minors_idr;
 struct class *drm_class;
 struct proc_dir_entry *drm_proc_root;
 struct dentry *drm_debugfs_root;
+
+int drm_err(const char *func, const char *format, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	int r;
+
+	va_start(args, format);
+
+	vaf.fmt = format;
+	vaf.va = &args;
+
+	r = printk(KERN_ERR "[" DRM_NAME ":%s] *ERROR* %pV", func, &vaf);
+
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(drm_err);
+
 void drm_ut_debug_printk(unsigned int request_level,
 			 const char *prefix,
 			 const char *function_name,
@@ -78,6 +98,7 @@ void drm_ut_debug_printk(unsigned int request_level,
 	}
 }
 EXPORT_SYMBOL(drm_ut_debug_printk);
+
 static int drm_minor_get_id(struct drm_device *dev, int type)
 {
 	int new_id;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 202424d..22db51d 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -126,6 +126,9 @@ extern void drm_ut_debug_printk(unsigned int request_level,
 				const char *prefix,
 				const char *function_name,
 				const char *format, ...);
+extern __attribute__((format (printf, 2, 3)))
+int drm_err(const char *func, const char *format, ...);
+
 /***********************************************************************/
 /** \name DRM template customization defaults */
 /*@{*/
@@ -181,21 +184,11 @@ extern void drm_ut_debug_printk(unsigned int request_level,
  * \param fmt printf() like format string.
  * \param arg arguments
  */
-#define DRM_ERROR(fmt, arg...) \
-	printk(KERN_ERR "[" DRM_NAME ":%s] *ERROR* " fmt , __func__ , ##arg)
-
-/**
- * Memory error output.
- *
- * \param area memory area where the error occurred.
- * \param fmt printf() like format string.
- * \param arg arguments
- */
-#define DRM_MEM_ERROR(area, fmt, arg...) \
-	printk(KERN_ERR "[" DRM_NAME ":%s:%s] *ERROR* " fmt , __func__, \
-	       drm_mem_stats[area].name , ##arg)
+#define DRM_ERROR(fmt, ...)				\
+	drm_err(__func__, fmt, ##__VA_ARGS__)
 
-#define DRM_INFO(fmt, arg...)  printk(KERN_INFO "[" DRM_NAME "] " fmt , ##arg)
+#define DRM_INFO(fmt, ...)				\
+	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
 
 /**
  * Debug output.
-- 
1.7.4.2.g597a6.dirty

[PATCH 2/2] drm: Verify debug message arguments

[Joe Perches]

Add __attribute__((format (printf, 4, 5))) to drm_ut_debug_printk
and fix fallout.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/drm_irq.c               |    9 +++++----
 drivers/gpu/drm/i915/intel_bios.c       |    6 +++---
 drivers/gpu/drm/i915/intel_display.c    |    8 ++++----
 drivers/gpu/drm/radeon/radeon_display.c |    5 +++--
 include/drm/drmP.h                      |    3 ++-
 5 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 741457b..62ced75 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -684,10 +684,11 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 	 */
 	*vblank_time = ns_to_timeval(timeval_to_ns(&raw_time) - delta_ns);
 
-	DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %d.%d -> %d.%d [e %d us, %d rep]\n",
-		  crtc, (int) vbl_status, hpos, vpos, raw_time.tv_sec,
-		  raw_time.tv_usec, vblank_time->tv_sec, vblank_time->tv_usec,
-		  (int) duration_ns/1000, i);
+	DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
+		  crtc, (int)vbl_status, hpos, vpos,
+		  (long)raw_time.tv_sec, (long)raw_time.tv_usec,
+		  (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
+		  (int)duration_ns/1000, i);
 
 	vbl_status = DRM_VBLANKTIME_SCANOUTPOS_METHOD;
 	if (invbl)
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index fb5b4d4..927442a 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -214,9 +214,9 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	    i915_lvds_downclock) {
 		dev_priv->lvds_downclock_avail = 1;
 		dev_priv->lvds_downclock = temp_downclock;
-		DRM_DEBUG_KMS("LVDS downclock is found in VBT. ",
-				"Normal Clock %dKHz, downclock %dKHz\n",
-				temp_downclock, panel_fixed_mode->clock);
+		DRM_DEBUG_KMS("LVDS downclock is found in VBT. "
+			      "Normal Clock %dKHz, downclock %dKHz\n",
+			      temp_downclock, panel_fixed_mode->clock);
 	}
 	return;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 432fc04..63bc2af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3497,11 +3497,11 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
 		1000;
 	entries_required = DIV_ROUND_UP(entries_required, wm->cacheline_size);
 
-	DRM_DEBUG_KMS("FIFO entries required for mode: %d\n", entries_required);
+	DRM_DEBUG_KMS("FIFO entries required for mode: %ld\n", entries_required);
 
 	wm_size = fifo_size - (entries_required + wm->guard_size);
 
-	DRM_DEBUG_KMS("FIFO watermark level: %d\n", wm_size);
+	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
 
 	/* Don't promote wm_size to unsigned... */
 	if (wm_size > (long)wm->max_wm)
@@ -3820,13 +3820,13 @@ static bool g4x_check_srwm(struct drm_device *dev,
 		      display_wm, cursor_wm);
 
 	if (display_wm > display->max_wm) {
-		DRM_DEBUG_KMS("display watermark is too large(%d), disabling\n",
+		DRM_DEBUG_KMS("display watermark is too large(%d/%ld), disabling\n",
 			      display_wm, display->max_wm);
 		return false;
 	}
 
 	if (cursor_wm > cursor->max_wm) {
-		DRM_DEBUG_KMS("cursor watermark is too large(%d), disabling\n",
+		DRM_DEBUG_KMS("cursor watermark is too large(%d/%ld), disabling\n",
 			      cursor_wm, cursor->max_wm);
 		return false;
 	}
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index bdbab5c..0671934 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1087,8 +1087,9 @@ void radeon_compute_pll_legacy(struct radeon_pll *pll,
 	*frac_fb_div_p = best_frac_feedback_div;
 	*ref_div_p = best_ref_div;
 	*post_div_p = best_post_div;
-	DRM_DEBUG_KMS("%d %d, pll dividers - fb: %d.%d ref: %d, post %d\n",
-		      freq, best_freq / 1000, best_feedback_div, best_frac_feedback_div,
+	DRM_DEBUG_KMS("%lld %d, pll dividers - fb: %d.%d ref: %d, post %d\n",
+		      (long long)freq,
+		      best_freq / 1000, best_feedback_div, best_frac_feedback_div,
 		      best_ref_div, best_post_div);
 
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 22db51d..4ab866e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -122,7 +122,8 @@ struct drm_device;
  * using the DRM_DEBUG_KMS and DRM_DEBUG.
  */
 
-extern void drm_ut_debug_printk(unsigned int request_level,
+extern __attribute__((format (printf, 4, 5)))
+void drm_ut_debug_printk(unsigned int request_level,
 				const char *prefix,
 				const char *function_name,
 				const char *format, ...);
-- 
1.7.4.2.g597a6.dirty

Re: [PATCH 1/2] drm: Create and use drm_err

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/17/2011 08:35 PM, Joe Perches wrote:
> Reduce drm text size ~1% by using drm_err and
> printf extension %pV to emit error messages.
> 
> Remove unused macro DRM_MEM_ERROR.
> 
> $ size drivers/gpu/drm/built-in.o*
>    text	   data	    bss	    dec	    hex	filename
>  361159	   9663	    256	 371078	  5a986	drivers/gpu/drm/built-in.o.new
>  365416	   9663	    256	 375335	  5ba27	drivers/gpu/drm/built-in.o.old
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/drm_stub.c |   21 +++++++++++++++++++++
>  include/drm/drmP.h         |   21 +++++++--------------
>  2 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 001273d..6d7b083 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -62,6 +62,26 @@ struct idr drm_minors_idr;
>  struct class *drm_class;
>  struct proc_dir_entry *drm_proc_root;
>  struct dentry *drm_debugfs_root;
> +
> +int drm_err(const char *func, const char *format, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;
> +	int r;
> +
> +	va_start(args, format);
> +
> +	vaf.fmt = format;
> +	vaf.va = &args;
> +
> +	r = printk(KERN_ERR "[" DRM_NAME ":%s] *ERROR* %pV", func, &vaf);

This code has ben reorganized a lot over the years, so this comment may
be bogus.  However...

I believe that DRM_NAME is a define, and drm_stub.c is in common code.
As a result, won't this change cause something different to get logged?

> +
> +	va_end(args);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(drm_err);
> +
>  void drm_ut_debug_printk(unsigned int request_level,
>  			 const char *prefix,
>  			 const char *function_name,
> @@ -78,6 +98,7 @@ void drm_ut_debug_printk(unsigned int request_level,
>  	}
>  }
>  EXPORT_SYMBOL(drm_ut_debug_printk);
> +
>  static int drm_minor_get_id(struct drm_device *dev, int type)
>  {
>  	int new_id;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 202424d..22db51d 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -126,6 +126,9 @@ extern void drm_ut_debug_printk(unsigned int request_level,
>  				const char *prefix,
>  				const char *function_name,
>  				const char *format, ...);
> +extern __attribute__((format (printf, 2, 3)))
> +int drm_err(const char *func, const char *format, ...);
> +
>  /***********************************************************************/
>  /** \name DRM template customization defaults */
>  /*@{*/
> @@ -181,21 +184,11 @@ extern void drm_ut_debug_printk(unsigned int request_level,
>   * \param fmt printf() like format string.
>   * \param arg arguments
>   */
> -#define DRM_ERROR(fmt, arg...) \
> -	printk(KERN_ERR "[" DRM_NAME ":%s] *ERROR* " fmt , __func__ , ##arg)
> -
> -/**
> - * Memory error output.
> - *
> - * \param area memory area where the error occurred.
> - * \param fmt printf() like format string.
> - * \param arg arguments
> - */
> -#define DRM_MEM_ERROR(area, fmt, arg...) \
> -	printk(KERN_ERR "[" DRM_NAME ":%s:%s] *ERROR* " fmt , __func__, \
> -	       drm_mem_stats[area].name , ##arg)
> +#define DRM_ERROR(fmt, ...)				\
> +	drm_err(__func__, fmt, ##__VA_ARGS__)
>  
> -#define DRM_INFO(fmt, arg...)  printk(KERN_INFO "[" DRM_NAME "] " fmt , ##arg)
> +#define DRM_INFO(fmt, ...)				\
> +	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
>  
>  /**
>   * Debug output.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2swbEACgkQX1gOwKyEAw+w/ACfd2QMJWOQU5f9sgavPXkfPPfW
GygAn2D8bjKkIV8wOXYC1fOI9w4DPWHj
=q3FM
-----END PGP SIGNATURE-----

Re: [PATCH 1/2] drm: Create and use drm_err

[Dave Airlie]

On Mon, 2011-04-18 at 15:56 -0700, Ian Romanick wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/17/2011 08:35 PM, Joe Perches wrote:
> > Reduce drm text size ~1% by using drm_err and
> > printf extension %pV to emit error messages.
> > 
> > Remove unused macro DRM_MEM_ERROR.
> > 
> > $ size drivers/gpu/drm/built-in.o*
> >    text	   data	    bss	    dec	    hex	filename
> >  361159	   9663	    256	 371078	  5a986	drivers/gpu/drm/built-in.o.new
> >  365416	   9663	    256	 375335	  5ba27	drivers/gpu/drm/built-in.o.old
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  drivers/gpu/drm/drm_stub.c |   21 +++++++++++++++++++++
> >  include/drm/drmP.h         |   21 +++++++--------------
> >  2 files changed, 28 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> > index 001273d..6d7b083 100644
> > --- a/drivers/gpu/drm/drm_stub.c
> > +++ b/drivers/gpu/drm/drm_stub.c
> > @@ -62,6 +62,26 @@ struct idr drm_minors_idr;
> >  struct class *drm_class;
> >  struct proc_dir_entry *drm_proc_root;
> >  struct dentry *drm_debugfs_root;
> > +
> > +int drm_err(const char *func, const char *format, ...)
> > +{
> > +	struct va_format vaf;
> > +	va_list args;
> > +	int r;
> > +
> > +	va_start(args, format);
> > +
> > +	vaf.fmt = format;
> > +	vaf.va = &args;
> > +
> > +	r = printk(KERN_ERR "[" DRM_NAME ":%s] *ERROR* %pV", func, &vaf);
> 
> This code has ben reorganized a lot over the years, so this comment may
> be bogus.  However...
> 
> I believe that DRM_NAME is a define, and drm_stub.c is in common code.
> As a result, won't this change cause something different to get logged?

No I think we hacked that out a long time past thankfully, so its always
"drm" now.

Dave.

Re: [PATCH 2/2] drm: Verify debug message arguments

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/17/2011 08:35 PM, Joe Perches wrote:
> Add __attribute__((format (printf, 4, 5))) to drm_ut_debug_printk
> and fix fallout.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Aside from the comment below about intel_bios.c,

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

especially the changes in intel_display.c.  yikes...

> ---
>  drivers/gpu/drm/drm_irq.c               |    9 +++++----
>  drivers/gpu/drm/i915/intel_bios.c       |    6 +++---
>  drivers/gpu/drm/i915/intel_display.c    |    8 ++++----
>  drivers/gpu/drm/radeon/radeon_display.c |    5 +++--
>  include/drm/drmP.h                      |    3 ++-
>  5 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 741457b..62ced75 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -684,10 +684,11 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>  	 */
>  	*vblank_time = ns_to_timeval(timeval_to_ns(&raw_time) - delta_ns);
>  
> -	DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %d.%d -> %d.%d [e %d us, %d rep]\n",
> -		  crtc, (int) vbl_status, hpos, vpos, raw_time.tv_sec,
> -		  raw_time.tv_usec, vblank_time->tv_sec, vblank_time->tv_usec,
> -		  (int) duration_ns/1000, i);
> +	DRM_DEBUG("crtc %d : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n",
> +		  crtc, (int)vbl_status, hpos, vpos,
> +		  (long)raw_time.tv_sec, (long)raw_time.tv_usec,
> +		  (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
> +		  (int)duration_ns/1000, i);
>  
>  	vbl_status = DRM_VBLANKTIME_SCANOUTPOS_METHOD;
>  	if (invbl)
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index fb5b4d4..927442a 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -214,9 +214,9 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  	    i915_lvds_downclock) {
>  		dev_priv->lvds_downclock_avail = 1;
>  		dev_priv->lvds_downclock = temp_downclock;
> -		DRM_DEBUG_KMS("LVDS downclock is found in VBT. ",
> -				"Normal Clock %dKHz, downclock %dKHz\n",
> -				temp_downclock, panel_fixed_mode->clock);
> +		DRM_DEBUG_KMS("LVDS downclock is found in VBT. "
> +			      "Normal Clock %dKHz, downclock %dKHz\n",
> +			      temp_downclock, panel_fixed_mode->clock);
>  	}
>  	return;
>  }

Does this hunk only change white space, or am I missing something?

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 432fc04..63bc2af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3497,11 +3497,11 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
>  		1000;
>  	entries_required = DIV_ROUND_UP(entries_required, wm->cacheline_size);
>  
> -	DRM_DEBUG_KMS("FIFO entries required for mode: %d\n", entries_required);
> +	DRM_DEBUG_KMS("FIFO entries required for mode: %ld\n", entries_required);
>  
>  	wm_size = fifo_size - (entries_required + wm->guard_size);
>  
> -	DRM_DEBUG_KMS("FIFO watermark level: %d\n", wm_size);
> +	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
>  
>  	/* Don't promote wm_size to unsigned... */
>  	if (wm_size > (long)wm->max_wm)
> @@ -3820,13 +3820,13 @@ static bool g4x_check_srwm(struct drm_device *dev,
>  		      display_wm, cursor_wm);
>  
>  	if (display_wm > display->max_wm) {
> -		DRM_DEBUG_KMS("display watermark is too large(%d), disabling\n",
> +		DRM_DEBUG_KMS("display watermark is too large(%d/%ld), disabling\n",
>  			      display_wm, display->max_wm);
>  		return false;
>  	}
>  
>  	if (cursor_wm > cursor->max_wm) {
> -		DRM_DEBUG_KMS("cursor watermark is too large(%d), disabling\n",
> +		DRM_DEBUG_KMS("cursor watermark is too large(%d/%ld), disabling\n",
>  			      cursor_wm, cursor->max_wm);
>  		return false;
>  	}
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index bdbab5c..0671934 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -1087,8 +1087,9 @@ void radeon_compute_pll_legacy(struct radeon_pll *pll,
>  	*frac_fb_div_p = best_frac_feedback_div;
>  	*ref_div_p = best_ref_div;
>  	*post_div_p = best_post_div;
> -	DRM_DEBUG_KMS("%d %d, pll dividers - fb: %d.%d ref: %d, post %d\n",
> -		      freq, best_freq / 1000, best_feedback_div, best_frac_feedback_div,
> +	DRM_DEBUG_KMS("%lld %d, pll dividers - fb: %d.%d ref: %d, post %d\n",
> +		      (long long)freq,
> +		      best_freq / 1000, best_feedback_div, best_frac_feedback_div,
>  		      best_ref_div, best_post_div);
>  
>  }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 22db51d..4ab866e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -122,7 +122,8 @@ struct drm_device;
>   * using the DRM_DEBUG_KMS and DRM_DEBUG.
>   */
>  
> -extern void drm_ut_debug_printk(unsigned int request_level,
> +extern __attribute__((format (printf, 4, 5)))
> +void drm_ut_debug_printk(unsigned int request_level,
>  				const char *prefix,
>  				const char *function_name,
>  				const char *format, ...);
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2swrgACgkQX1gOwKyEAw+b8wCfdNqYUGinwLaXk7PeNNu8ExXe
39EAoIWQPkZ3z8Au22tTM3cXOQlIuAH4
=4RWh
-----END PGP SIGNATURE-----

Re: [PATCH 1/2] drm: Create and use drm_err

[Joe Perches]

On Mon, 2011-04-18 at 15:56 -0700, Ian Romanick wrote:
> I believe that DRM_NAME is a define, and drm_stub.c is in common code.
> As a result, won't this change cause something different to get logged?

Nope.  DRM_NAME is #defined only once.

include/drm/drm.h:#define DRM_NAME	"drm"	

The only real reason to do this is the "*ERROR*"
used in:

> > -#define DRM_ERROR(fmt, arg...) \
> > -	printk(KERN_ERR "[" DRM_NAME ":%s] *ERROR* " fmt , __func__ , ##arg)

It'd be more or less similar to use:

#define DRM_ERROR(fmt, ...)	\
	pr_err("%s: " fmt, __func__, ##__VA_ARGS__)

or just convert DRM_ERROR to pr_err and DRM_INFO to pr_info.

Re: [PATCH 2/2] drm: Verify debug message arguments

[Joe Perches]

On Mon, 2011-04-18 at 16:01 -0700, Ian Romanick wrote:
> > @@ -214,9 +214,9 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
> >  	    i915_lvds_downclock) {
> >  		dev_priv->lvds_downclock_avail = 1;
> >  		dev_priv->lvds_downclock = temp_downclock;
> > -		DRM_DEBUG_KMS("LVDS downclock is found in VBT. ",
> > -				"Normal Clock %dKHz, downclock %dKHz\n",
> > -				temp_downclock, panel_fixed_mode->clock);
> > +		DRM_DEBUG_KMS("LVDS downclock is found in VBT. "
> > +			      "Normal Clock %dKHz, downclock %dKHz\n",
> > +			      temp_downclock, panel_fixed_mode->clock);
> >  	}
> >  	return;
> >  }
> Does this hunk only change white space, or am I missing something?

No, you're right.  It's just whitespace.
I prefer arguments aligned to open paren.

Re: [PATCH 2/2] drm: Verify debug message arguments

[Marcin Slusarz]

On Mon, Apr 18, 2011 at 04:09:11PM -0700, Joe Perches wrote:
> On Mon, 2011-04-18 at 16:01 -0700, Ian Romanick wrote:
> > > @@ -214,9 +214,9 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
> > >  	    i915_lvds_downclock) {
> > >  		dev_priv->lvds_downclock_avail = 1;
> > >  		dev_priv->lvds_downclock = temp_downclock;
> > > -		DRM_DEBUG_KMS("LVDS downclock is found in VBT. ",
> > > -				"Normal Clock %dKHz, downclock %dKHz\n",
> > > -				temp_downclock, panel_fixed_mode->clock);
> > > +		DRM_DEBUG_KMS("LVDS downclock is found in VBT. "
> > > +			      "Normal Clock %dKHz, downclock %dKHz\n",
> > > +			      temp_downclock, panel_fixed_mode->clock);
> > >  	}
> > >  	return;
> > >  }
> > Does this hunk only change white space, or am I missing something?
> 
> No, you're right.  It's just whitespace.
> I prefer arguments aligned to open paren.

It's not just whitespace. Look at the end of first line.

Marcin

Re: [PATCH 2/2] drm: Verify debug message arguments

[Joe Perches]

On Tue, 2011-04-19 at 18:26 +0200, Marcin Slusarz wrote:
> On Mon, Apr 18, 2011 at 04:09:11PM -0700, Joe Perches wrote:
> > On Mon, 2011-04-18 at 16:01 -0700, Ian Romanick wrote:
> > > > -		DRM_DEBUG_KMS("LVDS downclock is found in VBT. ",
> > > > -				"Normal Clock %dKHz, downclock %dKHz\n",
> > > > -				temp_downclock, panel_fixed_mode->clock);
> > > > +		DRM_DEBUG_KMS("LVDS downclock is found in VBT. "
> > > > +			      "Normal Clock %dKHz, downclock %dKHz\n",
> > > > +			      temp_downclock, panel_fixed_mode->clock);
> > > Does this hunk only change white space, or am I missing something?
> > No, you're right.  It's just whitespace.
> > I prefer arguments aligned to open paren.
> It's not just whitespace. Look at the end of first line.

Heh.  Thanks Marcin.

Teach me to just at the code in the patch.

It's a format error as the first quoted string has
a comma at the end so the arguments after the comma
were ignored.