[Intel-xe] [PATCH 11/12] drm/xe/device: Introduce xe_device_probe_early
Lucas De Marchi
lucas.demarchi at intel.com
Tue Nov 14 14:18:11 UTC 2023
On Tue, Nov 14, 2023 at 11:02:15AM +0100, Michał Winiarski wrote:
>On Mon, Nov 13, 2023 at 09:11:18AM -0600, Lucas De Marchi wrote:
>> On Wed, Nov 08, 2023 at 01:33:43AM +0100, Michał Winiarski wrote:
>> > SR-IOV VF doesn't have access to MMIO registers used to determine
>> > graphics/media ID. It can however communicate with GuC.
>> > Introduce xe_device_probe_early, which will eventually initialize enough
>> > hardware to allow the driver to communicate with GuC early during probe.
>> > This will allow both VF and PF/native driver to have unified probe
>> > ordering.
>> > Initialize root tile MMIO and TTM sys mgr early, as both are needed to
>> > communicate with GuC firmware earlier during probe.
>> >
>> > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
>> > ---
>> > drivers/gpu/drm/xe/tests/xe_pci.c | 1 +
>> > drivers/gpu/drm/xe/xe_device.c | 21 +++++++++++++++------
>> > drivers/gpu/drm/xe/xe_device.h | 5 +++++
>> > drivers/gpu/drm/xe/xe_mmio.c | 17 +++++++++--------
>> > drivers/gpu/drm/xe/xe_mmio.h | 1 +
>> > drivers/gpu/drm/xe/xe_pci.c | 26 +++++++++++++++++++++-----
>> > 6 files changed, 52 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/tests/xe_pci.c b/drivers/gpu/drm/xe/tests/xe_pci.c
>> > index a40879da2fbe1..733df9a270db2 100644
>> > --- a/drivers/gpu/drm/xe/tests/xe_pci.c
>> > +++ b/drivers/gpu/drm/xe/tests/xe_pci.c
>> > @@ -143,6 +143,7 @@ int xe_pci_fake_device_init(struct xe_device *xe, enum xe_platform platform,
>> > return -ENODEV;
>> >
>> > done:
>> > + xe_info_init_early(xe, desc, subplatform_desc);
>> > xe_info_init(xe, desc, subplatform_desc);
>> >
>> > return 0;
>> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> > index 696f997bd2c93..19faddb779808 100644
>> > --- a/drivers/gpu/drm/xe/xe_device.c
>> > +++ b/drivers/gpu/drm/xe/xe_device.c
>> > @@ -364,6 +364,21 @@ static int xe_set_dma_info(struct xe_device *xe)
>> > return err;
>> > }
>> >
>>
>> The _early variants have different meaning all throughout this patch
>> series. Keep that consistent in future will be challenge if we don't
>> document what the split is between the _early (which used to be sw-only
>> part) and the normal init/probe.
>>
>> Please add a comment on top of each _early() variant you add with the
>> split of what should be there and what should rather be in the "normal"
>> init.
>
>Note that even in the "future" we might want to avoid having a rigid
>split between sw-only and HW communication.
>We do use MMIO regs for Graphics/Media IP version detection. Same thing
>will apply to SR-IOV VF mode detection. Those are pretty basic things
>that need to be done very early during probe.
>If we go sw-only route for "early" probe - we will end up initializing
>data structures that won't even be used by the driver, which is wasteful
>and makes the probe harder to understand. We would probably also need
>extra code (asserts) to warn on uninitialized resource usage.
>
>Or, we could just map everything as early as possible. Early would then
>just mean "early". Which is also what it means today, as the existing
>code prior to this series already has "early" functions that are not
>sw-only.
yeah, but all I asked is that for each case you make an explicit comment
on what is "early" for that particular case. Because it's not clear and
not consistent and will be a mess in a few month if we just keep adding
stuff at random places.
thanks
Lucas De Marchi
>
>Thanks,
>-Michał
>
>>
>> Lucas De Marchi
>>
>> > +int xe_device_probe_early(struct xe_device *xe)
>> > +{
>> > + int err;
>> > +
>> > + err = xe_mmio_init(xe);
>> > + if (err)
>> > + return err;
>> > +
>> > + xe_mmio_root_tile_init(xe);
>> > +
>> > + xe_ttm_sys_mgr_init(xe);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > int xe_device_probe(struct xe_device *xe)
>> > {
>> > struct xe_tile *tile;
>> > @@ -382,10 +397,6 @@ int xe_device_probe(struct xe_device *xe)
>> > if (err)
>> > return err;
>> >
>> > - err = xe_mmio_init(xe);
>> > - if (err)
>> > - return err;
>> > -
>> > xe_mmio_probe_tiles(xe);
>> >
>> > err = drmm_add_action_or_reset(&xe->drm, xe_driver_flr_fini, xe);
>> > @@ -418,8 +429,6 @@ int xe_device_probe(struct xe_device *xe)
>> > if (err)
>> > goto err_irq_shutdown;
>> >
>> > - xe_ttm_sys_mgr_init(xe);
>> > -
>> > for_each_tile(tile, xe, id) {
>> > err = xe_tile_init_noalloc(tile);
>> > if (err)
>> > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
>> > index ee89af6b1ea9c..5d7fd16f545c6 100644
>> > --- a/drivers/gpu/drm/xe/xe_device.h
>> > +++ b/drivers/gpu/drm/xe/xe_device.h
>> > @@ -37,6 +37,7 @@ static inline struct xe_device *ttm_to_xe_device(struct ttm_device *ttm)
>> >
>> > struct xe_device *xe_device_create(struct pci_dev *pdev,
>> > const struct pci_device_id *ent);
>> > +int xe_device_probe_early(struct xe_device *xe);
>> > int xe_device_probe(struct xe_device *xe);
>> > void xe_device_remove(struct xe_device *xe);
>> >
>> > @@ -122,6 +123,10 @@ static inline bool xe_device_uc_enabled(struct xe_device *xe)
>> > for ((id__) = 0; (id__) < (xe__)->info.tile_count; (id__)++) \
>> > for_each_if((tile__) = &(xe__)->tiles[(id__)])
>> >
>> > +#define for_each_remote_tile(tile__, xe__, id__) \
>> > + for ((id__) = 1; (id__) < (xe__)->info.tile_count; (id__)++) \
>> > + for_each_if((tile__) = &(xe__)->tiles[(id__)])
>> > +
>> > /*
>> > * FIXME: This only works for now since multi-tile and standalone media
>> > * happen to be mutually exclusive. Future platforms may change this...
>> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> > index 8945c601e1c54..526a6ac6be56a 100644
>> > --- a/drivers/gpu/drm/xe/xe_mmio.c
>> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> > @@ -15,10 +15,12 @@
>> > #include "regs/xe_regs.h"
>> > #include "xe_bo.h"
>> > #include "xe_device.h"
>> > +#include "xe_ggtt.h"
>> > #include "xe_gt.h"
>> > #include "xe_gt_mcr.h"
>> > #include "xe_macros.h"
>> > #include "xe_module.h"
>> > +#include "xe_tile.h"
>> >
>> > #define XEHP_MTCFG_ADDR XE_REG(0x101800)
>> > #define TILE_COUNT REG_GENMASK(15, 8)
>> > @@ -369,10 +371,8 @@ static void mmio_fini(struct drm_device *drm, void *arg)
>> >
>> > int xe_mmio_init(struct xe_device *xe)
>> > {
>> > - struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>> > struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> > const int mmio_bar = 0;
>> > - int err;
>> >
>> > /*
>> > * Map the entire BAR.
>> > @@ -386,15 +386,16 @@ int xe_mmio_init(struct xe_device *xe)
>> > return -EIO;
>> > }
>> >
>> > - err = drmm_add_action_or_reset(&xe->drm, mmio_fini, xe);
>> > - if (err)
>> > - return err;
>> > + return drmm_add_action_or_reset(&xe->drm, mmio_fini, xe);
>> > +}
>> > +
>> > +void xe_mmio_root_tile_init(struct xe_device *xe)
>> > +{
>> > + struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>> >
>> > /* Setup first tile; other tiles (if present) will be setup later. */
>> > - root_tile->mmio.size = xe->mmio.size;
>> > + root_tile->mmio.size = SZ_16M;
>> > root_tile->mmio.regs = xe->mmio.regs;
>> > -
>> > - return 0;
>> > }
>> >
>> > /**
>> > diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
>> > index 9f1a25cfd2cf0..894a792ac8abe 100644
>> > --- a/drivers/gpu/drm/xe/xe_mmio.h
>> > +++ b/drivers/gpu/drm/xe/xe_mmio.h
>> > @@ -21,6 +21,7 @@ struct xe_device;
>> > #define GEN12_LMEM_BAR 2
>> >
>> > int xe_mmio_init(struct xe_device *xe);
>> > +void xe_mmio_root_tile_init(struct xe_device *xe);
>> > void xe_mmio_probe_tiles(struct xe_device *xe);
>> >
>> > static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
>> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> > index 87c45c6e08aec..2f04f07dc17a4 100644
>> > --- a/drivers/gpu/drm/xe/xe_pci.c
>> > +++ b/drivers/gpu/drm/xe/xe_pci.c
>> > @@ -532,10 +532,12 @@ static void handle_gmdid(struct xe_device *xe,
>> > }
>> > }
>> >
>> > -static void xe_info_init_early(struct xe_device *xe,
>> > - const struct xe_device_desc *desc,
>> > - const struct xe_subplatform_desc *subplatform_desc)
>> > +static int xe_info_init_early(struct xe_device *xe,
>> > + const struct xe_device_desc *desc,
>> > + const struct xe_subplatform_desc *subplatform_desc)
>> > {
>> > + int err;
>> > +
>> > xe->info.platform = desc->platform;
>> > xe->info.subplatform = subplatform_desc ?
>> > subplatform_desc->subplatform : XE_SUBPLATFORM_NONE;
>> > @@ -549,6 +551,12 @@ static void xe_info_init_early(struct xe_device *xe,
>> > xe->info.enable_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
>> > enable_display &&
>> > desc->has_display;
>> > +
>> > + err = xe_tile_init_early(xe_device_get_root_tile(xe), xe, 0);
>> > + if (err)
>> > + return err;
>> > +
>> > + return 0;
>> > }
>> >
>> > static int xe_info_init(struct xe_device *xe,
>> > @@ -611,13 +619,15 @@ static int xe_info_init(struct xe_device *xe,
>> > */
>> > xe->info.tile_count = 1 + graphics_desc->max_remote_tiles;
>> >
>> > - for_each_tile(tile, xe, id) {
>> > + for_each_remote_tile(tile, xe, id) {
>> > int err;
>> >
>> > err = xe_tile_init_early(tile, xe, id);
>> > if (err)
>> > return err;
>> > + }
>> >
>> > + for_each_tile(tile, xe, id) {
>> > gt = tile->primary_gt;
>> > gt->info.id = xe->info.gt_count++;
>> > gt->info.type = XE_GT_TYPE_MAIN;
>> > @@ -717,7 +727,13 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> > pci_set_master(pdev);
>> > devm_add_action(&pdev->dev, xe_pci_clear_master, pdev);
>> >
>> > - xe_info_init_early(xe, desc, subplatform_desc);
>> > + err = xe_info_init_early(xe, desc, subplatform_desc);
>> > + if (err)
>> > + return err;
>> > +
>> > + err = xe_device_probe_early(xe);
>> > + if (err)
>> > + return err;
>> >
>> > err = xe_info_init(xe, desc, subplatform_desc);
>> > if (err)
>> > --
>> > 2.42.0
>> >
More information about the Intel-xe
mailing list