[CI-RESEND 1/3] drm: extra printk() wrapper macros

[CI-RESEND 1/3] drm: extra printk() wrapper macros

[Dave Gordon]

We had only DRM_INFO() and DRM_ERROR(), whereas the underlying printk()
provides several other useful intermediate levels such as NOTICE and
WARNING. So this patch fills out the set by providing both regular and
once-only macros for each of the levels INFO, NOTICE, and WARNING, using
a common underlying macro that does all the token-pasting.

DRM_ERROR is unchanged, as it's not just a printk wrapper.

v2:
    Fix whitespace, missing ## (Eric Engestrom)

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
---
 include/drm/drmP.h | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d377865..3669cdd 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -162,6 +162,26 @@ void drm_err(const char *format, ...);
 /** \name Macros to make printk easier */
 /*@{*/
 
+#define _DRM_PRINTK(once, level, fmt, ...)				\
+	do {								\
+		printk##once(KERN_##level "[" DRM_NAME "] " fmt,	\
+			     ##__VA_ARGS__);				\
+	} while (0)
+
+#define DRM_INFO(fmt, ...)						\
+	_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTE(fmt, ...)						\
+	_DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
+#define DRM_WARN(fmt, ...)						\
+	_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
+
+#define DRM_INFO_ONCE(fmt, ...)						\
+	_DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
+#define DRM_NOTE_ONCE(fmt, ...)						\
+	_DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
+#define DRM_WARN_ONCE(fmt, ...)						\
+	_DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
+
 /**
  * Error output.
  *
@@ -187,12 +207,6 @@ void drm_err(const char *format, ...);
 		drm_err(fmt, ##__VA_ARGS__);				\
 })
 
-#define DRM_INFO(fmt, ...)				\
-	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
-
-#define DRM_INFO_ONCE(fmt, ...)				\
-	printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
-
 /**
  * Debug output.
  *
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[CI-RESEND 2/3] drm/i915/guc: downgrade some DRM_ERROR() messages to DRM_WARN()

[Dave Gordon]

Where we're going to continue regardless of the problem, rather than
fail, then the message should be a WARNing rather than an ERROR.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..e299b64 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -114,10 +114,8 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
 		if (ret != -ETIMEDOUT)
 			ret = -EIO;
 
-		DRM_ERROR("GUC: host2guc action 0x%X failed. ret=%d "
-				"status=0x%08X response=0x%08X\n",
-				data[0], ret, status,
-				I915_READ(SOFT_SCRATCH(15)));
+		DRM_WARN("Action 0x%X failed; ret=%d status=0x%08X response=0x%08X\n",
+			 data[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
 
 		dev_priv->guc.action_fail += 1;
 		dev_priv->guc.action_err = ret;
@@ -553,8 +551,8 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
 		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
 			break;
 
-		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
-			  db_cmp.cookie, db_ret.cookie);
+		DRM_WARN("Cookie mismatch. Expected %d, found %d\n",
+			 db_cmp.cookie, db_ret.cookie);
 
 		/* update the cookie to newly read cookie from GuC */
 		db_cmp.cookie = db_ret.cookie;
@@ -726,8 +724,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 	/* Restore to original value */
 	err = guc_update_doorbell_id(guc, client, db_id);
 	if (err)
-		DRM_ERROR("Failed to restore doorbell to %d, err %d\n",
-			db_id, err);
+		DRM_WARN("Failed to restore doorbell to %d, err %d\n",
+			 db_id, err);
 
 	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
 		i915_reg_t drbreg = GEN8_DRBREGL(i);
@@ -819,8 +817,6 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
 	return client;
 
 err:
-	DRM_ERROR("FAILED to create priority %u GuC client!\n", priority);
-
 	guc_client_free(dev_priv, client);
 	return NULL;
 }
@@ -998,7 +994,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 				  GUC_CTX_PRIORITY_KMD_NORMAL,
 				  dev_priv->kernel_context);
 	if (!client) {
-		DRM_ERROR("Failed to create execbuf guc_client\n");
+		DRM_ERROR("Failed to create normal GuC client!\n");
 		return -ENOMEM;
 	}
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI-RESEND 2/3] drm/i915/guc: downgrade some DRM_ERROR() messages to DRM_WARN()

[Tvrtko Ursulin]

