Re: [PATCH 3/6] drm/xe: Move xe->info.devid|revid initialization

[Gustavo Sousa <gustavo.sousa@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> writes:

> On 5/25/2026 7:47 PM, Gustavo Sousa wrote:
>> Michal Wajdeczko <michal.wajdeczko@intel.com> writes:
>> 
>>> The xe_info_init_early() is a place where we initialize those of
>>> the xe->info fields that do not require any additional hardware
>>> probes. Move the initialization of the devid/revid also there, but
>>> to avoid breaking the kunit helper, which also calls this function,
>>> keep their initialization separate in sub-function so we can easily
>>> stub it when running the kunit test.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/tests/xe_pci.c      | 12 ++++++++++++
>>>  drivers/gpu/drm/xe/tests/xe_pci_test.h |  2 ++
>>>  drivers/gpu/drm/xe/xe_device.c         |  2 --
>>>  drivers/gpu/drm/xe/xe_pci.c            | 12 ++++++++++++
>>>  4 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/tests/xe_pci.c b/drivers/gpu/drm/xe/tests/xe_pci.c
>>> index 860409c579f8..7c3428ab4c89 100644
>>> --- a/drivers/gpu/drm/xe/tests/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/tests/xe_pci.c
>>> @@ -311,6 +311,17 @@ const void *xe_pci_id_gen_param(struct kunit *test, const void *prev, char *desc
>>>  }
>>>  EXPORT_SYMBOL_IF_KUNIT(xe_pci_id_gen_param);
>>>  
>>> +static void fake_init_devid(struct xe_device *xe)
>>> +{
>>> +	struct kunit *test = kunit_get_current_test();
>>> +	struct xe_pci_fake_data *data = test->priv;
>>> +
>>> +	if (data) {
>>> +		xe->info.devid = data->devid;
>>> +		xe->info.revid = data->revid;
>>> +	}
>>> +}
>>> +
>>>  static int fake_read_gmdid(struct xe_device *xe, enum xe_gmdid_type type,
>>>  			   u32 *ver, u32 *revid)
>>>  {
>>> @@ -369,6 +380,7 @@ int xe_pci_fake_device_init(struct xe_device *xe)
>>>  	xe->sriov.__mode = data && data->sriov_mode ?
>>>  			   data->sriov_mode : XE_SRIOV_MODE_NONE;
>>>  
>>> +	kunit_activate_static_stub(test, init_devid, fake_init_devid);
>>>  	kunit_activate_static_stub(test, read_gmdid, fake_read_gmdid);
>>>  	kunit_activate_static_stub(test, xe_info_probe_tile_count,
>>>  				   fake_xe_info_probe_tile_count);
>>> diff --git a/drivers/gpu/drm/xe/tests/xe_pci_test.h b/drivers/gpu/drm/xe/tests/xe_pci_test.h
>>> index 30505d1cbefc..46f961f75ecd 100644
>>> --- a/drivers/gpu/drm/xe/tests/xe_pci_test.h
>>> +++ b/drivers/gpu/drm/xe/tests/xe_pci_test.h
>>> @@ -22,6 +22,8 @@ struct xe_pci_fake_data {
>>>  	struct xe_step_info step;
>>>  	u32 graphics_verx100;
>>>  	u32 media_verx100;
>>> +	u16 devid;
>>> +	u8 revid;
>> 
>> Do we need to add those fields?
>> 
>> Looking through the rest of the series, I don't see us using them.  I
>> think we could just have fake_init_devid() do nothing for now.  We could
>> come back and add support for having devid and revid values when/if we
>> need them.
>
> hmm, we can return later, but OTOH this is a trivial change with near
> zero cost (as this struct is used in kunit only) so why postpone it?

Because it currently looks like dead code to me and maybe a distraction
for someone reading the code.

If we are confident that we are going to need it in the near future, I
guess it would be okay (although I personally would postpone to be done
in the series/patch that starts using it).  Otherwise, I would avoid it.

--
Gustavo Sousa

>
> existing code can still keep it uninitialized (zero) and any new test
> that would like to use devid/revid can be written immediately 
>
> and actually I was considering updating fake_xe_info_probe_tile_count
> to use new xe_pci_fake_data.tile_count field just for completeness ;)
>
>> 
>> --
>> Gustavo Sousa
>> 
>>>  };
>>>  
>>>  int xe_pci_fake_device_init(struct xe_device *xe);
>>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>>> index cdc7e0935c13..b498147dcf61 100644
>>> --- a/drivers/gpu/drm/xe/xe_device.c
>>> +++ b/drivers/gpu/drm/xe/xe_device.c
>>> @@ -521,8 +521,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev)
>>>  	if (err)
>>>  		return ERR_PTR(err);
>>>  
>>> -	xe->info.devid = pdev->device;
>>> -	xe->info.revid = pdev->revision;
>>>  	xe->atomic_svm_timeslice_ms = 5;
>>>  	xe->min_run_period_lr_ms = 5;
>>>  
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index b1f5f0098f26..6b1a4cb7ccb0 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -727,6 +727,16 @@ static int handle_gmdid(struct xe_device *xe,
>>>  	return 0;
>>>  }
>>>  
>>> +static void init_devid(struct xe_device *xe)
>>> +{
>>> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>> +
>>> +	KUNIT_STATIC_STUB_REDIRECT(init_devid, xe);
>>> +
>>> +	xe->info.devid = pdev->device;
>>> +	xe->info.revid = pdev->revision;
>>> +}
>>> +
>>>  /*
>>>   * Initialize device info content that only depends on static driver_data
>>>   * passed to the driver at probe time from PCI ID table.
>>> @@ -742,6 +752,8 @@ static int xe_info_init_early(struct xe_device *xe,
>>>  	xe->info.subplatform = subplatform_desc ?
>>>  		subplatform_desc->subplatform : XE_SUBPLATFORM_NONE;
>>>  
>>> +	init_devid(xe);
>>> +
>>>  	xe->info.dma_mask_size = desc->dma_mask_size;
>>>  	xe->info.va_bits = desc->va_bits;
>>>  	xe->info.vm_max_level = desc->vm_max_level;
>>> -- 
>>> 2.47.1