[Intel-xe] [PATCH v3 20/20] drm/xe: Initialize GuC earlier during probe
Michał Winiarski
michal.winiarski at intel.com
Tue Nov 21 16:07:57 UTC 2023
On Wed, Nov 15, 2023 at 03:09:31PM +0000, Matthew Brost wrote:
> On Tue, Nov 14, 2023 at 02:02:31PM +0100, Michał Winiarski wrote:
> > SR-IOV VF has limited access to MMIO registers. Fortunately, it is able
> > to access a curated subset that is needed to initialize the driver by
> > communicating with SR-IOV PF using GuC CT.
> > Initialize GuC earlier in order to keep the unified probe ordering
> > between VF and PF modes.
> >
> > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_device.c | 11 +++++++++++
> > drivers/gpu/drm/xe/xe_gt.c | 16 ++++------------
> > drivers/gpu/drm/xe/xe_guc.c | 15 ++++++++++-----
> > drivers/gpu/drm/xe/xe_uc.c | 35 ++++++++++++++++++++++++++--------
> > 4 files changed, 52 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 531aca74e8eb4..ecbf1effecc41 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -32,6 +32,7 @@
> > #include "xe_pat.h"
> > #include "xe_pcode.h"
> > #include "xe_pm.h"
> > +#include "xe_uc.h"
> > #include "xe_query.h"
> > #include "xe_tile.h"
> > #include "xe_ttm_stolen_mgr.h"
> > @@ -414,6 +415,16 @@ int xe_device_probe(struct xe_device *xe)
> > return err;
> > }
> >
> > + for_each_gt(gt, xe, id) {
> > + err = xe_uc_init(>->uc);
> > + if (err)
> > + return err;
> > +
> > + err = xe_uc_init_hwconfig(>->uc);
> > + if (err)
> > + return err;
> > + }
> > +
>
> I think this should be a GT level function rather touching the UC layer
> here. Also maybe combine xe_force_wake_init_gt & xe_ggtt_init_early into
> this new function too if it makes sense.
>
> Also a comment of why this located wouldn't hurt either.
>
> Other than these nits the patch itslef makes sense to me.
>
> Matt
I'll introduce xe_gt_init_hwconfig for uc_init / uc_init_hwconfig.
Combining other things (forcewake / ggtt) into it is problematic and
doesn't really make sense to me.
GGTT is per-tile, while forcewake needs to be initialized for all GTs
(for TLB invalidation purpose) before doing GGTT mappings (which uc_init
is going to do), so we can't easily handle it in a single for_each_gt
loop.
Thanks,
-Michał
>
> > err = drmm_add_action_or_reset(&xe->drm, xe_driver_flr_fini, xe);
> > if (err)
> > return err;
> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > index 73665e4e66f22..bee661e1f164a 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.c
> > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > @@ -344,14 +344,6 @@ static int gt_fw_domain_init(struct xe_gt *gt)
> > goto err_force_wake;
> > }
> >
> > - err = xe_uc_init(>->uc);
> > - if (err)
> > - goto err_force_wake;
> > -
> > - err = xe_uc_init_hwconfig(>->uc);
> > - if (err)
> > - goto err_force_wake;
> > -
> > xe_gt_idle_sysfs_init(>->gtidle);
> >
> > /* XXX: Fake that we pull the engine mask from hwconfig blob */
> > @@ -415,10 +407,6 @@ static int all_fw_domain_init(struct xe_gt *gt)
> > if (err)
> > goto err_force_wake;
> >
> > - err = xe_uc_init_post_hwconfig(>->uc);
> > - if (err)
> > - goto err_force_wake;
> > -
> > if (!xe_gt_is_media_type(gt)) {
> > /*
> > * USM has its only SA pool to non-block behind user operations
> > @@ -442,6 +430,10 @@ static int all_fw_domain_init(struct xe_gt *gt)
> > }
> > }
> >
> > + err = xe_uc_init_post_hwconfig(>->uc);
> > + if (err)
> > + goto err_force_wake;
> > +
> > err = xe_uc_init_hw(>->uc);
> > if (err)
> > goto err_force_wake;
> > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> > index e04b04be32b7d..895bcc24ae2c8 100644
> > --- a/drivers/gpu/drm/xe/xe_guc.c
> > +++ b/drivers/gpu/drm/xe/xe_guc.c
> > @@ -572,26 +572,31 @@ static int __xe_guc_upload(struct xe_guc *guc)
> > */
> > int xe_guc_min_load_for_hwconfig(struct xe_guc *guc)
> > {
> > + struct xe_gt *gt = guc_to_gt(guc);
> > int ret;
> >
> > xe_guc_ads_populate_minimal(&guc->ads);
> >
> > + ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> > + if (ret)
> > + return ret;
> > +
> > /* Raise GT freq to speed up HuC/GuC load */
> > xe_guc_pc_init_early(&guc->pc);
> >
> > ret = __xe_guc_upload(guc);
> > if (ret)
> > - return ret;
> > + goto out;
> >
> > ret = xe_guc_hwconfig_init(guc);
> > if (ret)
> > - return ret;
> > + goto out;
> >
> > ret = xe_guc_enable_communication(guc);
> > - if (ret)
> > - return ret;
> > +out:
> > + xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > int xe_guc_upload(struct xe_guc *guc)
> > diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> > index 13e76e6805ca1..069207a7f2f21 100644
> > --- a/drivers/gpu/drm/xe/xe_uc.c
> > +++ b/drivers/gpu/drm/xe/xe_uc.c
> > @@ -29,8 +29,16 @@ uc_to_xe(struct xe_uc *uc)
> > /* Should be called once at driver load only */
> > int xe_uc_init(struct xe_uc *uc)
> > {
> > + struct xe_device *xe = uc_to_xe(uc);
> > + struct xe_gt *gt = uc_to_gt(uc);
> > int ret;
> >
> > + xe_device_mem_access_get(xe);
> > +
> > + ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> > + if (ret)
> > + goto err;
> > +
> > /*
> > * We call the GuC/HuC init functions even if GuC submission is off to
> > * correctly move our tracking of the FW state to "disabled".
> > @@ -38,26 +46,36 @@ int xe_uc_init(struct xe_uc *uc)
> >
> > ret = xe_guc_init(&uc->guc);
> > if (ret)
> > - goto err;
> > + goto err_fw;
> >
> > ret = xe_huc_init(&uc->huc);
> > if (ret)
> > - goto err;
> > + goto err_fw;
> >
> > if (!xe_device_uc_enabled(uc_to_xe(uc)))
> > - return 0;
> > + goto out;
> > +
> >
> > ret = xe_wopcm_init(&uc->wopcm);
> > if (ret)
> > - goto err;
> > + goto err_fw;
> >
> > ret = xe_guc_submit_init(&uc->guc);
> > if (ret)
> > - goto err;
> > + goto err_fw;
> > +
> > +out:
> > + xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> > +
> > + xe_device_mem_access_put(xe);
> >
> > return 0;
> >
> > +err_fw:
> > + xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> > err:
> > + xe_device_mem_access_put(xe);
> > +
> > return ret;
> > }
> >
> > @@ -117,17 +135,18 @@ int xe_uc_sanitize_reset(struct xe_uc *uc)
> > */
> > int xe_uc_init_hwconfig(struct xe_uc *uc)
> > {
> > + struct xe_device *xe = uc_to_xe(uc);
> > int ret;
> >
> > /* GuC submission not enabled, nothing to do */
> > if (!xe_device_uc_enabled(uc_to_xe(uc)))
> > return 0;
> >
> > + xe_device_mem_access_get(xe);
> > ret = xe_guc_min_load_for_hwconfig(&uc->guc);
> > - if (ret)
> > - return ret;
> > + xe_device_mem_access_put(xe);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > /*
> > --
> > 2.42.1
> >
More information about the Intel-xe
mailing list