Re: [PATCH 2/2] drm/xe/guc: Add support for G2G communications

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

<snip>

>>
>>> +}
>>> +
>>> +static bool xe_guc_g2g_wanted(struct xe_device *xe)
>>> +{
>>> +    /* Can't do GuC to GuC communication if there is only one GuC */
>>> +    if (xe->info.gt_count <= 1)
>>> +        return false;
>>> +
>>> +    /* No current user */
>>> +    return false;
>>> +}
>>> +
>>> +static int guc_g2g_alloc(struct xe_guc *guc)
>>> +{
>>> +    struct xe_gt *gt = guc_to_gt(guc);
>>> +    struct xe_device *xe = gt_to_xe(gt);
>>> +    struct xe_tile *tile = gt_to_tile(gt);
>>> +    struct xe_bo *bo;
>>> +    u32 g2g_size;
>>> +
>>> +    if (guc->g2g.bo)
>>> +        return 0;
>>> +
>>> +    if (gt->info.id != 0) {
>>> +        struct xe_gt *root_gt = xe_device_get_gt(xe, 0);
>>> +        struct xe_guc *root_guc = &root_gt->uc.guc;
>>> +        struct xe_bo *bo;
>>> +
>>> +        bo = xe_bo_get(root_guc->g2g.bo);
>>> +        if (IS_ERR(bo))
>>> +            return PTR_ERR(bo);
>>> +
>>> +        guc->g2g.bo = bo;
>>> +        guc->g2g.owned = false;
>>
>> It doesn't look like you're actually using the "owned" variable 
>> (which makes sense, because the BO is refcounted so it doesn't matter 
>> which GuC it was originally allocated for)
> Yeah, owned is only there for the selftest that comes later. But not 
> including it now means the g2g structure only has a single entry, so 
> really should not be a structure. Which suddenly makes the deltas for 
> adding in owned later significantly larger!

Can you explain why the selftest would need to know that? Can't review 
it now if we can't see how it is supposed to be used.

>
> Having said that, as Matthew pointed out, the code is currently 
> leaking reference counts as they are not auto-cleaned. So it does 
> actually need a fini function which will use the owned field to decide 
> whether to call put or not.

Why? If you do a get() from every GuC then you need to do a put() from 
every GuC, no need to check.

Daniele

>
> John.
>
>
>>
>> Daniele
>>
>>> +        return 0;
>>> +    }
>>> +
>>> +    g2g_size = guc_g2g_size(guc);
>>> +    bo = xe_managed_bo_create_pin_map(xe, tile, g2g_size,
>>> +                      XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>>> +                      XE_BO_FLAG_GGTT |
>>> +                      XE_BO_FLAG_GGTT_ALL |
>>> +                      XE_BO_FLAG_GGTT_INVALIDATE);
>>> +    if (IS_ERR(bo))
>>> +        return PTR_ERR(bo);
>>> +
>>> +    xe_map_memset(xe, &bo->vmap, 0, 0, g2g_size);
>>> +    guc->g2g.bo = bo;
>>> +    guc->g2g.owned = true;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int guc_g2g_start(struct xe_guc *guc)
>>> +{
>>> +    struct xe_gt *far_gt, *gt = guc_to_gt(guc);
>>> +    struct xe_device *xe = gt_to_xe(gt);
>>> +    unsigned int i, j;
>>> +    int t, err;
>>> +    bool have_dev;
>>> +
>>> +    if (!guc->g2g.bo) {
>>> +        int ret;
>>> +
>>> +        ret = guc_g2g_alloc(guc);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    /* GuC interface will need extending if more GT device types 
>>> are ever created. */
>>> +    xe_gt_assert(gt, (gt->info.type == XE_GT_TYPE_MAIN) || 
>>> (gt->info.type == XE_GT_TYPE_MEDIA));
>>> +
>>> +    /* Channel numbering depends on whether there are multiple GTs 
>>> per tile */
>>> +    have_dev = xe->info.gt_count > xe->info.tile_count;
>>> +
>>> +    for_each_gt(far_gt, xe, i) {
>>> +        u32 far_tile, far_dev;
>>> +
>>> +        if (far_gt->info.id == gt->info.id)
>>> +            continue;
>>> +
>>> +        far_tile = gt_to_tile(far_gt)->id;
>>> +        far_dev = G2G_DEV(far_gt);
>>> +
>>> +        for (t = 0; t < XE_G2G_TYPE_LIMIT; t++) {
>>> +            err = guc_g2g_register(guc, far_gt, t, have_dev);
>>> +            if (err) {
>>> +                while (--t >= 0)
>>> +                    guc_g2g_deregister(guc, far_tile, far_dev, t);
>>> +                goto err_deregister;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err_deregister:
>>> +    for_each_gt(far_gt, xe, j) {
>>> +        u32 tile, dev;
>>> +
>>> +        if (far_gt->info.id == gt->info.id)
>>> +            continue;
>>> +
>>> +        if (j >= i)
>>> +            break;
>>> +
>>> +        tile = gt_to_tile(far_gt)->id;
>>> +        dev = G2G_DEV(far_gt);
>>> +
>>> +        for (t = 0; t < XE_G2G_TYPE_LIMIT; t++)
>>> +            guc_g2g_deregister(guc, tile, dev, t);
>>> +    }
>>> +
>>> +    return err;
>>> +}
>>> +
>>>   static void guc_fini_hw(void *arg)
>>>   {
>>>       struct xe_guc *guc = arg;
>>> @@ -423,7 +688,16 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>>>     int xe_guc_post_load_init(struct xe_guc *guc)
>>>   {
>>> +    int ret;
>>> +
>>>       xe_guc_ads_populate_post_load(&guc->ads);
>>> +
>>> +    if (xe_guc_g2g_wanted(guc_to_xe(guc))) {
>>> +        ret = guc_g2g_start(guc);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>>       guc->submission_state.enabled = true;
>>>         return 0;
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_types.h 
>>> b/drivers/gpu/drm/xe/xe_guc_types.h
>>> index fa75f57bf5da..83a41ebcdc91 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_types.h
>>> @@ -64,6 +64,15 @@ struct xe_guc {
>>>       struct xe_guc_pc pc;
>>>       /** @dbm: GuC Doorbell Manager */
>>>       struct xe_guc_db_mgr dbm;
>>> +
>>> +    /** @g2g: GuC to GuC communication state */
>>> +    struct {
>>> +        /** @g2g.bo: Storage for GuC to GuC communication channels */
>>> +        struct xe_bo *bo;
>>> +        /** @g2g.owned: Is the BO owned by this GT or just mapped 
>>> in */
>>> +        bool owned;
>>> +    } g2g;
>>> +
>>>       /** @submission_state: GuC submission state */
>>>       struct {
>>>           /** @submission_state.idm: GuC context ID Manager */
>>> @@ -79,6 +88,7 @@ struct xe_guc {
>>>           /** @submission_state.fini_wq: submit fini wait queue */
>>>           wait_queue_head_t fini_wq;
>>>       } submission_state;
>>> +
>>>       /** @hwconfig: Hardware config state */
>>>       struct {
>>>           /** @hwconfig.bo: buffer object of the hardware config */
>>
>