> -----Original Message----- > From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx> > Sent: Wednesday, October 12, 2022 8:31 PM > To: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > Cc: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; tvrtko.ursulin@xxxxxxxxxxxxxxx; Auld, Matthew > <matthew.auld@xxxxxxxxx> > Subject: Re: [PATCH] drm/i915/dgfx: Temporary hammer to keep autosuspend > control 'on' > > On Wed, Oct 12, 2022 at 01:26:45PM +0300, Joonas Lahtinen wrote: > > I think I commented on this already, but the patch subject should > > really be as informative as possible like: "Disable PCI runtime PM on > > dGPUs" as that is exactly what the patch does. > > +1 here. Hi Joonas & Rodrigo, This patch don't disable runtime PM, it just keep autosupend control to 'on' , user or IGT tests can still enable autosupend by writing echo 'auto' > /sys/bus/pci/devices/bdf/power/control. So I believe I should keep the subject something like "Keep PCI autosupend 'on' dGPU" Other similar patch (https://patchwork.freedesktop.org/series/108477/) will disable the runtime PM completely. > > > > > Also bit unsure if the Fixes: tag should really point to the runtime > > PM commit but maybe instead to the introduction of LMEM commit. > I will remove the Fixes tag and replace it with introduction of LMEM tag. > indeed. if the runtime_pm by default came earlier then the LMEM, the > HAS_LMEM() won't even exist and it will break the ports. So here I should use IS_DGFX() instead of HAS_LMEM() ? Thanks, Anshuman Gupta. > > So, I'd say to go to whatever patch is the most recent between the 2. > > With those changes: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > > > Regards, Joonas > > > > Quoting Anshuman Gupta (2022-10-12 11:34:02) > > > DGFX platforms has lmem and cpu can access the lmem objects via mmap > > > and i915 internal i915_gem_object_pin_map() for > > > i915 own usages. Both of these methods has pre-requisite requirement > > > to keep GFX PCI endpoint in D0 for a supported iomem transaction > > > over PCI link. (Refer PCIe specs 5.3.1.4.1) > > > > > > Both DG1/DG2 have a hardware bug that violates the PCIe specs and > > > support the iomem read write transaction over PCIe bus despite > > > endpoint is D3 state. > > > Due to above H/W bug, we had never observed any issue with i915 > > > runtime PM versus lmem access. > > > But this issue becomes visible when PCIe gfx endpoint's upstream > > > bridge enters to D3, at this point any lmem read/write access will > > > be returned as unsupported request. But again this issue is not > > > observed on every platform because it has been observed on few host > > > machines > > > DG1/DG2 endpoint's upstream bridge does not bind with pcieport driver. > > > which really disables the PCIe power savings and leaves the bridge > > > at D0 state. > > > > > > Till we fix all issues related to runtime PM, we need to keep > > > autosupend control to 'on' on all discrete platforms with lmem. > > > > > > Fixes: 527bab0473f2 ("drm/i915/rpm: Enable runtime pm autosuspend by > > > default") > > > Suggested-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > index 6ed5786bcd29..410a5cb58a61 100644 > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > > @@ -591,8 +591,15 @@ void intel_runtime_pm_enable(struct > intel_runtime_pm *rpm) > > > pm_runtime_use_autosuspend(kdev); > > > } > > > > > > - /* Enable by default */ > > > - pm_runtime_allow(kdev); > > > + /* > > > + * FIXME: Temp hammer to keep autosupend disable on lmem > supported platforms. > > > + * As per PCIe specs 5.3.1.4.1, all iomem read write request over a PCIe > > > + * function will be unsupported in case PCIe endpoint function is in D3. > > > + * Let's keep i915 autosuspend control 'on' till we fix all known issue > > > + * with lmem access in D3. > > > + */ > > > + if (!HAS_LMEM(i915)) > > > + pm_runtime_allow(kdev); > > > > > > /* > > > * The core calls the driver load handler with an RPM reference held. > > > -- > > > 2.25.1 > > >
