Re: [PATCH v4 3/6] drm/i915/guc: refactor doorbell management code

[Dave Gordon <david.s.gordon@xxxxxxxxx>]

On 07/04/16 22:26, Yu Dai wrote:
> On 04/07/2016 10:21 AM, 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.
> >
> > Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_guc_submission.c | 88
> > ++++++++++++++++++------------
> >   1 file changed, 53 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 2171759..2fc69f1 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -175,8 +175,48 @@ static int host2guc_sample_forcewake(struct
> > intel_guc *guc,
> >    * client object which contains the page being used for the doorbell
> >    */
> > +static int guc_update_doorbell_id(struct i915_guc_client *client,
> > +                  struct guc_doorbell_info *doorbell,
> > +                  u16 new_id)
> > +{
> > +    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);
> > +    }
> > +
> > +    /* 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;
> > +
>
> We may cache the vmap of context pool for its life cycle to avoid these
> copies. That is why a generic vmap helper function is really nice to have.
>
> Alex

Yes, I'm hoping we'll make some progress with that now that Chris has 
merged some new vmap code.

Meanwhile can you please review all of the patches in this series?
Thanks,
.Dave.

> > +    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);
> > +}
> > +
> >   static void guc_init_doorbell(struct intel_guc *guc,
> > -                  struct i915_guc_client *client)
> > +                  struct i915_guc_client *client,
> > +                  uint16_t db_id)
> >   {
> >       struct guc_doorbell_info *doorbell;
> >       void *base;
> > @@ -184,8 +224,7 @@ static void guc_init_doorbell(struct intel_guc *guc,
> >       base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
> > 0));
> >       doorbell = base + client->doorbell_offset;
> > -    doorbell->db_status = 1;
> > -    doorbell->cookie = 0;
> > +    guc_update_doorbell_id(client, doorbell, db_id);
> >       kunmap_atomic(base);
> >   }
> > @@ -193,27 +232,16 @@ static void guc_init_doorbell(struct intel_guc
> > *guc,
> >   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;
> >       void *base;
> > -    i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
> > -    int value;
> >       base = kmap_atomic(i915_gem_object_get_page(client->client_obj,
> > 0));
> >       doorbell = base + client->doorbell_offset;
> > -    doorbell->db_status = 0;
> > +    guc_update_doorbell_id(client, doorbell, GUC_INVALID_DOORBELL_ID);
> >       kunmap_atomic(base);
> > -    I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
> > -
> > -    value = I915_READ(drbreg);
> > -    WARN_ON((value & GEN8_DRB_VALID) != 0);
> > -
> > -    I915_WRITE(GEN8_DRBREGU(client->doorbell_id), 0);
> > -    I915_WRITE(drbreg, 0);
> > -
> >       /* XXX: wait for any interrupts */
> >       /* XXX: wait for workqueue to drain */
> >   }
> > @@ -260,7 +288,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);
> >       DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
> >               hi_pri ? "high" : "normal", id);
> > @@ -268,11 +296,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.
> >    */
> > @@ -667,15 +690,7 @@ static void guc_client_free(struct drm_device *dev,
> >       if (!client)
> >           return;
> > -    if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) {
> > -        /*
> > -         * First disable the doorbell, then tell the GuC we've
> > -         * finished with it, finally deallocate it in our bitmap
> > -         */
> > -        guc_disable_doorbell(guc, client);
> > -        host2guc_release_doorbell(guc, client);
> > -        release_doorbell(guc, client->doorbell_id);
> > -    }
> > +    guc_disable_doorbell(guc, client);
> >       /*
> >        * XXX: wait for any outstanding submissions before freeing memory.
> > @@ -712,6 +727,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)
> > @@ -750,22 +766,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;

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