[Intel-xe] [CI 07/14] drm/xe: Split xe_info_init

[Lucas De Marchi]

On Wed, Nov 29, 2023 at 10:45:02PM +0100, Michał Winiarski wrote:
>Parts of xe_info_init are only dealing with processing driver_data.
>Extract it into xe_info_init_early to be able to use it earlier during
>probe.
>
>Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
>Reviewed-by: Matthew Brost <matthew.brost at intel.com>
>Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>---
> drivers/gpu/drm/xe/tests/xe_pci.c |  1 +
> drivers/gpu/drm/xe/xe_pci.c       | 44 ++++++++++++++++++++++---------
> 2 files changed, 32 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/tests/xe_pci.c b/drivers/gpu/drm/xe/tests/xe_pci.c
>index 306ff8cb35cb1..f2cdc9bb4612a 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_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>index 074ed456eba6d..e5f1fefcee120 100644
>--- a/drivers/gpu/drm/xe/xe_pci.c
>+++ b/drivers/gpu/drm/xe/xe_pci.c
>@@ -535,6 +535,35 @@ static void handle_gmdid(struct xe_device *xe,
> 	}
> }
>
>+/*
>+ * Initialize device info content that only depends on static driver_data passed to the driver at
>+ * probe time from PCI ID table.

The preference code width is still 80 cols, even if we don't warn if it
goes to 100 since some times it's better to keep it in one line rather
than split, which is not the case for comments/documentation like this.
Everywhere in this file has comments up to 80, so it becomes
inconsistent if we keep it like this.

>+ */
>+static void xe_info_init_early(struct xe_device *xe,
>+			       const struct xe_device_desc *desc,
>+			       const struct xe_subplatform_desc *subplatform_desc)
>+{
>+	xe->info.platform = desc->platform;
>+	xe->info.subplatform = subplatform_desc ?
>+		subplatform_desc->subplatform : XE_SUBPLATFORM_NONE;
>+
>+	xe->info.is_dgfx = desc->is_dgfx;
>+	xe->info.has_heci_gscfi = desc->has_heci_gscfi;
>+	xe->info.has_llc = desc->has_llc;
>+	xe->info.has_sriov = desc->has_sriov;
>+	xe->info.bypass_mtcfg = desc->bypass_mtcfg;
>+	xe->info.supports_mmio_ext = desc->supports_mmio_ext;
>+
>+	xe->info.enable_display = IS_ENABLED(CONFIG_DRM_XE_DISPLAY) &&
>+				  xe_modparam.enable_display &&
>+				  desc->has_display;
>+}
>+
>+/*
>+ * Initialize device info content that does require knowledge about graphics / media IP version.
>+ * Make sure that GT / tile structures allocated by the driver match the data present in device
>+ * info.
>+ */
> static int xe_info_init(struct xe_device *xe,
> 			const struct xe_device_desc *desc,
> 			const struct xe_subplatform_desc *subplatform_desc)
>@@ -546,10 +575,6 @@ static int xe_info_init(struct xe_device *xe,
> 	struct xe_gt *gt;
> 	u8 id;
>
>-	xe->info.platform = desc->platform;
>-	xe->info.subplatform = subplatform_desc ?
>-		subplatform_desc->subplatform : XE_SUBPLATFORM_NONE;
>-
> 	/*
> 	 * If this platform supports GMD_ID, we'll detect the proper IP
> 	 * descriptor to use from hardware registers. desc->graphics will only
>@@ -575,14 +600,8 @@ static int xe_info_init(struct xe_device *xe,
> 	if (!graphics_desc)
> 		return -ENODEV;
>
>-	xe->info.is_dgfx = desc->is_dgfx;
>-	xe->info.has_heci_gscfi = desc->has_heci_gscfi;
> 	xe->info.graphics_name = graphics_desc->name;
> 	xe->info.media_name = media_desc ? media_desc->name : "none";
>-	xe->info.has_llc = desc->has_llc;
>-	xe->info.has_sriov = desc->has_sriov;
>-	xe->info.bypass_mtcfg = desc->bypass_mtcfg;

now we have a conflict since this got renamed to skip and there's a new
skip. I fixed this locally, but then noticed the split is not 100%
accurate. Why don't we have skip_guc_pc in the in the early variant?

Does it make sence to have both xe_info_init() and xe_info_init_early()
receive xe_device_desc? It seems to me we would have a more clear cut
split of functionality if we had:

	xe_info_init_early(xe, desc, subplatform_desc);
	... /* find the correct graphics and media desc */
	xe_info_init(xe, graphics_desc, media_desc);

btw, subplatform_desc is not used in xe_info_init()..

Lucas De Marchi