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@xxxxxxx>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> >
> >> @@ -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.