On 19/07/16 13:20, Dave Gordon wrote:
> Where we're going to continue regardless of the problem, rather than
> fail, then the message should be a WARNing rather than an ERROR.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 2112e02..e299b64 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -114,10 +114,8 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len)
>   		if (ret != -ETIMEDOUT)
>   			ret = -EIO;
>
> -		DRM_ERROR("GUC: host2guc action 0x%X failed. ret=%d "
> -				"status=0x%08X response=0x%08X\n",
> -				data[0], ret, status,
> -				I915_READ(SOFT_SCRATCH(15)));
> +		DRM_WARN("Action 0x%X failed; ret=%d status=0x%08X response=0x%08X\n",
> +			 data[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
>
>   		dev_priv->guc.action_fail += 1;
>   		dev_priv->guc.action_err = ret;
> @@ -553,8 +551,8 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
>   		if (db_ret.db_status == GUC_DOORBELL_DISABLED)
>   			break;
>
> -		DRM_ERROR("Cookie mismatch. Expected %d, returned %d\n",
> -			  db_cmp.cookie, db_ret.cookie);
> +		DRM_WARN("Cookie mismatch. Expected %d, found %d\n",
> +			 db_cmp.cookie, db_ret.cookie);
>
>   		/* update the cookie to newly read cookie from GuC */
>   		db_cmp.cookie = db_ret.cookie;
> @@ -726,8 +724,8 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
>   	/* Restore to original value */
>   	err = guc_update_doorbell_id(guc, client, db_id);
>   	if (err)
> -		DRM_ERROR("Failed to restore doorbell to %d, err %d\n",
> -			db_id, err);
> +		DRM_WARN("Failed to restore doorbell to %d, err %d\n",
> +			 db_id, err);
>
>   	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>   		i915_reg_t drbreg = GEN8_DRBREGL(i);
> @@ -819,8 +817,6 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
>   	return client;
>
>   err:
> -	DRM_ERROR("FAILED to create priority %u GuC client!\n", priority);
> -
>   	guc_client_free(dev_priv, client);
>   	return NULL;
>   }
> @@ -998,7 +994,7 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>   				  GUC_CTX_PRIORITY_KMD_NORMAL,
>   				  dev_priv->kernel_context);
>   	if (!client) {
> -		DRM_ERROR("Failed to create execbuf guc_client\n");
> +		DRM_ERROR("Failed to create normal GuC client!\n");
>   		return -ENOMEM;
>   	}
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[CI-RESEND 3/3] drm/i915/guc: revisit GuC loader message levels

[Dave Gordon]

Some downgraded from DRM_ERROR() to DRM_WARN() or DRM_NOTE(),
a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(),
and one eliminated completely.

v2: different permutation of levels :)

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 34 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 605c696..a2f4fa4 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private *dev_priv)
 
 static u32 get_core_family(struct drm_i915_private *dev_priv)
 {
-	switch (INTEL_INFO(dev_priv)->gen) {
+	u32 gen = INTEL_GEN(dev_priv);
+
+	switch (gen) {
 	case 9:
 		return GFXCORE_FAMILY_GEN9;
 
 	default:
-		DRM_ERROR("GUC: unsupported core family\n");
+		DRM_WARN("GEN%d does not support GuC operation\n", gen);
 		return GFXCORE_FAMILY_UNKNOWN;
 	}
 }
@@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev)
 		goto fail;
 	} else if (*fw_path == '\0') {
 		/* Device has a GuC but we don't know what f/w to load? */
-		DRM_INFO("No GuC firmware known for this platform\n");
+		DRM_WARN("No GuC firmware known for this platform\n");
 		err = -ENODEV;
 		goto fail;
 	}
@@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev)
 		 * that the state and timing are fairly predictable
 		 */
 		err = i915_reset_guc(dev_priv);
-		if (err) {
-			DRM_ERROR("GuC reset failed: %d\n", err);
+		if (err)
 			goto fail;
-		}
 
 		err = guc_ucode_xfer(dev_priv);
 		if (!err)
@@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev)
 	else if (err == 0)
 		DRM_INFO("GuC firmware load skipped\n");
 	else if (ret != -EIO)
-		DRM_INFO("GuC firmware load failed: %d\n", err);
+		DRM_NOTE("GuC firmware load failed: %d\n", err);
 	else
