Re: [PATCH] drm/i915/sdvo: Fix up debug output to not split lines

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Hi Daniel,

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> It leads to a big mess when stuff interleaves. Especially with the new
> patch I've submitted for the drm core to no longer artificially split
> up debug messages.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c | 55 ++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index a583e8f718a7..e88ad95df08f 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -413,23 +413,34 @@ static const struct _sdvo_cmd_name {
>  static void intel_sdvo_debug_write(struct intel_sdvo *intel_sdvo, u8 cmd,
>  				   const void *args, int args_len)
>  {
> -	int i;
> +	int i, pos = 0;
> +#define BUF_LEN 256
> +	char buffer[BUF_LEN];
> +
> +#define BUF_PRINT(args...) \
> +	pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args)
> +

You want scnprintf here as it returns the true number of bytes written
instead of what would have been written. Also the size argument includes
the the trailing null space. Consider:

BUG_ON((pos += scnprintf(buffer + pos, max_t(int, BUF_LEN - pos, 0), args)) >= BUF_LEN)

> -	DRM_DEBUG_KMS("%s: W: %02X ",
> -				SDVO_NAME(intel_sdvo), cmd);
> -	for (i = 0; i < args_len; i++)
> -		DRM_LOG_KMS("%02X ", ((u8 *)args)[i]);
> -	for (; i < 8; i++)
> -		DRM_LOG_KMS("   ");
> +	for (i = 0; i < args_len; i++) {
> +		BUF_PRINT("%02X ", ((u8 *)args)[i]);
> +	}
> +	for (; i < 8; i++) {
> +		BUF_PRINT("   ");
> +	}
>  	for (i = 0; i < ARRAY_SIZE(sdvo_cmd_names); i++) {
>  		if (cmd == sdvo_cmd_names[i].cmd) {
> -			DRM_LOG_KMS("(%s)", sdvo_cmd_names[i].name);
> +			BUF_PRINT("(%s)", sdvo_cmd_names[i].name);
>  			break;
>  		}
>  	}
> -	if (i == ARRAY_SIZE(sdvo_cmd_names))
> -		DRM_LOG_KMS("(%02X)", cmd);
> -	DRM_LOG_KMS("\n");
> +	if (i == ARRAY_SIZE(sdvo_cmd_names)) {
> +		BUF_PRINT("(%02X)", cmd);
> +	}
> +	BUG_ON(pos >= BUF_LEN - 1);

pos >= BUF_LEN is enough if you choose not to BUG_ON on each scnprintf

> +#undef BUF_PRINT
> +#undef BUF_LEN
> +
> +	DRM_DEBUG_KMS("%s: W: %02X %s\n", SDVO_NAME(intel_sdvo), cmd, buffer);
>  }
>  
>  static const char *cmd_status_names[] = {
> @@ -512,9 +523,10 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>  {
>  	u8 retry = 15; /* 5 quick checks, followed by 10 long checks */
>  	u8 status;
> -	int i;
> +	int i, pos = 0;
> +#define BUF_LEN 256
> +	char buffer[BUF_LEN];
>  
> -	DRM_DEBUG_KMS("%s: R: ", SDVO_NAME(intel_sdvo));
>  
>  	/*
>  	 * The documentation states that all commands will be
> @@ -551,10 +563,13 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>  			goto log_fail;
>  	}
>  
> +#define BUF_PRINT(args...) \
> +	pos += snprintf(buffer + pos, max_t(int, BUF_LEN - pos - 1, 0), args)
> +
^^ Ditto

>  	if (status <= SDVO_CMD_STATUS_SCALING_NOT_SUPP)
> -		DRM_LOG_KMS("(%s)", cmd_status_names[status]);
> +		BUF_PRINT("(%s)", cmd_status_names[status]);
>  	else
> -		DRM_LOG_KMS("(??? %d)", status);
> +		BUF_PRINT("(??? %d)", status);
>  
>  	if (status != SDVO_CMD_STATUS_SUCCESS)
>  		goto log_fail;
> @@ -565,13 +580,17 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>  					  SDVO_I2C_RETURN_0 + i,
>  					  &((u8 *)response)[i]))
>  			goto log_fail;
> -		DRM_LOG_KMS(" %02X", ((u8 *)response)[i]);
> +		BUF_PRINT(" %02X", ((u8 *)response)[i]);
>  	}
> -	DRM_LOG_KMS("\n");
> +	BUG_ON(pos >= BUF_LEN - 1);
> +#undef BUF_PRINT
> +#undef BUF_LEN
> +
> +	DRM_DEBUG_KMS("%s: R: %s\n", SDVO_NAME(intel_sdvo), buffer);
>  	return true;
>  
>  log_fail:
> -	DRM_LOG_KMS("... failed\n");
> +	DRM_DEBUG_KMS("%s: R: ... failed\n", SDVO_NAME(intel_sdvo));
>  	return false;
>  }
>  
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx