[RFC 03/34] drm/xe: Fix display runtime_pm handling

[Matthew Auld]

On 14/02/2024 18:05, Rodrigo Vivi wrote:
> On Mon, Feb 05, 2024 at 09:11:37AM +0000, Matthew Auld wrote:
>> On 26/01/2024 20:30, Rodrigo Vivi wrote:
>>> i915's intel_runtime_pm_get_if_in_use actually calls the
>>> pm_runtime_get_if_active() with ign_usage_count = false, but Xe
>>> was erroneously calling it with true because of the mem_access cases.
>>
>> Good catch.
>>
>>> This can lead to unbalanced references.
>>
>> Is there an actual imbalance here though? Is it not just a case of being
>> overzealous in keeping the device awake when it is not currently "in_use" vs
>> "if_active"? If the api increments the usage count we will still decrement
>> it later, regardless of active vs in-use, AFAICT.
> 
> Indeed.
> 
> s/This can lead to unbalanced references./This can lead to unnecessary
> references getting hold here and device never getting into the runtime
> suspended state./g
> 
>>
>>>
>>> Let's use directly the 'if_in_use' function provided by linux/pm_runtime.
>>>
>>> Also, already start this new function protected from the runtime
>>> recursion, since runtime_pm will need to call for display functions
>>> for a proper D3Cold flow.
>>>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> ---
>>>    .../gpu/drm/xe/compat-i915-headers/i915_drv.h   |  2 +-
>>>    drivers/gpu/drm/xe/xe_pm.c                      | 17 +++++++++++++++++
>>>    drivers/gpu/drm/xe/xe_pm.h                      |  1 +
>>>    3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
>>> index 420eba0e4be0..ad5864d1dd74 100644
>>> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
>>> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
>>> @@ -177,7 +177,7 @@ static inline intel_wakeref_t intel_runtime_pm_get_if_in_use(struct xe_runtime_p
>>>    {
>>>    	struct xe_device *xe = container_of(pm, struct xe_device, runtime_pm);
>>> -	return xe_pm_runtime_get_if_active(xe);
>>> +	return xe_pm_runtime_get_if_in_use(xe);
>>>    }
>>>    static inline void intel_runtime_pm_put_unchecked(struct xe_runtime_pm *pm)
>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>> index bd35fe9f6227..19f88cb7715b 100644
>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>> @@ -417,6 +417,23 @@ int xe_pm_runtime_get_if_active(struct xe_device *xe)
>>>    	return pm_runtime_get_if_active(xe->drm.dev, true);
>>>    }
>>> +/**
>>> + * xe_pm_runtime_get_if_in_use - Get a runtime_pm reference and resume if needed
>>> + * @xe: xe device instance
>>> + *
>>> + * Returns: True if device is awake and the reference was taken, false otherwise.
>>> + */
>>> +bool xe_pm_runtime_get_if_in_use(struct xe_device *xe)
>>> +{
>>> +	if (xe_pm_read_callback_task(xe) == current) {
>>> +		/* The device is awake, grab the ref and move on */
>>> +		pm_runtime_get_noresume(xe->drm.dev);
>>> +		return true;
>>> +	}
>>> +
>>> +	return pm_runtime_get_if_in_use(xe->drm.dev) >= 0;
>>
>> This is doing atomic_inc_not_zero() underneath for the "in_use" case AFAICT.
>> If the usage count is zero it doesn't increment it and returns 0. Does that
>> not lead to an imbalance? Should this rather be > 0?

^^ did you see the comment here also?

>>
>>> +}
>>> +
>>>    /**
>>>     * xe_pm_assert_unbounded_bridge - Disable PM on unbounded pcie parent bridge
>>>     * @xe: xe device instance
>>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>>> index 64a97c6726a7..9d372cbf388b 100644
>>> --- a/drivers/gpu/drm/xe/xe_pm.h
>>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>>> @@ -28,6 +28,7 @@ int xe_pm_runtime_resume(struct xe_device *xe);
>>>    int xe_pm_runtime_get(struct xe_device *xe);
>>>    int xe_pm_runtime_put(struct xe_device *xe);
>>>    int xe_pm_runtime_get_if_active(struct xe_device *xe);
>>> +bool xe_pm_runtime_get_if_in_use(struct xe_device *xe);
>>>    void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
>>>    int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
>>>    void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);