-		DRM_ERROR("GuC firmware load failed: %d\n", err);
+		DRM_WARN("GuC firmware load failed: %d\n", err);
 
 	if (i915.enable_guc_submission) {
 		if (fw_path == NULL)
 			DRM_INFO("GuC submission without firmware not supported\n");
 		if (ret == 0)
-			DRM_INFO("Falling back from GuC submission to execlist mode\n");
+			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
 		else
 			DRM_ERROR("GuC init failed: %d\n", ret);
 	}
@@ -571,7 +571,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	/* Check the size of the blob before examining buffer contents */
 	if (fw->size < sizeof(struct guc_css_header)) {
-		DRM_ERROR("Firmware header is missing\n");
+		DRM_NOTE("Firmware header is missing\n");
 		goto fail;
 	}
 
@@ -583,7 +583,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
 
 	if (guc_fw->header_size != sizeof(struct guc_css_header)) {
-		DRM_ERROR("CSS header definition mismatch\n");
+		DRM_NOTE("CSS header definition mismatch\n");
 		goto fail;
 	}
 
@@ -593,7 +593,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	/* now RSA */
 	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
-		DRM_ERROR("RSA key size is bad\n");
+		DRM_NOTE("RSA key size is bad\n");
 		goto fail;
 	}
 	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
@@ -602,14 +602,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 	/* At least, it should have header, uCode and RSA. Size of all three. */
 	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
 	if (fw->size < size) {
-		DRM_ERROR("Missing firmware components\n");
+		DRM_NOTE("Missing firmware components\n");
 		goto fail;
 	}
 
 	/* Header and uCode will be loaded to WOPCM. Size of the two. */
 	size = guc_fw->header_size + guc_fw->ucode_size;
 	if (size > guc_wopcm_size(to_i915(dev))) {
-		DRM_ERROR("Firmware is too large to fit in WOPCM\n");
+		DRM_NOTE("Firmware is too large to fit in WOPCM\n");
 		goto fail;
 	}
 
@@ -624,7 +624,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 
 	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
 	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
-		DRM_ERROR("GuC firmware version %d.%d, required %d.%d\n",
+		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
 			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
 			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
 		err = -ENOEXEC;
@@ -654,10 +654,10 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
 	return;
 
 fail:
+	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
+		 guc_fw->guc_fw_path, err);
 	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
 		err, fw, guc_fw->guc_fw_obj);
-	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
-		  guc_fw->guc_fw_path, err);
 
 	mutex_lock(&dev->struct_mutex);
 	obj = guc_fw->guc_fw_obj;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI-RESEND 3/3] drm/i915/guc: revisit GuC loader message levels

[Tvrtko Ursulin]

