Re: [PATCH v4 1/4] drm/i915: Fix false-positive assert_rpm_wakelock_held in i915_pmic_bus_access_notifier

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 20-08-17 15:05, Hans de Goede wrote:
> Hi,
> 
> Note this v4 is send from my gmail in an attempt to keep the
> X-Mailer: git-send-email header which the test-infra wants, but
> that seems to have failed.
> 
> I've also just send another copy through my isp-s mail server,
> but I've not received a copy of that copy myself ? If anyone
> else has a second copy of this series can you please check if
> the git-send-email header is there ?
> 
> If not can someone resend this series so that it can go through
> the test infra ?

Ok, it seems that this first attempt (sending through gmail)
actually worked, but I could not see the header because it
gets stripped by the RH mail infra on receiving it too...

Anyways this series now has gone through the Fi.CI.BAT
without problems.

If someone can pick these up and push them to drm-intel-next-queued
that would be great.

Regards,

Hans



> 
> On 20-08-17 14:51, Hans de Goede wrote:
>> assert_rpm_wakelock_held is triggered from i915_pmic_bus_access_notifier
>> even though it gets unregistered on (runtime) suspend, this is caused
>> by a race happening under the following circumstances:
>>
>> intel_runtime_pm_put does:
>>
>>     atomic_dec(&dev_priv->pm.wakeref_count);
>>
>>     pm_runtime_mark_last_busy(kdev);
>>     pm_runtime_put_autosuspend(kdev);
>>
>> And pm_runtime_put_autosuspend calls intel_runtime_suspend from
>> a workqueue, so there is ample of time between the atomic_dec() and
>> intel_runtime_suspend() unregistering the notifier. If the notifier
>> gets called in this windowd assert_rpm_wakelock_held falsely triggers
>> (at this point we're not runtime-suspended yet).
>>
>> This commit adds disable_rpm_wakeref_asserts and
>> enable_rpm_wakeref_asserts calls around the
>> intel_uncore_forcewake_get(FORCEWAKE_ALL) call in
>> i915_pmic_bus_access_notifier fixing the false-positive WARN_ON.
>>
>> Reported-by: FKr <bugs-freedesktop@ubermail.me>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Reviewed-by: Imre Deak <imre.deak@intel.com>
>> ---
>> Changes in v2:
>> -Rebase on current (July 6th 2017) drm-next
>>
>> Changes in v3:
>> -Reword comment explaining why disabling the wakeref asserts is
>>   ok and necessary
>> -Add Imre's Reviewed-by
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 1d7b879cc68c..2d3aad319229 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1171,8 +1171,15 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
>>            * bus, which will be busy after this notification, leading to:
>>            * "render: timed out waiting for forcewake ack request."
>>            * errors.
>> +         *
>> +         * The notifier is unregistered during intel_runtime_suspend(),
>> +         * so it's ok to access the HW here without holding a RPM
>> +         * wake reference -> disable wakeref asserts for the time of
>> +         * the access.
>>            */
>> +        disable_rpm_wakeref_asserts(dev_priv);
>>           intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> +        enable_rpm_wakeref_asserts(dev_priv);
>>           break;
>>       case MBI_PMIC_BUS_ACCESS_END:
>>           intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel