[Intel-gfx] [PATCH 3/4] drm/i915/guc: refactor doorbell management code

[Dave Gordon]

On 08/06/16 14:11, Tvrtko Ursulin wrote:
>
> On 08/06/16 11:55, Dave Gordon wrote:
>> During a hibernate/resume cycle, the driver, the GuC, and the doorbell
>> hardware can end up in inconsistent states. This patch refactors the
>> driver's handling and tracking of doorbells, in preparation for a later
>> one which will resolve the issue.
>
> Perhaps a few word on the goal, method and result of refactoring. Would
> be good for documentation and easier review.

The refactoring is exactly that; there's very little new code.
All we're doing is centralising all doorbell management into
just one function and turning the (outermost) old functions
into wrappers for the single function.

There are three resources to be managed:
1. Cachelines: a single line within the client-object's page 0
    is snooped for writes by the host.
2. Doorbell registers: each defines a cacheline to be snooped.
3. Bitmap: tracks which 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 part 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 we don't need to do them separately they're now collected into 
the setup part of the common function.

>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 87
>> ++++++++++++++++++------------
>>   1 file changed, 53 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 7510841..eef67c8 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -174,36 +174,63 @@ 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 i915_guc_client *client,
>> +                  struct guc_doorbell_info *doorbell,
>> +                  u16 new_id)
>>   {
>> -    struct guc_doorbell_info *doorbell;
>> +    struct sg_table *sg = client->guc->ctx_pool_obj->pages;
>> +    void *doorbell_bitmap = client->guc->doorbell_bitmap;
>> +    struct guc_context_desc desc;
>> +    size_t len;
>> +
>> +    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(client->guc, client);
>> +        clear_bit(client->doorbell_id, doorbell_bitmap);
>> +    }
>>
>> -    doorbell = client->client_base + client->doorbell_offset;
>> +    /* 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(client->doorbell_id, doorbell_bitmap);
>>       doorbell->db_status = GUC_DOORBELL_ENABLED;
>>       doorbell->cookie = 0;
>> +    return host2guc_allocate_doorbell(client->guc, client);
>
> A bunch of new code here which wasn't done on doorbell init before.
> Populating the ctx_pool_obj and hust2guc call. I think the commit needs
> to explain what is happening here.

The only new code (and new capability) is the block
     /* 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.

The host2guc call is not new, but it was previously called
separately after the return from guc_init_doorbell().

> Maybe it is mostly a matter of copying over some text from the cover
> letter, but ideally it should explain what it is doing more precisely.

I'll put some of this reply into the commit message

>>   }
>>
>> -static void guc_disable_doorbell(struct intel_guc *guc,
>> -                 struct i915_guc_client *client)
>> +static void guc_init_doorbell(struct intel_guc *guc,
>> +                  struct i915_guc_client *client,
>> +                  uint16_t db_id)
>>   {
>> -    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;
>> -
>> -    I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>> +    guc_update_doorbell_id(client, doorbell, db_id);
>> +}
>
> This function does not end up doing anything now, just a pass-through to
> guc_update_doorbell_id.

Correct. It's retained to make it clear that here we're initialising a 
newly-created client with a doorbell assignment for the first time, as 
opposed to updating an existing doorbell assignment. Similarly we retain 
guc_disable_doorbell() as an alias for changing the doorbell ID (back) 
to INVALID.

> What happens to the write to drbreg ? It is completely gone and also
> from the init doorbell path together with the write to another register.

Those writes were useless. Turns out those registers are not writeable 
by the CPU; they can *only* be updated by the GuC!

>> -    value = I915_READ(drbreg);
>> -    WARN_ON((value & GEN8_DRB_VALID) != 0);
>> +static void guc_disable_doorbell(struct intel_guc *guc,
>> +                 struct i915_guc_client *client)
>> +{
>> +    struct guc_doorbell_info *doorbell;
>>
>> -    I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
>> -    I915_WRITE(drbreg, 0);
>> +    doorbell = client->client_base + client->doorbell_offset;
>> +    guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
>>
>>       /* XXX: wait for any interrupts */
>>       /* XXX: wait for workqueue to drain */
>> @@ -251,7 +278,7 @@ static uint16_t assign_doorbell(struct intel_guc
>> *guc, uint32_t priority)
>>       if (id == end)
>>           id = GUC_INVALID_DOORBELL_ID;
>>       else
>> -        bitmap_set(guc->doorbell_bitmap, id, 1);
>> +        set_bit(id, guc->doorbell_bitmap);
>
> set_bit is atomic - is there a reason it is required here?
>
> Actually the same question for the clear_bit above, where it was
> bitmap_clear before.

The bitmap_set/clear functions don't have a comparable TEST
operator, so we have to use test_bit(). The others were just
changed for symmetry. We don't care about atomic or not, and
this is rarely executed (setup/teardown) code so we don't worry
about performance either.

Actually, though, I'd expect the one-bit functions set_bit()
and clear_bit() to be MORE efficient than the general multibit
operations bitmap_set/clear. They should get inlined too :)

We could use __set/clear_bit() if we really don't want the LOCK.

New version later ...

.Dave.

>>       DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
>>               hi_pri ? "high" : "normal", id);
>> @@ -259,11 +286,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)
>> -{
>> -    bitmap_clear(guc->doorbell_bitmap, id, 1);
>> -}
>> -
>>   /*
>>    * Initialise the process descriptor shared with the GuC firmware.
>>    */
>> @@ -657,8 +679,6 @@ 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
>> @@ -666,12 +686,8 @@ static void guc_client_free(struct drm_device *dev,
>>            * GuC that we've finished with it, finally deallocate
>>            * it in our bitmap
>>            */
>> -        if (db_id != GUC_INVALID_DOORBELL_ID) {
>> +        if (client->doorbell_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);
>> -        }
>>
>>           kunmap(kmap_to_page(client->client_base));
>>       }
>> @@ -706,6 +722,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)
>> @@ -746,22 +763,24 @@ 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);
>> +    guc_init_doorbell(guc, client, db_id);
>>
>>       /* XXX: Any cache flushes needed? General domain mgmt calls? */
>>
>>       if (host2guc_allocate_doorbell(guc, client))
>>           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;
>>
>>
>
> Regards,
> Tvrtko