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

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

On 13/06/16 16:59, Dave Gordon wrote:
> 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 ...

Yes fine, have it outside this series for simplicity. So for this patch:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


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