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

[Dave Gordon <david.s.gordon@intel.com>]

On 13/06/16 11:53, Tvrtko Ursulin wrote:
>
> 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

Roger wilco, but it looks simpler if I make it a followup [7/6]?
Otherwise (if I mix it into this rework) git scrambles the diff
around so its less easy to see how the code was reorganised.

Update patch will follow soon ...

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