On 13/04/16 18:50, Yu Dai wrote:
>
> On 04/07/2016 10:21 AM, Dave Gordon wrote:
>> During a hibernate/resume cycle, the whole system is reset, including
>> the GuC and the doorbell hardware. Then the system is booted up,
drivers
>> are loaded, etc -- the GuC firmware may be loaded and set running
at this
>> point. But then, the booted kernel is replaced by the hibernated
image,
>> and this resumed kernel will also try to reload the GuC firmware
(which
>> will fail). To recover, we reset the GuC and try again (which should
>> work). But this GuC reset doesn't also reset the doorbell hardware, so
>> it can be left in a state inconsistent with that assumed by the driver
>> and the GuC.
>>
>> It would be better if the GuC reset also cleared all doorbell state,
>> but that's not how the hardware currently works; also, the driver
cannot
>> directly reprogram the doorbell hardware (only the GuC can do that).
>>
>> So this patch cycles through all doorbells, assigning and releasing
each
>> in turn, so that all the doorbell hardware is left in a consistent
state,
>> no matter how it was programmed by the previously-running kernel
and/or
>> GuC firmware.
>>
>> This patch can be removed if/when the GuC firmware is updated so
that it
>> (re)initialises the doorbell hardware after every firmware (re)load.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_guc_submission.c | 46
>> +++++++++++++++++++++++++++++-
>> 1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 2fc69f1..f466eab 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -707,6 +707,50 @@ static void guc_client_free(struct drm_device
*dev,
>> kfree(client);
>> }
>> +/*
>> + * Borrow the first client to set up & tear down every doorbell
>> + * in turn, to ensure that all doorbell h/w is (re)initialised.
>> + */
>> +static void guc_init_doorbell_hw(struct intel_guc *guc)
>> +{
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> + struct i915_guc_client *client = guc->execbuf_client;
>> + struct guc_doorbell_info *doorbell;
>> + uint16_t db_id, i;
>> + void *base;
>> + int ret;
>> +
>> + base =
kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
>> + doorbell = base + client->doorbell_offset;
>> + db_id = client->doorbell_id;
>> +
>> + for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>> + i915_reg_t drbreg = GEN8_DRBREGL(i);
>> + u32 value = I915_READ(drbreg);
>> +
>> + ret = guc_update_doorbell_id(client, doorbell, i);
>> +
>> + if (((value & GUC_DOORBELL_ENABLED) && (i != db_id)) || ret)
>> + DRM_DEBUG_DRIVER("Doorbell reg 0x%x was 0x%x, ret %d\n",
>> + drbreg.reg, value, ret);
>> + }
>> +
>> + /* Restore to original value */
>> + guc_update_doorbell_id(client, doorbell, db_id);
>> +
>> + for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
>> + i915_reg_t drbreg = GEN8_DRBREGL(i);
>> + u32 value = I915_READ(drbreg);
>> +
>> + if ((value & GUC_DOORBELL_ENABLED) && (i != db_id))
>> + DRM_DEBUG_DRIVER("Doorbell reg 0x%x finally 0x%x\n",
>> + drbreg.reg, value);
>> +
>> + }
>> +
>
> The for loop above is not needed. It can be merged into previous
loop by
> print out new drbreg value (read it again after update_doorbell_id).
>
> At this point, only need to check if db_id is correctly enabled or not
> by print out I915_READ(GEN8_DRBREGL(db_id)).
>
> Alex
No, the idea is not to check that the GuC call has *enabled* each
selected doorbell, but to check that after the end of the first loop,
and the subsequent restore, all *other* doorbells have been *disabled*.
We're only *selecting* each doorbell so that we can then *deselect* it
as a side effect of selecting the next one!
Hence separate loop required ...
.Dave.
