[PATCH 3/8] drm/xe: Simplify GuC early initialization
Lucas De Marchi
lucas.demarchi at intel.com
Tue Apr 1 19:02:28 UTC 2025
On Wed, Mar 12, 2025 at 11:43:27AM +0100, Maarten Lankhorst wrote:
>Hey,
>
>On 2025-03-11 21:48, Lucas De Marchi wrote:
>> On Mon, Mar 10, 2025 at 09:06:48PM +0100, Maarten Lankhorst wrote:
>>> Add a 2-stage GuC init. An early one for everything that is needed
>>
>> it's already a 2-stage load with
>>
>> 1) min load for hwconfig;
>> 2) normal load
>>
>> can you detail how are the split now?
>>
>>> for VF, and a full one that loads GuC and is allowed to do allocations.
>>>
>>> Signed-off-by: Maarten Lankhorst <dev at lankhorst.se>
>>> [mlankhorst: Fix british spelling]
>>
>> what is this trailer supposed to mean?
>>
>>> ---
>>> drivers/gpu/drm/xe/tests/xe_guc_relay_test.c | 2 +-
>>> drivers/gpu/drm/xe/xe_device.c | 16 ------
>>> drivers/gpu/drm/xe/xe_gt.c | 9 +++-
>>> drivers/gpu/drm/xe/xe_guc.c | 51 ++++++++++++--------
>>> drivers/gpu/drm/xe/xe_guc.h | 1 +
>>> drivers/gpu/drm/xe/xe_guc_ct.c | 28 +++++++----
>>> drivers/gpu/drm/xe/xe_guc_ct.h | 1 +
>>> drivers/gpu/drm/xe/xe_guc_relay.c | 6 +--
>>> drivers/gpu/drm/xe/xe_guc_relay.h | 2 +-
>>> drivers/gpu/drm/xe/xe_uc.c | 16 ++++++
>>> drivers/gpu/drm/xe/xe_uc.h | 1 +
>>> 11 files changed, 82 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/tests/xe_guc_relay_test.c b/drivers/gpu/drm/xe/tests/xe_guc_relay_test.c
>>> index 13701451b9235..b951d55ef1b32 100644
>>> --- a/drivers/gpu/drm/xe/tests/xe_guc_relay_test.c
>>> +++ b/drivers/gpu/drm/xe/tests/xe_guc_relay_test.c
>>> @@ -42,7 +42,7 @@ static int relay_test_init(struct kunit *test)
>>> kunit_activate_static_stub(test, relay_get_totalvfs,
>>> replacement_relay_get_totalvfs);
>>>
>>> - KUNIT_ASSERT_EQ(test, xe_guc_relay_init(relay), 0);
>>> + KUNIT_ASSERT_EQ(test, xe_guc_relay_init_noalloc(relay), 0);
>>> KUNIT_EXPECT_TRUE(test, relay_is_ready(relay));
>>> relay->last_rid = TEST_RID - 1;
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>>> index 38b8e3e961a2f..343f7ac67eaee 100644
>>> --- a/drivers/gpu/drm/xe/xe_device.c
>>> +++ b/drivers/gpu/drm/xe/xe_device.c
>>> @@ -772,25 +772,9 @@ int xe_device_probe(struct xe_device *xe)
>>> err = xe_gt_init_early(gt);
>>> if (err)
>>> return err;
>>> -
>>> - /*
>>> - * Only after this point can GT-specific MMIO operations
>>> - * (including things like communication with the GuC)
>>> - * be performed.
>>> - */
>>> - xe_gt_mmio_init(gt);
>>> }
>>>
>>> for_each_tile(tile, xe, id) {
>>> - if (IS_SRIOV_VF(xe)) {
>>> - xe_guc_comm_init_early(&tile->primary_gt->uc.guc);
>>> - err = xe_gt_sriov_vf_bootstrap(tile->primary_gt);
>>> - if (err)
>>> - return err;
>>> - err = xe_gt_sriov_vf_query_config(tile->primary_gt);
>>> - if (err)
>>> - return err;
>>> - }
>>> err = xe_ggtt_init_early(tile->mem.ggtt);
>>> if (err)
>>> return err;
>>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>>> index 10a9e3c72b360..a0b9052c41b0f 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>>> @@ -375,7 +375,14 @@ int xe_gt_init_early(struct xe_gt *gt)
>>> if (err)
>>> return err;
>>>
>>> - return 0;
>>> + /*
>>> + * Only after this point can GT-specific MMIO operations
>>> + * (including things like communication with the GuC)
>>> + * be performed.
>>> + */
>>> + xe_gt_mmio_init(gt);
>>> +
>>> + return xe_uc_init_noalloc(>->uc);
>>> }
>>>
>>> static void dump_pat_on_error(struct xe_gt *gt)
>>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>>> index ecf597f9d80c6..bd8c3e5a5c9fa 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>>> @@ -626,21 +626,11 @@ static int xe_guc_realloc_post_hwconfig(struct xe_guc *guc)
>>> return 0;
>>> }
>>>
>>> -static int vf_guc_init(struct xe_guc *guc)
>>> +static int vf_guc_init_noalloc(struct xe_guc *guc)
>>> {
>>> struct xe_gt *gt = guc_to_gt(guc);
>>> int err;
>>>
>>> - xe_guc_comm_init_early(guc);
>>> -
>>> - err = xe_guc_ct_init(&guc->ct);
>>> - if (err)
>>> - return err;
>>> -
>>> - err = xe_guc_relay_init(&guc->relay);
>>> - if (err)
>>> - return err;
>>> -
>>> err = xe_gt_sriov_vf_bootstrap(gt);
>>> if (err)
>>> return err;
>>> @@ -652,6 +642,35 @@ static int vf_guc_init(struct xe_guc *guc)
>>> return 0;
>>> }
>>>
>>> +int xe_guc_init_noalloc(struct xe_guc *guc)
>>> +{
>>> + struct xe_device *xe = guc_to_xe(guc);
>>> + struct xe_gt *gt = guc_to_gt(guc);
>>> + int ret;
>>> +
>>> + xe_guc_comm_init_early(guc);
>>> +
>>> + ret = xe_guc_ct_init_noalloc(&guc->ct);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + ret = xe_guc_relay_init_noalloc(&guc->relay);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + if (IS_SRIOV_VF(xe)) {
>>> + ret = vf_guc_init_noalloc(guc);
>>> + if (ret)
>>> + goto out;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +out:
>>> + xe_gt_err(gt, "GuC init failed with %pe\n", ERR_PTR(ret));
>>> + return ret;
>>> +}
>>> +
>>> int xe_guc_init(struct xe_guc *guc)
>>> {
>>> struct xe_device *xe = guc_to_xe(guc);
>>> @@ -661,13 +680,13 @@ int xe_guc_init(struct xe_guc *guc)
>>> guc->fw.type = XE_UC_FW_TYPE_GUC;
>>> ret = xe_uc_fw_init(&guc->fw);
>>> if (ret)
>>> - goto out;
>>> + return ret;
>>>
>>> if (!xe_uc_fw_is_enabled(&guc->fw))
>>> return 0;
>>>
>>> if (IS_SRIOV_VF(xe)) {
>>> - ret = vf_guc_init(guc);
>>> + ret = xe_guc_ct_init(&guc->ct);
>>> if (ret)
>>> goto out;
>>> return 0;
>>> @@ -689,10 +708,6 @@ int xe_guc_init(struct xe_guc *guc)
>>> if (ret)
>>> goto out;
>>>
>>> - ret = xe_guc_relay_init(&guc->relay);
>>> - if (ret)
>>> - goto out;
>>> -
>>> xe_uc_fw_change_status(&guc->fw, XE_UC_FIRMWARE_LOADABLE);
>>>
>>> ret = devm_add_action_or_reset(xe->drm.dev, guc_fini_hw, guc);
>>> @@ -701,8 +716,6 @@ int xe_guc_init(struct xe_guc *guc)
>>>
>>> guc_init_params(guc);
>>>
>>> - xe_guc_comm_init_early(guc);
>>> -
>>> return 0;
>>>
>>> out:
>>> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
>>> index 58338be445585..965bf72912009 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc.h
>>> @@ -26,6 +26,7 @@
>>> struct drm_printer;
>>>
>>> void xe_guc_comm_init_early(struct xe_guc *guc);
>>> +int xe_guc_init_noalloc(struct xe_guc *guc);
>>> int xe_guc_init(struct xe_guc *guc);
>>> int xe_guc_init_post_hwconfig(struct xe_guc *guc);
>>> int xe_guc_post_load_init(struct xe_guc *guc);
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>>> index 72ad576fc18eb..ffeda89b136ba 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>>> @@ -204,12 +204,10 @@ static void primelockdep(struct xe_guc_ct *ct)
>>> fs_reclaim_release(GFP_KERNEL);
>>> }
>>>
>>> -int xe_guc_ct_init(struct xe_guc_ct *ct)
>>> +int xe_guc_ct_init_noalloc(struct xe_guc_ct *ct)
>>> {
>>> struct xe_device *xe = ct_to_xe(ct);
>>> struct xe_gt *gt = ct_to_gt(ct);
>>> - struct xe_tile *tile = gt_to_tile(gt);
>>> - struct xe_bo *bo;
>>> int err;
>>>
>>> xe_gt_assert(gt, !(guc_ct_size() % PAGE_SIZE));
>>> @@ -235,6 +233,23 @@ int xe_guc_ct_init(struct xe_guc_ct *ct)
>>>
>>> primelockdep(ct);
>>>
>>> + err = drmm_add_action_or_reset(&xe->drm, guc_ct_fini, ct);
>>> + if (err)
>>> + return err;
>>> +
>>> + xe_gt_assert(gt, ct->state == XE_GUC_CT_STATE_NOT_INITIALIZED);
>>> + ct->state = XE_GUC_CT_STATE_DISABLED;
>>> + return 0;
>>> +}
>>> +ALLOW_ERROR_INJECTION(xe_guc_ct_init_noalloc, ERRNO); /* See xe_pci_probe() */
>>> +
>>> +int xe_guc_ct_init(struct xe_guc_ct *ct)
>>> +{
>>> + struct xe_device *xe = ct_to_xe(ct);
>>> + struct xe_gt *gt = ct_to_gt(ct);
>>> + struct xe_tile *tile = gt_to_tile(gt);
>>> + struct xe_bo *bo;
>>> +
>>> bo = xe_managed_bo_create_pin_map(xe, tile, guc_ct_size(),
>>> XE_BO_FLAG_SYSTEM |
>>> XE_BO_FLAG_GGTT |
>>> @@ -243,13 +258,6 @@ int xe_guc_ct_init(struct xe_guc_ct *ct)
>>> return PTR_ERR(bo);
>>>
>>> ct->bo = bo;
>>> -
>>> - err = drmm_add_action_or_reset(&xe->drm, guc_ct_fini, ct);
>>> - if (err)
>>> - return err;
>>> -
>>> - xe_gt_assert(gt, ct->state == XE_GUC_CT_STATE_NOT_INITIALIZED);
>>> - ct->state = XE_GUC_CT_STATE_DISABLED;
>>> return 0;
>>> }
>>> ALLOW_ERROR_INJECTION(xe_guc_ct_init, ERRNO); /* See xe_pci_probe() */
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
>>> index 82c4ae458dda3..9f28fc135890d 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
>>> @@ -11,6 +11,7 @@
>>> struct drm_printer;
>>> struct xe_device;
>>>
>>> +int xe_guc_ct_init_noalloc(struct xe_guc_ct *ct);
>>> int xe_guc_ct_init(struct xe_guc_ct *ct);
>>> int xe_guc_ct_enable(struct xe_guc_ct *ct);
>>> void xe_guc_ct_disable(struct xe_guc_ct *ct);
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_relay.c b/drivers/gpu/drm/xe/xe_guc_relay.c
>>> index e5dc94f3e6181..dc60955a0271a 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_relay.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_relay.c
>>> @@ -322,7 +322,7 @@ static void __fini_relay(struct drm_device *drm, void *arg)
>>> }
>>>
>>> /**
>>> - * xe_guc_relay_init - Initialize a &xe_guc_relay
>>> + * xe_guc_relay_init_noalloc - Initialize a &xe_guc_relay
>>> * @relay: the &xe_guc_relay to initialize
>>> *
>>> * Initialize remaining members of &xe_guc_relay that may depend
>>> @@ -330,7 +330,7 @@ static void __fini_relay(struct drm_device *drm, void *arg)
>>> *
>>> * Return: 0 on success or a negative error code on failure.
>>> */
>>> -int xe_guc_relay_init(struct xe_guc_relay *relay)
>>> +int xe_guc_relay_init_noalloc(struct xe_guc_relay *relay)
>>> {
>>> const int XE_RELAY_MEMPOOL_MIN_NUM = 1;
>>> struct xe_device *xe = relay_to_xe(relay);
>>> @@ -356,7 +356,7 @@ int xe_guc_relay_init(struct xe_guc_relay *relay)
>>>
>>> return drmm_add_action_or_reset(&xe->drm, __fini_relay, relay);
>>> }
>>> -ALLOW_ERROR_INJECTION(xe_guc_relay_init, ERRNO); /* See xe_pci_probe() */
>>> +ALLOW_ERROR_INJECTION(xe_guc_relay_init_noalloc, ERRNO); /* See xe_pci_probe() */
>>>
>>> static u32 to_relay_error(int err)
>>> {
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_relay.h b/drivers/gpu/drm/xe/xe_guc_relay.h
>>> index 385429aa188ab..e0afff4542cfc 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_relay.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_relay.h
>>> @@ -11,7 +11,7 @@
>>>
>>> struct xe_guc_relay;
>>>
>>> -int xe_guc_relay_init(struct xe_guc_relay *relay);
>>> +int xe_guc_relay_init_noalloc(struct xe_guc_relay *relay);
>>>
>>> int xe_guc_relay_send_to_pf(struct xe_guc_relay *relay,
>>> const u32 *msg, u32 len, u32 *buf, u32 buf_size);
>>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>>> index c14bd22820441..6adff7dffea50 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc.c
>>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>>> @@ -33,6 +33,22 @@ uc_to_xe(struct xe_uc *uc)
>>> }
>>>
>>> /* Should be called once at driver load only */
>>> +int xe_uc_init_noalloc(struct xe_uc *uc)
>>> +{
>>> + int ret;
>>> +
>>> + ret = xe_guc_init_noalloc(&uc->guc);
>>> + if (ret)
>>> + goto err;
>>> +
>>> + /* HuC and GSC have no early dependencies and can be completely initialized during full xe_uc_init(). */
>>> + return 0;
>>> +
>>> +err:
>>> + xe_gt_err(uc_to_gt(uc), "Failed to early initialize uC (%pe)\n", ERR_PTR(ret));
>>> + return ret;
>>> +}
>>> +
>>> int xe_uc_init(struct xe_uc *uc)
>>> {
>>> int ret;
>>> diff --git a/drivers/gpu/drm/xe/xe_uc.h b/drivers/gpu/drm/xe/xe_uc.h
>>> index 3813c1ede450e..4003eb79757a7 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc.h
>>> +++ b/drivers/gpu/drm/xe/xe_uc.h
>>> @@ -8,6 +8,7 @@
>>>
>>> struct xe_uc;
>>>
>>> +int xe_uc_init_noalloc(struct xe_uc *uc);
>>> int xe_uc_init(struct xe_uc *uc);
>>> int xe_uc_init_hwconfig(struct xe_uc *uc);
>>> int xe_uc_init_post_hwconfig(struct xe_uc *uc);
>>
>>
>> I think we need some documentation and maybe init state keeping.
>> It used to be: init_early() and init(), with the first usually being
>> only about SW, no hw. But even with some hw initialization, the order
>> was clear: now with a mix of init(), init_noalloc(), init_early(),
>> init_hwconfig() it's hard to follow.
>
>Since uc_init is called right before init_hwconfig, those could be merged into etiher
>xe_uc_init or xe_uc_init_hwconfig?
>
>I'm also for renaming xe_uc_init_noalloc to xe_uc_init_early.
>
>I think the steps we have now boil (with this series) down to this:
>- GT early init. Here some communication need to happen for VF, so early initialisation can be performed.
> Some sw init to support this may happen.
>- Full UC init, now any necessary allocation can be performed, hwconfig hasn't happened yet. Functions uc_init() and uc_init_hwconfig() -- merge?
>- Post HWConfig init
>- xe_uc_init_hw, could potentially be called from post HWConfig init and renamed to uc_(reinit/prepare)_hw() for reset? Probably bikeshed.
I'm fine if we do some renaming on top.
Lucas De Marchi
More information about the Intel-xe
mailing list