On 19/07/16 13:20, Dave Gordon wrote:
> Some downgraded from DRM_ERROR() to DRM_WARN() or DRM_NOTE(),
> a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(),
> and one eliminated completely.
>
> v2: different permutation of levels :)
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_loader.c | 34 ++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 605c696..a2f4fa4 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private *dev_priv)
>
>   static u32 get_core_family(struct drm_i915_private *dev_priv)
>   {
> -	switch (INTEL_INFO(dev_priv)->gen) {
> +	u32 gen = INTEL_GEN(dev_priv);
> +
> +	switch (gen) {
>   	case 9:
>   		return GFXCORE_FAMILY_GEN9;
>
>   	default:
> -		DRM_ERROR("GUC: unsupported core family\n");
> +		DRM_WARN("GEN%d does not support GuC operation\n", gen);

This looks more like a WARN_ON condition to me, something developers 
need to notice extremely easily in development and will never happen in 
deployment.

>   		return GFXCORE_FAMILY_UNKNOWN;
>   	}
>   }
> @@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev)
>   		goto fail;
>   	} else if (*fw_path == '\0') {
>   		/* Device has a GuC but we don't know what f/w to load? */
> -		DRM_INFO("No GuC firmware known for this platform\n");
> +		DRM_WARN("No GuC firmware known for this platform\n");

This looks the same to me (WARN_ON), it can only happen if someone 
messes up things in development.

>   		err = -ENODEV;
>   		goto fail;
>   	}
> @@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev)
>   		 * that the state and timing are fairly predictable
>   		 */
>   		err = i915_reset_guc(dev_priv);
> -		if (err) {
> -			DRM_ERROR("GuC reset failed: %d\n", err);
> +		if (err)
>   			goto fail;
> -		}
>
>   		err = guc_ucode_xfer(dev_priv);
>   		if (!err)
> @@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev)
>   	else if (err == 0)
>   		DRM_INFO("GuC firmware load skipped\n");
>   	else if (ret != -EIO)
> -		DRM_INFO("GuC firmware load failed: %d\n", err);
> +		DRM_NOTE("GuC firmware load failed: %d\n", err);
>   	else
> -		DRM_ERROR("GuC firmware load failed: %d\n", err);
> +		DRM_WARN("GuC firmware load failed: %d\n", err);
>
>   	if (i915.enable_guc_submission) {
>   		if (fw_path == NULL)
>   			DRM_INFO("GuC submission without firmware not supported\n");
>   		if (ret == 0)
> -			DRM_INFO("Falling back from GuC submission to execlist mode\n");
> +			DRM_NOTE("Falling back from GuC submission to execlist mode\n");
>   		else
>   			DRM_ERROR("GuC init failed: %d\n", ret);
>   	}
> @@ -571,7 +571,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>   	/* Check the size of the blob before examining buffer contents */
>   	if (fw->size < sizeof(struct guc_css_header)) {
> -		DRM_ERROR("Firmware header is missing\n");
> +		DRM_NOTE("Firmware header is missing\n");
>   		goto fail;
>   	}
>
> @@ -583,7 +583,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>   		css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
>
>   	if (guc_fw->header_size != sizeof(struct guc_css_header)) {
> -		DRM_ERROR("CSS header definition mismatch\n");
> +		DRM_NOTE("CSS header definition mismatch\n");
>   		goto fail;
>   	}
>
> @@ -593,7 +593,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>   	/* now RSA */
>   	if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
> -		DRM_ERROR("RSA key size is bad\n");
> +		DRM_NOTE("RSA key size is bad\n");
>   		goto fail;
>   	}
>   	guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
> @@ -602,14 +602,14 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>   	/* At least, it should have header, uCode and RSA. Size of all three. */
>   	size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
>   	if (fw->size < size) {
> -		DRM_ERROR("Missing firmware components\n");
> +		DRM_NOTE("Missing firmware components\n");
>   		goto fail;
>   	}
>
>   	/* Header and uCode will be loaded to WOPCM. Size of the two. */
>   	size = guc_fw->header_size + guc_fw->ucode_size;
>   	if (size > guc_wopcm_size(to_i915(dev))) {
> -		DRM_ERROR("Firmware is too large to fit in WOPCM\n");
> +		DRM_NOTE("Firmware is too large to fit in WOPCM\n");
>   		goto fail;
>   	}
>
> @@ -624,7 +624,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>
>   	if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
>   	    guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
> -		DRM_ERROR("GuC firmware version %d.%d, required %d.%d\n",
> +		DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
>   			guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
>   			guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
>   		err = -ENOEXEC;
> @@ -654,10 +654,10 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw)
>   	return;
>
>   fail:
> +	DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
> +		 guc_fw->guc_fw_path, err);
>   	DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
>   		err, fw, guc_fw->guc_fw_obj);

As commented before, I would tidy these two into one while dropping the 
uninteresting debug like object pointe and fw name pointer.

> -	DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
> -		  guc_fw->guc_fw_path, err);
>
>   	mutex_lock(&dev->struct_mutex);
>   	obj = guc_fw->guc_fw_obj;
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Also, you can keep it if you decide to act on my suggestions from above.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI-RESEND 3/3] drm/i915/guc: revisit GuC loader message levels

[Dave Gordon]

