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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> 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.

Not exactly what happens, in amd_pmf_get_gpu_handle() pdev changes on each 
call so I don't know why you stated it is refering to the "pci root".

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

No, please don't do that, it would be misleading.

> Am I missing something?

I meant that at some point of the walk through the PCI devices, you have 
a PCI device pdev with ->vendor PCI_VENDOR_ID_AMD when that if condition 
above matched. Please explain why you need to do another lookup with 
pci_get_device() at that point (with the same ->vendor and ->device as 
shown below)?

> >> +		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 */


-- 
 i.