[Intel-xe] [PATCH v3] drm/xe/pm: Disable PM on unbounded pcie parent bridge

[Sudeep Holla]

On Mon, May 22, 2023 at 02:03:35PM -0400, Rodrigo Vivi wrote:
> On Mon, May 22, 2023 at 12:28:20PM -0400, Gupta, Anshuman wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Gupta, Anshuman
> > > Sent: Friday, May 19, 2023 10:28 AM
> > > To: Rodrigo Vivi <rodrigo.vivi at kernel.org>
> > > Cc: intel-xe at lists.freedesktop.org; Nikula, Jani <jani.nikula at intel.com>; Vivi,
> > > Rodrigo <rodrigo.vivi at intel.com>
> > > Subject: RE: [Intel-xe] [PATCH v3] drm/xe/pm: Disable PM on unbounded
> > > pcie parent bridge
> > > 
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Rodrigo Vivi <rodrigo.vivi at kernel.org>
> > > > Sent: Thursday, May 18, 2023 10:41 PM
> > > > To: Gupta, Anshuman <anshuman.gupta at intel.com>
> > > > Cc: intel-xe at lists.freedesktop.org; Nikula, Jani
> > > > <jani.nikula at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > > Subject: Re: [Intel-xe] [PATCH v3] drm/xe/pm: Disable PM on unbounded
> > > > pcie parent bridge
> > > >
> > > > On Thu, May 18, 2023 at 02:22:59PM +0530, Anshuman Gupta wrote:
> > > > > Intel Discrete GFX cards gfx may have multiple PCIe endpoints, they
> > > > > connects to root port via pcie upstream switch port(USP) and virtual
> > > > > pcie switch port(VSP), sometimes VSP pcie devices doesn't bind to
> > > > > pcieport driver. Without pcieport driver, pcie PM comes without any
> > > > > warranty and with unbounded VSP gfx card won't transition to low
> > > > > power pcie device and link state therefore assert dev_warn on
> > > > > unbounded VSP and disable xe driver PM support.
> > > > >
> > > > > v2:
> > > > > - Disable Xe PCI PM support. [Rodrigo]
> > > > > v3:
> > > > > - Changed subject and Rebase.
> > > > >
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> 
> Cc: Sudeep Holla <sudeep.holla at arm.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> 
> > > > > ---
> > > > >  drivers/gpu/drm/xe/xe_pci.c | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c
> > > > > b/drivers/gpu/drm/xe/xe_pci.c index e789a50a1310..e09d76b7755a
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > > @@ -603,6 +603,19 @@ static void xe_pci_remove(struct pci_dev *pdev)
> > > > >  	pci_set_drvdata(pdev, NULL);
> > > > >  }
> > > > >
> > > >
> > /snip
> > > > Probably the safest place to call this is at xe_register_pci_driver
> > > > right before registering it, no?!
> > > > possible?
> > Hi Rodrigo ,
> > For safety purpose  how about to use device_set_pm_not_required()
> > I had already tested it locally, above helper removed device PM support.
> 
> There are not many device drivers using that directly and no conditional
> usage.
>

While I agree that it was not used by much drivers when it was initially
introduced by me, grepping it for now listed quite a few drivers like
cxl and nvdimm. So if disabling PM is right thing to do(which I have no
idea), using this helper is much cleaner than the approach in the original
patch as in [1].

> However I liked that very much and when taking a look to the commit that added
> that, I don't see any blocker on why not use it to remove the PM support
> on this case that we cannot support with our discrete devices.
> 
> 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model")
> 
> So I CC'ed here Sudeep and Rafael so they can raise any concerns they
> might have.
>

None from me.

> FYI, the initial patch with the reasoning on why we need to disable PM:
>

I don't have much domain knowledge to acknowledge that it is the right
thing, but the approach using helper seems right one for me.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/all/20230518085259.4083944-1-anshuman.gupta@intel.com