On 19/07/16 15:26, Tvrtko Ursulin wrote:
>
> On 19/07/16 13:20, Dave Gordon wrote:
>> Some downgraded from DRM_ERROR() to DRM_WARN() or DRM_NOTE(),
>> a few upgraded from DRM_INFO() to DRM_NOTE() or DRM_WARN(),
>> and one eliminated completely.
>>
>> v2: different permutation of levels :)
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc_loader.c | 34
>> ++++++++++++++++-----------------
>>   1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index 605c696..a2f4fa4 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -140,12 +140,14 @@ static u32 get_gttype(struct drm_i915_private
>> *dev_priv)
>>
>>   static u32 get_core_family(struct drm_i915_private *dev_priv)
>>   {
>> -    switch (INTEL_INFO(dev_priv)->gen) {
>> +    u32 gen = INTEL_GEN(dev_priv);
>> +
>> +    switch (gen) {
>>       case 9:
>>           return GFXCORE_FAMILY_GEN9;
>>
>>       default:
>> -        DRM_ERROR("GUC: unsupported core family\n");
>> +        DRM_WARN("GEN%d does not support GuC operation\n", gen);
>
> This looks more like a WARN_ON condition to me, something developers
> need to notice extremely easily in development and will never happen in
> deployment.

OK; the check in the caller below should have prevented us reaching this 
code if the hardware ID isn't known to the driver. Changed to WARN(1, ...).

>>           return GFXCORE_FAMILY_UNKNOWN;
>>       }
>>   }
>> @@ -433,7 +435,7 @@ int intel_guc_setup(struct drm_device *dev)
>>           goto fail;
>>       } else if (*fw_path == '\0') {
>>           /* Device has a GuC but we don't know what f/w to load? */
>> -        DRM_INFO("No GuC firmware known for this platform\n");
>> +        DRM_WARN("No GuC firmware known for this platform\n");
>
> This looks the same to me (WARN_ON), it can only happen if someone
> messes up things in development.

Maybe. This is the outer check that protects the code above, and as such 
it's the first place we report unrecognised hardware. But we don't want 
to prevent the driver from working (via fallback to execlists) so 
developers can at least boot new hardware and not just get a blank 
screen. So a WARNING of some type is right, but does the stack trace 
from WARN() add any value? If not -- and I think not -- DRM_WARN() is 
what we want.

>>           err = -ENODEV;
>>           goto fail;
>>       }
>> @@ -471,10 +473,8 @@ int intel_guc_setup(struct drm_device *dev)
>>            * that the state and timing are fairly predictable
>>            */
>>           err = i915_reset_guc(dev_priv);
>> -        if (err) {
>> -            DRM_ERROR("GuC reset failed: %d\n", err);
>> +        if (err)
>>               goto fail;
>> -        }
>>
>>           err = guc_ucode_xfer(dev_priv);
>>           if (!err)
>> @@ -532,15 +532,15 @@ int intel_guc_setup(struct drm_device *dev)
>>       else if (err == 0)
>>           DRM_INFO("GuC firmware load skipped\n");
>>       else if (ret != -EIO)
>> -        DRM_INFO("GuC firmware load failed: %d\n", err);
>> +        DRM_NOTE("GuC firmware load failed: %d\n", err);
>>       else
>> -        DRM_ERROR("GuC firmware load failed: %d\n", err);
>> +        DRM_WARN("GuC firmware load failed: %d\n", err);
>>
>>       if (i915.enable_guc_submission) {
>>           if (fw_path == NULL)
>>               DRM_INFO("GuC submission without firmware not
>> supported\n");
>>           if (ret == 0)
>> -            DRM_INFO("Falling back from GuC submission to execlist
>> mode\n");
>> +            DRM_NOTE("Falling back from GuC submission to execlist
>> mode\n");
>>           else
>>               DRM_ERROR("GuC init failed: %d\n", ret);
>>       }
>> @@ -571,7 +571,7 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>
>>       /* Check the size of the blob before examining buffer contents */
>>       if (fw->size < sizeof(struct guc_css_header)) {
>> -        DRM_ERROR("Firmware header is missing\n");
>> +        DRM_NOTE("Firmware header is missing\n");
>>           goto fail;
>>       }
>>
>> @@ -583,7 +583,7 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>           css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
>>
>>       if (guc_fw->header_size != sizeof(struct guc_css_header)) {
>> -        DRM_ERROR("CSS header definition mismatch\n");
>> +        DRM_NOTE("CSS header definition mismatch\n");
>>           goto fail;
>>       }
>>
>> @@ -593,7 +593,7 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>
>>       /* now RSA */
>>       if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
>> -        DRM_ERROR("RSA key size is bad\n");
>> +        DRM_NOTE("RSA key size is bad\n");
>>           goto fail;
>>       }
>>       guc_fw->rsa_offset = guc_fw->ucode_offset + guc_fw->ucode_size;
>> @@ -602,14 +602,14 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>       /* At least, it should have header, uCode and RSA. Size of all
>> three. */
>>       size = guc_fw->header_size + guc_fw->ucode_size + guc_fw->rsa_size;
>>       if (fw->size < size) {
>> -        DRM_ERROR("Missing firmware components\n");
>> +        DRM_NOTE("Missing firmware components\n");
>>           goto fail;
>>       }
>>
>>       /* Header and uCode will be loaded to WOPCM. Size of the two. */
>>       size = guc_fw->header_size + guc_fw->ucode_size;
>>       if (size > guc_wopcm_size(to_i915(dev))) {
>> -        DRM_ERROR("Firmware is too large to fit in WOPCM\n");
>> +        DRM_NOTE("Firmware is too large to fit in WOPCM\n");
>>           goto fail;
>>       }
>>
>> @@ -624,7 +624,7 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>
>>       if (guc_fw->guc_fw_major_found != guc_fw->guc_fw_major_wanted ||
>>           guc_fw->guc_fw_minor_found < guc_fw->guc_fw_minor_wanted) {
>> -        DRM_ERROR("GuC firmware version %d.%d, required %d.%d\n",
>> +        DRM_NOTE("GuC firmware version %d.%d, required %d.%d\n",
>>               guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
>>               guc_fw->guc_fw_major_wanted, guc_fw->guc_fw_minor_wanted);
>>           err = -ENOEXEC;
>> @@ -654,10 +654,10 @@ static void guc_fw_fetch(struct drm_device *dev,
>> struct intel_guc_fw *guc_fw)
>>       return;
>>
>>   fail:
>> +    DRM_WARN("Failed to fetch valid GuC firmware from %s (error %d)\n",
>> +         guc_fw->guc_fw_path, err);
>>       DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj
>> %p\n",
>>           err, fw, guc_fw->guc_fw_obj);
>
> As commented before, I would tidy these two into one while dropping the
> uninteresting debug like object pointe and fw name pointer.

Can't be done. User/admin doesn't want to see internal details, 
developer needs them. The pointers are important not so much for their 
specific values but for NULL/non-NULL -- for debugging we like to know 
whether we hit a lookup error or an allocation failure, or something 
else. So two different messages for two different audiences.

.Dave.

>> -    DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
>> -          guc_fw->guc_fw_path, err);
>>
>>       mutex_lock(&dev->struct_mutex);
>>       obj = guc_fw->guc_fw_obj;
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Also, you can keep it if you decide to act on my suggestions from above.
>
> Regards,
> Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Ro.CI.BAT: success for series starting with [CI-RESEND,1/3] drm: extra printk() wrapper macros

[Patchwork]

== Series Details ==

Series: series starting with [CI-RESEND,1/3] drm: extra printk() wrapper macros
URL   : https://patchwork.freedesktop.org/series/10036/
State : success

== Summary ==

Series 10036v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/10036/revisions/1/mbox

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (ro-skl3-i5-6260u)
Test gem_sync:
        Subgroup basic-store-each:
                fail       -> DMESG-FAIL (ro-bdw-i7-5600u)

fi-hsw-i7-4770k  total:243  pass:213  dwarn:0   dfail:0   fail:10  skip:20 
fi-kbl-qkkr      total:243  pass:178  dwarn:27  dfail:1   fail:10  skip:27 
fi-skl-i5-6260u  total:243  pass:222  dwarn:0   dfail:0   fail:9   skip:12 
fi-skl-i7-6700k  total:243  pass:208  dwarn:0   dfail:0   fail:9   skip:26 
fi-snb-i7-2600   total:243  pass:193  dwarn:0   dfail:0   fail:10  skip:40 
ro-bdw-i5-5250u  total:244  pass:217  dwarn:4   dfail:0   fail:10  skip:13 
ro-bdw-i7-5557U  total:244  pass:219  dwarn:1   dfail:0   fail:9   skip:15 
ro-bdw-i7-5600u  total:244  pass:201  dwarn:0   dfail:1   fail:10  skip:32 
ro-bsw-n3050     total:218  pass:173  dwarn:0   dfail:0   fail:2   skip:42 
ro-byt-n2820     total:244  pass:194  dwarn:0   dfail:0   fail:12  skip:38 
ro-hsw-i3-4010u  total:244  pass:209  dwarn:0   dfail:0   fail:11  skip:24 
ro-hsw-i7-4770r  total:244  pass:209  dwarn:0   dfail:0   fail:11  skip:24 
ro-ilk-i7-620lm  total:244  pass:169  dwarn:0   dfail:0   fail:12  skip:63 
ro-ilk1-i5-650   total:239  pass:170  dwarn:0   dfail:0   fail:11  skip:58 
ro-ivb-i7-3770   total:244  pass:200  dwarn:0   dfail:0   fail:11  skip:33 
ro-skl3-i5-6260u total:244  pass:222  dwarn:0   dfail:0   fail:10  skip:12 
ro-snb-i7-2620M  total:244  pass:190  dwarn:0   dfail:0   fail:12  skip:42 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1531/

635555a drm-intel-nightly: 2016y-07m-19d-13h-02m-39s UTC integration manifest
b23d573 drm/i915/guc: revisit GuC loader message levels
950c1a4 drm/i915/guc: downgrade some DRM_ERROR() messages to DRM_WARN()
5e8e33b drm: extra printk() wrapper macros

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Ro.CI.BAT: success for series starting with [CI-RESEND,1/3] drm: extra printk() wrapper macros

[Dave Gordon]

On 19/07/16 14:53, Patchwork wrote:
> == Series Details ==
>
> Series: series starting with [CI-RESEND,1/3] drm: extra printk() wrapper macros
> URL   : https://patchwork.freedesktop.org/series/10036/
> State : success
>
> == Summary ==
>
> Series 10036v1 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/10036/revisions/1/mbox
>
> Test drv_module_reload_basic:
>                  dmesg-warn -> PASS       (ro-skl3-i5-6260u)
> Test gem_sync:
>          Subgroup basic-store-each:
>                  fail       -> DMESG-FAIL (ro-bdw-i7-5600u)

This is (now) Bug 96974
https://bugs.freedesktop.org/show_bug.cgi?id=96974
[BAT BDW] gem_sync / basic-store-each fails sporadically

I filed this too, but Villa beat me by a couple of minutes,
so my bug 96975 is now a dup of 96974.

Maybe we should get another identical machine and see whether
the results are consistent, 'cos the other BDWs aren't showing
this at all.

.Dave.

> fi-hsw-i7-4770k  total:243  pass:213  dwarn:0   dfail:0   fail:10  skip:20
> fi-kbl-qkkr      total:243  pass:178  dwarn:27  dfail:1   fail:10  skip:27
> fi-skl-i5-6260u  total:243  pass:222  dwarn:0   dfail:0   fail:9   skip:12
> fi-skl-i7-6700k  total:243  pass:208  dwarn:0   dfail:0   fail:9   skip:26
> fi-snb-i7-2600   total:243  pass:193  dwarn:0   dfail:0   fail:10  skip:40
> ro-bdw-i5-5250u  total:244  pass:217  dwarn:4   dfail:0   fail:10  skip:13
> ro-bdw-i7-5557U  total:244  pass:219  dwarn:1   dfail:0   fail:9   skip:15
> ro-bdw-i7-5600u  total:244  pass:201  dwarn:0   dfail:1   fail:10  skip:32
> ro-bsw-n3050     total:218  pass:173  dwarn:0   dfail:0   fail:2   skip:42
> ro-byt-n2820     total:244  pass:194  dwarn:0   dfail:0   fail:12  skip:38
> ro-hsw-i3-4010u  total:244  pass:209  dwarn:0   dfail:0   fail:11  skip:24
> ro-hsw-i7-4770r  total:244  pass:209  dwarn:0   dfail:0   fail:11  skip:24
> ro-ilk-i7-620lm  total:244  pass:169  dwarn:0   dfail:0   fail:12  skip:63
> ro-ilk1-i5-650   total:239  pass:170  dwarn:0   dfail:0   fail:11  skip:58
> ro-ivb-i7-3770   total:244  pass:200  dwarn:0   dfail:0   fail:11  skip:33
> ro-skl3-i5-6260u total:244  pass:222  dwarn:0   dfail:0   fail:10  skip:12
> ro-snb-i7-2620M  total:244  pass:190  dwarn:0   dfail:0   fail:12  skip:42
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1531/
>
> 635555a drm-intel-nightly: 2016y-07m-19d-13h-02m-39s UTC integration manifest
> b23d573 drm/i915/guc: revisit GuC loader message levels
> 950c1a4 drm/i915/guc: downgrade some DRM_ERROR() messages to DRM_WARN()
> 5e8e33b drm: extra printk() wrapper macros
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx