[PATCH 1/9] drm: Add drm_mode_debug_printmodeline_raw

[PATCH 1/9] drm: Add drm_mode_debug_printmodeline_raw

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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 <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
 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,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm: Add drm_mode_debug_printmodeline_raw

[Jani Nikula]

On Wed, 27 Apr 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> 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/87fuu7jp8x.fsf@intel.com

>
> Existing drm_mode_debug_printmodeline calls the new
> helper and the format is unchanged in this patch.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  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,

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm: Add drm_mode_debug_printmodeline_raw

[Tvrtko Ursulin]

On 27/04/16 13:35, Jani Nikula wrote:
> On Wed, 27 Apr 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/9] drm: Recognize invalid (all-zero) modes in drm_mode_debug_printmodeline(_raw)

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Instead of a long string full of zeros in various forms
print out "---" for more tidy appearace in the logs.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_modes.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index fd4795e2c8db..da9012dc4bba 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -49,13 +49,26 @@
  */
 void drm_mode_debug_printmodeline_raw(const struct drm_display_mode *mode)
 {
-	if (drm_debug & DRM_UT_KMS)
+	if (!(drm_debug & DRM_UT_KMS))
+		return;
+
+	if (mode->base.id == 0 &&
+	    (mode->name == NULL || mode->name[0] == 0) &&
+	    mode->vrefresh == 0 && mode->clock == 0 &&
+	    mode->hdisplay == 0 && mode->hsync_start == 0 &&
+	    mode->hsync_end == 0 && mode->htotal == 0 &&
+	    mode->vdisplay == 0 && mode->vsync_start == 0 &&
+	    mode->vsync_end == 0 && mode->vtotal == 0 &&
+	    mode->type == 0 && mode->flags == 0) {
+		printk("---");
+	} else {
 		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);
+			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);
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 9/9] drm: Quiet down drm_mode_getconnector

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Debug logging in this function does not provide any information
apart that the userspace is calling an ioctl on the connector.

There is not any info on the connector provided at all and
since there are other ioctls userspace typically calls which
do log useful things about the same connectors, remove this
one to make things a little bit more readable when KMS debugging
is turned on.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_crtc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4e5b015a5e3a..ffb01d91ae32 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2143,8 +2143,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 
 	memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
-
 	mutex_lock(&dev->mode_config.mutex);
 
 	connector = drm_connector_find(dev, out_resp->connector_id);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 9/9] drm: Quiet down drm_mode_getconnector

[Daniel Vetter]

On Wed, Apr 27, 2016 at 11:07:02AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Debug logging in this function does not provide any information
> apart that the userspace is calling an ioctl on the connector.
> 
> There is not any info on the connector provided at all and
> since there are other ioctls userspace typically calls which
> do log useful things about the same connectors, remove this
> one to make things a little bit more readable when KMS debugging
> is turned on.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org

Yeah this seems over the top debug noise. If we want this, we could easily
add a generic debugfs file, with a lot more info on top.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4e5b015a5e3a..ffb01d91ae32 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2143,8 +2143,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  
>  	memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
>  
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
> -
>  	mutex_lock(&dev->mode_config.mutex);
>  
>  	connector = drm_connector_find(dev, out_resp->connector_id);
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel