Re: [PATCH] drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref
On Fri, 19 Apr 2024, Imre Deak 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
>> Cc: Francois Dugast
>> Cc: Ville Syrjälä
>> 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
>
> Reviewed-by: Imre Deak
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
Re: [PATCH] drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref
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
> Cc: Francois Dugast
> Cc: Ville Syrjälä
> 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
Reviewed-by: Imre Deak
> ---
> 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
>
Re: [PATCH] drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref
On Fri, Apr 19, 2024 at 01:30:21AM +0300, Imre Deak wrote:
> On Thu, Apr 18, 2024 at 06:13:20PM -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.
> >
> > Cc: Imre Deak
> > Cc: Francois Dugast
> > Cc: Ville Syrjälä
> > 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
> > ---
> > .../gpu/drm/i915/display/intel_display_power.c| 6 --
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +--
> > 2 files changed, 5 insertions(+), 16 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..e27b2ab82da0 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 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,10 +285,9 @@ 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);
> > pm_runtime_get_noresume(rpm->kdev);
> >
> > - intel_runtime_pm_acquire(rpm, true);
> > + intel_runtime_pm_acquire(rpm, false);
>
> This needs to stay a wakelock, so that the HW access in
> release_async_put_domains() will not lead to a wakelock not held assert.
> Only the above assert_rpm_wakelock_held() needs to be changed to
> assert_rpm_raw_wakeref_held().
hmm I see. That was actually my first attempt here, but then went down
to check the intel_runtime_pm_acquire and got confused and ended up
trying to convert everything... will resend it right now.
Thank you!
>
> >
> > return track_intel_runtime_pm_wakeref(rpm);
> > }
> > --
> > 2.44.0
> >
Re: [PATCH] drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref
On Thu, Apr 18, 2024 at 06:13:20PM -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.
>
> Cc: Imre Deak
> Cc: Francois Dugast
> Cc: Ville Syrjälä
> 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
> ---
> .../gpu/drm/i915/display/intel_display_power.c| 6 --
> drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +--
> 2 files changed, 5 insertions(+), 16 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..e27b2ab82da0 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 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,10 +285,9 @@ 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);
> pm_runtime_get_noresume(rpm->kdev);
>
> - intel_runtime_pm_acquire(rpm, true);
> + intel_runtime_pm_acquire(rpm, false);
This needs to stay a wakelock, so that the HW access in
release_async_put_domains() will not lead to a wakelock not held assert.
Only the above assert_rpm_wakelock_held() needs to be changed to
assert_rpm_raw_wakeref_held().
>
> return track_intel_runtime_pm_wakeref(rpm);
> }
> --
> 2.44.0
>
