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

[John Harrison <john.c.harrison@intel.com>]

On 11/18/2024 08:34, Daniele Ceraolo Spurio wrote:
> <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.
Same as for below - so it can free and re-allocate as necessary.

>
>>
>> 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.
The root GT doesn't do a get. It has a reference automatically from 
having created the object. Same as all the other GuC related objects 
that get created but never have a get/put called on them.

John.

>
> 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 */
>>>
>>
>