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

[Michał Winiarski]

On Thu, Nov 30, 2023 at 11:46:08AM -0600, Lucas De Marchi wrote:
> 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.

I'll change it here as well as in the following patches.

> 
> > + */
> > +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()..

Doing xe_info_init(xe, graphics_desc, media_desc) makes sense to me.

Thanks,
-Michał

> 
> Lucas De Marchi