Re: [PATCH v2 12/16] platform/x86/amd/pmf: Add PMF-AMDGPU get interface

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 10/4/2023 6:19 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> 
>> In order to provide GPU inputs to TA for the Smart PC solution to work, we
>> need to have interface between the PMF driver and the AMDGPU driver.
>>
>> Add the initial code path for get interface from AMDGPU.
>>
>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
>> @@ -355,6 +356,21 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
>>  	return amd_pmf_start_policy_engine(dev);
>>  }
>>  
>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data)
>> +{
>> +	struct amd_pmf_dev *dev = data;
>> +
>> +	if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
>> +		/* get the amdgpu handle from the pci root after walking through the pci bus */
> 
> I can see from the code that you assign to amdgpu handle so this comment 
> added no information.
> 
> It doesn't really answer at all why you're doing this second step. Based 
> on the give parameters to pci_get_device(), it looks as if you're asking 
> for the same device you already have in pdev to be searched to you.

Not sure if I understand you remark completely.

amd_pmf_get_gpu_handle() is a callback function for pci_walk_bus
(which is done below).

What I am trying to do here is to get the PCI handle for the GPU
device by walking the PCI bus.

I think the 'pdev' here refers to the pci root, using that root we
walk the entire tree and only stop walking when we find a handle to
GPU device.

Do you want me to change the "pdev" parameter to be renamed as "root" ?

Am I missing something?

Thanks,
Shyam

> 
>> +		dev->gfx_data.gpu_dev = pci_get_device(pdev->vendor, pdev->device, NULL);
>> +		if (dev->gfx_data.gpu_dev) {
>> +			pci_dev_put(pdev);
>> +			return 1; /* stop walking */
>> +		}
>> +	}
>> +	return 0; /* continue walking */
>> +}
>> +
>>  static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
>>  {
>>  	return ver->impl_id == TEE_IMPL_ID_AMDTEE;
>> @@ -451,6 +467,15 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>  	INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
>>  	amd_pmf_set_dram_addr(dev);
>>  	amd_pmf_get_bios_buffer(dev);
>> +
>> +	/* get amdgpu handle */
>> +	pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev);
>> +	if (!dev->gfx_data.gpu_dev)
>> +		dev_err(dev->dev, "GPU handle not found!\n");
>> +
>> +	if (!amd_pmf_gpu_init(&dev->gfx_data))
>> +		dev->gfx_data.gpu_dev_en = true;
>> +
> 
>