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

[Daniele Ceraolo Spurio]

On 17/10/18 09:37, Michal Wajdeczko wrote:
> 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 ?
> 

Can do. I didn't to begin with because we should catch invalid IDs 
before we reach here, but an extra debug check won't hurt

>> +    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
> 

It is, but even in FW it is defined as equal to GUC_NUM_DOORBELLS

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

The problem with that is that we'd have to either move 
GUC_DOORBELL_INVALID as well or include intel_guc_reg.h from 
intel_guc_fwif.h. The latter should be semantically ok I think, since 
some aspects of the interface are dependent on HW

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

ack

>>     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?

ack

Thanks,
Daniele

> 
> with all these nitpicks hopefully accepted,
> 
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> 
> Regards,
> Michal