[Intel-gfx] [PATCH] drm/i915/dgfx: Temporary hammer to keep autosuspend control 'on'

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Wednesday, October 12, 2022 8:31 PM
> To: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> gfx at lists.freedesktop.org; tvrtko.ursulin at linux.intel.com; Auld, Matthew
> <matthew.auld at intel.com>
> 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 at intel.com>
> 
> 
> >
> > 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 at intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > > ---
> > >  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
> > >