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

[Matthew Auld <matthew.auld@intel.com>]

On 12/10/2022 15:57, Rodrigo Vivi wrote:
> On Wed, Oct 12, 2022 at 10:48:30AM +0100, Matthew Auld wrote:
>> On 12/10/2022 09:34, Anshuman Gupta wrote:
>>> 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")
>>
>> So with this change all the runtime pm stuff is disabled on dgfx? i.e
>> intel_runtime_pm_get() always returns zero or so? Wondering if we should
>> also revert ad74457a6b5a ("drm/i915/dgfx: Release mmap on rpm suspend") for
>> now, since that still needs some more fixes...
> 
> I don't believe we need to revert that. That's already one step forward towards
> the final solution. It is not complete but it is not wrong.
> And it is orthogonal to this protection right now.

That commit has some known bugs though, see 
https://patchwork.freedesktop.org/patch/504444/?series=108972&rev=1. But 
that patch appears stuck for a while now, so my question was if we 
should just revert for now, or does this patch now effectively make 
those known bugs a non-issue...

> 
>>
>>> Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@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.