Re: [PATCH v2 5/6] drm/i915/guc: refactor doorbell management code

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 13/06/16 11:25, Dave Gordon wrote:
> On 13/06/16 10:48, Tvrtko Ursulin wrote:
>>
>> On 10/06/16 17:51, Dave Gordon wrote:
>>> This patch refactors the driver's handling and tracking of doorbells, in
>>> preparation for a later one which will resolve a suspend-resume issue.
>>>
>>> There are three resources to be managed:
>>> 1. Cachelines: a single line within the client-object's page 0
>>>     is snooped by doorbell hardware for writes from the host.
>>> 2. Doorbell registers: each defines one cacheline to be snooped.
>>> 3. Bitmap: tracks which doorbell registers are in use.
>>>
>>> The doorbell setup/teardown protocol starts with:
>>> 1. Pick a cacheline: select_doorbell_cacheline()
>>> 2. Find an available doorbell register: assign_doorbell()
>>> (These values are passed to the GuC via the shared context
>>> descriptor; this part of the sequence remains unchanged).
>>>
>>> 3. Update the bitmap to reflect registers-in-use
>>> 4. Prepare the cacheline for use by setting its status to ENABLED
>>> 5. Ask the GuC to program the doorbell to snoop the cacheline
>>>
>>> and of course teardown is very similar:
>>> 6. Set the cacheline to DISABLED
>>> 7. Ask the GuC to reprogram the doorbell to stop snooping
>>> 8. Record that the doorbell is not in use.
>>>
>>> Operations 6-8 (guc_disable_doorbell(), host2guc_release_doorbell(), and
>>> release_doorbell()) were called in sequence from guc_client_free(), but
>>> are now moved into the teardown phase of the common function.
>>>
>>> Steps 4-5 (guc_init_doorbell() and host2guc_allocate_doorbell()) were
>>> similarly done as sequential steps in guc_client_alloc(), but since it
>>> turns out that we don't need to be able to do them separately they're
>>> now collected into the setup phase of the common function.
>>>
>>> The only new code (and new capability) is the block tagged
>>>      /* Update the GuC's idea of the doorbell ID */
>>> i.e. we can now *change* the doorbell register used by an existing
>>> client, whereas previously it was set once for the entire lifetime
>>> of the client. We will use this new feature in the next patch.
>>>
>>> v2: Trivial independent fixes pushed ahead as separate patches.
>>>      MUCH longer commit message :) [Tvrtko Ursulin]
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 94
>>> +++++++++++++++++-------------
>>>   1 file changed, 53 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> index 45b33f8..1833bfd 100644
>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>> @@ -174,31 +174,59 @@ static int host2guc_sample_forcewake(struct
>>> intel_guc *guc,
>>>    * client object which contains the page being used for the doorbell
>>>    */
>>>
>>> -static void guc_init_doorbell(struct intel_guc *guc,
>>> -                  struct i915_guc_client *client)
>>> +static int guc_update_doorbell_id(struct intel_guc *guc,
>>> +                  struct i915_guc_client *client,
>>> +                  u16 new_id)
>>>   {
>>> +    struct sg_table *sg = guc->ctx_pool_obj->pages;
>>> +    void *doorbell_bitmap = guc->doorbell_bitmap;
>>>       struct guc_doorbell_info *doorbell;
>>> +    struct guc_context_desc desc;
>>> +    size_t len;
>>>
>>>       doorbell = client->client_base + client->doorbell_offset;
>>>
>>> -    doorbell->db_status = GUC_DOORBELL_ENABLED;
>>> +    if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
>>> +        test_bit(client->doorbell_id, doorbell_bitmap)) {
>>> +        /* Deactivate the old doorbell */
>>> +        doorbell->db_status = GUC_DOORBELL_DISABLED;
>>> +        (void)host2guc_release_doorbell(guc, client);
>>> +        __clear_bit(client->doorbell_id, doorbell_bitmap);
>>> +    }
>>> +
>>> +    /* Update the GuC's idea of the doorbell ID */
>>> +    len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
>>> +                 sizeof(desc) * client->ctx_index);
>>> +    if (len != sizeof(desc))
>>> +        return -EFAULT;
>>> +    desc.db_id = new_id;
>>> +    len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
>>> +                 sizeof(desc) * client->ctx_index);
>>> +    if (len != sizeof(desc))
>>> +        return -EFAULT;
>>> +
>>> +    client->doorbell_id = new_id;
>>> +    if (new_id == GUC_INVALID_DOORBELL_ID)
>>> +        return 0;
>>> +
>>> +    /* Activate the new doorbell */
>>> +    __set_bit(new_id, doorbell_bitmap);
>>
>> Is this the same bit as in assign_doorbell so redundant?
>
> It is the same bit, and yes, it will be redundant during the initial
> setup, but when we come to *re*assign the association between a client
> and a doorbell (in the next patch) then it won't be.
>
> We could also choose to have assign_doorbell() NOT update the map, so
> then this would be the only place where the bitmap gets updated. I'd
> have to rename it though, as it would no longer be assigning anything!

Maybe pick_doorbell or find_free_doorbell? It would be a little bit 
clearer and safer (early returns above could leave the bitmap set) with 
a single bitmap update location so I think it would be worth it.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx