[Intel-gfx] [PATCH 3/4] drm/i915/guc: do not print drbreg on error

[Michal Wajdeczko]

On Wed, 17 Oct 2018 02:17:17 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio at intel.com> wrote:

> The only content of the register apart from the valid bit is the lower
> part of the physical memory address. If the valid bit is 0 the address
> is meaningless, while if it is 1 we don't know which descriptor it came
> from (since the doorbell is in an unexpected state) so we can't match it
> to an expected value. Since we already print the state of the valid bit,
> stop priniting the full register contents as they're just confusing.

typo in priniting

> While at it, move the checking of the valid bit to a common helper.
>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_submission.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c  
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 8c3b5a9facee..cba84480dad9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -192,6 +192,12 @@ static struct guc_doorbell_info  
> *__get_doorbell(struct intel_guc_client *client)
>  	return client->vaddr + client->doorbell_offset;
>  }
> +static bool __doorbell_valid(struct intel_guc *guc, u16 db_id)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);

should we add GEM_BUG_ON(db_id > GUC_NUM_DOORBELLS) here ?

> +	return I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID;
> +}
> +
>  static void __init_doorbell(struct intel_guc_client *client)
>  {
>  	struct guc_doorbell_info *doorbell;
> @@ -203,7 +209,6 @@ static void __init_doorbell(struct intel_guc_client  
> *client)
> static void __fini_doorbell(struct intel_guc_client *client)
>  {
> -	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
>  	struct guc_doorbell_info *doorbell;
>  	u16 db_id = client->doorbell_id;
> @@ -214,7 +219,7 @@ static void __fini_doorbell(struct intel_guc_client  
> *client)
>  	 * to go to zero after updating db_status before we call the GuC to
>  	 * release the doorbell
>  	 */
> -	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID),  
> 10))
> +	if (wait_for_us(!__doorbell_valid(client->guc, db_id), 10))
>  		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
>  }
> @@ -866,20 +871,17 @@ guc_reset_prepare(struct intel_engine_cs *engine)
>  /* Check that a doorbell register is in the expected state */
>  static bool doorbell_ok(struct intel_guc *guc, u16 db_id)
>  {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	u32 drbregl;
>  	bool valid;
> 	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);

btw, maybe better to check against GUC_NUM_DOORBELLS ?
GUC_DOORBELL_INVALID looks like a fw defined value

and is it ok that we define GUC_NUM_DOORBELLS in intel_guc_fwif.h ?
maybe better place for it is near GEN8_DRBREGL in intel_guc_reg.h

> -	drbregl = I915_READ(GEN8_DRBREGL(db_id));
> -	valid = drbregl & GEN8_DRB_VALID;
> +	valid = __doorbell_valid(guc, db_id);
> 	if (test_bit(db_id, guc->doorbell_bitmap) == valid)
>  		return true;
> -	DRM_DEBUG_DRIVER("Doorbell %d has unexpected state (0x%x): valid=%s\n",
> -			 db_id, drbregl, yesno(valid));
> +	DRM_DEBUG_DRIVER("Doorbell %d has unexpected state: valid=%s\n",
> +			 db_id, yesno(valid));

db_id is unsigned so you can use %u (or %hu) for it

> 	return false;
>  }

later in guc_verify_doorbells() we are stopping after detecting first
mismatched doorbell - maybe we should scan all doorbells to get debug
logs for all unexpected states?

with all these nitpicks hopefully accepted,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>

Regards,
Michal