[Intel-gfx] [PATCH v2] drm/i915: Sanitize GuC client initialization

[Daniele Ceraolo Spurio]

On 15/02/17 23:44, Chris Wilson wrote:
> On Wed, Feb 15, 2017 at 06:28:59PM -0800, Daniele Ceraolo Spurio wrote:
>> On 14/02/17 05:53, Joonas Lahtinen wrote:
>>> -static void guc_disable_doorbell(struct intel_guc *guc,
>>> -				 struct i915_guc_client *client)
>>> +static int __destroy_doorbell(struct i915_guc_client *client)
>>> {
>>> -	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
>>> +	struct guc_doorbell_info *doorbell;
>>>
>>> -	/* XXX: wait for any interrupts */
>>> -	/* XXX: wait for workqueue to drain */
>>> +	doorbell = __get_doorbell(client);
>>> +	doorbell->db_status = GUC_DOORBELL_DISABLED;
>>> +	doorbell->cookie = 0;
>>> +
>>
>> Not from this patch (but since we're at it...), I did a bit of
>> digging and I've found that doorbell release flow requires SW to
>> poll the GEN8_DRBREGL(db_id) register after updating
>> doorbell->db_status to wait for the GEN8_DRB_VALID bit to go to
>> zero. This ensures that any pending events are processed before we
>> call into GuC to release the doorbell. Note that GuC will fail the
>> DEALLOCATE_DOORBELL action if the bit has not gone to zero yet. This
>> is currently not an issue, probably because we use a single doorbell
>> and we don't ring it near release time, but the situation could
>> change in the future so I believe it is worth to fix it now. I can
>> follow up with an additional patch if you want to keep this one as
>> refactoring only.
>
> Just keep in mind that we currently do a disable after GPU reset. The guc
> may not know what we are talking about :)
> -Chris
>

Yep, we should move the release to before reset or, if the 
client/doorbell page stays pinned across reset, limit ourselves to just 
updating the db status in memory (which will cause the validity bit to 
be updated accordingly) without notifying GuC, because as you said GuC 
will have no idea of what we're referring to.

I'm also seeing a doorbell release failure during driver unload:

[drm] INTEL_GUC_SEND: Action 0x20 failed; ret=-110 status=0x00000020 
response=0x00000000

I haven't looked int it yet (I was waiting for this patch to go in 
first), but when I reload the driver the status of the doorbells seems 
to be ok so probably just a communication issue with GuC.

Thanks,
Daniele