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

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

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!

.Dave.

>>       doorbell->cookie = 0;
>> +    doorbell->db_status = GUC_DOORBELL_ENABLED;
>> +    return host2guc_allocate_doorbell(guc, client);
>> +}
>> +
>> +static int guc_init_doorbell(struct intel_guc *guc,
>> +                  struct i915_guc_client *client,
>> +                  uint16_t db_id)
>> +{
>> +    return guc_update_doorbell_id(guc, client, db_id);
>>   }
>>
>>   static void guc_disable_doorbell(struct intel_guc *guc,
>>                    struct i915_guc_client *client)
>>   {
>> -    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> -    struct guc_doorbell_info *doorbell;
>> -    i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>> -    int value;
>> -
>> -    doorbell = client->client_base + client->doorbell_offset;
>> -
>> -    doorbell->db_status = GUC_DOORBELL_DISABLED;
>> -
>> -    value = I915_READ(drbreg);
>> -    WARN_ON((value & GEN8_DRB_VALID) != 0);
>> +    (void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
>>
>>       /* XXX: wait for any interrupts */
>>       /* XXX: wait for workqueue to drain */
>> @@ -254,11 +282,6 @@ static uint16_t assign_doorbell(struct intel_guc
>> *guc, uint32_t priority)
>>       return id;
>>   }
>>
>> -static void release_doorbell(struct intel_guc *guc, uint16_t id)
>> -{
>> -    __clear_bit(id, guc->doorbell_bitmap);
>> -}
>> -
>>   /*
>>    * Initialise the process descriptor shared with the GuC firmware.
>>    */
>> @@ -652,21 +675,11 @@ static void guc_client_free(struct drm_device *dev,
>>        */
>>
>>       if (client->client_base) {
>> -        uint16_t db_id = client->doorbell_id;
>> -
>>           /*
>> -         * If we got as far as setting up a doorbell, make sure
>> -         * we shut it down before unmapping & deallocating the
>> -         * memory. So first disable the doorbell, then tell the
>> -         * GuC that we've finished with it, finally deallocate
>> -         * it in our bitmap
>> +         * If we got as far as setting up a doorbell, make sure we
>> +         * shut it down before unmapping & deallocating the memory.
>>            */
>> -        if (db_id != GUC_INVALID_DOORBELL_ID) {
>> -            guc_disable_doorbell(guc, client);
>> -            if (test_bit(db_id, guc->doorbell_bitmap))
>> -                host2guc_release_doorbell(guc, client);
>> -            release_doorbell(guc, db_id);
>> -        }
>> +        guc_disable_doorbell(guc, client);
>>
>>           kunmap(kmap_to_page(client->client_base));
>>       }
>> @@ -701,6 +714,7 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_guc *guc = &dev_priv->guc;
>>       struct drm_i915_gem_object *obj;
>> +    uint16_t db_id;
>>
>>       client = kzalloc(sizeof(*client), GFP_KERNEL);
>>       if (!client)
>> @@ -741,22 +755,20 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>>       else
>>           client->proc_desc_offset = (GUC_DB_SIZE / 2);
>>
>> -    client->doorbell_id = assign_doorbell(guc, client->priority);
>> -    if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
>> +    db_id = assign_doorbell(guc, client->priority);
>> +    if (db_id == GUC_INVALID_DOORBELL_ID)
>>           /* XXX: evict a doorbell instead */
>>           goto err;
>>
>>       guc_init_proc_desc(guc, client);
>>       guc_init_ctx_desc(guc, client);
>> -    guc_init_doorbell(guc, client);
>> -
>> -    /* XXX: Any cache flushes needed? General domain mgmt calls? */
>> -
>> -    if (host2guc_allocate_doorbell(guc, client))
>> +    if (guc_init_doorbell(guc, client, db_id))
>>           goto err;
>>
>> -    DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id
>> %u\n",
>> -        priority, client, client->ctx_index, client->doorbell_id);
>> +    DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
>> +        priority, client, client->ctx_index);
>> +    DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
>> +        client->doorbell_id, client->doorbell_offset);
>>
>>       return client;
>>
>>
>
> Otherwise looks good to me, much more easier to understand what is
> happening now.
>
> Regards,
>
> Tvrtko

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