[Intel-gfx] [PATCH 2/3] drm/i915/guc: downgrade some DRM_ERROR() messages to DRM_WARN()

[Tvrtko Ursulin]

On 11/07/16 19:01, 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 at 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)));

Hm, this does propagate the error code to the callers some which will 
act and log the failure. Majority won't though - like suspend/resume 
etc. In those cases it feels more like an error than a warning.

>
>   		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);

This one is interesting, error is propagated out a bit but then ignored 
in actual command submission.

If the above message means command will not be submitted error is 
probably more appropriate. Or perhaps we cannot tell if the command was 
submitted or not in this case?

>
>   		/* 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;
>   	}
>
>

Regards,

Tvrtko