Re: [PATCH] drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 19 Apr 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Apr 18, 2024 at 06:37:56PM -0400, Rodrigo Vivi wrote:
>> In the past, the noresume function was used by the GEM code to ensure
>> wakelocks were held and bump its usage. This is no longer the case
>> and this function was totally unused until it started to be used again
>> by display with commit 77e619a82fc3 ("drm/i915/display: convert inner
>> wakeref get towards get_if_in_use")
>> 
>> However, on the display code, most of the callers are using the
>> raw wakeref, rather then the wakelock version. What caused a
>> major regression caught by CI.
>> 
>> Another option to this patch is to go with the original plan and
>> use the get_if_in_use variant in the display code, what is enough
>> to fulfil our needs. Then, an extra patch to delete the unused
>> _noresume variant.
>> 
>> v2: Keep grabbing wakelock but only assert for wakeref. (Imre)
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Francois Dugast <francois.dugast@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Fixes: 77e619a82fc3 ("drm/i915/display: convert inner wakeref get towards get_if_in_use")
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10875
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks for the patch and review. Pushed to drm-xe-next where the
offending commit was pushed.

BR,
Jani.


>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display_power.c |  6 ------
>>  drivers/gpu/drm/i915/intel_runtime_pm.c            | 14 +++++---------
>>  2 files changed, 5 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>> index 048943d0a881..03dc7edcc443 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> @@ -640,12 +640,6 @@ release_async_put_domains(struct i915_power_domains *power_domains,
>>  	enum intel_display_power_domain domain;
>>  	intel_wakeref_t wakeref;
>>  
>> -	/*
>> -	 * The caller must hold already raw wakeref, upgrade that to a proper
>> -	 * wakeref to make the state checker happy about the HW access during
>> -	 * power well disabling.
>> -	 */
>> -	assert_rpm_raw_wakeref_held(rpm);
>>  	wakeref = intel_runtime_pm_get_noresume(rpm);
>>  
>>  	for_each_power_domain(domain, mask) {
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index d4e844128826..2d0647aca964 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -272,15 +272,11 @@ intel_wakeref_t intel_runtime_pm_get_if_active(struct intel_runtime_pm *rpm)
>>   * intel_runtime_pm_get_noresume - grab a runtime pm reference
>>   * @rpm: the intel_runtime_pm structure
>>   *
>> - * This function grabs a device-level runtime pm reference (mostly used for GEM
>> - * code to ensure the GTT or GT is on).
>> + * This function grabs a device-level runtime pm reference.
>>   *
>> - * It will _not_ power up the device but instead only check that it's powered
>> - * on.  Therefore it is only valid to call this functions from contexts where
>> - * the device is known to be powered up and where trying to power it up would
>> - * result in hilarity and deadlocks. That pretty much means only the system
>> - * suspend/resume code where this is used to grab runtime pm references for
>> - * delayed setup down in work items.
>> + * It will _not_ resume the device but instead only get an extra wakeref.
>> + * Therefore it is only valid to call this functions from contexts where
>> + * the device is known to be active and with another wakeref previously hold.
>>   *
>>   * Any runtime pm reference obtained by this function must have a symmetric
>>   * call to intel_runtime_pm_put() to release the reference again.
>> @@ -289,7 +285,7 @@ intel_wakeref_t intel_runtime_pm_get_if_active(struct intel_runtime_pm *rpm)
>>   */
>>  intel_wakeref_t intel_runtime_pm_get_noresume(struct intel_runtime_pm *rpm)
>>  {
>> -	assert_rpm_wakelock_held(rpm);
>> +	assert_rpm_raw_wakeref_held(rpm);
>>  	pm_runtime_get_noresume(rpm->kdev);
>>  
>>  	intel_runtime_pm_acquire(rpm, true);
>> -- 
>> 2.44.0
>> 

-- 
Jani Nikula, Intel