Re: [PATCH] drm/xe/guc: Move ADS BO realloc to xe_guc_ads code

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

On 9/8/2025 1:20 AM, Michal Wajdeczko wrote:
> On 9/5/2025 10:03 PM, John Harrison wrote:
>> On 9/5/2025 7:35 AM, Michal Wajdeczko wrote:
>>> We already have dedicated GuC ADS function that is called during
>>> the post_hwconfig step. Move ADS BO reallocation there to have all
>>> of ADS BO management together. Note that since any BO reinit must
>>> be done prior to the point where we prepare GuC boot params, as
>>> those include BO offsets, we have to move up call to ADS function.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/xe_guc.c     | 10 +++++-----
>>>    drivers/gpu/drm/xe/xe_guc_ads.c |  8 ++++++++
>>>    2 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>>> index b3a6408a5760..a62c483c9d88 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>>> @@ -705,10 +705,6 @@ static int xe_guc_realloc_post_hwconfig(struct xe_guc *guc)
>>>        if (ret)
>>>            return ret;
>>>    -    ret = xe_managed_bo_reinit_in_vram(xe, tile, &guc->ads.bo);
>>> -    if (ret)
>>> -        return ret;
>>> -
>> But the sole purpose of 'xe_guc_realloc_post_hwconfig' is to do the re-init in VRAM for all GuC objects. Why split the ADS object out and then have to re-order other things to cope. This is the sensible place for it to happen.
> But those "GuC" objects are not directly owned by the GuC (aka xe_guc), nor were directly allocated by the xe_guc.
> Instead they were allocated by the GuC sub-components in their corresponding init() functions:
>
> * GuC CT - see xe_guc_ct_init()
> * GuC ADS - see xe_guc_ads_init()
> * GuC Log - see xe_guc_log_init()
> *..
>
> So since we logically separated CT/ADS/LOG into dedicated sub-components, handling their internal data from the GuC level is a clear violation of the layering and we should continue to let sub-components manage their data on their own, and this means that it is up to them to reinit theirs BO at the post-hwc step.
>
> Note that in case of ADS it might even mean that ADS size can be adjusted as we know actual config (something that is not possible to be done at upper GuC level)
Then surely the correct change is to move all those re-init calls into 
per-sub-component sub-functions that are called from 
xe_guc_realloc_post_hwconfig. Layering is just a matter of adding extra 
function calls into the stack. The point is that this function was 
explicitly added as being the correct point in time / sequence to do the 
relloc operations. So having most reallocs at that point and one 
somewhere else entirely and having to hack around other sequencing to 
make that work, does not seem like the correct solution.

John.

>
>> John.
>>
>>
>>>        return 0;
>>>    }
>>>    @@ -847,6 +843,10 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>>>        if (ret)
>>>            return ret;
>>>    +    ret = xe_guc_ads_init_post_hwconfig(&guc->ads);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>        guc_init_params_post_hwconfig(guc);
>>>          ret = xe_guc_submit_init(guc, ~0);
>>> @@ -869,7 +869,7 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>>>        if (ret)
>>>            return ret;
>>>    -    return xe_guc_ads_init_post_hwconfig(&guc->ads);
>>> +    return 0;
>>>    }
>>>      int xe_guc_post_load_init(struct xe_guc *guc)
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>>> index 5631722f34f5..fc2c37ca9dbc 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>> @@ -424,11 +424,19 @@ ALLOW_ERROR_INJECTION(xe_guc_ads_init, ERRNO); /* See xe_pci_probe() */
>>>     */
>>>    int xe_guc_ads_init_post_hwconfig(struct xe_guc_ads *ads)
>>>    {
>>> +    struct xe_device *xe = ads_to_xe(ads);
>>>        struct xe_gt *gt = ads_to_gt(ads);
>>>        u32 prev_regset_size = ads->regset_size;
>>> +    int ret;
>>>          xe_gt_assert(gt, ads->bo);
>>>    +    if (IS_DGFX(xe)) {
>>> +        ret = xe_managed_bo_reinit_in_vram(xe, gt->tile, &ads->bo);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>>        ads->golden_lrc_size = calculate_golden_lrc_size(ads);
>>>        /* Calculate Capture size with worst size */
>>>        ads->capture_size = xe_guc_capture_ads_input_worst_size(ads_to_guc(ads));